Don't make all bookmark buttons the same width.

Make thinner if possible.
Feelin luv 4 pink.

BUG=http://crbug.com/16942

TEST=2 main parts:
1)
Create bookmarks (click 'Star').
Make sure long titles get trimmed.
Make sure small titles have smaller buttons and text does NOT get trimmed.
Quit and launch with bookmark bar open.
Make sure things are still fine (icons load after bar first assembled).
2)
Quit Chromium.
Rename a LONG button title by editing ~/Library/Application Support/Chromium/Default/Bookmarks.
Relaunch Chromium.
Make sure the button is now smaller but is not "trimmed" (e.g. all the text is there).
Repeat for 5 buttons with different short (5-10 char) titles on each.
Make sure the buttons are smaller and the text is NOT trimmed.


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

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

3 files changed, 212 insertions, 42 deletions

diff --git a/chrome/browser/cocoa/bookmark_bar_controller.h b/chrome/browser/cocoa/bookmark_bar_controller.h
index 1e98b93..662da61 100644
--- a/chrome/browser/cocoa/bookmark_bar_controller.h
+++ b/chrome/browser/cocoa/bookmark_bar_controller.h
@@ -40,6 +40,9 @@ class PrefService;
   BOOL contentViewHasOffset_;
   BOOL barShouldBeShown_;
 
+  // Our bookmark buttons, ordered from L-->R.
+  scoped_nsobject<NSMutableArray> buttons_;
+
   // If the bar is disabled, we hide it and ignore show/hide commands.
   // Set when using fullscreen mode.
   BOOL barIsEnabled_;
@@ -115,6 +118,10 @@ class PrefService;
 @interface BookmarkBarController(TestingAPI)
 // Set the delegate for a unit test.
 - (void)setDelegate:(id<BookmarkURLOpener>)delegate;
+- (void)clearBookmarkBar;
+- (NSArray*)buttons;
+- (NSRect)frameForBookmarkButtonFromCell:(NSCell*)cell xOffset:(int*)xOffset;
+- (void)checkForBookmarkButtonGrowth:(NSButton*)button;
 @end
 
 #endif  // CHROME_BROWSER_COCOA_BOOKMARK_BAR_CONTROLLER_H_
diff --git a/chrome/browser/cocoa/bookmark_bar_controller.mm b/chrome/browser/cocoa/bookmark_bar_controller.mm
index 7d1ea55..d4af000 100644
--- a/chrome/browser/cocoa/bookmark_bar_controller.mm
+++ b/chrome/browser/cocoa/bookmark_bar_controller.mm
@@ -36,7 +36,7 @@ const int kBookmarkBarWebframeHeightAdjustment = 25;
 // Magic numbers from Cole
 const CGFloat kDefaultBookmarkWidth = 150.0;
 const CGFloat kBookmarkVerticalPadding = 2.0;
-const CGFloat kBookmarkHorizontalPadding = 8.0;
+const CGFloat kBookmarkHorizontalPadding = 1.0;
 };
 
 @implementation BookmarkBarController
@@ -53,6 +53,7 @@ const CGFloat kBookmarkHorizontalPadding = 8.0;
     parentView_ = parentView;
     webContentView_ = webContentView;
     infoBarsView_ = infoBarsView;
+    buttons_.reset([[NSMutableArray alloc] init]);
     delegate_ = delegate;
   }
   return self;
@@ -237,16 +238,15 @@ const CGFloat kBookmarkHorizontalPadding = 8.0;
                          node->GetParent()->IndexOfChild(node));
 }
 
-// Delete all items from the bookmark bar.  TODO(jrg): once the
-// bookmark bar has other subviews (e.g. "off the side" button/menu,
-// "Other Bookmarks"), etc, this routine will need revisiting.
+// Delete all bookmarks from the bookmark bar.
 - (void)clearBookmarkBar {
-  [[self view] setSubviews:[NSArray array]];
+  [buttons_ makeObjectsPerformSelector:@selector(removeFromSuperview)];
+  [buttons_ removeAllObjects];
 }
 
 // Return an autoreleased NSCell suitable for a bookmark button.
 // TODO(jrg): move much of the cell config into the BookmarkButtonCell class.
-- (NSCell *)cellForBookmarkNode:(const BookmarkNode*)node frame:(NSRect)frame {
+- (NSCell*)cellForBookmarkNode:(const BookmarkNode*)node {
   NSString* title = base::SysWideToNSString(node->GetTitle());
   NSButtonCell *cell = [[[BookmarkButtonCell alloc] initTextCell:nil]
                          autorelease];
@@ -268,22 +268,59 @@ const CGFloat kBookmarkHorizontalPadding = 8.0;
   return cell;
 }
 
-// TODO(jrg): accomodation for bookmarks less than minimum width in
-// size (like Windows)?
-- (NSRect)frameForBookmarkAtIndex:(int)index {
+// Return an appropriate width for the given bookmark button cell.
+// The "+1" is needed because, sometimes, Cocoa is off by one.
+// Example: for a bookmark named "Moma" or "SFGate", it is one pixel
+// too small.  For a bookmark named "SFGateFooWoo", it is just fine.
+- (CGFloat)widthForBookmarkButtonCell:(NSCell*)cell {
+  CGFloat desired = [cell cellSize].width + 1;
+  return std::min(desired, kDefaultBookmarkWidth);
+}
+
+// Returns a frame appropriate for the given bookmark cell, suitable
+// for creating an NSButton that will contain it.  |xOffset| is the X
+// offset for the frame; it is increased to be an appropriate X offset
+// for the next button.
+- (NSRect)frameForBookmarkButtonFromCell:(NSCell*)cell
+                                 xOffset:(int*)xOffset {
   NSRect bounds = [[self view] bounds];
   // TODO: be smarter about this; the animator delays the right height
   if (bounds.size.height == 0)
     bounds.size.height = kBookmarkBarHeight;
-
   NSRect frame = NSInsetRect(bounds,
                              kBookmarkHorizontalPadding,
                              kBookmarkVerticalPadding);
-  frame.origin.x += (kDefaultBookmarkWidth * index);
-  frame.size.width = kDefaultBookmarkWidth;
+  frame.size.width = [self widthForBookmarkButtonCell:cell];
+
+  // Add an X offset based on what we've already done
+  frame.origin.x += *xOffset;
+
+  // And up the X offset for next time.
+  *xOffset = NSMaxX(frame);
+
   return frame;
 }
 
+// A bookmark button's contents changed.  Check for growth
+// (e.g. increase the width up to the maximum).  If we grew, move
+// other bookmark buttons over.
+- (void)checkForBookmarkButtonGrowth:(NSButton*)button {
+  NSRect frame = [button frame];
+  CGFloat desiredSize = [self widthForBookmarkButtonCell:[button cell]];
+  CGFloat delta = desiredSize - frame.size.width;
+  if (delta) {
+    frame.size.width = desiredSize;
+    [button setFrame:frame];
+    for (NSButton* each_button in buttons_.get()) {
+      NSRect each_frame = [each_button frame];
+      if (each_frame.origin.x > frame.origin.x) {
+        each_frame.origin.x += delta;
+        [each_button setFrame:each_frame];
+      }
+    }
+  }
+}
+
 // Add all items from the given model to our bookmark bar.
 // TODO(jrg): lots of things!
 //  - bookmark folders (e.g. menu from the button)
@@ -291,26 +328,27 @@ const CGFloat kBookmarkHorizontalPadding = 8.0;
 //    screen
 //  - ...
 //
-// TODO(jrg): contextual menu (e.g. Open In New Tab) for each button
-// in this function)
-//
 // 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.
 - (void)addNodesToBar:(const BookmarkNode*)node {
+  int x_offset = 0;
   for (int i = 0; i < node->GetChildCount(); i++) {
     const BookmarkNode* child = node->GetChild(i);
 
-    NSRect frame = [self frameForBookmarkAtIndex:i];
+    NSCell* cell = [self cellForBookmarkNode:child];
+    NSRect frame = [self frameForBookmarkButtonFromCell:cell xOffset:&x_offset];
     NSButton* button = [[[NSButton alloc] initWithFrame:frame]
                          autorelease];
     DCHECK(button);
+    [buttons_ addObject:button];
+
     // [NSButton setCell:] warns to NOT use setCell: other than in the
     // initializer of a control.  However, we are using a basic
     // NSButton whose initializer does not take an NSCell as an
     // object.  To honor the assumed semantics, we do nothing with
     // NSButton between alloc/init and setCell:.
-    [button setCell:[self cellForBookmarkNode:child frame:frame]];
+    [button setCell:cell];
 
     if (child->is_folder()) {
       // For now just disable the button if it's a folder.
@@ -375,10 +413,15 @@ const CGFloat kBookmarkHorizontalPadding = 8.0;
 
 // TODO(jrg): linear searching is bad.
 // Need a BookmarkNode-->NSCell mapping.
+//
+// TODO(jrg): if the bookmark bar is open on launch, we see the
+// buttons all placed, then "scooted over" as the favicons load.  If
+// this looks bad I may need to change widthForBookmarkButtonCell to
+// add space for an image even if not there on the assumption that
+// favicons will eventually load.
 - (void)nodeFavIconLoaded:(BookmarkModel*)model
                      node:(const BookmarkNode*)node {
-  NSArray* views = [[self view] subviews];
-  for (NSButton* button in views) {
+  for (NSButton* button in buttons_.get()) {
     NSButtonCell* cell = [button cell];
     void* pointer = [[cell representedObject] pointerValue];
     const BookmarkNode* cellnode = static_cast<const BookmarkNode*>(pointer);
@@ -387,6 +430,10 @@ const CGFloat kBookmarkHorizontalPadding = 8.0;
       if (image) {
         [cell setImage:image];
         [cell setImagePosition:NSImageLeft];
+        // Adding an image means we might need more room for the
+        // bookmark.  Test for it by growing the button (if needed)
+        // and shifting everything else over.
+        [self checkForBookmarkButtonGrowth:button];
       }
       return;
     }
@@ -403,4 +450,8 @@ const CGFloat kBookmarkHorizontalPadding = 8.0;
   delegate_ = delegate;
 }
 
+- (NSArray*)buttons {
+  return buttons_.get();
+}
+
 @end
diff --git a/chrome/browser/cocoa/bookmark_bar_controller_unittest.mm b/chrome/browser/cocoa/bookmark_bar_controller_unittest.mm
index f08978e..e1dbdbc 100644
--- a/chrome/browser/cocoa/bookmark_bar_controller_unittest.mm
+++ b/chrome/browser/cocoa/bookmark_bar_controller_unittest.mm
@@ -27,6 +27,29 @@
 @end
 
 
+// NSCell that is pre-provided with a desired size that becomes the
+// return value for -(NSSize)cellSize:.
+@interface CellWithDesiredSize : NSCell {
+ @private
+  NSSize cellSize_;
+}
+@property(readonly) NSSize cellSize;
+@end
+
+@implementation CellWithDesiredSize
+
+@synthesize cellSize = cellSize_;
+
+- (id)initTextCell:(NSString*)string desiredSize:(NSSize)size {
+  if ((self = [super initTextCell:string])) {
+    cellSize_ = size;
+  }
+  return self;
+}
+
+@end
+
+
 namespace {
 
 static const int kContentAreaHeight = 500;
@@ -51,6 +74,15 @@ class BookmarkBarControllerTest : public testing::Test {
                                           infoBarsView:infobar_view_.get()
                                               delegate:nil]);
     [bar_ view];  // force loading of the nib
+
+    // Awkwardness to look like we've been installed.
+    [parent_view_ addSubview:[bar_ view]];
+    NSRect frame = [[[bar_ view] superview] frame];
+    frame.origin.y = 100;
+    [[[bar_ view] superview] setFrame:frame];
+
+    // make sure it's open so certain things aren't no-ops
+    [bar_ toggleBookmarkBar];
   }
 
   CocoaTestHelper cocoa_helper_;  // Inits Cocoa, creates window, etc...
@@ -62,16 +94,15 @@ class BookmarkBarControllerTest : public testing::Test {
 };
 
 TEST_F(BookmarkBarControllerTest, ShowHide) {
-  // Assume hidden by default in a new profile.
+  // The test class opens the bar by default since many actions are
+  // no-ops with it closed.  Set back to closed as a baseline.
+  if ([bar_ isBookmarkBarVisible])
+    [bar_ toggleBookmarkBar];
+
+  // Start hidden.
   EXPECT_FALSE([bar_ isBookmarkBarVisible]);
   EXPECT_TRUE([[bar_ view] isHidden]);
 
-  // Awkwardness to look like we've been installed.
-  [parent_view_ addSubview:[bar_ view]];
-  NSRect frame = [[[bar_ view] superview] frame];
-  frame.origin.y = 100;
-  [[[bar_ view] superview] setFrame:frame];
-
   // Show and hide it by toggling.
   [bar_ toggleBookmarkBar];
   EXPECT_TRUE([bar_ isBookmarkBarVisible]);
@@ -151,31 +182,112 @@ TEST_F(BookmarkBarControllerTest, OpenBookmarkFromMenus) {
   }
 }
 
+TEST_F(BookmarkBarControllerTest, TestAddRemoveAndClear) {
+  BookmarkModel* model = helper_.profile()->GetBookmarkModel();
 
-// TODO(jrg): Make sure showing the bookmark bar calls loaded: (to
-// process bookmarks)
-TEST_F(BookmarkBarControllerTest, ShowAndLoad) {
+  EXPECT_EQ(0U, [[bar_ buttons] count]);
+  unsigned int initial_subview_count = [[[bar_ view] subviews] count];
+
+  // Make sure a redundant call doesn't choke
+  [bar_ clearBookmarkBar];
+  EXPECT_EQ(0U, [[bar_ buttons] count]);
+  EXPECT_EQ(initial_subview_count, [[[bar_ view] subviews] count]);
+
+  GURL gurl1("http://superfriends.hall-of-justice.edu");
+  std::wstring title1(L"Protectors of the Universe");
+  model->SetURLStarred(gurl1, title1, true);
+  EXPECT_EQ(1U, [[bar_ buttons] count]);
+  EXPECT_EQ(1+initial_subview_count, [[[bar_ view] subviews] count]);
+
+  GURL gurl2("http://legion-of-doom.gov");
+  std::wstring title2(L"Bad doodz");
+  model->SetURLStarred(gurl2, title2, true);
+  EXPECT_EQ(2U, [[bar_ buttons] count]);
+  EXPECT_EQ(2+initial_subview_count, [[[bar_ view] subviews] count]);
+
+  for (int i = 0; i < 3; i++) {
+    // is_starred=false --> remove the bookmark
+    model->SetURLStarred(gurl2, title2, false);
+    EXPECT_EQ(1U, [[bar_ buttons] count]);
+    EXPECT_EQ(1+initial_subview_count, [[[bar_ view] subviews] count]);
+
+    // and bring it back
+    model->SetURLStarred(gurl2, title2, true);
+    EXPECT_EQ(2U, [[bar_ buttons] count]);
+    EXPECT_EQ(2+initial_subview_count, [[[bar_ view] subviews] count]);
+  }
+
+  [bar_ clearBookmarkBar];
+  EXPECT_EQ(0U, [[bar_ buttons] count]);
+  EXPECT_EQ(initial_subview_count, [[[bar_ view] subviews] count]);
+
+  // Explicit test of loaded: since this is a convenient spot
+  [bar_ loaded:model];
+  EXPECT_EQ(2U, [[bar_ buttons] count]);
+  EXPECT_EQ(2+initial_subview_count, [[[bar_ view] subviews] count]);
 }
 
-// TODO(jrg): Make sure adding 1 bookmark adds 1 subview, and removing
-// 1 removes 1 subview.  (We can't test for a simple Clear since there
-// will soon be views in here which aren't bookmarks.)
-TEST_F(BookmarkBarControllerTest, ViewChanges) {
+// Make sure that each button we add marches to the right and does not
+// overlap with the previous one.
+TEST_F(BookmarkBarControllerTest, TestButtonMarch) {
+  scoped_nsobject<NSMutableArray> cells([[NSMutableArray alloc] init]);
+
+  CGFloat widths[] = { 10, 10, 100, 10, 500, 500, 80000, 60000, 1, 345 };
+  for (unsigned int i = 0; i < arraysize(widths); i++) {
+    NSCell* cell = [[CellWithDesiredSize alloc]
+                     initTextCell:@"foo"
+                      desiredSize:NSMakeSize(widths[i], 30)];
+    [cells addObject:cell];
+    [cell release];
+  }
+
+  int x_offset = 0;
+  CGFloat x_end = x_offset;  // end of the previous button
+  for (unsigned int i = 0; i < arraysize(widths); i++) {
+    NSRect r = [bar_ frameForBookmarkButtonFromCell:[cells objectAtIndex:i]
+                                            xOffset:&x_offset];
+    EXPECT_GE(r.origin.x, x_end);
+    x_end = NSMaxX(r);
+  }
 }
 
-// TODO(jrg): Make sure loaded: does something useful
-TEST_F(BookmarkBarControllerTest, Loaded) {
-  // Clear; make sure no views
-  // Call loaded:
-  // Make sure subviews
+TEST_F(BookmarkBarControllerTest, CheckForGrowth) {
+  BookmarkModel* model = helper_.profile()->GetBookmarkModel();
+  GURL gurl1("http://www.google.com");
+  std::wstring title1(L"x");
+  model->SetURLStarred(gurl1, title1, true);
+
+  GURL gurl2("http://www.google.com/blah");
+  std::wstring title2(L"y");
+  model->SetURLStarred(gurl2, title2, true);
+
+  EXPECT_EQ(2U, [[bar_ buttons] count]);
+  CGFloat width_1 = [[[bar_ buttons] objectAtIndex:0] frame].size.width;
+  CGFloat x_2 = [[[bar_ buttons] objectAtIndex:1] frame].origin.x;
+
+  NSButton* first = [[bar_ buttons] objectAtIndex:0];
+  [[first cell] setTitle:@"This is a really big title; watch out mom!"];
+  [bar_ checkForBookmarkButtonGrowth:first];
+
+  // Make sure the 1st button is now wider, the 2nd one is moved over,
+  // and they don't overlap.
+  NSRect frame_1 = [[[bar_ buttons] objectAtIndex:0] frame];
+  NSRect frame_2 = [[[bar_ buttons] objectAtIndex:1] frame];
+  EXPECT_GT(frame_1.size.width, width_1);
+  EXPECT_GT(frame_2.origin.x, x_2);
+  EXPECT_GE(frame_2.origin.x, frame_1.origin.x + frame_1.size.width);
 }
 
-// TODO(jrg): Test cellForBookmarkNode:
-TEST_F(BookmarkBarControllerTest, Cell) {
+// TODO(jrg): write a test to confirm that nodeFavIconLoaded calls
+// checkForBookmarkButtonGrowth:.
+
+// TODO(jrg): Make sure showing the bookmark bar calls loaded: (to
+// process bookmarks)
+TEST_F(BookmarkBarControllerTest, ShowAndLoad) {
 }
 
-// TODO(jrg): Test frameForBookmarkAtIndex
-TEST_F(BookmarkBarControllerTest, FrameAtIndex) {
+// TODO(jrg): Test cellForBookmarkNode:
+TEST_F(BookmarkBarControllerTest, Cell) {
 }
 
 TEST_F(BookmarkBarControllerTest, Contents) {