Changes BookmarkBarView to only create buttons as needed

chrome/browser/ui/views/bookmarks/bookmark_bar_view_unittest.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/bookmarks/bookmark_bar_view.h"
 
 #include "base/prefs/pref_service.h"
+#include "base/strings/utf_string_conversions.h"
 #include "base/values.h"
 #include "chrome/browser/bookmarks/bookmark_model_factory.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/search_engines/template_url_service_factory.h"
 #include "chrome/browser/ui/app_list/app_list_util.h"
 #include "chrome/browser/ui/views/bookmarks/bookmark_bar_view_test_helper.h"
 #include "chrome/common/pref_names.h"
 #include "chrome/common/url_constants.h"
 #include "chrome/test/base/browser_with_test_window_test.h"
 #include "chrome/test/base/scoped_testing_local_state.h"
 #include "chrome/test/base/testing_browser_process.h"
 #include "chrome/test/base/testing_pref_service_syncable.h"
 #include "components/bookmarks/browser/bookmark_model.h"
 #include "components/bookmarks/test/bookmark_test_helpers.h"
 #include "components/search_engines/search_terms_data.h"
 #include "components/search_engines/template_url_service.h"
 #include "components/search_engines/template_url_service_client.h"
 #include "ui/views/controls/button/label_button.h"
+#include "ui/views/controls/button/menu_button.h"
 
 class BookmarkBarViewTest : public BrowserWithTestWindowTest {
  public:
   BookmarkBarViewTest() {}
 
   void SetUp() override {
     BrowserWithTestWindowTest::SetUp();
     local_state_.reset(
         new ScopedTestingLocalState(TestingBrowserProcess::GetGlobal()));
   }
 
   void TearDown() override {
     test_helper_.reset();
     bookmark_bar_view_.reset();
     local_state_.reset();
     BrowserWithTestWindowTest::TearDown();
   }
 
  protected:
-  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, &BookmarkBarViewTest::CreateTemplateURLService);
-    return profile;
+  // Returns a string containing the label of each of the buttons on the
+  // bookmark bar. Each label is separated by a space.
+  std::string GetStringForButtons() {
+    std::string result;
+    for (int i = 0; i < test_helper_->GetBookmarkButtonCount(); ++i) {

[msw, 2014/10/27 20:40:25]

Doesn't GetBookmarkButtonCount include invisible buttons that were added but
don't fit (like when shrinking the window, reordering nodes, etc?). I don't
think that nuance is obvious, and it may lead to incorrect testing (or worse).

[sky, 2014/10/27 21:48:18]

Indeed. I changed things around and made the names better reflect what is
returned.

+      if (i != 0)
+        result += " ";
+      result +=
+          base::UTF16ToASCII(test_helper_->GetBookmarkButton(i)->GetText());
+    }
+    return result;
   }
 
+  // Continues sizing the bookmark bar until it has |count| buttons.
+  void SizeUntil(int count) {
+    const int start_width = bookmark_bar_view_->width();
+    const int height = bookmark_bar_view_->GetPreferredSize().height();
+    for (int i = 0; i < 100 &&

[msw, 2014/10/27 20:40:25]

nit: why not just do a while loop that increases bbv_->width() and nix |i|?

[sky, 2014/10/27 21:48:18]

If this gets to 100 something is likely very wrong, which is why I went with the
for loop.

[msw, 2014/10/27 22:50:18]

Acknowledged.

+             test_helper_->GetBookmarkButtonCount() < count; ++i) {
+      bookmark_bar_view_->SetBounds(0, 0, start_width + i * 10, height);
+      bookmark_bar_view_->Layout();
+    }
+  }
+
+  const BookmarkNode* GetBookmarkBarNode() {
+    return BookmarkModelFactory::GetForProfile(profile())->bookmark_bar_node();
+  }
+
+  void WaitForBookmarkModelToLoad() {
+    bookmarks::test::WaitForBookmarkModelToLoad(
+        BookmarkModelFactory::GetForProfile(profile()));
+  }
+
+  // Adds nodes to the bookmark bar node from |string|. See
+  // bookmarks::test::AddNodesFromModelString() for details on |string|.
+  void AddNodesToBookmarkBarFromModelString(const std::string& string) {
+    bookmarks::test::AddNodesFromModelString(
+        BookmarkModelFactory::GetForProfile(profile()),
+        GetBookmarkBarNode(),
+        string);
+  }
   // Creates the BookmarkBarView and BookmarkBarViewTestHelper. Generally you'll
   // want to use CreateBookmarkModelAndBookmarkBarView(), but use this if
   // need to create the BookmarkBarView after the model has populated.
   void CreateBookmarkBarView() {
     bookmark_bar_view_.reset(new BookmarkBarView(browser(), nullptr));
     test_helper_.reset(new BookmarkBarViewTestHelper(bookmark_bar_view_.get()));
   }
 
   // Creates the model, blocking until it loads, then creates the
   // BookmarkBarView.
   void CreateBookmarkModelAndBookmarkBarView() {
     profile()->CreateBookmarkModel(true);
-    bookmarks::test::WaitForBookmarkModelToLoad(
-        BookmarkModelFactory::GetForProfile(profile()));
+    WaitForBookmarkModelToLoad();
     CreateBookmarkBarView();
   }
 
+  // BrowserWithTestWindowTest:
+  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, &BookmarkBarViewTest::CreateTemplateURLService);
+    return profile;
+  }
+
   scoped_ptr<BookmarkBarViewTestHelper> test_helper_;
   scoped_ptr<BookmarkBarView> bookmark_bar_view_;
 
  private:
   static KeyedService* CreateTemplateURLService(
       content::BrowserContext* profile) {
     return new TemplateURLService(
         static_cast<Profile*>(profile)->GetPrefs(),
         make_scoped_ptr(new SearchTermsData), NULL,
         scoped_ptr<TemplateURLServiceClient>(), NULL, NULL, base::Closure());
@@ skipping 21 matching lines @@
   } else {
     EXPECT_TRUE(test_helper_->apps_page_shortcut()->visible());
   }
 
   // Make sure we can also properly transition from true to false.
   browser()->profile()->GetPrefs()->SetBoolean(
       bookmarks::prefs::kShowAppsShortcutInBookmarkBar, false);
   EXPECT_FALSE(test_helper_->apps_page_shortcut()->visible());
 }
 
+// Various assertions around visibilty of the overflow_button.
+TEST_F(BookmarkBarViewTest, OverflowVisibility) {
+  profile()->CreateBookmarkModel(true);
+  CreateBookmarkBarView();
+  EXPECT_FALSE(test_helper_->overflow_button()->visible());
+
+  WaitForBookmarkModelToLoad();
+  AddNodesToBookmarkBarFromModelString("a b c d e f ");
+  EXPECT_TRUE(test_helper_->overflow_button()->visible());
+
+  SizeUntil(1);
+  EXPECT_EQ(1, test_helper_->GetBookmarkButtonCount());
+  EXPECT_TRUE(test_helper_->overflow_button()->visible());
+
+  // Go really big, which should force all buttons to be added.
+  bookmark_bar_view_->SetBounds(
+      0, 0, 5000, bookmark_bar_view_->bounds().height());
+  bookmark_bar_view_->Layout();
+  EXPECT_EQ(6, test_helper_->GetBookmarkButtonCount());
+  EXPECT_FALSE(test_helper_->overflow_button()->visible());

[msw, 2014/10/27 20:40:25]

nit: test shrinking the bookmark bar view here.

[sky, 2014/10/27 21:48:18]

Done.

+}
+
+// Verifies buttons get added correctly when BookmarkBarView is created after
+// the model and the model has nodes.
+TEST_F(BookmarkBarViewTest, ButtonsDynamicallyAddedAfterModelHasNodes) {
+  profile()->CreateBookmarkModel(true);
+  WaitForBookmarkModelToLoad();
+  AddNodesToBookmarkBarFromModelString("a b c d e f ");
+  CreateBookmarkBarView();
+  EXPECT_EQ(0, test_helper_->GetBookmarkButtonCount());
+
+  SizeUntil(1);
+  EXPECT_EQ(1, test_helper_->GetBookmarkButtonCount());
+
+  // Go really big, which should force all buttons to be added.
+  bookmark_bar_view_->SetBounds(
+      0, 0, 5000, bookmark_bar_view_->bounds().height());
+  bookmark_bar_view_->Layout();
+  EXPECT_EQ(6, test_helper_->GetBookmarkButtonCount());
+}
+
+// Verifies buttons are added as the model and size change.

[msw, 2014/10/27 20:40:25]

So this test is just like above, but doesn't call WaitForBookmarkModelToLoad()?
Can you verify that the model hasn't yet loaded before some operation, and has
indeed loaded afterward, as expected?

[sky, 2014/10/27 21:48:18]

Done.

+TEST_F(BookmarkBarViewTest, ButtonsDynamicallyAdded) {
+  CreateBookmarkModelAndBookmarkBarView();
+  AddNodesToBookmarkBarFromModelString("a b c d e f ");
+  EXPECT_EQ(0, test_helper_->GetBookmarkButtonCount());
+  SizeUntil(1);
+  EXPECT_EQ(1, test_helper_->GetBookmarkButtonCount());
+
+  // Go really big, which should force all buttons to be added.
+  bookmark_bar_view_->SetBounds(
+      0, 0, 5000, bookmark_bar_view_->bounds().height());
+  bookmark_bar_view_->Layout();
+  EXPECT_EQ(6, test_helper_->GetBookmarkButtonCount());
+}
+
+TEST_F(BookmarkBarViewTest, AddNodesWhenBarAlreadySized) {
+  CreateBookmarkModelAndBookmarkBarView();
+  bookmark_bar_view_->SetBounds(
+      0, 0, 5000, bookmark_bar_view_->bounds().height());
+  AddNodesToBookmarkBarFromModelString("a b c d e f ");
+  bookmark_bar_view_->Layout();
+  EXPECT_EQ("a b c d e f", GetStringForButtons());
+}
+
+// Various assertions for removing nodes.
+TEST_F(BookmarkBarViewTest, RemoveNode) {
+  CreateBookmarkModelAndBookmarkBarView();
+  const BookmarkNode* bookmark_bar_node =
+      BookmarkModelFactory::GetForProfile(profile())->bookmark_bar_node();
+  AddNodesToBookmarkBarFromModelString("a b c d e f ");
+  EXPECT_EQ(0, test_helper_->GetBookmarkButtonCount());

[msw, 2014/10/27 20:40:25]

nit: is it worthwhile for multiple tests to expect 0/1 around SizeUntil?

[sky, 2014/10/27 21:48:18]

I view it like a DCHECK. If this fails then the rest of the test results should
be suspect. I think it's good to know when things went wrong.

[msw, 2014/10/27 22:50:18]

Acknowledged.

+  SizeUntil(1);
+  EXPECT_EQ(1, test_helper_->GetBookmarkButtonCount());
+
+  // Remove the 2nd node, should still only have 1 visible.
+  BookmarkModelFactory::GetForProfile(profile())->Remove(bookmark_bar_node, 1);
+  EXPECT_EQ(1, test_helper_->GetBookmarkButtonCount());

[msw, 2014/10/27 20:40:25]

nit: EXPECT_EQ("a", GetStringForButtons()); instead?

[sky, 2014/10/27 21:48:18]

Done.

+
+  // Remove the first node, should force a new button (for the 'c' node).
+  BookmarkModelFactory::GetForProfile(profile())->Remove(bookmark_bar_node, 0);
+  ASSERT_EQ("c", GetStringForButtons());
+}
+
+// Assertions for moving a node on the bookmark bar.
+TEST_F(BookmarkBarViewTest, MoveNode) {
+  CreateBookmarkModelAndBookmarkBarView();
+  const BookmarkNode* bookmark_bar_node =
+      BookmarkModelFactory::GetForProfile(profile())->bookmark_bar_node();
+  AddNodesToBookmarkBarFromModelString("a b c d e f ");
+  EXPECT_EQ(0, test_helper_->GetBookmarkButtonCount());
+
+  // Move 'c' first resulting in 'c a b d e f'.
+  BookmarkModelFactory::GetForProfile(profile())
+      ->Move(bookmark_bar_node->GetChild(2), bookmark_bar_node, 0);
+  EXPECT_EQ(0, test_helper_->GetBookmarkButtonCount());
+
+  // Make enough room for 1 node.
+  SizeUntil(1);
+  EXPECT_EQ("c", GetStringForButtons());
+
+  // Move 'f' first, resulting in 'f c a b d e'.
+  BookmarkModelFactory::GetForProfile(profile())
+      ->Move(bookmark_bar_node->GetChild(5), bookmark_bar_node, 0);
+  // Even though only one node fits, the second node isn't deleted.
+  SizeUntil(2);
+  EXPECT_EQ("f c", GetStringForButtons());
+
+  // Move 'f' to the end, resulting in 'c a b d e f'.
+  BookmarkModelFactory::GetForProfile(profile())
+      ->Move(bookmark_bar_node->GetChild(0), bookmark_bar_node, 6);
+  // Even though only one node fits, the second node isn't deleted.

[msw, 2014/10/27 20:40:25]

nit: revise this comment to match the check.

[sky, 2014/10/27 21:48:18]

Done.

+  EXPECT_EQ("c", GetStringForButtons());
+
+  // Move 'c' after 'a', resulting in 'a c b d e f'.
+  BookmarkModelFactory::GetForProfile(profile())
+      ->Move(bookmark_bar_node->GetChild(0), bookmark_bar_node, 2);
+  // Even though only one node fits, the second node isn't deleted.

[msw, 2014/10/27 20:40:25]

nit: revise this comment to match the check.

[sky, 2014/10/27 21:48:18]

Done.

+  EXPECT_EQ("a", GetStringForButtons());
+}
+
+// Assertions for changing the title of a node.

[msw, 2014/10/27 20:40:25]

Should we test that increasing the name length could cause a button to hide?

[sky, 2014/10/27 21:48:18]

Done.

+TEST_F(BookmarkBarViewTest, ChangeTitle) {
+  CreateBookmarkModelAndBookmarkBarView();
+  const BookmarkNode* bookmark_bar_node = GetBookmarkBarNode();
+  AddNodesToBookmarkBarFromModelString("a b c d e f ");
+  EXPECT_EQ(0, test_helper_->GetBookmarkButtonCount());
+
+  BookmarkModelFactory::GetForProfile(profile())
+      ->SetTitle(bookmark_bar_node->GetChild(0), base::ASCIIToUTF16("a1"));
+  EXPECT_EQ(0, test_helper_->GetBookmarkButtonCount());
+
+  // Make enough room for 1 node.
+  SizeUntil(1);
+  EXPECT_EQ("a1", GetStringForButtons());
+
+  BookmarkModelFactory::GetForProfile(profile())
+      ->SetTitle(bookmark_bar_node->GetChild(1), base::ASCIIToUTF16("b1"));
+  EXPECT_EQ("a1", GetStringForButtons());
+
+  BookmarkModelFactory::GetForProfile(profile())
+      ->SetTitle(bookmark_bar_node->GetChild(5), base::ASCIIToUTF16("f1"));
+  EXPECT_EQ("a1", GetStringForButtons());
+
+  BookmarkModelFactory::GetForProfile(profile())
+      ->SetTitle(bookmark_bar_node->GetChild(3), base::ASCIIToUTF16("d1"));
+
+  bookmark_bar_view_->SetBounds(
+      0, 0, 5000, bookmark_bar_view_->bounds().height());
+  bookmark_bar_view_->Layout();
+  EXPECT_EQ("a1 b1 c d1 e f1", GetStringForButtons());
+}
+
 #if !defined(OS_CHROMEOS)
 // Verifies that the apps shortcut is shown or hidden following the policy
 // value. This policy (and the apps shortcut) isn't present on ChromeOS.
 TEST_F(BookmarkBarViewTest, ManagedShowAppsShortcutInBookmarksBar) {
   CreateBookmarkModelAndBookmarkBarView();
   // By default, the pref is not managed and the apps shortcut is shown.
   TestingPrefServiceSyncable* prefs = profile()->GetTestingPrefService();
   EXPECT_FALSE(prefs->IsManagedPreference(
       bookmarks::prefs::kShowAppsShortcutInBookmarkBar));
   EXPECT_TRUE(test_helper_->apps_page_shortcut()->visible());
 
   // Hide the apps shortcut by policy, via the managed pref.
   prefs->SetManagedPref(bookmarks::prefs::kShowAppsShortcutInBookmarkBar,
                         new base::FundamentalValue(false));
   EXPECT_FALSE(test_helper_->apps_page_shortcut()->visible());
 
   // And try showing it via policy too.
   prefs->SetManagedPref(bookmarks::prefs::kShowAppsShortcutInBookmarkBar,
                         new base::FundamentalValue(true));
   EXPECT_TRUE(test_helper_->apps_page_shortcut()->visible());
 }
 #endif