Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2846233003:
📰 Record user actions on the NTP and Home sheet (Closed)
Description[Suggestions] Record user actions on the NTP and Home sheet
Added user actions:
- Suggestions.Card.ActionTapped
- Suggestions.Card.SwipedAway
- Suggestions.Card.Tapped
- Suggestions.Category.Dismissed
- Suggestions.Category.Fetch
- Suggestions.Category.ViewAll
- Suggestions.Content.Dismissed
- Suggestions.Content.Opened
- Suggestions.Site.RemovalUndone
- Suggestions.Site.Removed
- Suggestions.SurfaceHidden
- Suggestions.SurfaceVisible
- Suggestions.Tile.Tapped
BUG=693593
Review-Url: https://codereview.chromium.org/2846233003
Cr-Commit-Position: refs/heads/master@{#470358}
Committed: https://chromium.googlesource.com/chromium/src/+/f6500c14fa6592426db531673f484877cacdd651
Patch Set 1 #
Total comments: 19
Patch Set 2 : Address comments, rebase, add action.xml changes #Patch Set 3 : address more comments, add missing actions #Patch Set 4 : Add Suggestions.SurfaceHidden for ChromeHome #Patch Set 5 : rebase, update actions.xml #Patch Set 6 : Ready for actual review #
Total comments: 10
Patch Set 7 : rebase and address comments #
Total comments: 1
Patch Set 8 : rename Actions #Patch Set 9 : Address bauerb@'s comment #
Total comments: 3
Dependent Patchsets: Messages
Total messages: 57 (33 generated)
Description was changed from ========== [Suggestions] Record user actions on the NTP and Home sheet BUG=693593 ========== to ========== [Suggestions] Record user actions on the NTP and Home sheet BUG=693593 ==========
The CQ bit was checked by dgn@chromium.org 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...
dgn@chromium.org changed reviewers: + bauerb@chromium.org, treib@chromium.org
Still draft-ish, didn't pre-review it yet, but I have a question: https://codereview.chromium.org/2846233003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/suggestions/SuggestionsMetrics.java (right): https://codereview.chromium.org/2846233003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/suggestions/SuggestionsMetrics.java:17: public static void recordSurfaceOpened() { Q: Should I record all possible ways to get back to the surface? Including app and tab switching?
https://codereview.chromium.org/2846233003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageUma.java (right): https://codereview.chromium.org/2846233003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageUma.java:368: // initiated visit anywhere instead of only in the NTP. Hm yes, I guess that metric just doesn't make sense anymore? https://codereview.chromium.org/2846233003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/suggestions/SuggestionsEventReporterBridge.java (right): https://codereview.chromium.org/2846233003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/suggestions/SuggestionsEventReporterBridge.java:20: nativeOnSurfaceOpened(); // TODO(dgn): we would end up calling that very often! WAI? Talk to jkrcal? The interpretation of this event might have to change. https://codereview.chromium.org/2846233003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/suggestions/SuggestionsMetrics.java (right): https://codereview.chromium.org/2846233003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/suggestions/SuggestionsMetrics.java:17: public static void recordSurfaceOpened() { On 2017/04/28 14:31:40, dgn wrote: > Q: Should I record all possible ways to get back to the surface? Including app > and tab switching? Sounds like that might be useful...? Probably as different user actions though? https://codereview.chromium.org/2846233003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/suggestions/SuggestionsMetrics.java:44: RecordUserAction.record("Suggestions.Tile.RemovalUndone"); What's the difference between Site and Tile? https://codereview.chromium.org/2846233003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/suggestions/SuggestionsMetrics.java:53: } Hm, most of these events are passed to C++ anyway, where we record histograms for them, right? Shouldn't we record the user actions in the same place? Otherwise iOS will have to duplicate them.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
jkrcal@chromium.org changed reviewers: + jkrcal@chromium.org
https://codereview.chromium.org/2846233003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/suggestions/SuggestionsEventReporterBridge.java (right): https://codereview.chromium.org/2846233003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/suggestions/SuggestionsEventReporterBridge.java:20: nativeOnSurfaceOpened(); // TODO(dgn): we would end up calling that very often! WAI? On 2017/04/28 15:12:05, Marc Treib wrote: > Talk to jkrcal? The interpretation of this event might have to change. I think this is WAI (if I get it right that this event goes to the scheduler). This is what comes immediately before the user has the chance to see suggestions. We need to make sure these are not too old.
https://codereview.chromium.org/2846233003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageUma.java (right): https://codereview.chromium.org/2846233003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageUma.java:368: // initiated visit anywhere instead of only in the NTP. On 2017/04/28 15:12:05, Marc Treib wrote: > Hm yes, I guess that metric just doesn't make sense anymore? I can restrict that to non chrome home, making its scope obvious and avoiding to record the rare cases where this happens on ChromeHome enabled profiles. https://codereview.chromium.org/2846233003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/suggestions/SuggestionsEventReporterBridge.java (right): https://codereview.chromium.org/2846233003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/suggestions/SuggestionsEventReporterBridge.java:20: nativeOnSurfaceOpened(); // TODO(dgn): we would end up calling that very often! WAI? On 2017/05/02 08:01:50, jkrcal wrote: > On 2017/04/28 15:12:05, Marc Treib wrote: > > Talk to jkrcal? The interpretation of this event might have to change. > > I think this is WAI (if I get it right that this event goes to the scheduler). > This is what comes immediately before the user has the chance to see > suggestions. We need to make sure these are not too old. Acknowledged. https://codereview.chromium.org/2846233003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/suggestions/SuggestionsMetrics.java (right): https://codereview.chromium.org/2846233003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/suggestions/SuggestionsMetrics.java:17: public static void recordSurfaceOpened() { On 2017/04/28 15:12:05, Marc Treib wrote: > On 2017/04/28 14:31:40, dgn wrote: > > Q: Should I record all possible ways to get back to the surface? Including app > > and tab switching? > > Sounds like that might be useful...? Probably as different user actions though? We already have events for that: MobileComeToForeground, MobileToolbarShowStackView, MobileTabSwitched. I was mostly wondering if we should have the Suggestions.SurfaceOpened be used as a basic "we can see all the suggestion events as children of this" kind of event. Maybe is should be more like "Suggestions.SwitchedIn" kind of event, paired to "Suggestions.SwitchedOut", that would allow seeing things like tab closing navigations, bottomsheet close etc. resulting in users closing the zine UI. So we might consider that a follow up "feature", once we start looking a action sequences and decide we need more detail somewhere. https://codereview.chromium.org/2846233003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/suggestions/SuggestionsMetrics.java:44: RecordUserAction.record("Suggestions.Tile.RemovalUndone"); On 2017/04/28 15:12:05, Marc Treib wrote: > What's the difference between Site and Tile? I'm trying to distinguish the UI elements from the data being affected. For example dismissing a card can result in dismissing a single suggestion or the whole section. Trying to have the same thing with Tiles, but we only show a single thing there at the moment, so they are basically the same. https://codereview.chromium.org/2846233003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/suggestions/SuggestionsMetrics.java:53: } On 2017/04/28 15:12:05, Marc Treib wrote: > Hm, most of these events are passed to C++ anyway, where we record histograms > for them, right? Shouldn't we record the user actions in the same place? > Otherwise iOS will have to duplicate them. For the second half of these event, yes. Moving them to c++ then.
https://codereview.chromium.org/2846233003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageUma.java (right): https://codereview.chromium.org/2846233003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageUma.java:368: // initiated visit anywhere instead of only in the NTP. On 2017/05/02 13:43:20, dgn wrote: > On 2017/04/28 15:12:05, Marc Treib wrote: > > Hm yes, I guess that metric just doesn't make sense anymore? > > I can restrict that to non chrome home, making its scope obvious and avoiding to > record the rare cases where this happens on ChromeHome enabled profiles. Sure, sgtm. https://codereview.chromium.org/2846233003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/suggestions/SuggestionsMetrics.java (right): https://codereview.chromium.org/2846233003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/suggestions/SuggestionsMetrics.java:17: public static void recordSurfaceOpened() { On 2017/05/02 13:43:20, dgn wrote: > On 2017/04/28 15:12:05, Marc Treib wrote: > > On 2017/04/28 14:31:40, dgn wrote: > > > Q: Should I record all possible ways to get back to the surface? Including > app > > > and tab switching? > > > > Sounds like that might be useful...? Probably as different user actions > though? > > We already have events for that: MobileComeToForeground, > MobileToolbarShowStackView, MobileTabSwitched. > > I was mostly wondering if we should have the Suggestions.SurfaceOpened be used > as a basic "we can see all the suggestion events as children of this" kind of > event. Maybe is should be more like "Suggestions.SwitchedIn" kind of event, > paired to "Suggestions.SwitchedOut", that would allow seeing things like tab > closing navigations, bottomsheet close etc. resulting in users closing the zine > UI. That does sound useful. > So we might consider that a follow up "feature", once we start looking a action > sequences and decide we need more detail somewhere. At the time we notice we need this, it'll probably be too late to add it. So I'd say, if you see a reasonable chance that we'll want those actions, then add them sooner rather than later. https://codereview.chromium.org/2846233003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/suggestions/SuggestionsMetrics.java:44: RecordUserAction.record("Suggestions.Tile.RemovalUndone"); On 2017/05/02 13:43:20, dgn wrote: > On 2017/04/28 15:12:05, Marc Treib wrote: > > What's the difference between Site and Tile? > > I'm trying to distinguish the UI elements from the data being affected. For > example dismissing a card can result in dismissing a single suggestion or the > whole section. Trying to have the same thing with Tiles, but we only show a > single thing there at the moment, so they are basically the same. I'm still confused. So blacklisting a tile will give "Site.Removed", and undoing it will give "Tile.RemovalUndone"? Why?
Description was changed from ========== [Suggestions] Record user actions on the NTP and Home sheet BUG=693593 ========== to ========== [Suggestions] Record user actions on the NTP and Home sheet Added user actions: - Suggestions.Action.Fetch - Suggestions.Action.ViewAll - Suggestions.Card.ActionTapped - Suggestions.Card.SwipedAway - Suggestions.Card.Tapped - Suggestions.Category.Dismissed - Suggestions.Content.Dismissed - Suggestions.Site.RemovalUndone - Suggestions.Site.Removed - Suggestions.SurfaceOpened - Suggestions.Tile.Tapped BUG=693593 ==========
Description was changed from ========== [Suggestions] Record user actions on the NTP and Home sheet Added user actions: - Suggestions.Action.Fetch - Suggestions.Action.ViewAll - Suggestions.Card.ActionTapped - Suggestions.Card.SwipedAway - Suggestions.Card.Tapped - Suggestions.Category.Dismissed - Suggestions.Content.Dismissed - Suggestions.Site.RemovalUndone - Suggestions.Site.Removed - Suggestions.SurfaceOpened - Suggestions.Tile.Tapped BUG=693593 ========== to ========== [Suggestions] Record user actions on the NTP and Home sheet Added user actions: - Suggestions.Action.Fetch - Suggestions.Action.ViewAll - Suggestions.Card.ActionTapped - Suggestions.Card.SwipedAway - Suggestions.Card.Tapped - Suggestions.Category.Dismissed - Suggestions.Content.Opened - Suggestions.Content.Dismissed - Suggestions.Site.RemovalUndone - Suggestions.Site.Removed - Suggestions.SurfaceOpened - Suggestions.Tile.Tapped BUG=693593 ==========
https://codereview.chromium.org/2846233003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/suggestions/SuggestionsMetrics.java (right): https://codereview.chromium.org/2846233003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/suggestions/SuggestionsMetrics.java:17: public static void recordSurfaceOpened() { On 2017/05/02 at 14:03:46, Marc Treib wrote: > On 2017/05/02 13:43:20, dgn wrote: > > On 2017/04/28 15:12:05, Marc Treib wrote: > > > On 2017/04/28 14:31:40, dgn wrote: > > > > Q: Should I record all possible ways to get back to the surface? Including > > app > > > > and tab switching? > > > > > > Sounds like that might be useful...? Probably as different user actions > > though? > > > > We already have events for that: MobileComeToForeground, > > MobileToolbarShowStackView, MobileTabSwitched. > > > > I was mostly wondering if we should have the Suggestions.SurfaceOpened be used > > as a basic "we can see all the suggestion events as children of this" kind of > > event. Maybe is should be more like "Suggestions.SwitchedIn" kind of event, > > paired to "Suggestions.SwitchedOut", that would allow seeing things like tab > > closing navigations, bottomsheet close etc. resulting in users closing the zine > > UI. > > That does sound useful. > > > So we might consider that a follow up "feature", once we start looking a action > > sequences and decide we need more detail somewhere. > > At the time we notice we need this, it'll probably be too late to add it. So I'd say, if you see a reasonable chance that we'll want those actions, then add them sooner rather than later. My 2 cents: Yes, having a SwitchedIn/Out action is indeed a good idea and we should have it from the beginning on. Actually I would extend it to SwitchedIn{Half|Full}/Out. The SwitchedIn* events should behave like the old MobileNTPShown action. They should get recorded whenever the suggestion surface becomes visible in any way. With that we can easily answer questions like: - What do users usually do when they see the suggestion surface? - What have user usually done to reach our surface? etc etc
The CQ bit was checked by dgn@chromium.org 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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by dgn@chromium.org to run a CQ dry run
https://codereview.chromium.org/2846233003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageUma.java (right): https://codereview.chromium.org/2846233003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageUma.java:368: // initiated visit anywhere instead of only in the NTP. On 2017/05/02 14:03:46, Marc Treib wrote: > On 2017/05/02 13:43:20, dgn wrote: > > On 2017/04/28 15:12:05, Marc Treib wrote: > > > Hm yes, I guess that metric just doesn't make sense anymore? > > > > I can restrict that to non chrome home, making its scope obvious and avoiding > to > > record the rare cases where this happens on ChromeHome enabled profiles. > > Sure, sgtm. Done. https://codereview.chromium.org/2846233003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/suggestions/SuggestionsMetrics.java (right): https://codereview.chromium.org/2846233003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/suggestions/SuggestionsMetrics.java:17: public static void recordSurfaceOpened() { On 2017/05/02 16:00:06, finkm wrote: > On 2017/05/02 at 14:03:46, Marc Treib wrote: > > On 2017/05/02 13:43:20, dgn wrote: > > > On 2017/04/28 15:12:05, Marc Treib wrote: > > > > On 2017/04/28 14:31:40, dgn wrote: > > > > > Q: Should I record all possible ways to get back to the surface? > Including > > > app > > > > > and tab switching? > > > > > > > > Sounds like that might be useful...? Probably as different user actions > > > though? > > > > > > We already have events for that: MobileComeToForeground, > > > MobileToolbarShowStackView, MobileTabSwitched. > > > > > > I was mostly wondering if we should have the Suggestions.SurfaceOpened be > used > > > as a basic "we can see all the suggestion events as children of this" kind > of > > > event. Maybe is should be more like "Suggestions.SwitchedIn" kind of event, > > > paired to "Suggestions.SwitchedOut", that would allow seeing things like tab > > > closing navigations, bottomsheet close etc. resulting in users closing the > zine > > > UI. > > > > That does sound useful. > > > > > So we might consider that a follow up "feature", once we start looking a > action > > > sequences and decide we need more detail somewhere. > > > > At the time we notice we need this, it'll probably be too late to add it. So > I'd say, if you see a reasonable chance that we'll want those actions, then add > them sooner rather than later. > > My 2 cents: > Yes, having a SwitchedIn/Out action is indeed a good idea and we should have it > from the beginning on. Actually I would extend it to SwitchedIn{Half|Full}/Out. > The SwitchedIn* events should behave like the old MobileNTPShown action. They > should get recorded whenever the suggestion surface becomes visible in any way. > With that we can easily answer questions like: > - What do users usually do when they see the suggestion surface? > - What have user usually done to reach our surface? > etc etc I'm a bit confused about SwitchedInHalf and SwitchedInFull. We already have Android.ChromeHome.{Half|Full}State logged when the state of the bottom sheet changes. Would that cover what you were thinking about? I will add "universal" SwitchedIn/Out to all ways to open/close the Zine UI in the next patch set. https://codereview.chromium.org/2846233003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/suggestions/SuggestionsMetrics.java:44: RecordUserAction.record("Suggestions.Tile.RemovalUndone"); On 2017/05/02 14:03:46, Marc Treib wrote: > On 2017/05/02 13:43:20, dgn wrote: > > On 2017/04/28 15:12:05, Marc Treib wrote: > > > What's the difference between Site and Tile? > > > > I'm trying to distinguish the UI elements from the data being affected. For > > example dismissing a card can result in dismissing a single suggestion or the > > whole section. Trying to have the same thing with Tiles, but we only show a > > single thing there at the moment, so they are basically the same. > > I'm still confused. So blacklisting a tile will give "Site.Removed", and undoing > it will give "Tile.RemovalUndone"? Why? Ah the name difference here was due to resusing an existing action. Will mark it obsolete and replace with a consistent naming.
Dry run: 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 commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/05/02 at 20:32:52, dgn wrote: > https://codereview.chromium.org/2846233003/diff/1/chrome/android/java/src/org... > File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageUma.java (right): > > https://codereview.chromium.org/2846233003/diff/1/chrome/android/java/src/org... > chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageUma.java:368: // initiated visit anywhere instead of only in the NTP. > On 2017/05/02 14:03:46, Marc Treib wrote: > > On 2017/05/02 13:43:20, dgn wrote: > > > On 2017/04/28 15:12:05, Marc Treib wrote: > > > > Hm yes, I guess that metric just doesn't make sense anymore? > > > > > > I can restrict that to non chrome home, making its scope obvious and avoiding > > to > > > record the rare cases where this happens on ChromeHome enabled profiles. > > > > Sure, sgtm. > > Done. > > https://codereview.chromium.org/2846233003/diff/1/chrome/android/java/src/org... > File chrome/android/java/src/org/chromium/chrome/browser/suggestions/SuggestionsMetrics.java (right): > > https://codereview.chromium.org/2846233003/diff/1/chrome/android/java/src/org... > chrome/android/java/src/org/chromium/chrome/browser/suggestions/SuggestionsMetrics.java:17: public static void recordSurfaceOpened() { > On 2017/05/02 16:00:06, finkm wrote: > > On 2017/05/02 at 14:03:46, Marc Treib wrote: > > > On 2017/05/02 13:43:20, dgn wrote: > > > > On 2017/04/28 15:12:05, Marc Treib wrote: > > > > > On 2017/04/28 14:31:40, dgn wrote: > > > > > > Q: Should I record all possible ways to get back to the surface? > > Including > > > > app > > > > > > and tab switching? > > > > > > > > > > Sounds like that might be useful...? Probably as different user actions > > > > though? > > > > > > > > We already have events for that: MobileComeToForeground, > > > > MobileToolbarShowStackView, MobileTabSwitched. > > > > > > > > I was mostly wondering if we should have the Suggestions.SurfaceOpened be > > used > > > > as a basic "we can see all the suggestion events as children of this" kind > > of > > > > event. Maybe is should be more like "Suggestions.SwitchedIn" kind of event, > > > > paired to "Suggestions.SwitchedOut", that would allow seeing things like tab > > > > closing navigations, bottomsheet close etc. resulting in users closing the > > zine > > > > UI. > > > > > > That does sound useful. > > > > > > > So we might consider that a follow up "feature", once we start looking a > > action > > > > sequences and decide we need more detail somewhere. > > > > > > At the time we notice we need this, it'll probably be too late to add it. So > > I'd say, if you see a reasonable chance that we'll want those actions, then add > > them sooner rather than later. > > > > My 2 cents: > > Yes, having a SwitchedIn/Out action is indeed a good idea and we should have it > > from the beginning on. Actually I would extend it to SwitchedIn{Half|Full}/Out. > > The SwitchedIn* events should behave like the old MobileNTPShown action. They > > should get recorded whenever the suggestion surface becomes visible in any way. > > With that we can easily answer questions like: > > - What do users usually do when they see the suggestion surface? > > - What have user usually done to reach our surface? > > etc etc > > I'm a bit confused about SwitchedInHalf and SwitchedInFull. We already have Android.ChromeHome.{Half|Full}State logged when the state of the bottom sheet changes. Would that cover what you were thinking about? Correct, with the Android.ChromeHome.{Half|Full} we get the same information. But we have to look at a sequence of actions to come to the right result. It's much easier if we have actions which clearly mark the important events for us. Then we can just filter by these events and do not have to look at longer sequences. Let's say a user opens Chrome Home, leave Chromes and then comes back and clicks on a suggestion. It will look like this: Android.ChromeHome.HalfState Android.ChromeHome.FullState MobileGoToBackground MobileStartup.MainIntentReceived MobileComeToForeground NewTabPage_ContentSuggestions_ArticlesUsage If we now select Android.ChromeHome.FullState as our starting point of action sequences, we might miss that the user actually clicked on an article. It would be much easier if the sequence would like this: Android.ChromeHome.HalfState ContentSuggestions.HalfVisible Android.ChromeHome.FullyState ContentSuggestions.FullyVisible MobileGoToBackground MobileStartup.MainIntentReceived MobileComeToForeground ContentSuggestions.FullyVisible NewTabPage_ContentSuggestions_ArticlesUsage With that we select ContentSuggestions.FullyVisible as our starting point and would immediately see that the user once left Chrome when he saw our surface and once he clicked on it. Makes sense? Sames goes for ContentSuggestions.HalfVisible. This event helps us to figure out what users are doing when they have our surface half open. Do they swipe up? Do they click on a suggestion or do just leave? etc etc. > > I will add "universal" SwitchedIn/Out to all ways to open/close the Zine UI in the next patch set. > > https://codereview.chromium.org/2846233003/diff/1/chrome/android/java/src/org... > chrome/android/java/src/org/chromium/chrome/browser/suggestions/SuggestionsMetrics.java:44: RecordUserAction.record("Suggestions.Tile.RemovalUndone"); > On 2017/05/02 14:03:46, Marc Treib wrote: > > On 2017/05/02 13:43:20, dgn wrote: > > > On 2017/04/28 15:12:05, Marc Treib wrote: > > > > What's the difference between Site and Tile? > > > > > > I'm trying to distinguish the UI elements from the data being affected. For > > > example dismissing a card can result in dismissing a single suggestion or the > > > whole section. Trying to have the same thing with Tiles, but we only show a > > > single thing there at the moment, so they are basically the same. > > > > I'm still confused. So blacklisting a tile will give "Site.Removed", and undoing > > it will give "Tile.RemovalUndone"? Why? > > Ah the name difference here was due to resusing an existing action. Will mark it obsolete and replace with a consistent naming.
On 2017/05/03 10:43:00, finkm wrote:
> > I'm a bit confused about SwitchedInHalf and SwitchedInFull. We already have
> > Android.ChromeHome.{Half|Full}State logged when the state of the bottom
sheet
> > changes. Would that cover what you were thinking about?
>
> Correct, with the Android.ChromeHome.{Half|Full} we get the same information.
> But we have to look at a sequence of actions to come to the right result. It's
> much easier if we have actions which clearly mark the important events for us.
> Then we can just filter by these events and do not have to look at longer
> sequences.
>
> Let's say a user opens Chrome Home, leave Chromes and then comes back and
clicks
> on a suggestion. It will look like this:
>
>
> Android.ChromeHome.HalfState
> Android.ChromeHome.FullState
> MobileGoToBackground
> MobileStartup.MainIntentReceived
> MobileComeToForeground
> NewTabPage_ContentSuggestions_ArticlesUsage
>
> If we now select Android.ChromeHome.FullState as our starting point of action
> sequences, we might miss that the user actually clicked on an article. It
would
> be much easier if the sequence would like this:
>
> Android.ChromeHome.HalfState
> ContentSuggestions.HalfVisible
> Android.ChromeHome.FullyState
> ContentSuggestions.FullyVisible
> MobileGoToBackground
> MobileStartup.MainIntentReceived
> MobileComeToForeground
> ContentSuggestions.FullyVisible
> NewTabPage_ContentSuggestions_ArticlesUsage
>
> With that we select ContentSuggestions.FullyVisible as our starting point and
> would immediately see that the user once left Chrome when he saw our surface
and
> once he clicked on it. Makes sense?
> Sames goes for ContentSuggestions.HalfVisible. This event helps us to figure
out
> what users are doing when they have our surface half open. Do they swipe up?
Do
> they click on a suggestion or do just leave? etc etc.
Agreed with all the above. What I was suggesting though was to have a single
Suggestions.SurfaceVisible that would make the sequence be:
Android.ChromeHome.Opened
Suggestions.SurfaceVisible
Android.ChromeHome.HalfState
Android.ChromeHome.FullState
MobileGoToBackground
Suggestions.SurfaceHidden
MobileStartup.MainIntentReceived
MobileComeToForeground
Suggestions.SurfaceVisible
NewTabPage_ContentSuggestions_ArticlesUsage
Pros:
+ would capture all the "switch in" events under a single one, allowing for an
easy to use exploration starting point
+ would work across Chrome Home and non Chrome home
+ would still allow checking the state of the sheet by looking at the
Android.ChromeHome[Half|Full]State event in the chain
Cons:
+ Checking the state of the sheet by looking at the
Android.ChromeHome[Half|Full]State event in the chain is not as easy as it might
be far up
+ Maybe we want to separate the ChromeHome and typical Chrome events?
Another option would be to have the Suggestions.SurfaceVisible events but also
the Suggestions.[Half|Fully]Visible ones. NTP mode would have MobileNTPShown as
equivalent. So we keep the notion of general start/end points in the event
sequence. WDYT?
On 2017/05/03 at 14:36:21, dgn wrote:
> On 2017/05/03 10:43:00, finkm wrote:
> > > I'm a bit confused about SwitchedInHalf and SwitchedInFull. We already
have
> > > Android.ChromeHome.{Half|Full}State logged when the state of the bottom
sheet
> > > changes. Would that cover what you were thinking about?
> >
> > Correct, with the Android.ChromeHome.{Half|Full} we get the same
information.
> > But we have to look at a sequence of actions to come to the right result.
It's
> > much easier if we have actions which clearly mark the important events for
us.
> > Then we can just filter by these events and do not have to look at longer
> > sequences.
> >
> > Let's say a user opens Chrome Home, leave Chromes and then comes back and
clicks
> > on a suggestion. It will look like this:
> >
> >
> > Android.ChromeHome.HalfState
> > Android.ChromeHome.FullState
> > MobileGoToBackground
> > MobileStartup.MainIntentReceived
> > MobileComeToForeground
> > NewTabPage_ContentSuggestions_ArticlesUsage
> >
> > If we now select Android.ChromeHome.FullState as our starting point of
action
> > sequences, we might miss that the user actually clicked on an article. It
would
> > be much easier if the sequence would like this:
> >
> > Android.ChromeHome.HalfState
> > ContentSuggestions.HalfVisible
> > Android.ChromeHome.FullyState
> > ContentSuggestions.FullyVisible
> > MobileGoToBackground
> > MobileStartup.MainIntentReceived
> > MobileComeToForeground
> > ContentSuggestions.FullyVisible
> > NewTabPage_ContentSuggestions_ArticlesUsage
> >
> > With that we select ContentSuggestions.FullyVisible as our starting point
and
> > would immediately see that the user once left Chrome when he saw our surface
and
> > once he clicked on it. Makes sense?
> > Sames goes for ContentSuggestions.HalfVisible. This event helps us to figure
out
> > what users are doing when they have our surface half open. Do they swipe up?
Do
> > they click on a suggestion or do just leave? etc etc.
>
> Agreed with all the above. What I was suggesting though was to have a single
Suggestions.SurfaceVisible that would make the sequence be:
>
> Android.ChromeHome.Opened
> Suggestions.SurfaceVisible
> Android.ChromeHome.HalfState
> Android.ChromeHome.FullState
> MobileGoToBackground
> Suggestions.SurfaceHidden
> MobileStartup.MainIntentReceived
> MobileComeToForeground
> Suggestions.SurfaceVisible
> NewTabPage_ContentSuggestions_ArticlesUsage
>
> Pros:
> + would capture all the "switch in" events under a single one, allowing for an
easy to use exploration starting point
> + would work across Chrome Home and non Chrome home
> + would still allow checking the state of the sheet by looking at the
Android.ChromeHome[Half|Full]State event in the chain
> Cons:
> + Checking the state of the sheet by looking at the
Android.ChromeHome[Half|Full]State event in the chain is not as easy as it might
be far up
> + Maybe we want to separate the ChromeHome and typical Chrome events?
>
> Another option would be to have the Suggestions.SurfaceVisible events but also
the Suggestions.[Half|Fully]Visible ones. NTP mode would have MobileNTPShown as
equivalent. So we keep the notion of general start/end points in the event
sequence. WDYT?
Yes, let's do it that way. It's definitely interesting how many users transition
from half to full. Because of that I don't want to rely on ChromeHome's actions.
Description was changed from ========== [Suggestions] Record user actions on the NTP and Home sheet Added user actions: - Suggestions.Action.Fetch - Suggestions.Action.ViewAll - Suggestions.Card.ActionTapped - Suggestions.Card.SwipedAway - Suggestions.Card.Tapped - Suggestions.Category.Dismissed - Suggestions.Content.Opened - Suggestions.Content.Dismissed - Suggestions.Site.RemovalUndone - Suggestions.Site.Removed - Suggestions.SurfaceOpened - Suggestions.Tile.Tapped BUG=693593 ========== to ========== [Suggestions] Record user actions on the NTP and Home sheet Added user actions: - Suggestions.Action.Fetch - Suggestions.Action.ViewAll - Suggestions.Card.ActionTapped - Suggestions.Card.SwipedAway - Suggestions.Card.Tapped - Suggestions.Category.Dismissed - Suggestions.Content.Dismissed - Suggestions.Content.Opened - Suggestions.Content.OpenedLocal - Suggestions.Site.RemovalUndone - Suggestions.Site.Removed - Suggestions.SurfaceHidden - Suggestions.SurfaceVisible - Suggestions.Tile.Tapped BUG=693593 ==========
On 2017/05/04 09:30:43, finkm wrote:
> On 2017/05/03 at 14:36:21, dgn wrote:
> > On 2017/05/03 10:43:00, finkm wrote:
> > > > I'm a bit confused about SwitchedInHalf and SwitchedInFull. We already
> have
> > > > Android.ChromeHome.{Half|Full}State logged when the state of the bottom
> sheet
> > > > changes. Would that cover what you were thinking about?
> > >
> > > Correct, with the Android.ChromeHome.{Half|Full} we get the same
> information.
> > > But we have to look at a sequence of actions to come to the right result.
> It's
> > > much easier if we have actions which clearly mark the important events for
> us.
> > > Then we can just filter by these events and do not have to look at longer
> > > sequences.
> > >
> > > Let's say a user opens Chrome Home, leave Chromes and then comes back and
> clicks
> > > on a suggestion. It will look like this:
> > >
> > >
> > > Android.ChromeHome.HalfState
> > > Android.ChromeHome.FullState
> > > MobileGoToBackground
> > > MobileStartup.MainIntentReceived
> > > MobileComeToForeground
> > > NewTabPage_ContentSuggestions_ArticlesUsage
> > >
> > > If we now select Android.ChromeHome.FullState as our starting point of
> action
> > > sequences, we might miss that the user actually clicked on an article. It
> would
> > > be much easier if the sequence would like this:
> > >
> > > Android.ChromeHome.HalfState
> > > ContentSuggestions.HalfVisible
> > > Android.ChromeHome.FullyState
> > > ContentSuggestions.FullyVisible
> > > MobileGoToBackground
> > > MobileStartup.MainIntentReceived
> > > MobileComeToForeground
> > > ContentSuggestions.FullyVisible
> > > NewTabPage_ContentSuggestions_ArticlesUsage
> > >
> > > With that we select ContentSuggestions.FullyVisible as our starting point
> and
> > > would immediately see that the user once left Chrome when he saw our
surface
> and
> > > once he clicked on it. Makes sense?
> > > Sames goes for ContentSuggestions.HalfVisible. This event helps us to
figure
> out
> > > what users are doing when they have our surface half open. Do they swipe
up?
> Do
> > > they click on a suggestion or do just leave? etc etc.
> >
> > Agreed with all the above. What I was suggesting though was to have a single
> Suggestions.SurfaceVisible that would make the sequence be:
> >
> > Android.ChromeHome.Opened
> > Suggestions.SurfaceVisible
> > Android.ChromeHome.HalfState
> > Android.ChromeHome.FullState
> > MobileGoToBackground
> > Suggestions.SurfaceHidden
> > MobileStartup.MainIntentReceived
> > MobileComeToForeground
> > Suggestions.SurfaceVisible
> > NewTabPage_ContentSuggestions_ArticlesUsage
> >
> > Pros:
> > + would capture all the "switch in" events under a single one, allowing for
an
> easy to use exploration starting point
> > + would work across Chrome Home and non Chrome home
> > + would still allow checking the state of the sheet by looking at the
> Android.ChromeHome[Half|Full]State event in the chain
> > Cons:
> > + Checking the state of the sheet by looking at the
> Android.ChromeHome[Half|Full]State event in the chain is not as easy as it
might
> be far up
> > + Maybe we want to separate the ChromeHome and typical Chrome events?
> >
> > Another option would be to have the Suggestions.SurfaceVisible events but
also
> the Suggestions.[Half|Fully]Visible ones. NTP mode would have MobileNTPShown
as
> equivalent. So we keep the notion of general start/end points in the event
> sequence. WDYT?
>
> Yes, let's do it that way. It's definitely interesting how many users
transition
> from half to full. Because of that I don't want to rely on ChromeHome's
actions.
I'm moving implementing the more detailed Visibility notifications to a
follow-up CL.
Description was changed from ========== [Suggestions] Record user actions on the NTP and Home sheet Added user actions: - Suggestions.Action.Fetch - Suggestions.Action.ViewAll - Suggestions.Card.ActionTapped - Suggestions.Card.SwipedAway - Suggestions.Card.Tapped - Suggestions.Category.Dismissed - Suggestions.Content.Dismissed - Suggestions.Content.Opened - Suggestions.Content.OpenedLocal - Suggestions.Site.RemovalUndone - Suggestions.Site.Removed - Suggestions.SurfaceHidden - Suggestions.SurfaceVisible - Suggestions.Tile.Tapped BUG=693593 ========== to ========== [Suggestions] Record user actions on the NTP and Home sheet Added user actions: - Suggestions.Action.Fetch - Suggestions.Action.ViewAll - Suggestions.Card.ActionTapped - Suggestions.Card.SwipedAway - Suggestions.Card.Tapped - Suggestions.Category.Dismissed - Suggestions.Content.Dismissed - Suggestions.Content.Opened - Suggestions.Site.RemovalUndone - Suggestions.Site.Removed - Suggestions.SurfaceHidden - Suggestions.SurfaceVisible - Suggestions.Tile.Tapped BUG=693593 ==========
PTAL
Here is a sequence of user actions with the interactions added in:
- Launch Chrome
ThirdPartyCookieBlockingDisabled
MobileStartup.MainIntentReceived
MobileNTPOpenedInNewTab
MobileTabSwitched
MobileNTPOpenedInNewTab
MobileComeToForeground
Android.ChromeHome.Opened
Suggestions.SurfaceVisible
NewTabPage_ContentSuggestions_ArticlesUsage
Android.ChromeHome.HalfState
MobilePageLoaded
- Close Sheet
Android.ChromeHome.Closed
Suggestions.SurfaceHidden
MobileSideSwipeFinished
- Change Tab
MobileTabSwitched
MobilePageLoaded
- Open Sheet Half
Android.ChromeHome.Opened
Suggestions.SurfaceVisible
Android.ChromeHome.HalfState
- Open Sheet Full
Android.ChromeHome.FullState
- Open Article in new Tab
Suggestions.ContextMenu.Shown
NewTabPage_ContentSuggestions_ArticlesUsage
Suggestions.Content.Opened
Suggestions.ContextMenu.OpenItemInNewTab
MobilePageLoaded
- Dismiss Card
Suggestions.Card.SwipedAway
Suggestions.Content.Dismissed
- Dismiss Category
Suggestions.Card.SwipedAway
Suggestions.Category.Dismissed
- Remove Tile
Suggestions.ContextMenu.Shown
Suggestions.Site.Removed
Suggestions.ContextMenu.RemoveItem
- Undo Remove Tile
Suggestions.Site.RemovalUndone
- Tap Tile
Suggestions.Tile.Tapped
MobileNTPMostVisited
Android.ChromeHome.Closed
Suggestions.SurfaceHidden
MobilePageLoaded
https://codereview.chromium.org/2846233003/diff/100001/chrome/android/java/sr...
File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java
(right):
https://codereview.chromium.org/2846233003/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:504:
private void recordNTPHidden() {
renamed to have match with recordNtpShown, as they also do similar things.
The CQ bit was checked by dgn@chromium.org 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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Friendly ping :)
Sorry for the delay, I missed that this is waiting for my input. C++ LGTM; I didn't think about the actual metrics too much, since you seem to have discussed those already. https://codereview.chromium.org/2846233003/diff/100001/components/ntp_snippet... File components/ntp_snippets/content_suggestions_metrics.cc (right): https://codereview.chromium.org/2846233003/diff/100001/components/ntp_snippet... components/ntp_snippets/content_suggestions_metrics.cc:380: base::RecordAction(base::UserMetricsAction("Suggestions.Action.Fetch")); I might have missed some discussion about naming, but this seems a bit weird. Is "Dismissed" not an action? Or is ".Action" only for "global" things, not tied to either a suggestion or a category? https://codereview.chromium.org/2846233003/diff/100001/components/ntp_snippet... File components/ntp_snippets/content_suggestions_metrics.h (right): https://codereview.chromium.org/2846233003/diff/100001/components/ntp_snippet... components/ntp_snippets/content_suggestions_metrics.h:63: // User Actions nit: I'd remove the comment - these are not the only things here that record user actions, and also it doesn't really matter to the caller anyway. https://codereview.chromium.org/2846233003/diff/100001/components/ntp_tiles/m... File components/ntp_tiles/most_visited_sites.cc (right): https://codereview.chromium.org/2846233003/diff/100001/components/ntp_tiles/m... components/ntp_tiles/most_visited_sites.cc:138: if (add_url) { nit: Maybe it's time to combine the now-three "if (add_url)"s?
bauerb@ PTAL https://codereview.chromium.org/2846233003/diff/100001/components/ntp_snippet... File components/ntp_snippets/content_suggestions_metrics.cc (right): https://codereview.chromium.org/2846233003/diff/100001/components/ntp_snippet... components/ntp_snippets/content_suggestions_metrics.cc:380: base::RecordAction(base::UserMetricsAction("Suggestions.Action.Fetch")); On 2017/05/05 16:05:39, Marc Treib wrote: > I might have missed some discussion about naming, but this seems a bit weird. Is > "Dismissed" not an action? Or is ".Action" only for "global" things, not tied to > either a suggestion or a category? Action is for the things that happen when tapping the action (MORE) button: Fetch or ViewAll. ViewAll is all client side so is not represented here at the moment, although I could do it and pipe through to Java... I guess I could rename it to Suggestions.Category.Fetch and Suggestions.Category.ViewAll ? https://codereview.chromium.org/2846233003/diff/100001/components/ntp_snippet... File components/ntp_snippets/content_suggestions_metrics.h (right): https://codereview.chromium.org/2846233003/diff/100001/components/ntp_snippet... components/ntp_snippets/content_suggestions_metrics.h:63: // User Actions On 2017/05/05 16:05:39, Marc Treib wrote: > nit: I'd remove the comment - these are not the only things here that record > user actions, and also it doesn't really matter to the caller anyway. Done. https://codereview.chromium.org/2846233003/diff/100001/components/ntp_tiles/m... File components/ntp_tiles/most_visited_sites.cc (right): https://codereview.chromium.org/2846233003/diff/100001/components/ntp_tiles/m... components/ntp_tiles/most_visited_sites.cc:138: if (add_url) { On 2017/05/05 16:05:39, Marc Treib wrote: > nit: Maybe it's time to combine the now-three "if (add_url)"s? Do you mind if I do that in a separate patch? I can split that in AddBlacklistedUrl and RemoveBlacklistedUrl, but I'd have to propagate it to java and the other place it's used to not look clunky
https://codereview.chromium.org/2846233003/diff/100001/components/ntp_snippet... File components/ntp_snippets/content_suggestions_metrics.cc (right): https://codereview.chromium.org/2846233003/diff/100001/components/ntp_snippet... components/ntp_snippets/content_suggestions_metrics.cc:380: base::RecordAction(base::UserMetricsAction("Suggestions.Action.Fetch")); On 2017/05/08 12:40:50, dgn wrote: > On 2017/05/05 16:05:39, Marc Treib wrote: > > I might have missed some discussion about naming, but this seems a bit weird. > Is > > "Dismissed" not an action? Or is ".Action" only for "global" things, not tied > to > > either a suggestion or a category? > > Action is for the things that happen when tapping the action (MORE) button: > Fetch or ViewAll. ViewAll is all client side so is not represented here at the > moment, although I could do it and pipe through to Java... > > I guess I could rename it to Suggestions.Category.Fetch and > Suggestions.Category.ViewAll ? That seems more consistent to me, but I'll leave it to you, or whoever else has thought about these metrics :) https://codereview.chromium.org/2846233003/diff/100001/components/ntp_tiles/m... File components/ntp_tiles/most_visited_sites.cc (right): https://codereview.chromium.org/2846233003/diff/100001/components/ntp_tiles/m... components/ntp_tiles/most_visited_sites.cc:138: if (add_url) { On 2017/05/08 12:40:50, dgn wrote: > On 2017/05/05 16:05:39, Marc Treib wrote: > > nit: Maybe it's time to combine the now-three "if (add_url)"s? > > Do you mind if I do that in a separate patch? I can split that in > AddBlacklistedUrl and RemoveBlacklistedUrl, but I'd have to propagate it to java > and the other place it's used to not look clunky I just meant to reorder things within this method: if (add_url) { RecordAction(..) if (top_sites_) AddBlacklistedUrl(..); if (mv_source_ == TileSource::SUGGESTIONS_SERVICE) ... } else ... But anyway, it's optional.
https://codereview.chromium.org/2846233003/diff/100001/components/ntp_tiles/m... File components/ntp_tiles/most_visited_sites.cc (right): https://codereview.chromium.org/2846233003/diff/100001/components/ntp_tiles/m... components/ntp_tiles/most_visited_sites.cc:138: if (add_url) { On 2017/05/08 12:46:20, Marc Treib wrote: > On 2017/05/08 12:40:50, dgn wrote: > > On 2017/05/05 16:05:39, Marc Treib wrote: > > > nit: Maybe it's time to combine the now-three "if (add_url)"s? > > > > Do you mind if I do that in a separate patch? I can split that in > > AddBlacklistedUrl and RemoveBlacklistedUrl, but I'd have to propagate it to > java > > and the other place it's used to not look clunky > > I just meant to reorder things within this method: > > if (add_url) { > RecordAction(..) > if (top_sites_) AddBlacklistedUrl(..); > if (mv_source_ == TileSource::SUGGESTIONS_SERVICE) ... > } else ... > > But anyway, it's optional. yeah but because of the style guide you have to add line returns and braces... then it gets ugly with nested ifs and 2 separate methods look better (IMO) but I'm trying to keep this CL focused and not touch unrelated lines. I'll do the change in another CL and if it's preferred I will just do the small change you recommend anyway.
LGTM with a small suggestion: https://codereview.chromium.org/2846233003/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/suggestions/SuggestionsBottomSheetContent.java (right): https://codereview.chromium.org/2846233003/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/suggestions/SuggestionsBottomSheetContent.java:107: if (mBottomSheet.isSheetOpen()) { Oh, so it can happen that we initialize the sheet in the opened state (and therefore we never see the state _transition_ in onSheetOpened)? Would it make sense to extract this to a method that we can also call from onSheetOpened()?
Description was changed from ========== [Suggestions] Record user actions on the NTP and Home sheet Added user actions: - Suggestions.Action.Fetch - Suggestions.Action.ViewAll - Suggestions.Card.ActionTapped - Suggestions.Card.SwipedAway - Suggestions.Card.Tapped - Suggestions.Category.Dismissed - Suggestions.Content.Dismissed - Suggestions.Content.Opened - Suggestions.Site.RemovalUndone - Suggestions.Site.Removed - Suggestions.SurfaceHidden - Suggestions.SurfaceVisible - Suggestions.Tile.Tapped BUG=693593 ========== to ========== [Suggestions] Record user actions on the NTP and Home sheet Added user actions: - Suggestions.Card.ActionTapped - Suggestions.Card.SwipedAway - Suggestions.Card.Tapped - Suggestions.Category.Dismissed - Suggestions.Category.Fetch - Suggestions.Category.ViewAll - Suggestions.Content.Dismissed - Suggestions.Content.Opened - Suggestions.Site.RemovalUndone - Suggestions.Site.Removed - Suggestions.SurfaceHidden - Suggestions.SurfaceVisible - Suggestions.Tile.Tapped BUG=693593 ==========
dgn@chromium.org changed reviewers: + asvitkine@chromium.org
Thanks! asvitkine@: PTAL at actions.xml
The CQ bit was checked by dgn@chromium.org 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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2846233003/diff/160001/tools/metrics/actions/... File tools/metrics/actions/actions.xml (right): https://codereview.chromium.org/2846233003/diff/160001/tools/metrics/actions/... tools/metrics/actions/actions.xml:15867: <description>User requested fetching more content suggestions.</description> Nit: Maybe mention how the user did this? i.e. describe the UI used
https://codereview.chromium.org/2846233003/diff/160001/tools/metrics/actions/... File tools/metrics/actions/actions.xml (right): https://codereview.chromium.org/2846233003/diff/160001/tools/metrics/actions/... tools/metrics/actions/actions.xml:15867: <description>User requested fetching more content suggestions.</description> On 2017/05/09 14:53:50, Alexei Svitkine (slow) wrote: > Nit: Maybe mention how the user did this? i.e. describe the UI used We currently have only one way to do that, and that would be captured by Suggestions.Card.ActionTapped, but we are planning to add another way by just scrolling to the bottom of the feed. So this is more like an indirect user action, and "how" is supposed to be indicated through the sequence. Is it okay as is or should I mention to look at the sequence for more details?
The CQ bit was checked by dgn@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from treib@chromium.org, bauerb@chromium.org Link to the patchset: https://codereview.chromium.org/2846233003/#ps160001 (title: "Address bauerb@'s comment")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2846233003/diff/160001/tools/metrics/actions/... File tools/metrics/actions/actions.xml (right): https://codereview.chromium.org/2846233003/diff/160001/tools/metrics/actions/... tools/metrics/actions/actions.xml:15867: <description>User requested fetching more content suggestions.</description> On 2017/05/09 15:27:11, dgn wrote: > On 2017/05/09 14:53:50, Alexei Svitkine (slow) wrote: > > Nit: Maybe mention how the user did this? i.e. describe the UI used > > We currently have only one way to do that, and that would be captured by > Suggestions.Card.ActionTapped, but we are planning to add another way by just > scrolling to the bottom of the feed. So this is more like an indirect user > action, and "how" is supposed to be indicated through the sequence. Is it okay > as is or should I mention to look at the sequence for more details? I think it would be good to mention this bit of context in the description and mention the other metric by name there too.
CQ is committing da patch.
Bot data: {"patchset_id": 160001, "attempt_start_ts": 1494348719755570,
"parent_rev": "6b4562e3bb6ed5dd0f5b4fd8f291d9d6f9c0650b", "commit_rev":
"f6500c14fa6592426db531673f484877cacdd651"}
Message was sent while issue was closed.
Description was changed from ========== [Suggestions] Record user actions on the NTP and Home sheet Added user actions: - Suggestions.Card.ActionTapped - Suggestions.Card.SwipedAway - Suggestions.Card.Tapped - Suggestions.Category.Dismissed - Suggestions.Category.Fetch - Suggestions.Category.ViewAll - Suggestions.Content.Dismissed - Suggestions.Content.Opened - Suggestions.Site.RemovalUndone - Suggestions.Site.Removed - Suggestions.SurfaceHidden - Suggestions.SurfaceVisible - Suggestions.Tile.Tapped BUG=693593 ========== to ========== [Suggestions] Record user actions on the NTP and Home sheet Added user actions: - Suggestions.Card.ActionTapped - Suggestions.Card.SwipedAway - Suggestions.Card.Tapped - Suggestions.Category.Dismissed - Suggestions.Category.Fetch - Suggestions.Category.ViewAll - Suggestions.Content.Dismissed - Suggestions.Content.Opened - Suggestions.Site.RemovalUndone - Suggestions.Site.Removed - Suggestions.SurfaceHidden - Suggestions.SurfaceVisible - Suggestions.Tile.Tapped BUG=693593 Review-Url: https://codereview.chromium.org/2846233003 Cr-Commit-Position: refs/heads/master@{#470358} Committed: https://chromium.googlesource.com/chromium/src/+/f6500c14fa6592426db531673f48... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001) as https://chromium.googlesource.com/chromium/src/+/f6500c14fa6592426db531673f48... |
