Bookmarks: Fix issue with over collecting folder metrics.

Bug: 951915
Change-Id: I381227e4ea3719e1df8ad5b6bdffd26daacda54c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1603526
Commit-Queue: Hector Carmona <hcarmona@chromium.org>
Reviewed-by: calamity <calamity@chromium.org>
Reviewed-by: Mark Pearson <mpearson@chromium.org>
Auto-Submit: Hector Carmona <hcarmona@chromium.org>
Cr-Commit-Position: refs/heads/master@{#660925}

chrome/browser/resources/bookmarks/command_manager.js

@@ -358,11 +358,7 @@ cr.define('bookmarks', function() {
           this.openUrls_(this.expandUrls_(itemIds), command);
           break;
         case Command.OPEN:
-          const isFolder = itemIds.size == 1 &&
-              this.containsMatchingNode_(itemIds, function(node) {
-                return !node.url;
-              });
-          if (isFolder) {
+          if (this.isFolder_(itemIds)) {
             const folderId = Array.from(itemIds)[0];
             this.dispatch(
                 bookmarks.actions.selectFolder(folderId, state.nodes));
@@ -414,9 +410,8 @@ cr.define('bookmarks', function() {
         default:
           assert(false);
       }
-
-      bookmarks.util.recordEnumHistogram(
-          'BookmarkManager.CommandExecuted', command, Command.MAX_VALUE);
+      this.recordCommandHistogram_(
+          itemIds, 'BookmarkManager.CommandExecuted', command);
     },
 
     /**
@@ -433,9 +428,8 @@ cr.define('bookmarks', function() {
         if (shortcut.matchesEvent(e) && this.canExecute(command, itemIds)) {
           this.handle(command, itemIds);
 
-          bookmarks.util.recordEnumHistogram(
-              'BookmarkManager.CommandExecutedFromKeyboard', command,
-              Command.MAX_VALUE);
+          this.recordCommandHistogram_(
+              itemIds, 'BookmarkManager.CommandExecutedFromKeyboard', command);
           e.stopPropagation();
           e.preventDefault();
           return true;
@@ -579,6 +573,7 @@ cr.define('bookmarks', function() {
      * @param {!Set<string>} itemIds
      * @return {boolean} True if |itemIds| is a single bookmark (non-folder)
      *     node.
+     * @private
      */
     isSingleBookmark_: function(itemIds) {
       return itemIds.size == 1 &&
@@ -587,6 +582,16 @@ cr.define('bookmarks', function() {
           });
     },
 
+    /**
+     * @param {!Set<string>} itemIds
+     * @return {boolean}
+     * @private
+     */
+    isFolder_: function(itemIds) {
+      return itemIds.size == 1 &&
+          this.containsMatchingNode_(itemIds, node => !node.url);
+    },
+
     /**
      * @param {Command} command
      * @return {string}
@@ -722,6 +727,7 @@ cr.define('bookmarks', function() {
 
     /**
      * @param {Command} command
+     * @param {!Set<string>} itemIds
      * @return {boolean}
      * @private
      */
@@ -733,6 +739,21 @@ cr.define('bookmarks', function() {
            (this.globalCanEdit_ || this.isSingleBookmark_(itemIds)));
     },
 
+    /**
+     * @param {!Set<string>} itemIds
+     * @param {string} histogram
+     * @param {number} command
+     * @private
+     */
+    recordCommandHistogram_: function(itemIds, histogram, command) {
+      if (command == Command.OPEN) {
+        command = this.isFolder_(itemIds) ? Command.OPEN_FOLDER :
+                                            Command.OPEN_BOOKMARK;
+      }
+
+      bookmarks.util.recordEnumHistogram(histogram, command, Command.MAX_VALUE);
+    },
+
     /**
      * Show a toast with a bookmark |title| inserted into a label, with the
      * title ellipsised if necessary.

chrome/browser/resources/bookmarks/constants.js

@@ -32,7 +32,7 @@ const Command = {
   OPEN_INCOGNITO: 6,
   UNDO: 7,
   REDO: 8,
-  // OPEN triggers when you double-click an item.
+  // OPEN triggers when you double-click an item. NOT USED FOR METRICS.
   OPEN: 9,
   SELECT_ALL: 10,
   DESELECT_ALL: 11,
@@ -46,8 +46,12 @@ const Command = {
   EXPORT: 19,
   HELP_CENTER: 20,
 
+  // Added for more precise metrics purposes. OPEN is re-mapped to one of these.
+  OPEN_BOOKMARK: 21,
+  OPEN_FOLDER: 22,
+
   // Append new values to the end of the enum.
-  MAX_VALUE: 21,
+  MAX_VALUE: 23,
 };
 
 /**

tools/metrics/histograms/enums.xml

@@ -5173,7 +5173,7 @@ Unknown properties are collapsed to zero. -->
   <int value="6" label="Open in incognito"/>
   <int value="7" label="Undo"/>
   <int value="8" label="Redo"/>
-  <int value="9" label="Open (double click/enter)"/>
+  <int value="9" label="Open bookmark or folder (deprecated)"/>
   <int value="10" label="Select all"/>
   <int value="11" label="Deselect all"/>
   <int value="12" label="Copy"/>
@@ -5185,6 +5185,8 @@ Unknown properties are collapsed to zero. -->
   <int value="18" label="Import"/>
   <int value="19" label="Export"/>
   <int value="20" label="Help center"/>
+  <int value="21" label="Open bookmark (via double-click / enter)"/>
+  <int value="22" label="Open folder (via double-click / enter)"/>
 </enum>
 
 <enum name="BookmarkManagerMenuSource">