[NTP] Fixed usage of NewTabPage.ActionAndroid histogram

chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java

Index: chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java
diff --git a/chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java b/chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java
index 84a61f8dd909fc9b2d104693249940503bace7a5..40408c98fc89f20ee6aa732823a6bbfccdc8ee43 100644
--- a/chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java
+++ b/chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java
@@ -349,6 +349,15 @@ public class NewTabPage
         @Override
         public void trackSnippetCategoryActionClick(int category, int position) {
             mSnippetsBridge.onMoreButtonClicked(category, position);
+            switch (category) {
+                case KnownCategories.BOOKMARKS:
+                    NewTabPageUma.recordAction(NewTabPageUma.ACTION_OPENED_BOOKMARK);

[dgn, 2016/12/22 12:31:22]

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.

[finkm, 2016/12/22 13:53:42]

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. 

[dgn, 2016/12/22 14:32:21]

Added a comment there.
deferring to mpearson@ about how to proceed with renaming the actions.

[Mark P, 2016/12/22 18:42:57]

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.

[finkm, 2017/01/03 10:43:52]

Done. I added a new histogram and marked the old one as obsolete.

+                    break;
+                case KnownCategories.FOREIGN_TABS:

[dgn, 2016/12/22 12:31:22]

"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?

[finkm, 2016/12/22 13:53:42]

Same as above.

+                case KnownCategories.RECENT_TABS:
+                    NewTabPageUma.recordAction(NewTabPageUma.ACTION_OPENED_RECENTLY_CLOSED_ENTRY);
+                    break;
+            }
         }
 
         @Override