Alternate NTP: Show detached bookmark bar

With the new full page NTP the detached bookmark bar was no longer visible.

The problem was that we relied on an instance of NewTabUI to control the bookmark bar visibility. The new full page NTP no longer has an instance of NewTabUI so the bookmark bar was never visible.

Fix was to move the relevant code out of NewTabUI and into BookmarkTabHelper.

BUG=175146
TEST=Ran with instant extended enabled. Verified that the detached bookmark bar was visible.
Added a new test BookmarkInstantExtendedBrowsertest.DetachedBookmarkBar. Verified that the test fails without my patch and passes with it.

Review URL: https://chromiumcodereview.appspot.com/12217127

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@182165 0039d316-1c4b-4281-b951-d872f2087c98

chrome/browser/ui/bookmarks/bookmark_tab_helper.cc

@@ -6,24 +6,18 @@
 
 #include "chrome/browser/bookmarks/bookmark_model.h"
 #include "chrome/browser/bookmarks/bookmark_model_factory.h"
+#include "chrome/browser/defaults.h"
+#include "chrome/browser/prefs/pref_service_syncable.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/ui/bookmarks/bookmark_tab_helper_delegate.h"
 #include "chrome/browser/ui/webui/ntp/new_tab_ui.h"
+#include "chrome/common/pref_names.h"
+#include "chrome/common/url_constants.h"
+#include "content/public/browser/navigation_entry.h"
 #include "content/public/browser/web_contents.h"
 
 DEFINE_WEB_CONTENTS_USER_DATA_KEY(BookmarkTabHelper);
 
-namespace {
-
-bool CanShowBookmarkBar(content::WebUI* ui) {
-  if (!ui)
-    return false;
-  NewTabUI* new_tab = NewTabUI::FromWebUIController(ui->GetController());
-  return new_tab && new_tab->CanShowBookmarkBar();
-}
-
-}  // namespace
-
 BookmarkTabHelper::~BookmarkTabHelper() {
   if (bookmark_model_)
     bookmark_model_->RemoveObserver(this);
@@ -33,19 +27,28 @@ bool BookmarkTabHelper::ShouldShowBookmarkBar() const {
   if (web_contents()->ShowingInterstitialPage())
     return false;
 
-  // See WebContents::GetWebUIForCurrentState() comment for more info. This case
-  // is very similar, but for non-first loads, we want to use the committed
-  // entry. This is so the bookmarks bar disappears at the same time the page
-  // does.
-  if (web_contents()->GetController().GetLastCommittedEntry()) {
-    // Not the first load, always use the committed Web UI.
-    return CanShowBookmarkBar(web_contents()->GetCommittedWebUI());
+  // For non-first loads, we want to use the committed entry. This is so the
+  // bookmarks bar disappears at the same time the page does.
+  content::NavigationEntry* entry =
+      web_contents()->GetController().GetLastCommittedEntry();
+  if (!entry)
+    entry = web_contents()->GetController().GetVisibleEntry();
+  if (!entry)
+    return false;
+
+  GURL url = entry->GetVirtualURL();
+  if (url != GURL(chrome::kChromeUINewTabURL) ||
+      url.SchemeIs(chrome::kViewSourceScheme)) {
+    return false;
   }
 
-  // When it's the first load, we know either the pending one or the committed
-  // one will have the Web UI in it (see GetWebUIForCurrentState), and only one
-  // of them will be valid, so we can just check both.
-  return CanShowBookmarkBar(web_contents()->GetWebUI());
+  Profile* profile =
+      Profile::FromBrowserContext(web_contents()->GetBrowserContext());
+  PrefService* prefs = profile->GetPrefs();
+  bool disabled_by_policy =
+      prefs->IsManagedPreference(prefs::kShowBookmarkBar) &&
+      !prefs->GetBoolean(prefs::kShowBookmarkBar);
+  return browser_defaults::bookmarks_enabled && !disabled_by_policy;
 }
 
 BookmarkTabHelper::BookmarkTabHelper(content::WebContents* web_contents)

chrome/browser/ui/bookmarks/bookmark_unittest.cc

+// Copyright (c) 2013 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/search_engines/template_url_service.h"
+#include "chrome/browser/search_engines/template_url_service_factory.h"
+#include "chrome/browser/ui/search/search.h"
+#include "chrome/common/url_constants.h"
+#include "chrome/test/base/browser_with_test_window_test.h"
+
+typedef BrowserWithTestWindowTest BookmarkTest;
+
+// Verify that the detached bookmark bar is visible on the new tab page.
+TEST_F(BookmarkTest, DetachedBookmarkBarOnNTP) {
+  AddTab(browser(), GURL(chrome::kChromeUINewTabURL));
+  EXPECT_EQ(BookmarkBar::DETACHED, browser()->bookmark_bar_state());
+}
+
+class BookmarkInstantExtendedTest : public BrowserWithTestWindowTest {
+ public:
+  BookmarkInstantExtendedTest() {
+    chrome::search::EnableInstantExtendedAPIForTesting();
+  }
+
+ protected:
+  virtual TestingProfile* CreateProfile() OVERRIDE {
+    TestingProfile* profile = BrowserWithTestWindowTest::CreateProfile();
+    // TemplateURLService is normally NULL during testing. Instant extended
+    // needs this service so set a custom factory function.
+    TemplateURLServiceFactory::GetInstance()->SetTestingFactory(
+        profile, &BookmarkInstantExtendedTest::CreateTemplateURLService);
+    return profile;
+  }
+
+ private:
+  static ProfileKeyedService* CreateTemplateURLService(Profile* profile) {
+    return new TemplateURLService(profile);
+  }
+
+  DISALLOW_COPY_AND_ASSIGN(BookmarkInstantExtendedTest);
+};
+
+// Verify that in instant extended mode the detached bookmark bar is visible on
+// the new tab page.
+TEST_F(BookmarkInstantExtendedTest, DetachedBookmarkBarOnNTP) {
+  AddTab(browser(), GURL(chrome::kChromeUINewTabURL));
+  EXPECT_EQ(BookmarkBar::DETACHED, browser()->bookmark_bar_state());
+}

chrome/browser/ui/webui/ntp/new_tab_ui.cc

@@ -20,7 +20,6 @@
 #include "base/strings/string_number_conversions.h"
 #include "base/threading/thread.h"
 #include "base/utf_string_conversions.h"
-#include "chrome/browser/defaults.h"
 #include "chrome/browser/prefs/pref_registry_syncable.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/sessions/session_types.h"
@@ -224,17 +223,6 @@ void NewTabUI::StartTimingPaint(RenderViewHost* render_view_host) {
                &NewTabUI::PaintTimeout);
 }
 
-bool NewTabUI::CanShowBookmarkBar() const {
-  if (web_ui()->GetWebContents()->GetURL().SchemeIs(chrome::kViewSourceScheme))
-    return false;
-
-  PrefService* prefs = GetProfile()->GetPrefs();
-  bool disabled_by_policy =
-      prefs->IsManagedPreference(prefs::kShowBookmarkBar) &&
-      !prefs->GetBoolean(prefs::kShowBookmarkBar);
-  return browser_defaults::bookmarks_enabled && !disabled_by_policy;
-}
-
 void NewTabUI::RenderViewCreated(RenderViewHost* render_view_host) {
   StartTimingPaint(render_view_host);
 }

chrome/browser/ui/webui/ntp/new_tab_ui.h

@@ -60,10 +60,6 @@ class NewTabUI : public content::WebUIController,
   virtual void RenderViewReused(
       content::RenderViewHost* render_view_host) OVERRIDE;
 
-  // Returns true if the bookmark bar can be displayed over this webui, detached
-  // from the location bar.
-  bool CanShowBookmarkBar() const;
-
   bool showing_sync_bubble() { return showing_sync_bubble_; }
   void set_showing_sync_bubble(bool showing) { showing_sync_bubble_ = showing; }
 

chrome/chrome_tests_unit.gypi

@@ -1212,6 +1212,7 @@
         'browser/ui/bookmarks/bookmark_context_menu_controller_unittest.cc',
         'browser/ui/bookmarks/bookmark_prompt_controller_unittest.cc',
         'browser/ui/bookmarks/bookmark_ui_utils_unittest.cc',
+        'browser/ui/bookmarks/bookmark_unittest.cc',
         'browser/ui/browser_command_controller_unittest.cc',
         'browser/ui/browser_iterator_unittest.cc',
         'browser/ui/chrome_select_file_policy_unittest.cc',
@@ -2142,6 +2143,7 @@
             'browser/tab_contents/render_view_context_menu_unittest.cc',
             'browser/ui/bookmarks/bookmark_context_menu_controller_unittest.cc',
             'browser/ui/bookmarks/bookmark_prompt_controller_unittest.cc',
+            'browser/ui/bookmarks/bookmark_unittest.cc',
             'browser/ui/browser_command_controller_unittest.cc',
             'browser/ui/browser_iterator_unittest.cc',
             'browser/ui/fullscreen/fullscreen_controller_state_unittest.cc',