Change SimpleMenuModel on OSX to support dynamic icons

Change menus on OSX to update the icon dynamically.

Change MenuModel::IsLabelDynamicAt() to IsItemDynamicAt() to reflect its true
purpose.

Add SimpleMenuModel::GetIconForCommandId() to enable dynamic icons.

Update OSX menu_controller code to update the icon for dynamic menu items when
the menu is opened, to match the windows behavior.

BUG=66508
TEST=MenuControllerTest.Dynamic

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

[Andrew T Wilson (Slow), 2010-12-14 00:14:36]

Please take a look - this is the change we discussed last week for dynamic icons
in OSX menus.

This patch is actually fairly small - there are just a ton of files touched
because we renamed IsLabelForCommandIdDynamic() => IsItemForCommandIdDynamic().

[Evan Stade, 2010-12-14 01:20:13]

mostly LGTM

http://codereview.chromium.org/5697005/diff/4001/chrome/browser/gtk/menu_gtk.cc
File chrome/browser/gtk/menu_gtk.cc (right):

http://codereview.chromium.org/5697005/diff/4001/chrome/browser/gtk/menu_gtk....
chrome/browser/gtk/menu_gtk.cc:785: if (model->IsItemDynamicAt(id)) {
add TODO re: images (also in the native_menu_* files)

http://codereview.chromium.org/5697005/diff/4001/chrome/browser/ui/cocoa/menu...
File chrome/browser/ui/cocoa/menu_controller.mm (right):

http://codereview.chromium.org/5697005/diff/4001/chrome/browser/ui/cocoa/menu...
chrome/browser/ui/cocoa/menu_controller.mm:156: if (icon) {
no {}

http://codereview.chromium.org/5697005/diff/4001/chrome/browser/ui/cocoa/menu...
chrome/browser/ui/cocoa/menu_controller.mm:157: [(id)item setImage:icon];
it strikes me that you'll either want to setImage to a valid icon, or setImage
to nil, but this control flow allows for neither happening.

http://codereview.chromium.org/5697005/diff/4001/chrome/browser/ui/cocoa/menu...
File chrome/browser/ui/cocoa/menu_controller_unittest.mm (right):

http://codereview.chromium.org/5697005/diff/4001/chrome/browser/ui/cocoa/menu...
chrome/browser/ui/cocoa/menu_controller_unittest.mm:225: DynamicDelegate
delegate;
can you add a couple short comments walking th reader through the test

[Andrew T Wilson (Slow), 2010-12-14 18:23:27]

Please take another look.

http://codereview.chromium.org/5697005/diff/4001/chrome/browser/gtk/menu_gtk.cc
File chrome/browser/gtk/menu_gtk.cc (right):

http://codereview.chromium.org/5697005/diff/4001/chrome/browser/gtk/menu_gtk....
chrome/browser/gtk/menu_gtk.cc:785: if (model->IsItemDynamicAt(id)) {
On 2010/12/14 01:20:13, Evan Stade wrote:
> add TODO re: images (also in the native_menu_* files)

Done.

BTW, where are the native_menu_* implementations used? For example, the
native_menu_win.cc file doesn't seem to dynamically update icons, but I've
verified that dynamic icons works just fine on the windows Wrench menu so I'm
assuming it uses a different implementation...

http://codereview.chromium.org/5697005/diff/4001/chrome/browser/ui/cocoa/menu...
File chrome/browser/ui/cocoa/menu_controller.mm (right):

http://codereview.chromium.org/5697005/diff/4001/chrome/browser/ui/cocoa/menu...
chrome/browser/ui/cocoa/menu_controller.mm:156: if (icon) {
On 2010/12/14 01:20:13, Evan Stade wrote:
> no {}

Done.

http://codereview.chromium.org/5697005/diff/4001/chrome/browser/ui/cocoa/menu...
chrome/browser/ui/cocoa/menu_controller.mm:157: [(id)item setImage:icon];
On 2010/12/14 01:20:13, Evan Stade wrote:
> it strikes me that you'll either want to setImage to a valid icon, or setImage
> to nil, but this control flow allows for neither happening.

Good point - my previous code was just leaving the image unchanged, but it's
better to be deterministic and explicitly remove the icon rather than just
mysteriously failing.

http://codereview.chromium.org/5697005/diff/4001/chrome/browser/ui/cocoa/menu...
File chrome/browser/ui/cocoa/menu_controller_unittest.mm (right):

http://codereview.chromium.org/5697005/diff/4001/chrome/browser/ui/cocoa/menu...
chrome/browser/ui/cocoa/menu_controller_unittest.mm:225: DynamicDelegate
delegate;
On 2010/12/14 01:20:13, Evan Stade wrote:
> can you add a couple short comments walking th reader through the test

Done. Also added one more test to ensure we can clear the icon as well as set
it.

[Evan Stade, 2010-12-14 21:28:57]

LGTM

http://codereview.chromium.org/5697005/diff/4001/chrome/browser/gtk/menu_gtk.cc
File chrome/browser/gtk/menu_gtk.cc (right):

http://codereview.chromium.org/5697005/diff/4001/chrome/browser/gtk/menu_gtk....
chrome/browser/gtk/menu_gtk.cc:785: if (model->IsItemDynamicAt(id)) {
I think that's because the common pattern is to just destroy and rebuild the
menu view from the menu model every time it's shown

http://codereview.chromium.org/5697005/diff/30/chrome/browser/ui/cocoa/menu_c...
File chrome/browser/ui/cocoa/menu_controller.mm (right):

http://codereview.chromium.org/5697005/diff/30/chrome/browser/ui/cocoa/menu_c...
chrome/browser/ui/cocoa/menu_controller.mm:152: SkBitmap skiaIcon;
SkBitmap skiaIcon;
NSImage* icon = nil;
if (model->GetIconAt(modelIndex, &skiaIcon) && !skiaIcon.isNull())
  icon = gfx::SkBitmapToNSImage(skiaIcon);
[(id)item setImage:icon];

http://codereview.chromium.org/5697005/diff/30/chrome/browser/ui/cocoa/menu_c...
File chrome/browser/ui/cocoa/menu_controller_unittest.mm (right):

http://codereview.chromium.org/5697005/diff/30/chrome/browser/ui/cocoa/menu_c...
chrome/browser/ui/cocoa/menu_controller_unittest.mm:56: void
SetDynamicIcon(SkBitmap* icon) { icon_ = icon; }
nit: blank line here please