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..60f6f04c0cc08eb712ac38bb2796677f53fc1425 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,47 @@ 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 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.
@@ -63,11 +97,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 +152,159 @@ 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());
+
+  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.