Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2528303003:
[NTP] Fixed usage of NewTabPage.ActionAndroid histogram (Closed)
|
Created:
4 years ago by finkm Modified:
3 years, 11 months ago CC:
agrieve+watch_chromium.org, asvitkine+watch_chromium.org, browser-components-watch_chromium.org, chromium-reviews, ntp-dev+reviews_chromium.org, tfarina, tschumann Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description- deprecated histogram NewTabPage.ActionAndroid and replaced it with
NewTabPage.ActionAndroid2
- replaced action MobileNTPBookmark with MobileBookmarkManagerEntryOpened
- replaced action MobileNTPForeignSession with MobileRecentTabManagerTabFromOtherDeviceOpened
- replaced action MobileNTPRecentlyClosed with MobileRecentTabManagerRecentTabOpened
For details please refer to the linked bug.
BUG=662327
Review-Url: https://codereview.chromium.org/2528303003
Cr-Commit-Position: refs/heads/master@{#442599}
Committed: https://chromium.googlesource.com/chromium/src/+/422ecccc1f41cb2c5ec5632727a8265b40a5d1d2
Patch Set 1 #Patch Set 2 : Fixed the same issue for RecentTabs #Patch Set 3 : Rebase #
Total comments: 2
Patch Set 4 : Addressing comments #
Total comments: 6
Patch Set 5 : Addressing comments #Patch Set 6 : Addressing comments #
Total comments: 20
Patch Set 7 : Deprecated histogram NewTabPage.ActionAndroid #Patch Set 8 : Adjusted action names and descriptions #
Total comments: 2
Patch Set 9 : Changed order of UMA calls in ChromeTabbedActivity.java #
Total comments: 6
Patch Set 10 : Addressing comments #Patch Set 11 : Rebase #Messages
Total messages: 57 (36 generated)
Description was changed from ========== - deprecated action MobileNTPBookmark - repaced MobileNTPBookmark with MobileBookmarkOpen where applicable - fixed usage of NewTabPageUma.ACTION_OPENED_BOOKMARK BUG=662327 ========== to ========== - deprecated action MobileNTPBookmark - replaced MobileNTPBookmark with MobileBookmarkOpen where applicable - introduced action MobileForeignSessionOpen and MobileRecentlyClosedTabOpen - fixed usage of NewTabPageUma.ACTION_OPENED_BOOKMARK - fixed usage of NewTabPageUma.ACTION_OPENED_FOREIGN_SESSION BUG=662327 ==========
finkm@chromium.org changed reviewers: + bauerb@chromium.org, dgn@chromium.org, mpearson@chromium.org
Patchset #3 (id:40001) has been deleted
Description was changed from ========== - deprecated action MobileNTPBookmark - replaced MobileNTPBookmark with MobileBookmarkOpen where applicable - introduced action MobileForeignSessionOpen and MobileRecentlyClosedTabOpen - fixed usage of NewTabPageUma.ACTION_OPENED_BOOKMARK - fixed usage of NewTabPageUma.ACTION_OPENED_FOREIGN_SESSION BUG=662327 ========== to ========== - deprecated action MobileNTPBookmark - replaced MobileNTPBookmark with MobileBookmarkOpen where applicable - introduced action MobileForeignSessionOpen and MobileRecentlyClosedTabOpen - fixed usage of NewTabPageUma.ACTION_OPENED_BOOKMARK - fixed usage of NewTabPageUma.ACTION_OPENED_FOREIGN_SESSION For details please refer to the linked bug. BUG=662327 ==========
PTAL! Thx!
LGTM with a pair of nits: https://codereview.chromium.org/2528303003/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java (right): https://codereview.chromium.org/2528303003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java:1154: if (NewTabPage.isNTPUrl(currentTab.getUrl())) { Can you move this to line 1164 so it's grouped with the user metrics recording call? https://codereview.chromium.org/2528303003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java:1170: } Same here.
On 2016/12/21 at 14:14:03, bauerb wrote: > LGTM with a pair of nits: > > https://codereview.chromium.org/2528303003/diff/60001/chrome/android/java/src... > File chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java (right): > > https://codereview.chromium.org/2528303003/diff/60001/chrome/android/java/src... > chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java:1154: if (NewTabPage.isNTPUrl(currentTab.getUrl())) { > Can you move this to line 1164 so it's grouped with the user metrics recording call? I moved the metrics recording call up because I'm not sure if currentTab.getUrl() changes until line 1164. > > https://codereview.chromium.org/2528303003/diff/60001/chrome/android/java/src... > chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java:1170: } > Same here. Same here.
I only looked at actions.xml; please let me know if you wanted me to review more code. (You didn't make it clear in your original request.) --mark https://codereview.chromium.org/2528303003/diff/80001/tools/metrics/actions/a... File tools/metrics/actions/actions.xml (right): https://codereview.chromium.org/2528303003/diff/80001/tools/metrics/actions/a... tools/metrics/actions/actions.xml:9205: Action indicating the user has opened a bookmark from the bookmark manager. optional nit (general advice; I see it doesn't agree with some other mobile bookmark action descriptions though is a relatively common practice throughout this file): omit unnecessary words/phrases, e.g., drop "Action indicating" or maybe even "Action indicating the user has" https://codereview.chromium.org/2528303003/diff/80001/tools/metrics/actions/a... tools/metrics/actions/actions.xml:9413: Action indicating the user has opened a foreign session from the recent tabs nit: "foreign session" is generally only a term that member of the team know. The broader chromium community thinks of these as tabs from other devices. For better discoverability and understandability, I'd prefer you choose a better name such as MobileOpenTabFromOtherDevice or MobileTabFromOtherDeviceOpen. Even if you decide not to change the name, the description should mention "other devices" because I can easily imagine people saying, I hope we log this, and going to actions.xml and searching and hoping to find something. https://codereview.chromium.org/2528303003/diff/80001/tools/metrics/actions/a... tools/metrics/actions/actions.xml:9709: <obsolete>Deprecated as of 11/2016</obsolete> Please check to see whether this action is emitted on iOS.
@mpearson: Yes, only actions.xml please. Sorry, I forgot to mention that. PTAL. https://codereview.chromium.org/2528303003/diff/80001/tools/metrics/actions/a... File tools/metrics/actions/actions.xml (right): https://codereview.chromium.org/2528303003/diff/80001/tools/metrics/actions/a... tools/metrics/actions/actions.xml:9205: Action indicating the user has opened a bookmark from the bookmark manager. On 2016/12/21 at 18:13:13, Mark P wrote: > optional nit (general advice; I see it doesn't agree with some other mobile bookmark action descriptions though is a relatively common practice throughout this file): omit unnecessary words/phrases, e.g., drop "Action indicating" or maybe even "Action indicating the user has" Done https://codereview.chromium.org/2528303003/diff/80001/tools/metrics/actions/a... tools/metrics/actions/actions.xml:9413: Action indicating the user has opened a foreign session from the recent tabs On 2016/12/21 at 18:13:13, Mark P wrote: > nit: "foreign session" is generally only a term that member of the team know. The broader chromium community thinks of these as tabs from other devices. For better discoverability and understandability, I'd prefer you choose a better name such as MobileOpenTabFromOtherDevice or MobileTabFromOtherDeviceOpen. > > Even if you decide not to change the name, the description should mention "other devices" because I can easily imagine people saying, I hope we log this, and going to actions.xml and searching and hoping to find something. Done https://codereview.chromium.org/2528303003/diff/80001/tools/metrics/actions/a... tools/metrics/actions/actions.xml:9709: <obsolete>Deprecated as of 11/2016</obsolete> On 2016/12/21 at 18:13:13, Mark P wrote: > Please check to see whether this action is emitted on iOS. You're right. It's still used in iOS. I removed the obsolete tag.
https://codereview.chromium.org/2528303003/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkUtils.java (right): https://codereview.chromium.org/2528303003/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkUtils.java:232: RecordUserAction.record("MobileBookmarkOpen"); FYI: this is called only from the bookmark manager, it will not trigger when bookmarks from the NTP are opened. Also, the Action description actually matched its usage. https://codereview.chromium.org/2528303003/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java (right): https://codereview.chromium.org/2528303003/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:354: NewTabPageUma.recordAction(NewTabPageUma.ACTION_OPENED_BOOKMARK); ACTION_OPENED_BOOKMARK => MobileNTPBookmark => Description = "Action indicating the user has opened a bookmark from the bookmark manager." This will be called when the user opens the bookmark manager. I don't think it's the right action to record here. If what you are looking for is tracking bookmarks being opened, this happens here: https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr... We only record NewTabPageUma.ACTION_OPENED_SNIPPET for all of the categories it seems. https://codereview.chromium.org/2528303003/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:356: case KnownCategories.FOREIGN_TABS: "Foreign Tabs" feels like it should not be associated with ACTION_OPENED_RECENTLY_CLOSED_ENTRY (="MobileNTPRecentlyClosed") as action. There is also no description for it. Can you please update the description and owner for Metrics we care about? https://codereview.chromium.org/2528303003/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java (right): https://codereview.chromium.org/2528303003/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:513: NewTabPageUma.recordAction(NewTabPageUma.ACTION_OPENED_BOOKMARK); same as above file, the action description does not match with how it's being recorded. https://codereview.chromium.org/2528303003/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/RecentTabsManager.java (left): https://codereview.chromium.org/2528303003/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/RecentTabsManager.java:213: NewTabPageUma.recordAction(NewTabPageUma.ACTION_OPENED_FOREIGN_SESSION); Isn't that constant now unused? It could be removed then. https://codereview.chromium.org/2528303003/diff/120001/tools/metrics/actions/... File tools/metrics/actions/actions.xml (right): https://codereview.chromium.org/2528303003/diff/120001/tools/metrics/actions/... tools/metrics/actions/actions.xml:9931: Action indicating the user has opened a recently closed tab from the recent drop "Action indicating..."
https://codereview.chromium.org/2528303003/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkUtils.java (right): https://codereview.chromium.org/2528303003/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkUtils.java:232: RecordUserAction.record("MobileBookmarkOpen"); On 2016/12/22 at 12:31:22, dgn wrote: > FYI: this is called only from the bookmark manager, it will not trigger when bookmarks from the NTP are opened. Also, the Action description actually matched its usage. Yep. This is intentional. https://codereview.chromium.org/2528303003/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java (right): https://codereview.chromium.org/2528303003/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:354: NewTabPageUma.recordAction(NewTabPageUma.ACTION_OPENED_BOOKMARK); On 2016/12/22 at 12:31:22, dgn wrote: > ACTION_OPENED_BOOKMARK => MobileNTPBookmark => Description = "Action indicating the user has opened a bookmark from the bookmark manager." > > This will be called when the user opens the bookmark manager. I don't think it's the right action to record here. > > If what you are looking for is tracking bookmarks being opened, this happens here: https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr... We only record NewTabPageUma.ACTION_OPENED_SNIPPET for all of the categories it seems. I know. In a second step we should rename ACTION_OPENED_BOOKMARK to ACTION_OPENED_BOOKMARK_MANAGER and ACTION_OPENED_RECENTLY_CLOSED_ENTRY to ACTION_OPENED_RECENT_TABS_MANAGER. But this should happen in sync with iOS code. Currently this is a huge mess. Please refer to the bug for details. How would you suggest to proceed? Another approach could be to just abandon the old buckets and introduce new ones. https://codereview.chromium.org/2528303003/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:356: case KnownCategories.FOREIGN_TABS: On 2016/12/22 at 12:31:22, dgn wrote: > "Foreign Tabs" feels like it should not be associated with ACTION_OPENED_RECENTLY_CLOSED_ENTRY (="MobileNTPRecentlyClosed") as action. There is also no description for it. Can you please update the description and owner for Metrics we care about? Same as above.
https://codereview.chromium.org/2528303003/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java (right): https://codereview.chromium.org/2528303003/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:354: NewTabPageUma.recordAction(NewTabPageUma.ACTION_OPENED_BOOKMARK); On 2016/12/22 13:53:42, finkm wrote: > On 2016/12/22 at 12:31:22, dgn wrote: > > ACTION_OPENED_BOOKMARK => MobileNTPBookmark => Description = "Action > indicating the user has opened a bookmark from the bookmark manager." > > > > This will be called when the user opens the bookmark manager. I don't think > it's the right action to record here. > > > > If what you are looking for is tracking bookmarks being opened, this happens > here: > https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr... > We only record NewTabPageUma.ACTION_OPENED_SNIPPET for all of the categories it > seems. > > I know. In a second step we should rename ACTION_OPENED_BOOKMARK to > ACTION_OPENED_BOOKMARK_MANAGER and ACTION_OPENED_RECENTLY_CLOSED_ENTRY to > ACTION_OPENED_RECENT_TABS_MANAGER. But this should happen in sync with iOS code. > > Currently this is a huge mess. Please refer to the bug for details. Added a comment there. > How would > you suggest to proceed? Another approach could be to just abandon the old > buckets and introduce new ones. deferring to mpearson@ about how to proceed with renaming the actions.
https://codereview.chromium.org/2528303003/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkUtils.java (right): https://codereview.chromium.org/2528303003/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkUtils.java:232: RecordUserAction.record("MobileBookmarkOpen"); On 2016/12/22 13:53:42, finkm wrote: > On 2016/12/22 at 12:31:22, dgn wrote: > > FYI: this is called only from the bookmark manager, it will not trigger when > bookmarks from the NTP are opened. Also, the Action description actually matched > its usage. > > Yep. This is intentional. Are there bookmarks on the (mobile) NTP? If no, then the action description should mention that in passing. If so, then (i) the action description should point to where to find counts of the other open-bookmark interaction as well and vice versa and (ii) this action name should be revised to make it clear that this is only bookmark manager actions. https://codereview.chromium.org/2528303003/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java (right): https://codereview.chromium.org/2528303003/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:354: NewTabPageUma.recordAction(NewTabPageUma.ACTION_OPENED_BOOKMARK); On 2016/12/22 14:32:21, dgn wrote: > On 2016/12/22 13:53:42, finkm wrote: > > On 2016/12/22 at 12:31:22, dgn wrote: > > > ACTION_OPENED_BOOKMARK => MobileNTPBookmark => Description = "Action > > indicating the user has opened a bookmark from the bookmark manager." > > > > > > This will be called when the user opens the bookmark manager. I don't think > > it's the right action to record here. > > > > > > If what you are looking for is tracking bookmarks being opened, this happens > > here: > > > https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr... > > We only record NewTabPageUma.ACTION_OPENED_SNIPPET for all of the categories > it > > seems. > > > > I know. In a second step we should rename ACTION_OPENED_BOOKMARK to > > ACTION_OPENED_BOOKMARK_MANAGER and ACTION_OPENED_RECENTLY_CLOSED_ENTRY to > > ACTION_OPENED_RECENT_TABS_MANAGER. But this should happen in sync with iOS > code. > > > > Currently this is a huge mess. Please refer to the bug for details. > > Added a comment there. > > > How would > > you suggest to proceed? Another approach could be to just abandon the old > > buckets and introduce new ones. > > deferring to mpearson@ about how to proceed with renaming the actions. You're adding new places for these actions to be emitted that were not previously recorded. Users could've been doing these actions without them getting logged previously. This means that the default dashboard view for this histogram will be wrong; it'll be a mix of old data and new data which have different semantics. People will not always remember, oh, to make this histogram correct (not mixing different data), I need to slice by version 57.12.43.1+. Ugh. You should rename the histogram for clean split of old data from new and fix all the buckets and logging in fell swoop. https://codereview.chromium.org/2528303003/diff/120001/tools/metrics/actions/... File tools/metrics/actions/actions.xml (right): https://codereview.chromium.org/2528303003/diff/120001/tools/metrics/actions/... tools/metrics/actions/actions.xml:9204: <description>In the bookmark manager a bookmark was opened.</description> minor optional nit: prefer reverse order to put the important part first: A bookmark was opened from the bookmark manager. ditto below, e.g., A tab from another device was opened from the recent tabs manager. https://codereview.chromium.org/2528303003/diff/120001/tools/metrics/actions/... tools/metrics/actions/actions.xml:9697: Action indicating the user has opened a bookmark from the bookmark manager. Please revise this description explain what it is recording (on which platforms) and then when / how it partially changed and what replaced that part.
Patchset #10 (id:200001) has been deleted
Patchset #9 (id:180001) has been deleted
Patchset #8 (id:160001) has been deleted
finkm@chromium.org changed reviewers: + noyau@chromium.org
Patchset #8 (id:220001) has been deleted
Patchset #8 (id:240001) has been deleted
The CQ bit was checked by finkm@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: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
The CQ bit was checked by finkm@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 finkm@chromium.org
Patchset #9 (id:280001) has been deleted
Patchset #8 (id:260001) has been deleted
Patchset #8 (id:300001) has been deleted
The CQ bit was checked by finkm@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...
Description was changed from ========== - deprecated action MobileNTPBookmark - replaced MobileNTPBookmark with MobileBookmarkOpen where applicable - introduced action MobileForeignSessionOpen and MobileRecentlyClosedTabOpen - fixed usage of NewTabPageUma.ACTION_OPENED_BOOKMARK - fixed usage of NewTabPageUma.ACTION_OPENED_FOREIGN_SESSION For details please refer to the linked bug. BUG=662327 ========== to ========== - deprecated histogram NewTabPage.ActionAndroid and replaced it with NewTabPage.ActionAndroid2 - replaced action MobileNTPBookmark with MobileBookmarkManagerEntryOpened - replaced action MobileNTPForeignSession with MobileRecentTabManagerRecentTabOpened - replaced action MobileNTPRecentlyClosed with MobileRecentTabManagerRecentTabOpened For details please refer to the linked bug. BUG=662327 ==========
Description was changed from ========== - deprecated histogram NewTabPage.ActionAndroid and replaced it with NewTabPage.ActionAndroid2 - replaced action MobileNTPBookmark with MobileBookmarkManagerEntryOpened - replaced action MobileNTPForeignSession with MobileRecentTabManagerRecentTabOpened - replaced action MobileNTPRecentlyClosed with MobileRecentTabManagerRecentTabOpened For details please refer to the linked bug. BUG=662327 ========== to ========== - deprecated histogram NewTabPage.ActionAndroid and replaced it with NewTabPage.ActionAndroid2 - replaced action MobileNTPBookmark with MobileBookmarkManagerEntryOpened - replaced action MobileNTPForeignSession with MobileRecentTabManagerRecentTabOpened - replaced action MobileNTPRecentlyClosed with MobileRecentTabManagerRecentTabOpened For details please refer to the linked bug. BUG=662327 ==========
@noyau: Would you mind taking a look at the iOS related changes? Do they make sense? For context please refer to crbug/662327. @mpearson: I followed your advice and deprecated the entire histogram and created a new one. Please take another look at actions.xml & histograms.xml. @dgn: PTAL. Now it should be consistent. https://codereview.chromium.org/2528303003/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkUtils.java (right): https://codereview.chromium.org/2528303003/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkUtils.java:232: RecordUserAction.record("MobileBookmarkOpen"); On 2016/12/22 at 18:42:57, Mark P (away thru Jan 2 2017) wrote: > On 2016/12/22 13:53:42, finkm wrote: > > On 2016/12/22 at 12:31:22, dgn wrote: > > > FYI: this is called only from the bookmark manager, it will not trigger when > > bookmarks from the NTP are opened. Also, the Action description actually matched > > its usage. > > > > Yep. This is intentional. > > Are there bookmarks on the (mobile) NTP? > > If no, then the action description should mention that in passing. > > If so, then > (i) the action description should point to where to find counts of the other open-bookmark interaction as well and vice versa > and > (ii) this action name should be revised to make it clear that this is only bookmark manager actions. Done. https://codereview.chromium.org/2528303003/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java (right): https://codereview.chromium.org/2528303003/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:354: NewTabPageUma.recordAction(NewTabPageUma.ACTION_OPENED_BOOKMARK); On 2016/12/22 at 18:42:57, Mark P (away thru Jan 2 2017) wrote: > On 2016/12/22 14:32:21, dgn wrote: > > On 2016/12/22 13:53:42, finkm wrote: > > > On 2016/12/22 at 12:31:22, dgn wrote: > > > > ACTION_OPENED_BOOKMARK => MobileNTPBookmark => Description = "Action > > > indicating the user has opened a bookmark from the bookmark manager." > > > > > > > > This will be called when the user opens the bookmark manager. I don't think > > > it's the right action to record here. > > > > > > > > If what you are looking for is tracking bookmarks being opened, this happens > > > here: > > > > > https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr... > > > We only record NewTabPageUma.ACTION_OPENED_SNIPPET for all of the categories > > it > > > seems. > > > > > > I know. In a second step we should rename ACTION_OPENED_BOOKMARK to > > > ACTION_OPENED_BOOKMARK_MANAGER and ACTION_OPENED_RECENTLY_CLOSED_ENTRY to > > > ACTION_OPENED_RECENT_TABS_MANAGER. But this should happen in sync with iOS > > code. > > > > > > Currently this is a huge mess. Please refer to the bug for details. > > > > Added a comment there. > > > > > How would > > > you suggest to proceed? Another approach could be to just abandon the old > > > buckets and introduce new ones. > > > > deferring to mpearson@ about how to proceed with renaming the actions. > > You're adding new places for these actions to be emitted that were not previously recorded. Users could've been doing these actions without them getting logged previously. This means that the default dashboard view for this histogram will be wrong; it'll be a mix of old data and new data which have different semantics. People will not always remember, oh, to make this histogram correct (not mixing different data), I need to slice by version 57.12.43.1+. Ugh. > > You should rename the histogram for clean split of old data from new and fix all the buckets and logging in fell swoop. Done. I added a new histogram and marked the old one as obsolete. https://codereview.chromium.org/2528303003/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java (right): https://codereview.chromium.org/2528303003/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:513: NewTabPageUma.recordAction(NewTabPageUma.ACTION_OPENED_BOOKMARK); On 2016/12/22 at 12:31:22, dgn wrote: > same as above file, the action description does not match with how it's being recorded. Done. https://codereview.chromium.org/2528303003/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/RecentTabsManager.java (left): https://codereview.chromium.org/2528303003/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/RecentTabsManager.java:213: NewTabPageUma.recordAction(NewTabPageUma.ACTION_OPENED_FOREIGN_SESSION); On 2016/12/22 at 12:31:22, dgn wrote: > Isn't that constant now unused? It could be removed then. Done. https://codereview.chromium.org/2528303003/diff/120001/tools/metrics/actions/... File tools/metrics/actions/actions.xml (right): https://codereview.chromium.org/2528303003/diff/120001/tools/metrics/actions/... tools/metrics/actions/actions.xml:9204: <description>In the bookmark manager a bookmark was opened.</description> On 2016/12/22 at 18:42:57, Mark P (away thru Jan 2 2017) wrote: > minor optional nit: prefer reverse order to put the important part first: A bookmark was opened from the bookmark manager. > > ditto below, e.g., > A tab from another device was opened from the recent tabs manager. Done. https://codereview.chromium.org/2528303003/diff/120001/tools/metrics/actions/... tools/metrics/actions/actions.xml:9931: Action indicating the user has opened a recently closed tab from the recent On 2016/12/22 at 12:31:22, dgn wrote: > drop "Action indicating..." Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2528303003/diff/320001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java (right): https://codereview.chromium.org/2528303003/diff/320001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java:1154: RecordUserAction.record("MobileMenuAllBookmarks"); nitty nit: any reason why the order of the calls changed? I suppose the original code wanted to get the keyboard hidden as quickly as possible, maybe it the UMA recording should go back after the hideKeyboard() call? or inside the callback? OTOH I'm not sure what the point of that method is. Other menu items don't seem to need to explicitly ask for the keyboard to be hidden. I don't feel strongly about that anyway. At least this way it's consistent with the call order in the other functions.
Patchset #9 (id:340001) has been deleted
https://codereview.chromium.org/2528303003/diff/320001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java (right): https://codereview.chromium.org/2528303003/diff/320001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java:1154: RecordUserAction.record("MobileMenuAllBookmarks"); On 2017/01/04 at 11:53:26, dgn wrote: > nitty nit: any reason why the order of the calls changed? I suppose the original code wanted to get the keyboard hidden as quickly as possible, maybe it the UMA recording should go back after the hideKeyboard() call? or inside the callback? > > OTOH I'm not sure what the point of that method is. Other menu items don't seem to need to explicitly ask for the keyboard to be hidden. > > I don't feel strongly about that anyway. At least this way it's consistent with the call order in the other functions. Done. Good catch. When I started the CL, the NewTabPage.isNTPUrl(currentTab.getUrl()) check was done inside the final if block. So I had to do it before currentTab.getUrl() changes. And to have all UMA calls in one place, I changed the order. But now I moved the isNTP check to line 1129. With that I could restore the original order (which perhaps makes the UI react faster).
lgtm A big thinks for making the android action histogram and user actions more correct. This is great! (though frankly it was a real headache to review, trying to keep everything in my mind at once) I have a few comments below. I also suggest you test this extensively using about:user-actions and about:histograms, trying to find an entry to point to a surface that is or isn't being recorded as you expect. Tapping a tile (maybe bookmarked) from a widget? Tapping a most visited tile that happens to be bookmarked? Being on the NTP and having the recent tabs manager in an adjacent tab and swiping to it? etc. etc. cheers, mark https://codereview.chromium.org/2528303003/diff/360001/tools/metrics/actions/... File tools/metrics/actions/actions.xml (right): https://codereview.chromium.org/2528303003/diff/360001/tools/metrics/actions/... tools/metrics/actions/actions.xml:9709: <obsolete>Deprecated as of 01/2017</obsolete> Please add replaced with MobileRecentTabManagerTabFrom... or mostly replaced with ... ditto with the recently closed action and ditto with the MobileNTPBookmark action (Choose the former phrasing for each action if its a 1-on-1 replacement, i.e., just a name improvement; choose the latter if there's some change in when it's being emitted going on.) https://codereview.chromium.org/2528303003/diff/360001/tools/metrics/actions/... tools/metrics/actions/actions.xml:9938: A recently closed tab was opened from the recent tabs manager.Recently nit: space after period. https://codereview.chromium.org/2528303003/diff/360001/tools/metrics/actions/... tools/metrics/actions/actions.xml:9947: A tab from another device was opened from the recent tabs manager. Tab from nit: Tab from -> Tabs from
ios name change lgtm.
The CQ bit was checked by finkm@chromium.org
https://codereview.chromium.org/2528303003/diff/360001/tools/metrics/actions/... File tools/metrics/actions/actions.xml (right): https://codereview.chromium.org/2528303003/diff/360001/tools/metrics/actions/... tools/metrics/actions/actions.xml:9709: <obsolete>Deprecated as of 01/2017</obsolete> On 2017/01/06 at 00:04:39, Mark P wrote: > Please add > replaced with MobileRecentTabManagerTabFrom... > or > mostly replaced with ... > > ditto with the recently closed action > > and ditto with the MobileNTPBookmark action > > (Choose the former phrasing for each action if its a 1-on-1 replacement, i.e., just a name improvement; choose the latter if there's some change in when it's being emitted going on.) 3x done! https://codereview.chromium.org/2528303003/diff/360001/tools/metrics/actions/... tools/metrics/actions/actions.xml:9938: A recently closed tab was opened from the recent tabs manager.Recently On 2017/01/06 at 00:04:39, Mark P wrote: > nit: space after period. done https://codereview.chromium.org/2528303003/diff/360001/tools/metrics/actions/... tools/metrics/actions/actions.xml:9947: A tab from another device was opened from the recent tabs manager. Tab from On 2017/01/06 at 00:04:39, Mark P wrote: > nit: > Tab from > -> > Tabs from done
The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org, dgn@chromium.org, noyau@chromium.org, mpearson@chromium.org Link to the patchset: https://codereview.chromium.org/2528303003/#ps380001 (title: "Addressing comments")
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
Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
finkm@, Can you confirm you saw my message here about testing? thanks, mark On 2017/01/06 00:04:39, Mark P wrote: > lgtm > > A big thinks for making the android action histogram and user actions more > correct. This is great! > > (though frankly it was a real headache to review, trying to keep everything in > my mind at once) > > I have a few comments below. > > I also suggest you test this extensively using about:user-actions and > about:histograms, trying to find an entry to point to a surface that is or isn't > being recorded as you expect. Tapping a tile (maybe bookmarked) from a widget? > Tapping a most visited tile that happens to be bookmarked? Being on the NTP and > having the recent tabs manager in an adjacent tab and swiping to it? etc. etc. > > cheers, > mark
On 2017/01/09 at 20:16:11, mpearson wrote: > finkm@, > > Can you confirm you saw my message here about testing? > > thanks, > mark > > On 2017/01/06 00:04:39, Mark P wrote: > > lgtm > > > > A big thinks for making the android action histogram and user actions more > > correct. This is great! > > > > (though frankly it was a real headache to review, trying to keep everything in > > my mind at once) > > > > I have a few comments below. > > > > I also suggest you test this extensively using about:user-actions and > > about:histograms, trying to find an entry to point to a surface that is or isn't > > being recorded as you expect. Tapping a tile (maybe bookmarked) from a widget? > > Tapping a most visited tile that happens to be bookmarked? Being on the NTP and > > having the recent tabs manager in an adjacent tab and swiping to it? etc. etc. > > > > cheers, > > mark Hi Mark, yes, I saw it. Will do! - Michael
Patchset #11 (id:400001) has been deleted
Description was changed from ========== - deprecated histogram NewTabPage.ActionAndroid and replaced it with NewTabPage.ActionAndroid2 - replaced action MobileNTPBookmark with MobileBookmarkManagerEntryOpened - replaced action MobileNTPForeignSession with MobileRecentTabManagerRecentTabOpened - replaced action MobileNTPRecentlyClosed with MobileRecentTabManagerRecentTabOpened For details please refer to the linked bug. BUG=662327 ========== to ========== - deprecated histogram NewTabPage.ActionAndroid and replaced it with NewTabPage.ActionAndroid2 - replaced action MobileNTPBookmark with MobileBookmarkManagerEntryOpened - replaced action MobileNTPForeignSession with MobileRecentTabManagerTabFromOtherDeviceOpened - replaced action MobileNTPRecentlyClosed with MobileRecentTabManagerRecentTabOpened For details please refer to the linked bug. BUG=662327 ==========
The CQ bit was checked by finkm@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org, noyau@chromium.org, mpearson@chromium.org, dgn@chromium.org Link to the patchset: https://codereview.chromium.org/2528303003/#ps420001 (title: "Rebase")
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": 420001, "attempt_start_ts": 1484060707031700,
"parent_rev": "62a2eec96170dea8ee8f63bd6953a73ec5dec4dc", "commit_rev":
"422ecccc1f41cb2c5ec5632727a8265b40a5d1d2"}
Message was sent while issue was closed.
Description was changed from ========== - deprecated histogram NewTabPage.ActionAndroid and replaced it with NewTabPage.ActionAndroid2 - replaced action MobileNTPBookmark with MobileBookmarkManagerEntryOpened - replaced action MobileNTPForeignSession with MobileRecentTabManagerTabFromOtherDeviceOpened - replaced action MobileNTPRecentlyClosed with MobileRecentTabManagerRecentTabOpened For details please refer to the linked bug. BUG=662327 ========== to ========== - deprecated histogram NewTabPage.ActionAndroid and replaced it with NewTabPage.ActionAndroid2 - replaced action MobileNTPBookmark with MobileBookmarkManagerEntryOpened - replaced action MobileNTPForeignSession with MobileRecentTabManagerTabFromOtherDeviceOpened - replaced action MobileNTPRecentlyClosed with MobileRecentTabManagerRecentTabOpened For details please refer to the linked bug. BUG=662327 Review-Url: https://codereview.chromium.org/2528303003 Cr-Commit-Position: refs/heads/master@{#442599} Committed: https://chromium.googlesource.com/chromium/src/+/422ecccc1f41cb2c5ec5632727a8... ==========
Message was sent while issue was closed.
Committed patchset #11 (id:420001) as https://chromium.googlesource.com/chromium/src/+/422ecccc1f41cb2c5ec5632727a8... |
