Share the code that builds the page menu in a common model, make Mac and Win ...

Share the code that builds the page menu in a common model, make Mac and Win use it. Implement a mac menu controller that takes this model and generates an NSMenu. Remove the Page Menu NSMenu from Toolbar.xib since it's now created in code.
BUG=22646
TEST=page menu should work as before

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

[viettrungluu, 2009-12-08 22:32:28]

Looking ... but could you put a brief description of the changes to Toolbar.xib
in the description? Thanks.

[viettrungluu, 2009-12-09 01:03:28]

Mostly looks good, with only a few nits.

Have you run it through the try server?

http://codereview.chromium.org/465130/diff/1003/2013
File chrome/browser/cocoa/menu_controller.h (right):

http://codereview.chromium.org/465130/diff/1003/2013#newcode32
chrome/browser/cocoa/menu_controller.h:32:
useWithPopUpButtonCell:(BOOL)popUpButton;
I'm not thrilled with "popUpButton" as a variable name here. I like
"useWithCell" (which is what you use in the code for the implementation) more,
though maybe "useWithPopUp" may be even better.... Or something. You should
probably be consistent between the interface and the implementation though.

http://codereview.chromium.org/465130/diff/1003/2014
File chrome/browser/cocoa/menu_controller.mm (right):

http://codereview.chromium.org/465130/diff/1003/2014#newcode40
chrome/browser/cocoa/menu_controller.mm:40: -
(NSMenu*)menuFromModel:(menus::MenuModel*)model {
Needs a comment describing the method somewhere (here, or in the header file).

http://codereview.chromium.org/465130/diff/1003/2014#newcode61
chrome/browser/cocoa/menu_controller.mm:61: // anything from the model, this
method need the model index as the other
+ doesn't

http://codereview.chromium.org/465130/diff/1003/2014#newcode74
chrome/browser/cocoa/menu_controller.mm:74: model:(menus::MenuModel*)model {
I'm of the opinion that "model" should go before "modelIndex". Maybe it should
be "-addItemToMenu:atIndex:fromModel:modelIndex:"?

http://codereview.chromium.org/465130/diff/1003/2014#newcode98
chrome/browser/cocoa/menu_controller.mm:98: [item
setRepresentedObject:modelObject];  // Retains |modelObject|.
Is this actually used anywhere? (Never mind -- you use it below.) Could you
please put a comment in the header file saying that you do this?

(I'm a little worried about the pointer going stale, but maybe that isn't a
problem....)

http://codereview.chromium.org/465130/diff/1003/2014#newcode104
chrome/browser/cocoa/menu_controller.mm:104: // radio, etc) of each item in the
menu. The tag of the item is the model
Could you please put the "The tag of ..." comment in the header? Then you could
eliminate it here.

http://codereview.chromium.org/465130/diff/1003/2014#newcode106
chrome/browser/cocoa/menu_controller.mm:106: // between different ierarchical
menus).
+h

http://codereview.chromium.org/465130/diff/1003/2014#newcode119
chrome/browser/cocoa/menu_controller.mm:119: [(id)item setState:(checked ?
NSOnState : NSOffState)];
Could you DCHECK() that |item| is an |NSMenuItem| (or that it responds to
|-setState:|)?

I'm a *little* worried that calling |-setState:| from within
|-validateUserInterfaceItem:| might not be kosher (since the docs don't seem to
explicitly allow it), but logically it should be okay, I guess.

http://codereview.chromium.org/465130/diff/1003/2014#newcode126
chrome/browser/cocoa/menu_controller.mm:126: // item chosen. Its tag is the
model index and the model is the represented
(And eliminate "Its tag ..." here.)

http://codereview.chromium.org/465130/diff/1003/2020
File chrome/browser/cocoa/menu_controller_unittest.mm (right):

http://codereview.chromium.org/465130/diff/1003/2020#newcode15
chrome/browser/cocoa/menu_controller_unittest.mm:15: public:
This |public:| is a wee bit redundant.

http://codereview.chromium.org/465130/diff/1003/2020#newcode153
chrome/browser/cocoa/menu_controller_unittest.mm:153: }
Could you add a test to check that setting/changing the checkmarked status works
(in particular, doesn't crash/cause assertions to fail)?

http://codereview.chromium.org/465130/diff/1003/2012
File chrome/browser/cocoa/toolbar_controller.mm (right):

http://codereview.chromium.org/465130/diff/1003/2012#newcode97
chrome/browser/cocoa/toolbar_controller.mm:97: return
browser_->profile()->GetPrefs()->GetBoolean(
Could you use braces on this if ()?

http://codereview.chromium.org/465130/diff/1003/2016
File chrome/browser/page_menu_model.cc (right):

http://codereview.chromium.org/465130/diff/1003/2016#newcode16
chrome/browser/page_menu_model.cc:16:
PageMenuModel::PageMenuModel(menus::SimpleMenuModel::Delegate* delegate,
Poor PageMenuModel feels left out without a
/////...
// PageMenuModel
(but I'm sure it'll forgive you).

http://codereview.chromium.org/465130/diff/1003/2016#newcode17
chrome/browser/page_menu_model.cc:17: Browser* browser)
Could you indent this to align it after the (?

http://codereview.chromium.org/465130/diff/1003/2016#newcode96
chrome/browser/page_menu_model.cc:96: return
browser_->command_updater()->IsCommandEnabled(command_id);
I wonder if this will do the right thing when we stop disabling the upper-level
item on the Mac (so that the submenu will be displayed with all items displayed,
rather than just preventing the submenu from being displayed entirely).... But
that's a problem for another day.

[pink (ping after 24hrs), 2009-12-09 16:57:16]

fixed everything and put up a new cl for perusal.

http://codereview.chromium.org/465130/diff/1003/2013
File chrome/browser/cocoa/menu_controller.h (right):

http://codereview.chromium.org/465130/diff/1003/2013#newcode32
chrome/browser/cocoa/menu_controller.h:32:
useWithPopUpButtonCell:(BOOL)popUpButton;
On 2009/12/09 01:03:28, viettrungluu wrote:
> I'm not thrilled with "popUpButton" as a variable name here. I like
> "useWithCell" (which is what you use in the code for the implementation) more,
> though maybe "useWithPopUp" may be even better.... Or something. You should
> probably be consistent between the interface and the implementation though.

Done.

http://codereview.chromium.org/465130/diff/1003/2014
File chrome/browser/cocoa/menu_controller.mm (right):

http://codereview.chromium.org/465130/diff/1003/2014#newcode40
chrome/browser/cocoa/menu_controller.mm:40: -
(NSMenu*)menuFromModel:(menus::MenuModel*)model {
On 2009/12/09 01:03:28, viettrungluu wrote:
> Needs a comment describing the method somewhere (here, or in the header file).

Done.

http://codereview.chromium.org/465130/diff/1003/2014#newcode61
chrome/browser/cocoa/menu_controller.mm:61: // anything from the model, this
method need the model index as the other
On 2009/12/09 01:03:28, viettrungluu wrote:
> + doesn't

Done.

http://codereview.chromium.org/465130/diff/1003/2014#newcode74
chrome/browser/cocoa/menu_controller.mm:74: model:(menus::MenuModel*)model {
On 2009/12/09 01:03:28, viettrungluu wrote:
> I'm of the opinion that "model" should go before "modelIndex". Maybe it should
> be "-addItemToMenu:atIndex:fromModel:modelIndex:"?

Done.

http://codereview.chromium.org/465130/diff/1003/2014#newcode98
chrome/browser/cocoa/menu_controller.mm:98: [item
setRepresentedObject:modelObject];  // Retains |modelObject|.
On 2009/12/09 01:03:28, viettrungluu wrote:
> Is this actually used anywhere? (Never mind -- you use it below.) Could you
> please put a comment in the header file saying that you do this?

Comment added to the header.
 
> (I'm a little worried about the pointer going stale, but maybe that isn't a
> problem....)

Nothing we can really do about this. They're weak references, and the
pre-existing code always behaved this way.

http://codereview.chromium.org/465130/diff/1003/2014#newcode119
chrome/browser/cocoa/menu_controller.mm:119: [(id)item setState:(checked ?
NSOnState : NSOffState)];
On 2009/12/09 01:03:28, viettrungluu wrote:
> Could you DCHECK() that |item| is an |NSMenuItem| (or that it responds to
> |-setState:|)?
> 
> I'm a *little* worried that calling |-setState:| from within
> |-validateUserInterfaceItem:| might not be kosher (since the docs don't seem
to
> explicitly allow it), but logically it should be okay, I guess.

Done.

http://codereview.chromium.org/465130/diff/1003/2014#newcode126
chrome/browser/cocoa/menu_controller.mm:126: // item chosen. Its tag is the
model index and the model is the represented
On 2009/12/09 01:03:28, viettrungluu wrote:
> (And eliminate "Its tag ..." here.)

Done.

http://codereview.chromium.org/465130/diff/1003/2020
File chrome/browser/cocoa/menu_controller_unittest.mm (right):

http://codereview.chromium.org/465130/diff/1003/2020#newcode15
chrome/browser/cocoa/menu_controller_unittest.mm:15: public:
On 2009/12/09 01:03:28, viettrungluu wrote:
> This |public:| is a wee bit redundant.

Done.

http://codereview.chromium.org/465130/diff/1003/2020#newcode153
chrome/browser/cocoa/menu_controller_unittest.mm:153: }
On 2009/12/09 01:03:28, viettrungluu wrote:
> Could you add a test to check that setting/changing the checkmarked status
works
> (in particular, doesn't crash/cause assertions to fail)?

Done.

http://codereview.chromium.org/465130/diff/1003/2012
File chrome/browser/cocoa/toolbar_controller.mm (right):

http://codereview.chromium.org/465130/diff/1003/2012#newcode97
chrome/browser/cocoa/toolbar_controller.mm:97: return
browser_->profile()->GetPrefs()->GetBoolean(
On 2009/12/09 01:03:28, viettrungluu wrote:
> Could you use braces on this if ()?

Done.

http://codereview.chromium.org/465130/diff/1003/2016
File chrome/browser/page_menu_model.cc (right):

http://codereview.chromium.org/465130/diff/1003/2016#newcode16
chrome/browser/page_menu_model.cc:16:
PageMenuModel::PageMenuModel(menus::SimpleMenuModel::Delegate* delegate,
On 2009/12/09 01:03:28, viettrungluu wrote:
> Poor PageMenuModel feels left out without a
> /////...
> // PageMenuModel
> (but I'm sure it'll forgive you).

I asked, and it doesn't feel left out.

http://codereview.chromium.org/465130/diff/1003/2016#newcode17
chrome/browser/page_menu_model.cc:17: Browser* browser)
On 2009/12/09 01:03:28, viettrungluu wrote:
> Could you indent this to align it after the (?

Done.

[viettrungluu, 2009-12-09 17:17:21]

Thanks. LGTM.

http://codereview.chromium.org/465130/diff/1003/2016
File chrome/browser/page_menu_model.cc (right):

http://codereview.chromium.org/465130/diff/1003/2016#newcode16
chrome/browser/page_menu_model.cc:16:
PageMenuModel::PageMenuModel(menus::SimpleMenuModel::Delegate* delegate,
On 2009/12/09 16:57:16, pink wrote:
> On 2009/12/09 01:03:28, viettrungluu wrote:
> > Poor PageMenuModel feels left out without a
> > /////...
> > // PageMenuModel
> > (but I'm sure it'll forgive you).
> 
> I asked, and it doesn't feel left out.

I guess it does get to be first, after all.