Changes BookmarkBarView to only create buttons as needed

chrome/browser/ui/views/bookmarks/bookmark_bar_view_unittest.cc

Index: chrome/browser/ui/views/bookmarks/bookmark_bar_view_unittest.cc
diff --git a/chrome/browser/ui/views/bookmarks/bookmark_bar_view_unittest.cc b/chrome/browser/ui/views/bookmarks/bookmark_bar_view_unittest.cc
index 14609fb80bdb732af28f011c3725ac2b5e0b76a3..cce07b9a75e73e5d34afef51e388a2e21ba94b0a 100644
--- a/chrome/browser/ui/views/bookmarks/bookmark_bar_view_unittest.cc
+++ b/chrome/browser/ui/views/bookmarks/bookmark_bar_view_unittest.cc
@@ -5,6 +5,7 @@
 #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"
@@ -23,6 +24,7 @@
 #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:
@@ -42,15 +44,54 @@ class BookmarkBarViewTest : public BrowserWithTestWindowTest {
   }
 
  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 *visible* buttons on
+  // the bookmark bar. Each label is separated by a space.
+  std::string GetStringForVisibleButtons() {
+    std::string result;
+    for (int i = 0; i < test_helper_->GetBookmarkButtonCount() &&
+                        test_helper_->GetBookmarkButton(i)->visible();
+         ++i) {
+      if (i != 0)
+        result += " ";
+      result +=
+          base::UTF16ToASCII(test_helper_->GetBookmarkButton(i)->GetText());
+    }
+    return result;
+  }
+
+  // Continues sizing the bookmark bar until it has |count| buttons that are
+  // visible.
+  // NOTE: if the model has more than |count| buttons this results in
+  // |count| + 1 buttons.
+  void SizeUntilButtonsVisible(int count) {
+    const int start_width = bookmark_bar_view_->width();
+    const int height = bookmark_bar_view_->GetPreferredSize().height();
+    for (int i = 0;
+         i < 100 && (test_helper_->GetBookmarkButtonCount() < count ||
+                     !test_helper_->GetBookmarkButton(count - 1)->visible());
+         ++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.
@@ -63,11 +104,20 @@ class BookmarkBarViewTest : public BrowserWithTestWindowTest {
   // 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_;
 
@@ -109,6 +159,184 @@ TEST_F(BookmarkBarViewTest, AppsShortcutVisibility) {
   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());
+
+  SizeUntilButtonsVisible(1);
+  EXPECT_EQ(2, test_helper_->GetBookmarkButtonCount());
+  const int width_for_one = bookmark_bar_view_->bounds().width();
+  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());
+
+  bookmark_bar_view_->SetBounds(
+      0, 0, width_for_one, bookmark_bar_view_->bounds().height());
+  bookmark_bar_view_->Layout();
+  EXPECT_TRUE(test_helper_->overflow_button()->visible());
+}
+
+// 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();
+  EXPECT_TRUE(BookmarkModelFactory::GetForProfile(profile())->loaded());
+  AddNodesToBookmarkBarFromModelString("a b c d e f ");
+  CreateBookmarkBarView();
+  EXPECT_EQ(0, test_helper_->GetBookmarkButtonCount());
+
+  SizeUntilButtonsVisible(1);
+  EXPECT_EQ(2, 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.
+TEST_F(BookmarkBarViewTest, ButtonsDynamicallyAdded) {
+  CreateBookmarkModelAndBookmarkBarView();
+  EXPECT_TRUE(BookmarkModelFactory::GetForProfile(profile())->loaded());
+  AddNodesToBookmarkBarFromModelString("a b c d e f ");
+  EXPECT_EQ(0, test_helper_->GetBookmarkButtonCount());
+  SizeUntilButtonsVisible(1);
+  EXPECT_EQ(2, 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", GetStringForVisibleButtons());
+}
+
+// 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());
+  SizeUntilButtonsVisible(1);
+  EXPECT_EQ(2, test_helper_->GetBookmarkButtonCount());
+
+  // Remove the 2nd node, should still only have 1 visible.
+  BookmarkModelFactory::GetForProfile(profile())->Remove(bookmark_bar_node, 1);
+  EXPECT_EQ("a", GetStringForVisibleButtons());
+
+  // Remove the first node, should force a new button (for the 'c' node).
+  BookmarkModelFactory::GetForProfile(profile())->Remove(bookmark_bar_node, 0);
+  ASSERT_EQ("c", GetStringForVisibleButtons());
+}
+
+// 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.
+  SizeUntilButtonsVisible(1);
+  EXPECT_EQ("c", GetStringForVisibleButtons());
+
+  // Move 'f' first, resulting in 'f c a b d e'.
+  BookmarkModelFactory::GetForProfile(profile())
+      ->Move(bookmark_bar_node->GetChild(5), bookmark_bar_node, 0);
+  SizeUntilButtonsVisible(2);
+  EXPECT_EQ("f c", GetStringForVisibleButtons());
+
+  // 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);
+  SizeUntilButtonsVisible(2);
+  EXPECT_EQ("c a", GetStringForVisibleButtons());
+
+  // 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);
+  SizeUntilButtonsVisible(2);
+  EXPECT_EQ("a c", GetStringForVisibleButtons());
+}
+
+// Assertions for changing the title of a node.
+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.
+  SizeUntilButtonsVisible(1);
+  EXPECT_EQ("a1", GetStringForVisibleButtons());
+
+  BookmarkModelFactory::GetForProfile(profile())
+      ->SetTitle(bookmark_bar_node->GetChild(1), base::ASCIIToUTF16("b1"));
+  EXPECT_EQ("a1", GetStringForVisibleButtons());
+
+  BookmarkModelFactory::GetForProfile(profile())
+      ->SetTitle(bookmark_bar_node->GetChild(5), base::ASCIIToUTF16("f1"));
+  EXPECT_EQ("a1", GetStringForVisibleButtons());
+
+  BookmarkModelFactory::GetForProfile(profile())
+      ->SetTitle(bookmark_bar_node->GetChild(3), base::ASCIIToUTF16("d1"));
+
+  // Make the second button visible, changes the title of the first to something
+  // really long and make sure the second button hides.
+  SizeUntilButtonsVisible(2);
+  EXPECT_EQ("a1 b1", GetStringForVisibleButtons());
+  BookmarkModelFactory::GetForProfile(profile())
+      ->SetTitle(bookmark_bar_node->GetChild(0),
+                 base::ASCIIToUTF16("a_really_long_title"));
+  EXPECT_LE(1, test_helper_->GetBookmarkButtonCount());

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

nit: EXPECT_EQ("a1", GetStringForVisibleButtons());

[sky, 2014/10/27 23:00:49]

'a1' has changed to 'a_really_long_title', which is big enough that both buttons
are most likely not visible. This is why the assertion is now there should be
none or one button visible.

[msw, 2014/10/27 23:06:00]

Acknowledged.

+
+  // Change the title back and make sure the 2nd button is visible again. Don't
+  // use GetStringForVisibleButtons() here as more buttons may have been

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

nit: Is this still necessary? The visibility check that you added to
GetStringForVisibleButtons() should make it okay to test that string:
EXPECT_EQ("a1 b1", GetStringForVisibleButtons());

[sky, 2014/10/27 23:00:49]

Not strictly, but it seemed nice and kept the rest of the assertions easier to
grok.

[msw, 2014/10/27 23:06:00]

Acknowledged.

+  // created.
+  BookmarkModelFactory::GetForProfile(profile())
+      ->SetTitle(bookmark_bar_node->GetChild(0), base::ASCIIToUTF16("a1"));
+  ASSERT_LE(2, test_helper_->GetBookmarkButtonCount());
+  EXPECT_TRUE(test_helper_->GetBookmarkButton(0)->visible());
+  EXPECT_TRUE(test_helper_->GetBookmarkButton(1)->visible());
+
+  bookmark_bar_view_->SetBounds(
+      0, 0, 5000, bookmark_bar_view_->bounds().height());
+  bookmark_bar_view_->Layout();
+  EXPECT_EQ("a1 b1 c d1 e f1", GetStringForVisibleButtons());
+}
+
 #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.