Clean up the WrenchMenuModel so that it uses SimpleMenu::Delegate.

Clean up the WrenchMenuModel so that it uses SimpleMenu::Delegate.

This removes copy-pasted code in each platform implementation.

BUG=47320
TEST=compiles

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

[sky, 2010-08-23 21:55:59]

Nice, LGTM

http://codereview.chromium.org/3163023/diff/20001/21001
File app/menus/accelerator.h (right):

http://codereview.chromium.org/3163023/diff/20001/21001#newcode81
app/menus/accelerator.h:81: };
virtual protected destructor?

http://codereview.chromium.org/3163023/diff/20001/21011
File chrome/browser/wrench_menu_model.cc (right):

http://codereview.chromium.org/3163023/diff/20001/21011#newcode188
chrome/browser/wrench_menu_model.cc:188: WrenchMenuModel::WrenchMenuModel() :
model_(NULL) {
Position should match that of header.

[Robert Sesek, 2010-08-24 00:31:13]

http://codereview.chromium.org/3163023/diff/20001/21003
File chrome/browser/cocoa/toolbar_controller.mm (left):

http://codereview.chromium.org/3163023/diff/20001/21003#oldcode582
chrome/browser/cocoa/toolbar_controller.mm:582: [wrenchMenuController_
insertUpdateAvailableItem];
Removing this concerns me.  Does this break the update available notification in
the menu?  I think it will; see later comments as to why.

http://codereview.chromium.org/3163023/diff/20001/21004
File chrome/browser/cocoa/wrench_menu_controller.h (right):

http://codereview.chromium.org/3163023/diff/20001/21004#newcode43
chrome/browser/cocoa/wrench_menu_controller.h:43: WrenchMenuModel*
wrench_model_;
Unnecessary.  The MenuController already maintains a weak reference to the
MenuModel and there's an accessor that casts it to the WMM.  Remove.

http://codereview.chromium.org/3163023/diff/20001/21004#newcode51
chrome/browser/cocoa/wrench_menu_controller.h:51: -
(void)setWrenchMenuModel:(WrenchMenuModel*)model;
Remove.

http://codereview.chromium.org/3163023/diff/20001/21005
File chrome/browser/cocoa/wrench_menu_controller.mm (left):

http://codereview.chromium.org/3163023/diff/20001/21005#oldcode173
chrome/browser/cocoa/wrench_menu_controller.mm:173: -
(void)insertUpdateAvailableItem {
Cocoa's MenuController (and thus the WrenchMenuController) caches the NSMenu and
does not build it on-demand each time.  Therefore, unless it is explicitly
inserted (which it doesn't appear to be), it will never get added.  That was
http://crbug.com/50659.

And a note of warning: I ran into a Cocoa bug in which the custom views within
the Wrench menu cannot be reparented; the subviews all appear correct in the
debugger, but they do not draw for some reason.  This means that you cannot take
the custom view from one menu, remove them, and insert them as custom views into
a new menu.  Reparenting views will be necessary to get build-on-demand working.
 I do not think you can make this change.

http://codereview.chromium.org/3163023/diff/20001/21005
File chrome/browser/cocoa/wrench_menu_controller.mm (right):

http://codereview.chromium.org/3163023/diff/20001/21005#newcode66
chrome/browser/cocoa/wrench_menu_controller.mm:66: -
(void)setWrenchMenuModel:(WrenchMenuModel*)model {
Remove.

http://codereview.chromium.org/3163023/diff/20001/21005#newcode174
chrome/browser/cocoa/wrench_menu_controller.mm:174: return wrench_model_;
Keep this as it was.

[Robert Sesek, 2010-08-24 00:31:47]

I see that this was committed without any review of the Cocoa parts.  You need
to back this out.

[Elliot Glaysher, 2010-08-24 01:11:30]

On 2010/08/24 00:31:47, rsesek wrote:
> I see that this was committed without any review of the Cocoa parts.  You need
> to back this out.

Done.

> Does this break the update available notification in
the menu?

Correct. Some aspect of the upgrade chrome menu item will be broken on each
operating system (the menu item won't show up on mac and won't show the correct
icon on windows/linux). Normalization of this behaviour across the three
platforms is my next patch in this series.

The plan is not to regenerate the menu every time, but to check the model for
visibility, as we do for whether an item is enabled or not. But again, that's in
the future. Do you believe that this will run into the same bug?

> RE everything around: WrenchMenuModel* wrench_model_;

The reason I'm doing all of this was referred to in this conversation with
estade (http://codereview.chromium.org/2800015/diff/12026/26022#newcode39). As
written in this patch, WrenchMenuModel no longer is-a SimpleMenuModel. Though
now that I reread it (and grepped some other existing BlahMenuModel
implementations), it looks like a common pattern to derive from both
SimpleMenuModel and its delegate class. (ugh.)

[Robert Sesek, 2010-08-24 15:28:07]

On 2010/08/24 01:11:30, Elliot Glaysher wrote:
> On 2010/08/24 00:31:47, rsesek wrote:
> > I see that this was committed without any review of the Cocoa parts.  You
need
> > to back this out.
> 
> Done.
> 

Thank you; and sorry for the delay; I'm back to being an external contributor so
I have less time to spend on Chromium :(.

> > Does this break the update available notification in
> the menu?
> 
> Correct. Some aspect of the upgrade chrome menu item will be broken on each
> operating system (the menu item won't show up on mac and won't show the
correct
> icon on windows/linux). Normalization of this behaviour across the three
> platforms is my next patch in this series.
> 
> The plan is not to regenerate the menu every time, but to check the model for
> visibility, as we do for whether an item is enabled or not. But again, that's
in
> the future. Do you believe that this will run into the same bug?
> 

If we keep the built NSMenu cached, we won't hit the reparenting views bug.  But
you'll likely need the code in |-insertUpdateAvailableItem|.  I'd keep the
method impl around for now as that's likely what will need to be called when you
check if new items need to be visible.  Since the Mac has no About menu item,
you'll need to be inserting the update item when it needs to be there.  Is maybe
a better solution to give the abstract MenuModel a Delegate that broadcasts
insert/remove/update notifications?  That could also eliminiate the use of the
IsLabelDynamic() stuff.  This is probably outside the scope of this CL, but
maybe something to think about for the future.

> > RE everything around: WrenchMenuModel* wrench_model_;
> 
> The reason I'm doing all of this was referred to in this conversation with
> estade (http://codereview.chromium.org/2800015/diff/12026/26022#newcode39). As
> written in this patch, WrenchMenuModel no longer is-a SimpleMenuModel. Though
> now that I reread it (and grepped some other existing BlahMenuModel
> implementations), it looks like a common pattern to derive from both
> SimpleMenuModel and its delegate class. (ugh.)

Ah okay.   I've added some more comments to this CL now that I understand more
what you're doing with the class composition.  If this breaks things on all
platforms, I'm okay with that if you make those changes.  Will this be fixable
before the M7 branch date, though?

http://codereview.chromium.org/3163023/diff/20001/21004
File chrome/browser/cocoa/wrench_menu_controller.h (right):

http://codereview.chromium.org/3163023/diff/20001/21004#newcode43
chrome/browser/cocoa/wrench_menu_controller.h:43: WrenchMenuModel*
wrench_model_;
Name this wrenchMenuModel_ and add a comment as to how/why this differs from the
super's model_ member.

http://codereview.chromium.org/3163023/diff/20001/21004#newcode47
chrome/browser/cocoa/wrench_menu_controller.h:47: 
Add: @property (nonatomic, assign) WrenchMenuModel* wrenchMenuModel;

http://codereview.chromium.org/3163023/diff/20001/21004#newcode51
chrome/browser/cocoa/wrench_menu_controller.h:51: -
(void)setWrenchMenuModel:(WrenchMenuModel*)model;
On 2010/08/24 00:31:13, rsesek wrote:
> Remove.

Still remove this proto.

http://codereview.chromium.org/3163023/diff/20001/21005
File chrome/browser/cocoa/wrench_menu_controller.mm (right):

http://codereview.chromium.org/3163023/diff/20001/21005#newcode57
chrome/browser/cocoa/wrench_menu_controller.mm:57: @implementation
WrenchMenuController
Add: @synthesize wrenchMenuModel = wrenchMenuModel_;

http://codereview.chromium.org/3163023/diff/20001/21005#newcode66
chrome/browser/cocoa/wrench_menu_controller.mm:66: -
(void)setWrenchMenuModel:(WrenchMenuModel*)model {
On 2010/08/24 00:31:13, rsesek wrote:
> Remove.

Keep this.

http://codereview.chromium.org/3163023/diff/20001/21005#newcode173
chrome/browser/cocoa/wrench_menu_controller.mm:173: -
(WrenchMenuModel*)wrenchMenuModel {
Remove this (the compiler will generate it).

[Elliot Glaysher, 2010-08-24 22:55:09]

> If we keep the built NSMenu cached, we won't hit the reparenting views bug.
>  But
> you'll likely need the code in |-insertUpdateAvailableItem|.  I'd keep the
> method impl around for now as that's likely what will need to be called when
> you
> check if new items need to be visible.  Since the Mac has no About menu
> item,
> you'll need to be inserting the update item when it needs to be there.  Is
> maybe
> a better solution to give the abstract MenuModel a Delegate that broadcasts
> insert/remove/update notifications?

I'm not sure if we have a failure to communicate here. Why would I
have insert/remove/update notifications? Why can't I just use
-[NSMenuItem setHidden:]?

The current weird combo about/update menu item needs to go. I'm doing
this cleanup to make it easier to separate the update/about menu item,
and keep that logic cross platform.

-- Elliot

[Robert Sesek, 2010-08-25 00:32:01]

On 2010/08/24 22:55:09, Elliot Glaysher wrote:
> > If we keep the built NSMenu cached, we won't hit the reparenting views bug.
> >  But
> > you'll likely need the code in |-insertUpdateAvailableItem|.  I'd keep
the
> > method impl around for now as that's likely what will need to be called when
> > you
> > check if new items need to be visible.  Since the Mac has no About menu
> > item,
> > you'll need to be inserting the update item when it needs to be there.
 Is
> > maybe
> > a better solution to give the abstract MenuModel a Delegate that broadcasts
> > insert/remove/update notifications?
> 
> I'm not sure if we have a failure to communicate here. Why would I
> have insert/remove/update notifications? Why can't I just use
> -[NSMenuItem setHidden:]?
> 
> The current weird combo about/update menu item needs to go. I'm doing
> this cleanup to make it easier to separate the update/about menu item,
> and keep that logic cross platform.

Wow I just got schooled ;-).  You're right; setting hidden makes much more sense
than the dance I described above.  I completely forgot about that.