Avoid unnecessary visibility changes to BookmarkBarView

chrome/browser/ui/views/frame/browser_view_browsertest.cc

 // Copyright 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/ui/views/frame/browser_view.h"
 
+#include "base/prefs/pref_service.h"
 #include "chrome/browser/devtools/devtools_window_testing.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/browser_tabstrip.h"
 #include "chrome/browser/ui/tabs/tab_strip_model.h"
+#include "chrome/browser/ui/views/bookmarks/bookmark_bar_view.h"
+#include "chrome/browser/ui/views/bookmarks/bookmark_bar_view_observer.h"
+#include "chrome/common/url_constants.h"
 #include "chrome/test/base/in_process_browser_test.h"
+#include "chrome/test/base/ui_test_utils.h"
+#include "components/bookmarks/common/bookmark_pref_names.h"
 #include "content/public/browser/invalidate_type.h"
 #include "content/public/browser/web_contents.h"
 #include "content/public/browser/web_contents_observer.h"
 
 class BrowserViewTest : public InProcessBrowserTest {
  public:
   BrowserViewTest() : InProcessBrowserTest(), devtools_(NULL) {}
 
  protected:
   BrowserView* browser_view() {
@@ skipping 137 matching lines @@
   CloseDevToolsWindow();
   EXPECT_FALSE(devtools_web_view()->web_contents());
   EXPECT_EQ(full_bounds, devtools_web_view()->bounds());
   EXPECT_EQ(full_bounds, contents_web_view()->bounds());
 
   browser_view()->UpdateDevTools();
   EXPECT_FALSE(devtools_web_view()->web_contents());
   EXPECT_EQ(full_bounds, devtools_web_view()->bounds());
   EXPECT_EQ(full_bounds, contents_web_view()->bounds());
 }
+
+class BookmarkBarViewObserverImpl : public BookmarkBarViewObserver {
+ public:
+  BookmarkBarViewObserverImpl() : change_count_(0) {
+  }
+
+  int change_count() const { return change_count_; }
+  void clear_change_count() { change_count_ = 0; }
+
+  // BookmarkBarViewObserver:
+  virtual void OnBookmarkBarVisibilityChanged() OVERRIDE {
+    change_count_++;
+  }
+
+ private:
+  int change_count_;
+
+  DISALLOW_COPY_AND_ASSIGN(BookmarkBarViewObserverImpl);
+};
+
+// Verifies we don't unnecessarily change the visibility of the bookmarkbarview.

[James Cook, 2014/09/16 15:52:31]

nit: "bookmarkbarview" to either "BookmarkBarView" or "bookmark bar view".

[sky, 2014/09/16 16:22:08]

Done.

+IN_PROC_BROWSER_TEST_F(BrowserViewTest, AvoidUnnecessaryVisibilityChanges) {
+  // Create two tabs, the first empty and the second the ntp. Make it so the
+  // bookmarkbar isn't shown (meaning it'll only be shown when on the ntp).

[James Cook, 2014/09/16 15:52:31]

ditto: "bookmarkbar" just reads oddly to me

[sky, 2014/09/16 16:22:08]

Done.

+  browser()->profile()->GetPrefs()->SetBoolean(
+      bookmarks::prefs::kShowBookmarkBar, false);
+  GURL new_tab_url(chrome::kChromeUINewTabURL);
+  chrome::AddTabAt(browser(), GURL(), -1, true);
+  ui_test_utils::NavigateToURL(browser(), new_tab_url);
+
+  ASSERT_TRUE(browser_view()->bookmark_bar());
+  BookmarkBarViewObserverImpl observer;
+  BookmarkBarView* bookmark_bar = browser_view()->bookmark_bar();
+  bookmark_bar->AddObserver(&observer);
+  EXPECT_TRUE(bookmark_bar->visible());
+
+  // Go to empty tab. Bookmark bar should hide.
+  browser()->tab_strip_model()->ActivateTabAt(0, true);
+  EXPECT_FALSE(bookmark_bar->visible());
+  EXPECT_EQ(1, observer.change_count());
+  observer.clear_change_count();
+
+  // Go to ntp tab. Bookmark bar should show.
+  browser()->tab_strip_model()->ActivateTabAt(1, true);
+  EXPECT_TRUE(bookmark_bar->visible());
+  EXPECT_EQ(1, observer.change_count());
+  observer.clear_change_count();
+
+  // Repeat with the bookmark bar always visible.
+  browser()->profile()->GetPrefs()->SetBoolean(
+      bookmarks::prefs::kShowBookmarkBar, true);
+  browser()->tab_strip_model()->ActivateTabAt(1, true);
+  EXPECT_TRUE(bookmark_bar->visible());
+  observer.clear_change_count();
+
+  browser()->tab_strip_model()->ActivateTabAt(0, true);
+  EXPECT_TRUE(bookmark_bar->visible());
+  EXPECT_EQ(0, observer.change_count());
+  observer.clear_change_count();
+
+  browser()->tab_strip_model()->ActivateTabAt(1, true);
+  EXPECT_TRUE(bookmark_bar->visible());
+  EXPECT_EQ(0, observer.change_count());
+  observer.clear_change_count();
+
+  browser_view()->bookmark_bar()->RemoveObserver(&observer);
+}

[James Cook, 2014/09/16 15:52:31]

Nice test.