(Mac) Display the bookmark bar off-the-side menu on mouse down. Also align the menu.

(Mac) Display the bookmark bar off-the-side menu on mouse down. Also align the menu.
(Basically, make it like Safari's button.)

We make the button into a |MenuButton|, attach a fixed menu to it in the
nib, and make the |BookmarkBarController| the delegate for this menu so
that we can update the menu just before it's displayed.

As a side effect, also fixes bug 20813.

BUG=21093, 20813
TEST=Make sure the bookmark bar off-the-side button works properly under a variety of circumstances, making sure the menu is properly aligned, etc.

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

[John Grabowski, 2009-09-04 16:48:54]

Overall I like.
Minor changes requested.

http://codereview.chromium.org/199024/diff/1/4
File chrome/browser/cocoa/bookmark_bar_controller.mm (right):

http://codereview.chromium.org/199024/diff/1/4#newcode300
Line 300: // Called by any menus which have set us as our delegate (right now
just the
as our delegate --> as their delegate

http://codereview.chromium.org/199024/diff/1/4#newcode306
Line 306: - (void)menuNeedsUpdate:(NSMenu*)menu {
Consider a quick unit test plz

http://codereview.chromium.org/199024/diff/1/4#newcode322
Line 322: // TODO(viettrungluu): is this still needed?
Don't think so.  Button no longer there when it has no contents.
While you are here, can you remove this call and the function tagEmptyMenu:
itself?

http://codereview.chromium.org/199024/diff/1/5
File chrome/browser/cocoa/menu_button.h (right):

http://codereview.chromium.org/199024/diff/1/5#newcode23
Line 23: // delegate as usual.)
The above comment might be best encoded in the class itself to enforce the
invariant.  Else someone could easily add a [menu removeAllItems] to clear it
yet wonder why things break.

[viettrungluu, 2009-09-04 19:12:56]

Thanks, John. Could you please re-review?

http://codereview.chromium.org/199024/diff/1/4
File chrome/browser/cocoa/bookmark_bar_controller.mm (right):

http://codereview.chromium.org/199024/diff/1/4#newcode300
Line 300: // Called by any menus which have set us as our delegate (right now
just the
On 2009/09/04 16:48:55, John Grabowski wrote:
> as our delegate --> as their delegate
> 

Done.

http://codereview.chromium.org/199024/diff/1/4#newcode306
Line 306: - (void)menuNeedsUpdate:(NSMenu*)menu {
On 2009/09/04 16:48:55, John Grabowski wrote:
> Consider a quick unit test plz

Done. I split off the building of the menu into a separate method (which is
tested), since calling a delegate method directly seems wrong (and testing it as
a delegate method properly is tricky).

http://codereview.chromium.org/199024/diff/1/4#newcode322
Line 322: // TODO(viettrungluu): is this still needed?
On 2009/09/04 16:48:55, John Grabowski wrote:
> Don't think so.  Button no longer there when it has no contents.
> While you are here, can you remove this call and the function tagEmptyMenu:
> itself?

I removed the call. -tagEmptyMenu: is still called by -menuForFolderNode:
though. (I don't think empty bookmark folders are disabled, and I don't think we
want to.)

http://codereview.chromium.org/199024/diff/1/5
File chrome/browser/cocoa/menu_button.h (right):

http://codereview.chromium.org/199024/diff/1/5#newcode23
Line 23: // delegate as usual.)
On 2009/09/04 16:48:55, John Grabowski wrote:
> The above comment might be best encoded in the class itself to enforce the
> invariant.  Else someone could easily add a [menu removeAllItems] to clear it
> yet wonder why things break.

Yeah, the problem is that (a) it's a condition which really only needs to hold
when the menu is being displayed, and (b) the menu's delegate would be able to
mess things up anyway -- and the delegate is called after "we" have given up
control to Cocoa. (Elsewhere, for the back/forwards menus which use the similar
|DelayedMenuButton|, the menu starts out empty and is filled out on the fly by
the delegate.)

It's a terrible condition, but it's safe to say that it's Apple's fault. (It's a
condition which comes from |NSPopUpButtonCell| ... which similarly enforces this
condition by a comment in the documentation!) Getting around it, or eliminating
the condition, is too much trouble IMO.

:(

[John Grabowski, 2009-09-04 19:45:28]

LGTM

http://codereview.chromium.org/199024/diff/5001/2006
File chrome/browser/cocoa/bookmark_bar_controller_unittest.mm (right):

http://codereview.chromium.org/199024/diff/5001/2006#newcode527
Line 527: TEST_F(BookmarkBarControllerTest, TestBuildOffTheSideMenu) {
Excellent.