Changes BookmarkBarView to only create buttons as needed

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

 // Copyright (c) 2012 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 <algorithm>
 #include <limits>
 #include <string>
 #include <vector>
@@ skipping 765 matching lines @@
   }
   if (apps_page_shortcut_->visible()) {
     gfx::Size size = apps_page_shortcut_->GetPreferredSize();
     width += size.width() + kButtonPadding;
   }
 
   return gfx::Size(width, height);
 }
 
 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;
   int width = View::width() - kRightMargin - kLeftMargin;
   int height = chrome::kBookmarkBarHeight - kBottomMargin;
   int separator_margin = kSeparatorMargin;
 
   if (IsDetached()) {
     double current_state = 1 - size_animation_->GetCurrentValue();
     x += static_cast<int>(kNewtabHorizontalPadding * current_state);
@@ skipping 41 matching lines @@
       model_->bookmark_bar_node()->child_count() == 0;
   instructions_->SetVisible(show_instructions);
   if (show_instructions) {
     gfx::Size pref = instructions_->GetPreferredSize();
     instructions_->SetBounds(
         x + kInstructionsPadding, y,
         std::min(static_cast<int>(pref.width()),
                  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.
   if (bookmarks_separator_view_->visible()) {
     bookmarks_separator_view_->SetBounds(x,
                                          y - top_margin,
                                          bookmarks_separator_pref.width(),
                                          height + top_margin + kBottomMargin -
                                          separator_margin);
 
@@ skipping 268 matching lines @@
 
 void BookmarkBarView::OnBookmarkBubbleHidden() {
   StopThrobbing(false);
 }
 
 void BookmarkBarView::BookmarkModelLoaded(BookmarkModel* model,
                                           bool ids_reassigned) {
   // 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());
   managed_bookmarks_button_->SetAccessibleName(
       client_->managed_node()->GetTitle());
   managed_bookmarks_button_->SetText(client_->managed_node()->GetTitle());
   UpdateColors();
   UpdateOtherAndManagedButtonsVisibility();
   other_bookmarked_button_->SetEnabled(true);
   managed_bookmarks_button_->SetEnabled(true);
@@ skipping 13 matching lines @@
                                         const BookmarkNode* new_parent,
                                         int new_index) {
   bool was_throbbing = throbbing_view_ &&
       throbbing_view_ == DetermineViewToThrobFromRemove(old_parent, old_index);
   if (was_throbbing)
     throbbing_view_->StopThrobbing();
   bool needs_layout_and_paint =
       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();
 }
 
 void BookmarkBarView::BookmarkNodeAdded(BookmarkModel* model,
                                         const BookmarkNode* parent,
                                         int index) {
   if (BookmarkNodeAddedImpl(model, parent, index))
     LayoutAndPaint();
@@ skipping 296 matching lines @@
 }
 
 int BookmarkBarView::GetBookmarkButtonCount() const {
   // We contain six non-bookmark button views: managed bookmarks,
   // other bookmarks, bookmarks separator, chevrons (for overflow), apps page,
   // and the instruction label.
   return child_count() - 6;
 }
 
 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));
 }
 
 BookmarkLaunchLocation BookmarkBarView::GetBookmarkLaunchLocation() const {
   return IsDetached() ? BOOKMARK_LAUNCH_LOCATION_DETACHED_BAR :
                         BOOKMARK_LAUNCH_LOCATION_ATTACHED_BAR;
 }
 
 int BookmarkBarView::GetFirstHiddenNodeIndex() {
   const int bb_count = GetBookmarkButtonCount();
@@ skipping 98 matching lines @@
   if (node->is_url()) {
     const gfx::Image& favicon = model_->GetFavicon(node);
     if (!favicon.IsEmpty())
       button->SetImage(views::Button::STATE_NORMAL, *favicon.ToImageSkia());
     else
       button->SetImage(views::Button::STATE_NORMAL, GetDefaultFavicon());
   }
   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,
                                               const BookmarkNode* parent,
                                               int index) {
   const bool needs_layout = UpdateOtherAndManagedButtonsVisibility();
 
   StopThrobbing(true);
   // No need to start throbbing again as the bookmark bubble can't be up at
   // the same time as the user reorders.
 
   if (parent != model->bookmark_bar_node()) {
     // Only children of the bookmark_bar_node get buttons.
     return needs_layout;
   }
+  if (index >= GetBookmarkButtonCount())
+    return needs_layout;
+
   delete child_at(index);
   return true;
 }
 
 void BookmarkBarView::BookmarkNodeChangedImpl(BookmarkModel* model,
                                               const BookmarkNode* node) {
   if (node == client_->managed_node()) {
     // The managed node may have its title updated.
     managed_bookmarks_button_->SetAccessibleName(
         client_->managed_node()->GetTitle());
     managed_bookmarks_button_->SetText(client_->managed_node()->GetTitle());
     return;
   }
 
   if (node->parent() != model->bookmark_bar_node()) {
     // We only care about nodes on the bookmark bar.
     return;
   }
   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) {
   if (bookmark_drop_menu_) {
     if (bookmark_drop_menu_->node() == node) {
       // Already showing for the specified node.
       return;
     }
     bookmark_drop_menu_->Cancel();
   }
@@ skipping 256 matching lines @@
     return;
   apps_page_shortcut_->SetVisible(visible);
   UpdateBookmarksSeparatorVisibility();
   LayoutAndPaint();
 }
 
 void BookmarkBarView::OnShowManagedBookmarksPrefChanged() {
   if (UpdateOtherAndManagedButtonsVisibility())
     LayoutAndPaint();
 }