[MD Bookmarks] Fix singleton tab behavior.

chrome/browser/ui/chrome_pages.cc

Index: chrome/browser/ui/chrome_pages.cc
diff --git a/chrome/browser/ui/chrome_pages.cc b/chrome/browser/ui/chrome_pages.cc
index 78cf00f9e4c5215abd889d030db9d3ebe707217f..961f47e1441495e31a54a757cd5dc0036a93da4c 100644
--- a/chrome/browser/ui/chrome_pages.cc
+++ b/chrome/browser/ui/chrome_pages.cc
@@ -28,6 +28,7 @@
 #include "chrome/browser/ui/settings_window_manager.h"
 #include "chrome/browser/ui/singleton_tabs.h"
 #include "chrome/browser/ui/tabs/tab_strip_model.h"
+#include "chrome/browser/ui/webui/md_bookmarks/md_bookmarks_ui.h"
 #include "chrome/browser/ui/webui/options/content_settings_handler.h"
 #include "chrome/browser/ui/webui/site_settings_helper.h"
 #include "chrome/common/chrome_features.h"
@@ -67,15 +68,12 @@ namespace {
 
 const char kHashMark[] = "#";
 
-void OpenBookmarkManagerWithHash(Browser* browser,
-                                 const std::string& action,
-                                 int64_t node_id) {
-  base::RecordAction(UserMetricsAction("ShowBookmarkManager"));
-  base::RecordAction(UserMetricsAction("ShowBookmarks"));
-  NavigateParams params(GetSingletonTabNavigateParams(
-      browser,
-      GURL(kChromeUIBookmarksURL).Resolve(base::StringPrintf(
-          "/#%s%s", action.c_str(), base::Int64ToString(node_id).c_str()))));
+void OpenBookmarkManagerForNode(Browser* browser, int64_t node_id) {
+  GURL url = GURL(kChromeUIBookmarksURL)
+                 .Resolve(base::StringPrintf(
+                     MdBookmarksUI::IsEnabled() ? "/?id=%s" : "/#%s",
+                     base::Int64ToString(node_id).c_str()));
+  NavigateParams params(GetSingletonTabNavigateParams(browser, url));
   params.path_behavior = NavigateParams::IGNORE_AND_NAVIGATE;
   ShowSingletonTabOverwritingNTP(browser, params);
 }
@@ -186,13 +184,20 @@ std::string GenerateContentSettingsSearchQueryPath(int query_message_id) {
 void ShowBookmarkManager(Browser* browser) {
   base::RecordAction(UserMetricsAction("ShowBookmarkManager"));
   base::RecordAction(UserMetricsAction("ShowBookmarks"));
+  if (MdBookmarksUI::IsEnabled()) {
+    OpenBookmarkManagerForNode(browser, 1);

[sky, 2017/05/24 17:16:06]

Where does the 1 come from here?

[calamity, 2017/05/25 08:22:14]

It's the id for the bookmarks bar. This isn't actually defined in a constant
anywhere, but is guaranteed by the way the bookmarks model is constructed and
assumed by the old bookmark manager.

https://cs.chromium.org/chromium/src/components/bookmarks/browser/bookmark_mo...

creates the root nodes in order which uses generate_next_node_id() which starts
from 1.

The old bookmark manager hardcodes these in JS here

https://cs.chromium.org/chromium/src/chrome/browser/resources/bookmark_manage...

I've altered this to be a little more obvious about what it's doing, but maybe
it's worth adding these root folder ids as constants inside bookmark_model.h.
WDYT?

+    return;
+  }
+
   ShowSingletonTabOverwritingNTP(
       browser,
       GetSingletonTabNavigateParams(browser, GURL(kChromeUIBookmarksURL)));
 }
 
 void ShowBookmarkManagerForNode(Browser* browser, int64_t node_id) {
-  OpenBookmarkManagerWithHash(browser, std::string(), node_id);
+  base::RecordAction(UserMetricsAction("ShowBookmarkManager"));
+  base::RecordAction(UserMetricsAction("ShowBookmarks"));

[sky, 2017/05/24 17:16:06]

Why do we have two user-metrics-actions for this?

[calamity, 2017/05/25 08:22:14]

Good point. I audited and these are used for the same thing, but the calls are
still all wrong and messed up. Fixed it so that these two places are the only
places it gets logged.

+  OpenBookmarkManagerForNode(browser, node_id);
 }
 
 void ShowHistory(Browser* browser) {