[Mac] Replace the custom bookmark menus with native NSMenus.

[Mac] Replace the custom bookmark menus with native NSMenus.

This replaces a lot of custom code with an implementation that reuses the
architecture of the menubar Bookmarks menu.

This CL introduces two minor regressions (bugs pending):
* Hover state is lost after menu is opened. Mousing out/back over fixes.
* The off-the-side menu contains all items from the bookmark bar.

BUG=78151
TEST=Click on a bookmark folder button. A menu opens.

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

[mrossetti, 2011-08-04 02:16:27]

LGTM

Minor comments.

It's a pleasure to review a CL like this -- one so carefully prepared!

http://codereview.chromium.org/7465090/diff/1/chrome/browser/ui/cocoa/bookmar...
File chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller_unittest.mm
(right):

http://codereview.chromium.org/7465090/diff/1/chrome/browser/ui/cocoa/bookmar...
chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller_unittest.mm:1576:
TEST_F(BookmarkBarControllerDragDropTest,
Nit: Add comment on the anticipated disposition of this test.

http://codereview.chromium.org/7465090/diff/1/chrome/browser/ui/cocoa/bookmar...
File chrome/browser/ui/cocoa/bookmarks/bookmark_bar_folder_controller.h (right):

http://codereview.chromium.org/7465090/diff/1/chrome/browser/ui/cocoa/bookmar...
chrome/browser/ui/cocoa/bookmarks/bookmark_bar_folder_controller.h:21: // which
look sort of like menus.
Nit: They probably look just like menus now, right?

http://codereview.chromium.org/7465090/diff/1/chrome/browser/ui/cocoa/bookmar...
File chrome/browser/ui/cocoa/bookmarks/bookmark_bar_folder_controller.mm
(right):

http://codereview.chromium.org/7465090/diff/1/chrome/browser/ui/cocoa/bookmar...
chrome/browser/ui/cocoa/bookmarks/bookmark_bar_folder_controller.mm:81: // in
the first place, this will recurisvely pop open the menu because the
Nit: s/recurisvely/recursively/

http://codereview.chromium.org/7465090/diff/1/chrome/browser/ui/cocoa/bookmar...
File chrome/browser/ui/cocoa/bookmarks/bookmark_bar_unittest_helper.h (right):

http://codereview.chromium.org/7465090/diff/1/chrome/browser/ui/cocoa/bookmar...
chrome/browser/ui/cocoa/bookmarks/bookmark_bar_unittest_helper.h:16: NSUInteger
GetNumberOfMenuItems(BookmarkBarFolderController* folder);
Nit: How about just 'NumberOfMenuItems'?

http://codereview.chromium.org/7465090/diff/1/chrome/browser/ui/cocoa/bookmar...
chrome/browser/ui/cocoa/bookmarks/bookmark_bar_unittest_helper.h:19: void
CloseFolderSoon(BookmarkBarFolderController* folder, NSTimeInterval when);
Nit: 'when' should probably be 'delay' and I'd suggest renaming this function to
'CloseFolderAfterDelay'.

[Robert Sesek, 2011-08-04 15:37:02]

Thanks! Addressed all comments.

http://codereview.chromium.org/7465090/diff/1/chrome/browser/ui/cocoa/bookmar...
File chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller_unittest.mm
(right):

http://codereview.chromium.org/7465090/diff/1/chrome/browser/ui/cocoa/bookmar...
chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller_unittest.mm:1576:
TEST_F(BookmarkBarControllerDragDropTest,
On 2011/08/04 02:16:27, mrossetti wrote:
> Nit: Add comment on the anticipated disposition of this test.

Done.

http://codereview.chromium.org/7465090/diff/1/chrome/browser/ui/cocoa/bookmar...
File chrome/browser/ui/cocoa/bookmarks/bookmark_bar_folder_controller.h (right):

http://codereview.chromium.org/7465090/diff/1/chrome/browser/ui/cocoa/bookmar...
chrome/browser/ui/cocoa/bookmarks/bookmark_bar_folder_controller.h:21: // which
look sort of like menus.
On 2011/08/04 02:16:27, mrossetti wrote:
> Nit: They probably look just like menus now, right?

Done.

http://codereview.chromium.org/7465090/diff/1/chrome/browser/ui/cocoa/bookmar...
File chrome/browser/ui/cocoa/bookmarks/bookmark_bar_folder_controller.mm
(right):

http://codereview.chromium.org/7465090/diff/1/chrome/browser/ui/cocoa/bookmar...
chrome/browser/ui/cocoa/bookmarks/bookmark_bar_folder_controller.mm:81: // in
the first place, this will recurisvely pop open the menu because the
On 2011/08/04 02:16:27, mrossetti wrote:
> Nit: s/recurisvely/recursively/

Done.

http://codereview.chromium.org/7465090/diff/1/chrome/browser/ui/cocoa/bookmar...
File chrome/browser/ui/cocoa/bookmarks/bookmark_bar_unittest_helper.h (right):

http://codereview.chromium.org/7465090/diff/1/chrome/browser/ui/cocoa/bookmar...
chrome/browser/ui/cocoa/bookmarks/bookmark_bar_unittest_helper.h:16: NSUInteger
GetNumberOfMenuItems(BookmarkBarFolderController* folder);
On 2011/08/04 02:16:27, mrossetti wrote:
> Nit: How about just 'NumberOfMenuItems'?

Done.

http://codereview.chromium.org/7465090/diff/1/chrome/browser/ui/cocoa/bookmar...
chrome/browser/ui/cocoa/bookmarks/bookmark_bar_unittest_helper.h:19: void
CloseFolderSoon(BookmarkBarFolderController* folder, NSTimeInterval when);
On 2011/08/04 02:16:27, mrossetti wrote:
> Nit: 'when' should probably be 'delay' and I'd suggest renaming this function
to
> 'CloseFolderAfterDelay'.

Done.

[Avi (use Gerrit), 2011-08-04 18:11:07]

drive-by

http://codereview.chromium.org/7465090/diff/12001/chrome/browser/ui/cocoa/boo...
File chrome/browser/ui/cocoa/bookmarks/bookmark_bar_folder_controller.mm
(right):

http://codereview.chromium.org/7465090/diff/12001/chrome/browser/ui/cocoa/boo...
chrome/browser/ui/cocoa/bookmarks/bookmark_bar_folder_controller.mm:52: //
should be swithced to NSPopUpButtonCells so that this is taken care of
typo

http://codereview.chromium.org/7465090/diff/12001/chrome/browser/ui/cocoa/boo...
chrome/browser/ui/cocoa/bookmarks/bookmark_bar_folder_controller.mm:62: [NSMenu
popUpContextMenu:menu_
danger; have you tested this? context menus have things like "help", etc. Might
be worth switching to NSPopUpButtonCells earlier than later.

[Robert Sesek, 2011-08-04 19:00:35]

http://codereview.chromium.org/7465090/diff/12001/chrome/browser/ui/cocoa/boo...
File chrome/browser/ui/cocoa/bookmarks/bookmark_bar_folder_controller.mm
(right):

http://codereview.chromium.org/7465090/diff/12001/chrome/browser/ui/cocoa/boo...
chrome/browser/ui/cocoa/bookmarks/bookmark_bar_folder_controller.mm:52: //
should be swithced to NSPopUpButtonCells so that this is taken care of
On 2011/08/04 18:11:07, Avi wrote:
> typo

Done.

http://codereview.chromium.org/7465090/diff/12001/chrome/browser/ui/cocoa/boo...
chrome/browser/ui/cocoa/bookmarks/bookmark_bar_folder_controller.mm:62: [NSMenu
popUpContextMenu:menu_
On 2011/08/04 18:11:07, Avi wrote:
> danger; have you tested this?

Yes.