[composebox] Record whether context menu animations were shown or not [chromium/src : main]
Dhruv Kathpalia (Gerrit)
Dhruv Kathpalia voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Chromium IPC Reviews
- Tibor Goldschwendt
- manuk hovanesian
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
gwsq (Gerrit)
Message from gwsq![]( "Open in Gerrit")
From googleclient/chrome/chromium_gwsq/ipc/config.gwsq:
IPC: ff...@chromium.org
📎 It looks like you’re making a possibly security-sensitive change! 📎 IPC security review isn’t a rubberstamp, so your friendly security reviewer will need a fair amount of context to review your CL effectively. Please review your CL description and code comments to make sure they provide context for someone unfamiliar with your project/area. Pay special attention to where data comes from and which processes it flows between (and their privilege levels). Feel free to point your security reviewer at design docs, bugs, or other links if you can’t reasonably make a self-contained CL description. (Also see https://cbea.ms/git-commit/).
IPC reviewer(s): ff...@chromium.org
Reviewer source(s):
ff...@chromium.org is from context(googleclient/chrome/chromium_gwsq/ipc/config.gwsq)
Related details
- Fred Shih
- Tibor Goldschwendt
- manuk hovanesian
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Tibor Goldschwendt (Gerrit)
Tibor Goldschwendt added 3 comments![]( "Open in Gerrit")
<histogram name="Omnibox.ContextMenu.AnimationShown.ContextualTasks"What is the rationale for putting those into the Omnibox namespace?
<owner>dhruvka...@google.com</owner>
<summary>Please fix this WARNING reported by Metrics: It's preferred to list at least two owners, where the second is often a team mai...
It's preferred to list at least two owners, where the second is often a team mailing list or a src/path/to/OWNERS reference: https://chromium.googlesource.com/chromium/src.git/+/HEAD/tools/metrics/histograms/README.md#Owners.
<owner>dhruvka...@google.com</owner>
<summary>Please fix this WARNING reported by Metrics: It's preferred to list at least two owners, where the second is often a team mai...
It's preferred to list at least two owners, where the second is often a team mailing list or a src/path/to/OWNERS reference: https://chromium.googlesource.com/chromium/src.git/+/HEAD/tools/metrics/histograms/README.md#Owners.
Related details
- Dhruv Kathpalia
- Fred Shih
- manuk hovanesian
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. |
Dhruv Kathpalia (Gerrit)
Dhruv Kathpalia added 3 comments![]( "Open in Gerrit")
<histogram name="Omnibox.ContextMenu.AnimationShown.ContextualTasks"What is the rationale for putting those into the Omnibox namespace?
This metric was shared across the Realbox and the Nextbox. And I had noticed some metrics before which were common having the Omnibox namespace before. Eg: Omnibox.AimEntrypoint.Activated.UserTextPresent. Let me know if you think it should be changed to something more specific to NTP or Cobrowse.
Please fix this WARNING reported by Metrics: It's preferred to list at least two owners, where the second is often a team mai...
It's preferred to list at least two owners, where the second is often a team mailing list or a src/path/to/OWNERS reference: https://chromium.googlesource.com/chromium/src.git/+/HEAD/tools/metrics/histograms/README.md#Owners.
Done
Please fix this WARNING reported by Metrics: It's preferred to list at least two owners, where the second is often a team mai...
It's preferred to list at least two owners, where the second is often a team mailing list or a src/path/to/OWNERS reference: https://chromium.googlesource.com/chromium/src.git/+/HEAD/tools/metrics/histograms/README.md#Owners.
- Fred Shih
- Tibor Goldschwendt
- manuk hovanesian
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. |
Tibor Goldschwendt (Gerrit)
Tibor Goldschwendt voted and added 2 comments![]( "Open in Gerrit")
Votes added by Tibor Goldschwendt
| Code-Review | +1 |
2 comments
<histogram name="Omnibox.ContextMenu.AnimationShown.ContextualTasks"Dhruv KathpaliaWhat is the rationale for putting those into the Omnibox namespace?
This metric was shared across the Realbox and the Nextbox. And I had noticed some metrics before which were common having the Omnibox namespace before. Eg: Omnibox.AimEntrypoint.Activated.UserTextPresent. Let me know if you think it should be changed to something more specific to NTP or Cobrowse.
Gotcha. I think the metrics taxonomy is ripe for some refactor. But that shouldn't be a blocker to your CL right now.
Records whether the context menu (nextbox) animation was shown to the userNit: realbox?
Related details
- Dhruv Kathpalia
- Fred Shih
- manuk hovanesian
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. |
Fred Shih (Gerrit)
Fred Shih voted Code-Review+1![]( "Open in Gerrit")
- Dhruv Kathpalia
- manuk hovanesian
Dhruv Kathpalia (Gerrit)
Dhruv Kathpalia added 1 comment![]( "Open in Gerrit")
Records whether the context menu (nextbox) animation was shown to the userDhruv KathpaliaNit: realbox?
Done
Related details
- manuk hovanesian
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dhruv Kathpalia (Gerrit)
Dhruv Kathpalia added 1 comment![]( "Open in Gerrit")
Hi @jdon...@chromium.org,
Can you please review histograms.xml changes? Thanks
Related details
- Justin Donnelly
- manuk hovanesian
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Justin Donnelly (Gerrit)
Justin Donnelly voted and added 1 comment![]( "Open in Gerrit")
Votes added by Justin Donnelly
| Code-Review | +1 |
1 comment
Records whether the context menu (nextbox) animation was shown to the userAdd a description of _when_ this metric is recorded (here and in the other histogram as well).
See the "Recorded" statements in the two histograms above for examples but I assume yours will be something like an extra sentence at the end that says, "Recorded every time the context menu is opened" (or whatever your actual trigger is in this case).
Related details
- Dhruv Kathpalia
- manuk hovanesian
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. |
Dhruv Kathpalia (Gerrit)
Dhruv Kathpalia voted and added 1 comment![]( "Open in Gerrit")
Votes added by Dhruv Kathpalia
| Commit-Queue | +2 |
1 comment
Records whether the context menu (nextbox) animation was shown to the userAdd a description of _when_ this metric is recorded (here and in the other histogram as well).
See the "Recorded" statements in the two histograms above for examples but I assume yours will be something like an extra sentence at the end that says, "Recorded every time the context menu is opened" (or whatever your actual trigger is in this case).
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dhruv Kathpalia (Gerrit)
Dhruv Kathpalia voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
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
4 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: tools/metrics/histograms/metadata/omnibox/histograms.xml
Insertions: 3, Deletions: 1.
The diff is too large to show. Please review the diff.
```
Change information
[composebox] Record whether context menu animations were shown or not
Updates the context menu animation impression recording to accept a
boolean indicating whether the animation was actually shown or blocked.
This allows tracking the effectiveness of the animation limiting
experiment.
- M chrome/browser/contextual_tasks/contextual_tasks_extension_handler.cc
- M chrome/browser/contextual_tasks/contextual_tasks_extension_handler.h
- M chrome/browser/resources/contextual_tasks/composebox.ts
- M chrome/browser/resources/new_tab_page/app.ts
- M chrome/browser/ui/lens/lens_composebox_handler.cc
- M chrome/browser/ui/lens/lens_composebox_handler.h
- M chrome/browser/ui/webui/cr_components/composebox/composebox_handler.cc
- M chrome/browser/ui/webui/cr_components/composebox/composebox_handler.h
- M chrome/browser/ui/webui/cr_components/composebox/composebox_handler_unittest.cc
- M chrome/browser/ui/webui/new_tab_page/new_tab_page.mojom
- M chrome/browser/ui/webui/new_tab_page/new_tab_page_handler.cc
- M chrome/browser/ui/webui/new_tab_page/new_tab_page_handler.h
- M chrome/browser/ui/webui/new_tab_page/new_tab_page_handler_unittest.cc
- M chrome/test/data/webui/contextual_tasks/composebox_zero_state_test.ts
- M chrome/test/data/webui/new_tab_page/app_test.ts
- M tools/metrics/histograms/metadata/omnibox/histograms.xml
- M ui/webui/resources/cr_components/composebox/composebox.mojom
Code-Review: +1 by Fred Shih, +1 by Justin Donnelly, +1 by Tibor Goldschwendt
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |