glic: Set the initial invocation source on background invocation [chromium/src : main]
Ian Wells (Gerrit)
Ian Wells added 1 comment![]( "Open in Gerrit")
Related details
- Nick Birnie
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nick Birnie (Gerrit)
Nick Birnie added 2 comments![]( "Open in Gerrit")
return instance_metrics().initial_invocation_source();Its probably better style to create the const instance_metrics() but couldnt you have technically just used instance_metrics_.initial_invocation_source() here? Is there a reason beside style to do this?
void GlicInstanceMetrics::OnShowInactiveSidePanel(If initial_invocation_source_ is populated here then later when the instance is brought to the foreground, GlicInstanceImpl invokes OnOpen() under kReshowInactive. Inside OnOpen(), the if (!initial_invocation_source_.has_value()) block evaluates to false, which causes base::UmaHistogramEnumeration("Glic.Instance.InitialInvocationSource", source); to be skipped completely. That instance would finish its lifecycle dropping that metric entirely.
Jetski wrote a test that fails:
```
TEST_F(GlicInstanceMetricsTest, InitialInvocationSource_LoggedWhenStartingInactive) {
// 1. Simulate opening the instance in an inactive side panel via a background daisy-chain.
metrics_.OnShowInactiveSidePanel(mojom::InvocationSource::kDaisyChainOnFollowLink);
// 2. Later, the user activates the background tab, promoting the instance to be fully shown.
EXPECT_CALL(mock_tab_, GetTabHandle()).WillRepeatedly(testing::Return(1));
ShowOptions show_options{SidePanelShowOptions{mock_tab_}};
metrics_.OnOpen(mojom::InvocationSource::kReshowInactive, show_options);
// 3. EXPECTATION: The original entry point (`kDaisyChainOnFollowLink`) should have been logged.
// CURRENT BEHAVIOR: This fails expecting 1 sample but getting 0.
histogram_tester_.ExpectUniqueSample(
"Glic.Instance.InitialInvocationSource",
mojom::InvocationSource::kDaisyChainOnFollowLink, 1);
}
```
Related details
- Ian Wells
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ian Wells (Gerrit)
Ian Wells voted and added 2 comments![]( "Open in Gerrit")
Votes added by Ian Wells
| Commit-Queue | +1 |
2 comments
Its probably better style to create the const instance_metrics() but couldnt you have technically just used instance_metrics_.initial_invocation_source() here? Is there a reason beside style to do this?
Frankly I let Jetski run wild and didn't bother to change this. I appreciate your assumption that there was a good reason though, haha
There's no reason to add a const `instance_metrics()`, I'll change this function to use `instance_metrics_` directly.
If initial_invocation_source_ is populated here then later when the instance is brought to the foreground, GlicInstanceImpl invokes OnOpen() under kReshowInactive. Inside OnOpen(), the if (!initial_invocation_source_.has_value()) block evaluates to false, which causes base::UmaHistogramEnumeration("Glic.Instance.InitialInvocationSource", source); to be skipped completely. That instance would finish its lifecycle dropping that metric entirely.
Jetski wrote a test that fails:
```
TEST_F(GlicInstanceMetricsTest, InitialInvocationSource_LoggedWhenStartingInactive) {
// 1. Simulate opening the instance in an inactive side panel via a background daisy-chain.
metrics_.OnShowInactiveSidePanel(mojom::InvocationSource::kDaisyChainOnFollowLink);// 2. Later, the user activates the background tab, promoting the instance to be fully shown.
EXPECT_CALL(mock_tab_, GetTabHandle()).WillRepeatedly(testing::Return(1));
ShowOptions show_options{SidePanelShowOptions{mock_tab_}};
metrics_.OnOpen(mojom::InvocationSource::kReshowInactive, show_options);// 3. EXPECTATION: The original entry point (`kDaisyChainOnFollowLink`) should have been logged.
// CURRENT BEHAVIOR: This fails expecting 1 sample but getting 0.
histogram_tester_.ExpectUniqueSample(
"Glic.Instance.InitialInvocationSource",
mojom::InvocationSource::kDaisyChainOnFollowLink, 1);
}
```
Good catch. I've updated how OnOpen() decides whether to log InitialInvocationSource to allow for initial_invocation_source_ already having been set by a background invocation. Also, added your test unchanged.
IMO it's fine to change that metric in this way because kReshowInactive isn't a very useful signal for initial invocation source -- we should be getting strictly more information by logging whatever source was set when opening inactive.
Related details
- Nick Birnie
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nick Birnie (Gerrit)
Nick Birnie voted and added 1 comment![]( "Open in Gerrit")
Votes added by Nick Birnie
| Code-Review | +1 |
1 comment
Related details
- Ian Wells
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ian Wells (Gerrit)
Ian Wells voted and added 2 comments![]( "Open in Gerrit")
Votes added by Ian Wells
| Commit-Queue | +2 |
2 comments
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change with unreviewed changes![]( "Open in Gerrit")
Unreviewed changes
3 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: chrome/browser/glic/service/glic_instance_impl.h
Insertions: 1, Deletions: 1.
@@ -49,7 +49,7 @@
namespace tabs {
class TabInterface;
}
-//
+
namespace glic {
class ContextualCueingService;
class EmptyEmbedderDelegate;
```
Change information
glic: Set the initial invocation source on background invocation
This is meant to support UniversalCart which can invoke on background
tabs. This case causes GlicInstanceImpl::Show to exit early (inactive
embedder) without setting initial_invocation_source_. We don't want to
lose the initial invocation source in this case.
Also expose initial_invocation_source_ so that it can be used in
crrev.com/c/7963450.
- M chrome/browser/glic/public/glic_instance.h
- M chrome/browser/glic/service/glic_instance_impl.cc
- M chrome/browser/glic/service/glic_instance_impl.h
- M chrome/browser/glic/service/metrics/glic_instance_metrics.cc
- M chrome/browser/glic/service/metrics/glic_instance_metrics.h
- M chrome/browser/glic/service/metrics/glic_instance_metrics_unittest.cc
- M chrome/browser/glic/test_support/mock_glic_instance.h
Code-Review: +1 by Nick Birnie
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |