Changes BookmarkBarView to only create buttons as needed

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

Index: chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc
diff --git a/chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc b/chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc
index 772d0a9d7570fac7747d1515f1572d477cbb6ce0..83c98541bcd818ba98b9629adf4961b8c8a8c57c 100644
--- a/chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc
+++ b/chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc
@@ -783,6 +783,11 @@ gfx::Size BookmarkBarView::GetMinimumSize() const {
 }
 
 void BookmarkBarView::Layout() {
+  // If we have no model we're being destroyed and there is no point in doing a

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

nit: "Skip layout during destruction, when no model exists."

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

Done.

+  // layout.
+  if (!model_)
+    return;
+
   int x = kLeftMargin;
   int top_margin = IsDetached() ? kDetachedTopMargin : 0;
   int y = top_margin;
@@ -844,26 +849,44 @@ void BookmarkBarView::Layout() {
                  max_x - x),
         height);
   } else {
-    for (int i = 0; i < GetBookmarkButtonCount(); ++i) {
+    bool last_visible = x < max_x;
+    int button_count = GetBookmarkButtonCount();
+    for (int i = 0; i <= button_count; ++i) {
+      if (i == button_count) {
+        // Add another button if there is room for it (and there is another
+        // button to load).
+        if (!last_visible || !model_->loaded() ||
+            model_->bookmark_bar_node()->child_count() <= button_count)
+          break;
+        LoadNextBookmarkButton();
+        DCHECK_NE(button_count, GetBookmarkButtonCount());

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

nit: DCHECK_LT, if at all?

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

I removed the DCHECK as LoadNextBookmarkButton has it.

+        button_count = GetBookmarkButtonCount();
+      }
       views::View* child = child_at(i);
       gfx::Size pref = child->GetPreferredSize();
       int next_x = x + pref.width() + kButtonPadding;
-      child->SetVisible(next_x < max_x);
-      child->SetBounds(x, y, pref.width(), height);
+      last_visible = next_x < max_x;
+      child->SetVisible(last_visible);
+      // Only need to set bounds if the view is actually visible.
+      if (last_visible)
+        child->SetBounds(x, y, pref.width(), height);
       x = next_x;
     }
   }
 
   // Layout the right side of the bar.

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

nit: Remove this comment.

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

Done.

-  const bool all_visible = (GetBookmarkButtonCount() == 0 ||
-                            child_at(GetBookmarkButtonCount() - 1)->visible());
 
   // Layout the right side buttons.
   x = max_x + kButtonPadding;
 
   // The overflow button.
   overflow_button_->SetBounds(x, y, overflow_pref.width(), height);
-  overflow_button_->SetVisible(!all_visible);
+  const bool show_overflow =
+      model_->loaded() &&
+      (model_->bookmark_bar_node()->child_count() != GetBookmarkButtonCount() ||

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

nit: explicitly check for '>' instead of '!='.

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

Done.

+       (GetBookmarkButtonCount() > 0 &&
+        !GetBookmarkButton(GetBookmarkButtonCount() - 1)->visible()));
+  overflow_button_->SetVisible(show_overflow);
   x += overflow_pref.width();
 
   // Separator.
@@ -1152,11 +1175,7 @@ void BookmarkBarView::BookmarkModelLoaded(BookmarkModel* model,
   // There should be no buttons. If non-zero it means Load was invoked more than
   // once, or we didn't properly clear things. Either of which shouldn't happen.
   DCHECK_EQ(0, GetBookmarkButtonCount());
-  const BookmarkNode* node = model->bookmark_bar_node();
-  DCHECK(node);
-  // Create a button for each of the children on the bookmark bar.
-  for (int i = 0, child_count = node->child_count(); i < child_count; ++i)
-    AddChildViewAt(CreateBookmarkButton(node->GetChild(i)), i);
+  // The actual bookmark buttons are added from Layout().

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

nit: combine with the comment above.

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

Done.

   DCHECK(model->other_node());
   other_bookmarked_button_->SetAccessibleName(model->other_node()->GetTitle());
   other_bookmarked_button_->SetText(model->other_node()->GetTitle());
@@ -1190,7 +1209,7 @@ void BookmarkBarView::BookmarkNodeMoved(BookmarkModel* model,
       BookmarkNodeRemovedImpl(model, old_parent, old_index);
   if (BookmarkNodeAddedImpl(model, new_parent, new_index))
     needs_layout_and_paint = true;
-  if (was_throbbing)
+  if (was_throbbing && new_index < GetBookmarkButtonCount())
     StartThrobbing(new_parent->GetChild(new_index), false);
   if (needs_layout_and_paint)
     LayoutAndPaint();
@@ -1507,7 +1526,8 @@ int BookmarkBarView::GetBookmarkButtonCount() const {
 }
 
 views::LabelButton* BookmarkBarView::GetBookmarkButton(int index) {
-  DCHECK(index >= 0 && index < GetBookmarkButtonCount());
+  // CHECK as otherwise we may do the wrong cast.
+  CHECK(index >= 0 && index < GetBookmarkButtonCount());
   return static_cast<views::LabelButton*>(child_at(index));
 }
 
@@ -1626,22 +1646,31 @@ void BookmarkBarView::ConfigureButton(const BookmarkNode* node,
   button->SetMaxSize(gfx::Size(kMaxButtonWidth, 0));
 }
 
+void BookmarkBarView::LoadNextBookmarkButton() {

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

This is only called in Layout, consider in-lining there.

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

Done.

+  DCHECK(model_->loaded());
+  const int model_index = GetBookmarkButtonCount();
+  DCHECK_LT(model_index, model_->bookmark_bar_node()->child_count());
+  AddChildViewAt(
+      CreateBookmarkButton(model_->bookmark_bar_node()->GetChild(model_index)),
+      model_index);
+  // NOTE: no layout/schedulepaint here. We assume this is called during layout.
+}
+
 bool BookmarkBarView::BookmarkNodeAddedImpl(BookmarkModel* model,
                                             const BookmarkNode* parent,
                                             int index) {
   const bool needs_layout_and_paint = UpdateOtherAndManagedButtonsVisibility();
-  if (parent != model->bookmark_bar_node()) {
-    // Only children of the bookmark_bar_node get buttons.
+  if (parent != model->bookmark_bar_node())
     return needs_layout_and_paint;
+  if (index < GetBookmarkButtonCount()) {
+    const BookmarkNode* node = parent->GetChild(index);
+    AddChildViewAt(CreateBookmarkButton(node), index);
+    return true;
   }
-  DCHECK(index >= 0 && index <= GetBookmarkButtonCount());
-  const BookmarkNode* node = parent->GetChild(index);
-  ProfileSyncService* sync_service(ProfileSyncServiceFactory::
-      GetInstance()->GetForProfile(browser_->profile()));
-  if (!throbbing_view_ && sync_service && sync_service->FirstSetupInProgress())
-    StartThrobbing(node, true);
-  AddChildViewAt(CreateBookmarkButton(node), index);
-  return true;
+  // If the new node was added after the last button we've created we may be
+  // able to fit it. Assume we can by returning true, which forces a Layout()
+  // and creation of the button (if it fits).
+  return index == GetBookmarkButtonCount();
 }
 
 bool BookmarkBarView::BookmarkNodeRemovedImpl(BookmarkModel* model,
@@ -1657,6 +1686,9 @@ bool BookmarkBarView::BookmarkNodeRemovedImpl(BookmarkModel* model,
     // Only children of the bookmark_bar_node get buttons.
     return needs_layout;
   }
+  if (index >= GetBookmarkButtonCount())
+    return needs_layout;
+
   delete child_at(index);
   return true;
 }
@@ -1677,15 +1709,13 @@ void BookmarkBarView::BookmarkNodeChangedImpl(BookmarkModel* model,
   }
   int index = model->bookmark_bar_node()->GetIndexOf(node);
   DCHECK_NE(-1, index);
+  if (index >= GetBookmarkButtonCount())
+    return;  // Buttons are created as needed.
   views::LabelButton* button = GetBookmarkButton(index);
-  gfx::Size old_pref = button->GetPreferredSize();
+  const int old_pref_width = button->GetPreferredSize().width();
   ConfigureButton(node, button);
-  gfx::Size new_pref = button->GetPreferredSize();
-  if (old_pref.width() != new_pref.width()) {
+  if (old_pref_width != button->GetPreferredSize().width())
     LayoutAndPaint();
-  } else if (button->visible()) {
-    button->SchedulePaint();
-  }
 }
 
 void BookmarkBarView::ShowDropFolderForNode(const BookmarkNode* node) {