Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2831113002:
[Suggestions] Add User action UMA for context menu actions (Closed)
Description[Suggestions] Add User action UMA for context menu actions
Added recording of user actions for all context menu actions:
- open context menu
- open item in new tab
- open item in new window
- open item in an incognito tab
- download item
- remove item
- undo removal of item
BUG=713145
Review-Url: https://codereview.chromium.org/2831113002
Cr-Commit-Position: refs/heads/master@{#466932}
Committed: https://chromium.googlesource.com/chromium/src/+/20a75f52c468e41ecc06a80b3cd7486b203c00a7
Patch Set 1 #
Total comments: 23
Patch Set 2 : Renamed user action names. #Patch Set 3 : "Refactor" #
Messages
Total messages: 27 (12 generated)
galinap@google.com changed reviewers: + bauerb@chromium.org, dgn@chromium.org, mvanouwerkerk@chromium.org
PTAL
The CQ bit was checked by galinap@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
thanks! That's a good basis for discussion :) bauerb@ mvanouwerkerk@ do you have preferences for the naming? https://codereview.chromium.org/2831113002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ntp/ContextMenuManager.java (right): https://codereview.chromium.org/2831113002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/ContextMenuManager.java:128: // Record user action. nit: remove this comment, it's not adding anything to explain the code. The empty line is enough for separation purposes https://codereview.chromium.org/2831113002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/ContextMenuManager.java:191: RecordUserAction.record("NewTabPage_OpenItemInNewWindow"); I'd use ContentSuggestions.ContextMenu.OpenItemInNewWindow, and similar for strings below. bauerb@, mvanouwerkerk@, wdyt? the current UMA is all scoped to NewTabPage so this is consistent, but it's unlikely that we will rename user actions. I would prefer not to add new UMA with inaccurate names. https://codereview.chromium.org/2831113002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/suggestions/TileGroup.java (right): https://codereview.chromium.org/2831113002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/suggestions/TileGroup.java:490: RecordUserAction.record("NewTabPage_ItemRemovalUndone"); I'd use ContentSuggestions.Tile.RemovalUndone https://codereview.chromium.org/2831113002/diff/1/tools/metrics/actions/actio... File tools/metrics/actions/actions.xml (right): https://codereview.chromium.org/2831113002/diff/1/tools/metrics/actions/actio... tools/metrics/actions/actions.xml:10980: <owner>galinap@google.com</owner> mvanouwerkerk@: do you want to be owner for that? https://codereview.chromium.org/2831113002/diff/1/tools/metrics/actions/actio... tools/metrics/actions/actions.xml:10982: Android: Context menu is shown after a long click on an item in the New Tab Don't make the description overly specific to the new tab page. Chrome Home is a UI surface that features these items out of the context of a NTP https://codereview.chromium.org/2831113002/diff/1/tools/metrics/actions/actio... tools/metrics/actions/actions.xml:10990: Andorid: User clicked to download an item from the New Tab Page. typo on "Android"
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2831113002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ntp/ContextMenuManager.java (right): https://codereview.chromium.org/2831113002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/ContextMenuManager.java:191: RecordUserAction.record("NewTabPage_OpenItemInNewWindow"); On 2017/04/20 18:10:24, dgn wrote: > I'd use ContentSuggestions.ContextMenu.OpenItemInNewWindow, and similar for > strings below. > > bauerb@, mvanouwerkerk@, wdyt? the current UMA is all scoped to NewTabPage so > this is consistent, but it's unlikely that we will rename user actions. I would > prefer not to add new UMA with inaccurate names. Hm, OTOH inconsistent naming isn't great either :-/ I don't suppose we can define aliases for user actions?
https://codereview.chromium.org/2831113002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ntp/ContextMenuManager.java (right): https://codereview.chromium.org/2831113002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/ContextMenuManager.java:129: RecordUserAction.record("NewTabPage_ContextMenuShown"); What's the latest guidance on separators... underscores vs dots vs none? I find dots easier on the eye, but maybe that is not recommended. https://codereview.chromium.org/2831113002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/ContextMenuManager.java:191: RecordUserAction.record("NewTabPage_OpenItemInNewWindow"); On 2017/04/20 18:10:24, dgn wrote: > I'd use ContentSuggestions.ContextMenu.OpenItemInNewWindow, and similar for > strings below. > > bauerb@, mvanouwerkerk@, wdyt? the current UMA is all scoped to NewTabPage so > this is consistent, but it's unlikely that we will rename user actions. I would > prefer not to add new UMA with inaccurate names. I think ContentSuggestions is also not quite correct for a Tile, is that considered content? They're mostly top level site urls. How about Suggestions.ContextMenu.OpenItemInNewWindow ? https://codereview.chromium.org/2831113002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/ContextMenuManager.java:195: RecordUserAction.record("NewTabPage_OpenItemInNewTab"); It's a bit unfortunate that we cannot distinguish at all what kind of item we're talking about here. A tile, a card? For what category? This is a good improvement though, so I#m happy for this CL to go ahead as currently designed. https://codereview.chromium.org/2831113002/diff/1/tools/metrics/actions/actio... File tools/metrics/actions/actions.xml (right): https://codereview.chromium.org/2831113002/diff/1/tools/metrics/actions/actio... tools/metrics/actions/actions.xml:10980: <owner>galinap@google.com</owner> On 2017/04/20 18:10:24, dgn wrote: > mvanouwerkerk@: do you want to be owner for that? Sure. https://codereview.chromium.org/2831113002/diff/1/tools/metrics/actions/actio... tools/metrics/actions/actions.xml:11027: Android: User clicked to remove an item from the New Tab Page. It would be nice if we could also track dismissals / removals from swipes. Should that be a tracked separately from dismissal by context menu?
https://codereview.chromium.org/2831113002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ntp/ContextMenuManager.java (right): https://codereview.chromium.org/2831113002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/ContextMenuManager.java:195: RecordUserAction.record("NewTabPage_OpenItemInNewTab"); On 2017/04/24 13:13:21, Michael van Ouwerkerk wrote: > It's a bit unfortunate that we cannot distinguish at all what kind of item we're > talking about here. A tile, a card? For what category? This is a good > improvement though, so I#m happy for this CL to go ahead as currently designed. IMO distinguishing the type of content should be done at the level of the implementer. So we could have something like Suggestions.ContextMenu.OpenItemInNewWindow -> Suggestions.Downloads.Open and Suggestions.Card.Tap -> Suggestions.Downloads.Open (made up names), allowing also to have the breakdown of how items of specific categories are opened. So it could be done in a follow up CL looking more generally at all the interactions in the suggestions UI, and this one can land as-is https://codereview.chromium.org/2831113002/diff/1/tools/metrics/actions/actio... File tools/metrics/actions/actions.xml (right): https://codereview.chromium.org/2831113002/diff/1/tools/metrics/actions/actio... tools/metrics/actions/actions.xml:11027: Android: User clicked to remove an item from the New Tab Page. On 2017/04/24 13:13:22, Michael van Ouwerkerk wrote: > It would be nice if we could also track dismissals / removals from swipes. > Should that be a tracked separately from dismissal by context menu? I think tracking what part of the UI is used for which result would be nice, so yes, I would prefer tracking dismissals from swipes separately.
https://codereview.chromium.org/2831113002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ntp/ContextMenuManager.java (right): https://codereview.chromium.org/2831113002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/ContextMenuManager.java:128: // Record user action. On 2017/04/20 18:10:24, dgn wrote: > nit: remove this comment, it's not adding anything to explain the code. The > empty line is enough for separation purposes Done. https://codereview.chromium.org/2831113002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/ContextMenuManager.java:129: RecordUserAction.record("NewTabPage_ContextMenuShown"); On 2017/04/24 13:13:21, Michael van Ouwerkerk wrote: > What's the latest guidance on separators... underscores vs dots vs none? I find > dots easier on the eye, but maybe that is not recommended. I'm not sure how to check what the latest guidance is, but in the file every entry is different. I agree that dots are more clear, so I'll change to all use dots instead of underscores. https://codereview.chromium.org/2831113002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/ContextMenuManager.java:191: RecordUserAction.record("NewTabPage_OpenItemInNewWindow"); On 2017/04/24 13:13:21, Michael van Ouwerkerk wrote: > On 2017/04/20 18:10:24, dgn wrote: > > I'd use ContentSuggestions.ContextMenu.OpenItemInNewWindow, and similar for > > strings below. > > > > bauerb@, mvanouwerkerk@, wdyt? the current UMA is all scoped to NewTabPage so > > this is consistent, but it's unlikely that we will rename user actions. I > would > > prefer not to add new UMA with inaccurate names. > > I think ContentSuggestions is also not quite correct for a Tile, is that > considered content? They're mostly top level site urls. How about > Suggestions.ContextMenu.OpenItemInNewWindow ? It's true that "Suggestions" is more precise for this case, but this introduces more inconsistency as everywhere in the other user actions is used "_ContentSuggestions_smth" (underscores as well). Should I name it "Suggestions. ..." anyway? @dgn, @bauerb? https://codereview.chromium.org/2831113002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/suggestions/TileGroup.java (right): https://codereview.chromium.org/2831113002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/suggestions/TileGroup.java:490: RecordUserAction.record("NewTabPage_ItemRemovalUndone"); On 2017/04/20 18:10:24, dgn wrote: > I'd use ContentSuggestions.Tile.RemovalUndone Done. https://codereview.chromium.org/2831113002/diff/1/tools/metrics/actions/actio... File tools/metrics/actions/actions.xml (right): https://codereview.chromium.org/2831113002/diff/1/tools/metrics/actions/actio... tools/metrics/actions/actions.xml:10980: <owner>galinap@google.com</owner> On 2017/04/24 13:13:21, Michael van Ouwerkerk wrote: > On 2017/04/20 18:10:24, dgn wrote: > > mvanouwerkerk@: do you want to be owner for that? > > Sure. Done. https://codereview.chromium.org/2831113002/diff/1/tools/metrics/actions/actio... tools/metrics/actions/actions.xml:10982: Android: Context menu is shown after a long click on an item in the New Tab On 2017/04/20 18:10:24, dgn wrote: > Don't make the description overly specific to the new tab page. Chrome Home is a > UI surface that features these items out of the context of a NTP Done. https://codereview.chromium.org/2831113002/diff/1/tools/metrics/actions/actio... tools/metrics/actions/actions.xml:10990: Andorid: User clicked to download an item from the New Tab Page. On 2017/04/20 18:10:24, dgn wrote: > typo on "Android" Done. https://codereview.chromium.org/2831113002/diff/1/tools/metrics/actions/actio... tools/metrics/actions/actions.xml:11027: Android: User clicked to remove an item from the New Tab Page. On 2017/04/24 14:06:38, dgn wrote: > On 2017/04/24 13:13:22, Michael van Ouwerkerk wrote: > > It would be nice if we could also track dismissals / removals from swipes. > > Should that be a tracked separately from dismissal by context menu? > > I think tracking what part of the UI is used for which result would be nice, so > yes, I would prefer tracking dismissals from swipes separately. Right, I'll do that, too.
https://codereview.chromium.org/2831113002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ntp/ContextMenuManager.java (right): https://codereview.chromium.org/2831113002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/ContextMenuManager.java:191: RecordUserAction.record("NewTabPage_OpenItemInNewWindow"); On 2017/04/24 15:13:49, Galia wrote: > On 2017/04/24 13:13:21, Michael van Ouwerkerk wrote: > > On 2017/04/20 18:10:24, dgn wrote: > > > I'd use ContentSuggestions.ContextMenu.OpenItemInNewWindow, and similar for > > > strings below. > > > > > > bauerb@, mvanouwerkerk@, wdyt? the current UMA is all scoped to NewTabPage > so > > > this is consistent, but it's unlikely that we will rename user actions. I > > would > > > prefer not to add new UMA with inaccurate names. > > > > I think ContentSuggestions is also not quite correct for a Tile, is that > > considered content? They're mostly top level site urls. How about > > Suggestions.ContextMenu.OpenItemInNewWindow ? > > It's true that "Suggestions" is more precise for this case, but this introduces > more inconsistency as everywhere in the other user actions is used > "_ContentSuggestions_smth" (underscores as well). > Should I name it "Suggestions. ..." anyway? @dgn, @bauerb? Suggestions.foo lgtm. We have to start fixing our metrics somewhere.
PTAL
lgtm thanks!
On 2017/04/24 15:49:34, Galia wrote: > PTAL Thanks! lgtm
The CQ bit was checked by galinap@google.com
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by galinap@google.com
dgn@chromium.org changed reviewers: + asvitkine@chromium.org
asvitkine@: PTAL at the new additions to actions.xml
lgtm
The CQ bit was checked by galinap@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from dgn@chromium.org, mvanouwerkerk@chromium.org Link to the patchset: https://codereview.chromium.org/2831113002/#ps40001 (title: ""Refactor"")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1493108688037080,
"parent_rev": "58197789bfafee408d8fc728c0ed2f1da81bbbed", "commit_rev":
"20a75f52c468e41ecc06a80b3cd7486b203c00a7"}
Message was sent while issue was closed.
Description was changed from ========== [Suggestions] Add User action UMA for context menu actions Added recording of user actions for all context menu actions: - open context menu - open item in new tab - open item in new window - open item in an incognito tab - download item - remove item - undo removal of item BUG=713145 ========== to ========== [Suggestions] Add User action UMA for context menu actions Added recording of user actions for all context menu actions: - open context menu - open item in new tab - open item in new window - open item in an incognito tab - download item - remove item - undo removal of item BUG=713145 Review-Url: https://codereview.chromium.org/2831113002 Cr-Commit-Position: refs/heads/master@{#466932} Committed: https://chromium.googlesource.com/chromium/src/+/20a75f52c468e41ecc06a80b3cd7... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/20a75f52c468e41ecc06a80b3cd7... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
