[MD Bookmarks] Fix singleton tab behavior.

chrome/browser/ui/chrome_pages.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/ui/chrome_pages.h"
 
 #include <stddef.h>
 
 #include "base/command_line.h"
 #include "base/feature_list.h"
@@ skipping 10 matching lines @@
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/browser_finder.h"
 #include "chrome/browser/ui/browser_navigator_params.h"
 #include "chrome/browser/ui/browser_window.h"
 #include "chrome/browser/ui/extensions/app_launch_params.h"
 #include "chrome/browser/ui/extensions/application_launch.h"
 #include "chrome/browser/ui/scoped_tabbed_browser_displayer.h"
 #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"
 #include "chrome/common/chrome_switches.h"
 #include "chrome/common/url_constants.h"
 #include "components/signin/core/browser/signin_header_helper.h"
 #include "components/signin/core/common/profile_management_switches.h"
 #include "content/public/browser/web_contents.h"
 #include "extensions/browser/extension_prefs.h"
 #include "extensions/common/constants.h"
@@ skipping 19 matching lines @@
 #include "components/signin/core/browser/signin_manager.h"
 #endif
 
 using base::UserMetricsAction;
 
 namespace chrome {
 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);
 }
 
 void NavigateToSingletonTab(Browser* browser, const GURL& url) {
   NavigateParams params(GetSingletonTabNavigateParams(browser, url));
   params.path_behavior = NavigateParams::IGNORE_AND_NAVIGATE;
   ShowSingletonTabOverwritingNTP(browser, params);
 }
 
@@ skipping 90 matching lines @@
   return std::string(chrome::kDeprecatedOptionsSearchSubPage) + kHashMark +
          l10n_util::GetStringUTF8(query_message_id);
 }
 #endif
 
 }  // namespace
 
 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) {
   base::RecordAction(UserMetricsAction("ShowHistory"));
   NavigateParams params(
       GetSingletonTabNavigateParams(browser, GURL(kChromeUIHistoryURL)));
   params.path_behavior = NavigateParams::IGNORE_AND_NAVIGATE;
   ShowSingletonTabOverwritingNTP(browser, params);
 }
 
@@ skipping 248 matching lines @@
       SigninManagerFactory::GetForProfile(original_profile);
   DCHECK(manager->IsSigninAllowed());
   if (manager->IsAuthenticated())
     ShowSettings(browser);
   else
     ShowBrowserSignin(browser, access_point);
 }
 #endif
 
 }  // namespace chrome