Only create bookmark bar buttons that we need, not "for all".

Create the "off the side" menu on demand (since that has "all the rest" in it).

BUG= http://crbug.com/32843

TEST=1) Do the "sync 1000+ bookmarks" test from the bug and watch Chrome NOT run out of memory.
Make sure these bookmarks are all children of the bar for it to be even harder.
2) Add about 5 bookmarks.  Make sure you can grow and shrink (in
width) the window to cause the off-the-side chevron to appear and
disappear as appropriate, and that the off-the-side menu always has
the right contents (bookmarks which fall off the side).
3) Change theme then relaunch; make sure theme set on launch for bookmarks.
4) Using both a big and a small width, make sure the chevron shows (or
not, as appropriate) on launch of Chrome.

Review URL: http://codereview.chromium.org/853001

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@41359 0039d316-1c4b-4281-b951-d872f2087c98

3 files changed, 108 insertions, 58 deletions

diff --git a/chrome/browser/cocoa/bookmark_bar_controller.h b/chrome/browser/cocoa/bookmark_bar_controller.h
index f36a76e..8d2c117 100644
--- a/chrome/browser/cocoa/bookmark_bar_controller.h
+++ b/chrome/browser/cocoa/bookmark_bar_controller.h
@@ -187,6 +187,24 @@ willAnimateFromState:(bookmarks::VisualState)oldState
   // but that doesn't use this variable or need a delay so "hover" is
   // the wrong term.
   scoped_nsobject<BookmarkButton> hoverButton_;
+
+  // We save the view width when we add bookmark buttons.  This lets
+  // us avoid a rebuild until we've grown the window bigger than our
+  // initial build.
+  CGFloat savedFrameWidth_;
+
+  // The number of buttons we display in the bookmark bar.  This does
+  // not include the "off the side" chevron or the "Other Bookmarks"
+  // button.  We use this number to determine if we need to display
+  // the chevron, and to know what to place in the chevron's menu.
+  // Since we create everything before doing layout we can't be sure
+  // that all bookmark buttons we create will be visible.  Thus,
+  // [buttons_ count] isn't a definitive check.
+  int bookmarkBarDisplayedButtons_;
+
+  // We only build the off-the-side menu on demand.
+  // We flag a need to rebuild with this variable.
+  BOOL needToRebuildOffTheSideMenu_;
 }
 
 @property(readonly, nonatomic) bookmarks::VisualState visualState;
@@ -287,7 +305,7 @@ willAnimateFromState:(bookmarks::VisualState)oldState
 - (NSMenu *)menuForFolderNode:(const BookmarkNode*)node;
 - (int64)nodeIdFromMenuTag:(int32)tag;
 - (int32)menuTagFromNodeId:(int64)menuid;
-- (void)buildOffTheSideMenu;
+- (void)buildOffTheSideMenuIfNeeded;
 - (NSMenu*)offTheSideMenu;
 - (NSButton*)offTheSideButton;
 - (NSButton*)otherBookmarksButton;
diff --git a/chrome/browser/cocoa/bookmark_bar_controller.mm b/chrome/browser/cocoa/bookmark_bar_controller.mm
index d80a1cfe..e3e9e39 100644
--- a/chrome/browser/cocoa/bookmark_bar_controller.mm
+++ b/chrome/browser/cocoa/bookmark_bar_controller.mm
@@ -134,6 +134,10 @@ const NSTimeInterval kBookmarkBarAnimationDuration = 0.12;
 - (BackgroundGradientView*)backgroundGradientView;
 - (AnimatableView*)animatableView;
 
+// Create buttons for all items in the given bookmark node tree.
+// Modifies self->buttons_.  Do not add more buttons than will fit on the view.
+- (void)addNodesToButtonList:(const BookmarkNode*)node;
+
 // Puts stuff into the final visual state without animating, stopping a running
 // animation if necessary.
 - (void)finalizeVisualState;
@@ -169,7 +173,6 @@ const NSTimeInterval kBookmarkBarAnimationDuration = 0.12;
 - (int)preferredHeight;
 - (void)addNonBookmarkButtonsToView;
 - (void)addButtonsToView;
-- (void)resizeButtons;
 - (void)centerNoItemsLabel;
 - (NSImage*)getFavIconForNode:(const BookmarkNode*)node;
 - (void)setNodeForBarMenu;
@@ -364,14 +367,9 @@ const NSTimeInterval kBookmarkBarAnimationDuration = 0.12;
 // Check if we should enable or disable the off-the-side chevron.
 // Assumes that buttons which don't fit in the parent view are removed
 // from it.
-//
-// TODO(jrg): when we are smarter about creating buttons (e.g. don't
-// bother creating buttons which aren't visible), we'll have to be
-// smarter here too.
 - (void)showOrHideOffTheSideButton {
-  // Then determine if we'll hide or show it.
-  NSButton* button = [buttons_ lastObject];
-  if (button && ![button superview]) {
+  int bookmarkChildren = bookmarkModel_->GetBookmarkBarNode()->GetChildCount();
+  if (bookmarkChildren > bookmarkBarDisplayedButtons_) {
     [offTheSideButton_ setHidden:NO];
   } else {
     [offTheSideButton_ setHidden:YES];
@@ -384,7 +382,16 @@ const NSTimeInterval kBookmarkBarAnimationDuration = 0.12;
 
 // Called when our controlled frame has changed size.
 - (void)frameDidChange {
-  [self resizeButtons];
+  if (!bookmarkModel_->IsLoaded())
+    return;
+  CGFloat width = NSWidth([[self view] frame]);
+  if (width > savedFrameWidth_) {
+    savedFrameWidth_ = width;
+    [self clearBookmarkBar];
+    [self addNodesToButtonList:bookmarkModel_->GetBookmarkBarNode()];
+    [self updateTheme:[[[self view] window] themeProvider]];
+    [self addButtonsToView];
+  }
   [self positionOffTheSideButton];
   [self addButtonsToView];
   [self showOrHideOffTheSideButton];
@@ -1122,25 +1129,30 @@ static BOOL ValueInRangeInclusive(CGFloat low, CGFloat value, CGFloat high) {
   [self watchForClickOutside:YES];
 }
 
-// Rebuild the off-the-side menu, taking into account which buttons are
-// displayed.
-// TODO(jrg,viettrungluu): only (re)build the menu when necessary.
-// TODO(jrg): if we get smarter such that we don't even bother
-//   creating buttons which aren't visible, we'll need to be smarter
-//   here.
-- (void)buildOffTheSideMenu {
+// Rebuild the off-the-side menu.
+- (void)buildOffTheSideMenuIfNeeded {
   NSMenu* menu = [self offTheSideMenu];
   DCHECK(menu);
 
+  // Only rebuild if needed.  We determine we need a rebuild when the
+  // bookmark bar is cleared of buttons.
+  if (!needToRebuildOffTheSideMenu_)
+    return;
+  needToRebuildOffTheSideMenu_ = YES;
+
   // Remove old menu items (backwards order is as good as any); leave the
   // blank one at position 0 (see menu_button.h).
   for (NSInteger i = [menu numberOfItems] - 1; i >= 1 ; i--)
     [menu removeItemAtIndex:i];
 
-  // Add items corresponding to buttons which aren't displayed.
-  for (BookmarkButton* button in buttons_.get()) {
-    if (![button superview])
-      [self addNode:[button bookmarkNode] toMenu:menu];
+  // Add items corresponding to buttons which aren't displayed.  Since
+  // we build the buttons in the same order as the bookmark bar child
+  // count, we have a clear hint as to where to begin.
+  const BookmarkNode* barNode = bookmarkModel_->GetBookmarkBarNode();
+  for (int i = bookmarkBarDisplayedButtons_;
+       i < barNode->GetChildCount(); i++) {
+    const BookmarkNode* child = barNode->GetChild(i);
+    [self addNode:child toMenu:menu];
   }
 }
 
@@ -1150,10 +1162,10 @@ static BOOL ValueInRangeInclusive(CGFloat low, CGFloat value, CGFloat high) {
 }
 
 // Called by any menus which have set us as their delegate (right now just the
-// off-the-side menu?).
+// off-the-side menu).  This is the trigger for a delayed rebuild.
 - (void)menuNeedsUpdate:(NSMenu*)menu {
   if (menu == [self offTheSideMenu])
-    [self buildOffTheSideMenu];
+    [self buildOffTheSideMenuIfNeeded];
 }
 
 // As a convention we set the menu's delegate to be the button's cell
@@ -1295,6 +1307,8 @@ static BOOL ValueInRangeInclusive(CGFloat low, CGFloat value, CGFloat high) {
   [buttons_ makeObjectsPerformSelector:@selector(removeFromSuperview)];
   [buttons_ removeAllObjects];
   [self clearMenuTagMap];
+  needToRebuildOffTheSideMenu_ = YES;
+  bookmarkBarDisplayedButtons_ = 0;
 }
 
 // Return an autoreleased NSCell suitable for a bookmark button.
@@ -1395,8 +1409,6 @@ static BOOL ValueInRangeInclusive(CGFloat low, CGFloat value, CGFloat high) {
   [self openURL:node->GetURL() disposition:disposition];
 }
 
-// Create buttons for all items in the bookmark node tree.
-//
 // TODO(jrg): write a "build bar" so there is a nice spot for things
 // like the contextual menu which is invoked when not over a
 // bookmark.  On Safari that menu has a "new folder" option.
@@ -1404,12 +1416,18 @@ static BOOL ValueInRangeInclusive(CGFloat low, CGFloat value, CGFloat high) {
   BOOL hideNoItemWarning = node->GetChildCount() > 0;
   [[buttonView_ noItemTextfield] setHidden:hideNoItemWarning];
 
+  CGFloat maxViewX = NSMaxX([[self view] bounds]);
   int xOffset = 0;
   for (int i = 0; i < node->GetChildCount(); i++) {
     const BookmarkNode* child = node->GetChild(i);
 
     NSCell* cell = [self cellForBookmarkNode:child];
     NSRect frame = [self frameForBookmarkButtonFromCell:cell xOffset:&xOffset];
+
+    // Break early if the button is off the end of the parent view.
+    if (NSMinX(frame) >= maxViewX)
+      break;
+
     scoped_nsobject<BookmarkButton>
         button([[BookmarkButton alloc] initWithFrame:frame]);
     DCHECK(button.get());
@@ -1458,11 +1476,13 @@ static BOOL ValueInRangeInclusive(CGFloat low, CGFloat value, CGFloat high) {
 // the window resizes.
 - (void)addButtonsToView {
   NSView* superview = nil;
+  bookmarkBarDisplayedButtons_ = 0;
   for (NSButton* button in buttons_.get()) {
     superview = [button superview];
     if (NSMaxX([button frame]) <= NSMinX([offTheSideButton_ frame])) {
       if (!superview)
         [buttonView_ addSubview:button];
+      bookmarkBarDisplayedButtons_++;
     } else {
       if (superview)
         [button removeFromSuperview];
@@ -1470,31 +1490,6 @@ static BOOL ValueInRangeInclusive(CGFloat low, CGFloat value, CGFloat high) {
   }
 }
 
-// Helper for resizeButtons to resize buttons based on a new parent
-// view height.
-- (void)resizeButtonsInArray:(NSArray*)array {
-  NSRect parentBounds = [buttonView_ bounds];
-  CGFloat height = (bookmarks::kBookmarkButtonHeight -
-                    (bookmarks::kBookmarkVerticalPadding*2));
-  for (NSButton* button in array) {
-    NSRect frame = [button frame];
-    frame.size.height = height;
-    [button setFrame:frame];
-  }
-}
-
-// Resize our buttons; the parent view height may have changed.  This
-// applies to all bookmarks, the chevron, and the "Other Bookmarks"
-- (void)resizeButtons {
-  [self resizeButtonsInArray:buttons_.get()];
-  NSMutableArray* array = [NSMutableArray arrayWithObject:offTheSideButton_];
-  // We must handle resize before the bookmarks are loaded.  If not loaded,
-  // we have no otherBookmarksButton_ yet.
-  if (otherBookmarksButton_.get())
-    [array addObject:otherBookmarksButton_.get()];
-  [self resizeButtonsInArray:array];
-}
-
 // Create the button for "Other Bookmarks" on the right of the bar.
 - (void)createOtherBookmarksButton {
   // Can't create this until the model is loaded, but only need to
@@ -1522,9 +1517,6 @@ static BOOL ValueInRangeInclusive(CGFloat low, CGFloat value, CGFloat high) {
 
   // Now that it's here, move the chevron over.
   [self positionOffTheSideButton];
-
-  // Force it to be the right size, right now.
-  [self resizeButtons];
 }
 
 // TODO(jrg): for now this is brute force.
@@ -1533,12 +1525,12 @@ static BOOL ValueInRangeInclusive(CGFloat low, CGFloat value, CGFloat high) {
   if (!model->IsLoaded())
     return;
   // Else brute force nuke and build.
+  savedFrameWidth_ = NSWidth([[self view] frame]);
   const BookmarkNode* node = model->GetBookmarkBarNode();
   [self clearBookmarkBar];
   [self addNodesToButtonList:node];
   [self createOtherBookmarksButton];
   [self updateTheme:[[[self view] window] themeProvider]];
-  [self resizeButtons];
   [self positionOffTheSideButton];
   [self addNonBookmarkButtonsToView];
   [self addButtonsToView];
diff --git a/chrome/browser/cocoa/bookmark_bar_controller_unittest.mm b/chrome/browser/cocoa/bookmark_bar_controller_unittest.mm
index a6fe547..545aea0 100644
--- a/chrome/browser/cocoa/bookmark_bar_controller_unittest.mm
+++ b/chrome/browser/cocoa/bookmark_bar_controller_unittest.mm
@@ -555,6 +555,44 @@ TEST_F(BookmarkBarControllerTest, TestAddRemoveAndClear) {
   EXPECT_EQ(2+initial_subview_count, [[buttonView subviews] count]);
 }
 
+// Make sure we don't create too many buttons; we only really need
+// ones that will be visible.
+TEST_F(BookmarkBarControllerTest, TestButtonLimits) {
+  BookmarkModel* model = helper_.profile()->GetBookmarkModel();
+  EXPECT_EQ(0U, [[bar_ buttons] count]);
+
+  // Add one; make sure we see it.
+  const BookmarkNode* parent = model->GetBookmarkBarNode();
+  model->AddURL(parent, parent->GetChildCount(),
+                L"title", GURL("http://www.google.com"));
+  EXPECT_EQ(1U, [[bar_ buttons] count]);
+
+  // Add 30 which we expect to be 'too many'.  Make sure we don't see
+  // 30 buttons.
+  [bar_ clearBookmarkBar];
+  EXPECT_EQ(0U, [[bar_ buttons] count]);
+  for (int i=0; i<30; i++) {
+    model->AddURL(parent, parent->GetChildCount(),
+                  L"title", GURL("http://www.google.com"));
+  }
+  int count = [[bar_ buttons] count];
+  EXPECT_LT(count, 30L);
+
+  // Add 10 more (to the front of the list so the on-screen buttons
+  // would change) and make sure the count stays the same.
+  for (int i=0; i<10; i++) {
+    model->AddURL(parent, 0,  /* index is 0, so front, not end */
+                  L"title", GURL("http://www.google.com"));
+  }
+
+  // Finally, grow the view and make sure the button count goes up.
+  NSRect frame = [[bar_ view] frame];
+  frame.size.width += 600;
+  [[bar_ view] setFrame:frame];
+  int finalcount = [[bar_ buttons] count];
+  EXPECT_GT(finalcount, count);
+}
+
 // Make sure that each button we add marches to the right and does not
 // overlap with the previous one.
 TEST_F(BookmarkBarControllerTest, TestButtonMarch) {
@@ -730,7 +768,7 @@ TEST_F(BookmarkBarControllerTest, TestBuildOffTheSideMenu) {
 
   // Make sure things work when there's nothing. Note that there should always
   // be a blank first menu item.
-  [bar_ buildOffTheSideMenu];
+  [bar_ buildOffTheSideMenuIfNeeded];
   EXPECT_EQ(1, [menu numberOfItems]);
 
   // We add lots of bookmarks. At first, we expect nothing to be added to the
@@ -743,7 +781,7 @@ TEST_F(BookmarkBarControllerTest, TestBuildOffTheSideMenu) {
     model->AddURL(parent, parent->GetChildCount(),
                   L"very wide title",
                   GURL("http://www.foobar.com/"));
-    [bar_ buildOffTheSideMenu];
+    [bar_ buildOffTheSideMenuIfNeeded];
 
     if (num_off_the_side) {
       num_off_the_side++;
@@ -757,9 +795,11 @@ TEST_F(BookmarkBarControllerTest, TestBuildOffTheSideMenu) {
   EXPECT_GE(num_off_the_side, 20);
 
   // Reset, and check that the built menu is "empty" again.
-  [bar_ clearBookmarkBar];
+  const BookmarkNode* parent = model->GetBookmarkBarNode();
+  while (parent->GetChildCount())
+    model->Remove(parent, 0);
   EXPECT_EQ(0U, [[bar_ buttons] count]);
-  [bar_ buildOffTheSideMenu];
+  [bar_ buildOffTheSideMenuIfNeeded];
   EXPECT_EQ(1, [menu numberOfItems]);
 }