Add the "Other bookmarks" button on the right of the bookmark bar....

Add the "Other bookmarks" button on the right of the bookmark bar.

Lots of fixes to deal better with the floating bar on the
  NTP (e.g. chevron resize, chevron enabling only when there is
  overflow, ...)

Move layoutSubviews logic from bookmark_bar_toolbar_view to
  bookmark_bar_controller to better honor MVC.

BUG=http://crbug.com/24985, http://crbug.com/24827, http://crbug.com/22018

TEST=Lots of moves:
New browser with no bookmarks.
Go to New Tab Page.  Show Bookmark Bar.  
See "Other Bookmarks" button on right.
Do NOT see chevron.
Hover over and click on Other Bookmarks to be sure it doesn't draw out of bounds.
Disable bookmark bar (so it is now floating).
Repeat hover/click test.
Enable (so now attached).
Add bookmarks until chevron shows up.
Repeat hover/click test with chevron and Other Bookmarks.
Make browser wider and thinner; each time repeat hover/click test.
Detach bookmark bar (so floating on NTP).
Make browser wider and thinner; each time repeat hover/click test.
Carefully make browser wider/thinner so chevron appears/disappears.
Make sure chevron never overlaps a bookmark.
With chevron visible, quit & relaunch.  Make sure chevron visible.
With chevron NOT visible, quit & relaunch.  Make sure chevron not visible.



Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=30684

[pink (ping after 24hrs), 2009-10-30 13:55:09]

LGTM mostly style nits.

http://codereview.chromium.org/348017/diff/4001/5002
File chrome/browser/cocoa/bookmark_bar_controller.mm (right):

http://codereview.chromium.org/348017/diff/4001/5002#newcode198
Line 198: NSMakeRect(bookmarks::kNTPBookmarkBarPadding,
you can probably outdent this a bit so you have more room for the parameters and
avoid wrapping.

http://codereview.chromium.org/348017/diff/4001/5002#newcode445
Line 445: for (NSButton* each_button in buttons_.get()) {
obj-c naming rules apply. I'd just call it "button" because while it reads a
little better on the for line (a la "for each button"), when used in code below
"each button" makes little grammatical sense.

http://codereview.chromium.org/348017/diff/4001/5002#newcode693
Line 693: int x_offset = 0;
obj-c naming rules apply. xOffset.

http://codereview.chromium.org/348017/diff/4001/5002#newcode763
Line 763: for (NSButton* each_button in array) {
same comment as above. just call it "button"

http://codereview.chromium.org/348017/diff/4001/5002#newcode783
Line 783: - (void)createOtherBookmarksButton {
need a method comment here, no? i didn't see one in the header.

http://codereview.chromium.org/348017/diff/4001/5002#newcode791
Line 791: int dontCare = 0;
"ignored" instead of "dontCare"?

[viettrungluu, 2009-10-30 16:12:47]

Mostly nits, otherwise LG.

Also, in your commit message, could you describe your changes to the xib (makes
merging a xib and checking the correctness of merge easier)?

http://codereview.chromium.org/348017/diff/4001/5002
File chrome/browser/cocoa/bookmark_bar_controller.mm (right):

http://codereview.chromium.org/348017/diff/4001/5002#newcode144
Line 144: [offTheSideButton_ setFrame:frame];
I wonder if
  [offTheSideButton_ setFrameOrigin:frame.origin];
would be ever-so-slightly more efficient. <shrug>

http://codereview.chromium.org/348017/diff/4001/5002#newcode579
Line 579: // Delete all bookmarks and other buttons from the bookmark bar;
What do you mean "other buttons"? Bookmark folder buttons? (But not the
off-the-side button, nor the Other Bookmarks button?)

http://codereview.chromium.org/348017/diff/4001/5002#newcode752
Line 752: else {
} else {

http://codereview.chromium.org/348017/diff/4001/5002#newcode765
Line 765: frame.size.height = (bookmarks::kBookmarkButtonHeight -
The height doesn't depend on the button, so could you put that into a variable
defined outside the loop to make that more obvious?

http://codereview.chromium.org/348017/diff/4001/5002#newcode774
Line 774: [self resizeButtonsInArray:buttons_.get()];
The use of a helper seems like overkill to me, especially since it leads you to
have to use an |NSMutableArray| below. I'd just do:
  const CGFloat buttonHeight = ...;
  for (NSButton* button in _buttons.get()) { ... }
  if (otherBookmarksButtons_.get()) { ...}
  ...
where "..." are lines 764-767 above. It's just a different compromise though, so
I'll leave it up to you.

http://codereview.chromium.org/348017/diff/4001/5004
File chrome/browser/cocoa/bookmark_bar_controller_unittest.mm (right):

http://codereview.chromium.org/348017/diff/4001/5004#newcode239
Line 239: for (unsigned x=0; x<arraysize(sizes); x++) {
Per style guide, "x = 0" and "x < ...".