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

Review URL: http://codereview.chromium.org/5697005

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@69234 0039d316-1c4b-4281-b951-d872f2087c98

20 files changed, 113 insertions, 43 deletions

diff --git a/chrome/browser/autocomplete/autocomplete_edit_view_win.cc b/chrome/browser/autocomplete/autocomplete_edit_view_win.cc
index 358eec6..bd6b50e 100644
--- a/chrome/browser/autocomplete/autocomplete_edit_view_win.cc
+++ b/chrome/browser/autocomplete/autocomplete_edit_view_win.cc
@@ -1011,7 +1011,7 @@ bool AutocompleteEditViewWin::GetAcceleratorForCommandId(
   return parent_view_->GetWidget()->GetAccelerator(command_id, accelerator);
 }
 
-bool AutocompleteEditViewWin::IsLabelForCommandIdDynamic(int command_id) const {
+bool AutocompleteEditViewWin::IsItemForCommandIdDynamic(int command_id) const {
   // No need to change the default IDS_PASTE_AND_GO label unless this is a
   // search.
   return command_id == IDS_PASTE_AND_GO;
diff --git a/chrome/browser/autocomplete/autocomplete_edit_view_win.h b/chrome/browser/autocomplete/autocomplete_edit_view_win.h
index 457233e..904b9cd 100644
--- a/chrome/browser/autocomplete/autocomplete_edit_view_win.h
+++ b/chrome/browser/autocomplete/autocomplete_edit_view_win.h
@@ -209,7 +209,7 @@ class AutocompleteEditViewWin
   virtual bool IsCommandIdEnabled(int command_id) const;
   virtual bool GetAcceleratorForCommandId(int command_id,
                                           menus::Accelerator* accelerator);
-  virtual bool IsLabelForCommandIdDynamic(int command_id) const;
+  virtual bool IsItemForCommandIdDynamic(int command_id) const;
   virtual std::wstring GetLabelForCommandId(int command_id) const;
   virtual void ExecuteCommand(int command_id);
 
diff --git a/chrome/browser/chromeos/status/clock_menu_button.h b/chrome/browser/chromeos/status/clock_menu_button.h
index 76f214a..ee1d979 100644
--- a/chrome/browser/chromeos/status/clock_menu_button.h
+++ b/chrome/browser/chromeos/status/clock_menu_button.h
@@ -37,7 +37,7 @@ class ClockMenuButton : public StatusAreaButton,
   virtual menus::MenuModel::ItemType GetTypeAt(int index) const;
   virtual int GetCommandIdAt(int index) const { return index; }
   virtual string16 GetLabelAt(int index) const;
-  virtual bool IsLabelDynamicAt(int index) const { return true; }
+  virtual bool IsItemDynamicAt(int index) const { return true; }
   virtual bool GetAcceleratorAt(int index,
       menus::Accelerator* accelerator) const { return false; }
   virtual bool IsItemCheckedAt(int index) const { return false; }
diff --git a/chrome/browser/chromeos/status/input_method_menu.cc b/chrome/browser/chromeos/status/input_method_menu.cc
index 83082d3..1b659a0 100644
--- a/chrome/browser/chromeos/status/input_method_menu.cc
+++ b/chrome/browser/chromeos/status/input_method_menu.cc
@@ -181,7 +181,7 @@ int InputMethodMenu::GetCommandIdAt(int index) const {
   return 0;  // dummy
 }
 
-bool InputMethodMenu::IsLabelDynamicAt(int index) const {
+bool InputMethodMenu::IsItemDynamicAt(int index) const {
   // Menu content for the language button could change time by time.
   return true;
 }
diff --git a/chrome/browser/chromeos/status/input_method_menu.h b/chrome/browser/chromeos/status/input_method_menu.h
index a25ee36..78562d1 100644
--- a/chrome/browser/chromeos/status/input_method_menu.h
+++ b/chrome/browser/chromeos/status/input_method_menu.h
@@ -44,7 +44,7 @@ class InputMethodMenu : public views::ViewMenuDelegate,
   virtual menus::MenuModel::ItemType GetTypeAt(int index) const;
   virtual int GetCommandIdAt(int index) const;
   virtual string16 GetLabelAt(int index) const;
-  virtual bool IsLabelDynamicAt(int index) const;
+  virtual bool IsItemDynamicAt(int index) const;
   virtual bool GetAcceleratorAt(int index,
                                 menus::Accelerator* accelerator) const;
   virtual bool IsItemCheckedAt(int index) const;
diff --git a/chrome/browser/chromeos/status/network_menu.h b/chrome/browser/chromeos/status/network_menu.h
index ea399fe..4e72ad2 100644
--- a/chrome/browser/chromeos/status/network_menu.h
+++ b/chrome/browser/chromeos/status/network_menu.h
@@ -93,7 +93,7 @@ class NetworkMenu : public views::ViewMenuDelegate,
   virtual menus::MenuModel::ItemType GetTypeAt(int index) const;
   virtual int GetCommandIdAt(int index) const { return index; }
   virtual string16 GetLabelAt(int index) const;
-  virtual bool IsLabelDynamicAt(int index) const { return true; }
+  virtual bool IsItemDynamicAt(int index) const { return true; }
   virtual const gfx::Font* GetLabelFontAt(int index) const;
   virtual bool GetAcceleratorAt(int index,
       menus::Accelerator* accelerator) const { return false; }
diff --git a/chrome/browser/chromeos/status/power_menu_button.h b/chrome/browser/chromeos/status/power_menu_button.h
index b3503e8..d3146ac 100644
--- a/chrome/browser/chromeos/status/power_menu_button.h
+++ b/chrome/browser/chromeos/status/power_menu_button.h
@@ -36,7 +36,7 @@ class PowerMenuButton : public StatusAreaButton,
   virtual menus::MenuModel::ItemType GetTypeAt(int index) const;
   virtual int GetCommandIdAt(int index) const { return index; }
   virtual string16 GetLabelAt(int index) const;
-  virtual bool IsLabelDynamicAt(int index) const { return true; }
+  virtual bool IsItemDynamicAt(int index) const { return true; }
   virtual bool GetAcceleratorAt(int index,
       menus::Accelerator* accelerator) const { return false; }
   virtual bool IsItemCheckedAt(int index) const { return false; }
diff --git a/chrome/browser/download/download_shelf.cc b/chrome/browser/download/download_shelf.cc
index 404bb2b..3380789 100644
--- a/chrome/browser/download/download_shelf.cc
+++ b/chrome/browser/download/download_shelf.cc
@@ -114,7 +114,7 @@ bool DownloadShelfContextMenu::GetAcceleratorForCommandId(
   return false;
 }
 
-bool DownloadShelfContextMenu::IsLabelForCommandIdDynamic(
+bool DownloadShelfContextMenu::IsItemForCommandIdDynamic(
     int command_id) const {
   return command_id == TOGGLE_PAUSE;
 }
diff --git a/chrome/browser/download/download_shelf.h b/chrome/browser/download/download_shelf.h
index b85123c..ab565fa 100644
--- a/chrome/browser/download/download_shelf.h
+++ b/chrome/browser/download/download_shelf.h
@@ -73,7 +73,7 @@ class DownloadShelfContextMenu : public menus::SimpleMenuModel::Delegate {
   virtual void ExecuteCommand(int command_id);
   virtual bool GetAcceleratorForCommandId(int command_id,
                                           menus::Accelerator* accelerator);
-  virtual bool IsLabelForCommandIdDynamic(int command_id) const;
+  virtual bool IsItemForCommandIdDynamic(int command_id) const;
   virtual string16 GetLabelForCommandId(int command_id) const;
 
   // A model to control the cancel behavior.
diff --git a/chrome/browser/gtk/menu_gtk.cc b/chrome/browser/gtk/menu_gtk.cc
index 8becfa2..04450ef 100644
--- a/chrome/browser/gtk/menu_gtk.cc
+++ b/chrome/browser/gtk/menu_gtk.cc
@@ -744,7 +744,7 @@ void MenuGtk::SetButtonItemInfo(GtkWidget* button, gpointer userdata) {
   int index = GPOINTER_TO_INT(g_object_get_data(
       G_OBJECT(button), "button-model-id"));
 
-  if (model->IsLabelDynamicAt(index)) {
+  if (model->IsItemDynamicAt(index)) {
     std::string label =
         gfx::ConvertAcceleratorsFromWindowsStyle(
             UTF16ToUTF8(model->GetLabelAt(index)));
@@ -803,7 +803,8 @@ void MenuGtk::SetMenuItemInfo(GtkWidget* widget, gpointer userdata) {
 
     if (model->IsVisibleAt(id)) {
       // Update the menu item label if it is dynamic.
-      if (model->IsLabelDynamicAt(id)) {
+      // TODO(atwilson): Update the icon as well (http://crbug.com/66508).
+      if (model->IsItemDynamicAt(id)) {
         std::string label =
             gfx::ConvertAcceleratorsFromWindowsStyle(
                 UTF16ToUTF8(model->GetLabelAt(id)));
diff --git a/chrome/browser/notifications/notification_options_menu_model.cc b/chrome/browser/notifications/notification_options_menu_model.cc
index a100397..4190dcd 100644
--- a/chrome/browser/notifications/notification_options_menu_model.cc
+++ b/chrome/browser/notifications/notification_options_menu_model.cc
@@ -54,7 +54,7 @@ NotificationOptionsMenuModel::NotificationOptionsMenuModel(Balloon* balloon)
 NotificationOptionsMenuModel::~NotificationOptionsMenuModel() {
 }
 
-bool NotificationOptionsMenuModel::IsLabelForCommandIdDynamic(int command_id)
+bool NotificationOptionsMenuModel::IsItemForCommandIdDynamic(int command_id)
     const {
   return command_id == kTogglePermissionCommand ||
          command_id == kToggleExtensionCommand;
diff --git a/chrome/browser/notifications/notification_options_menu_model.h b/chrome/browser/notifications/notification_options_menu_model.h
index b276e11..f7f4c77 100644
--- a/chrome/browser/notifications/notification_options_menu_model.h
+++ b/chrome/browser/notifications/notification_options_menu_model.h
@@ -16,7 +16,7 @@ class NotificationOptionsMenuModel : public menus::SimpleMenuModel,
   virtual ~NotificationOptionsMenuModel();
 
   // Overridden from menus::SimpleMenuModel:
-  virtual bool IsLabelForCommandIdDynamic(int command_id) const;
+  virtual bool IsItemForCommandIdDynamic(int command_id) const;
   virtual string16 GetLabelForCommandId(int command_id) const;
 
   // Overridden from menus::SimpleMenuModel::Delegate:
diff --git a/chrome/browser/ui/cocoa/menu_controller.mm b/chrome/browser/ui/cocoa/menu_controller.mm
index 47f0c34..a6d314d 100644
--- a/chrome/browser/ui/cocoa/menu_controller.mm
+++ b/chrome/browser/ui/cocoa/menu_controller.mm
@@ -144,10 +144,16 @@
     DCHECK([(id)item isKindOfClass:[NSMenuItem class]]);
     [(id)item setState:(checked ? NSOnState : NSOffState)];
     [(id)item setHidden:(!model->IsVisibleAt(modelIndex))];
-    if (model->IsLabelDynamicAt(modelIndex)) {
+    if (model->IsItemDynamicAt(modelIndex)) {
+      // Update the label and the icon.
       NSString* label =
           l10n_util::FixUpWindowsStyleLabel(model->GetLabelAt(modelIndex));
       [(id)item setTitle:label];
+      SkBitmap skiaIcon;
+      NSImage* icon = nil;
+      if (model->GetIconAt(modelIndex, &skiaIcon) && !skiaIcon.isNull())
+        icon = gfx::SkBitmapToNSImage(skiaIcon);
+      [(id)item setImage:icon];
     }
     return model->IsEnabledAt(modelIndex);
   }
diff --git a/chrome/browser/ui/cocoa/menu_controller_unittest.mm b/chrome/browser/ui/cocoa/menu_controller_unittest.mm
index 9171adf..8e00c53 100644
--- a/chrome/browser/ui/cocoa/menu_controller_unittest.mm
+++ b/chrome/browser/ui/cocoa/menu_controller_unittest.mm
@@ -5,11 +5,14 @@
 #import <Cocoa/Cocoa.h>
 
 #include "app/menus/simple_menu_model.h"
+#include "app/resource_bundle.h"
 #include "base/sys_string_conversions.h"
 #include "base/utf_string_conversions.h"
 #include "chrome/browser/ui/cocoa/cocoa_test_helper.h"
 #include "chrome/browser/ui/cocoa/menu_controller.h"
+#include "grit/app_resources.h"
 #include "grit/generated_resources.h"
+#include "third_party/skia/include/core/SkBitmap.h"
 
 class MenuControllerTest : public CocoaTest {
 };
@@ -34,6 +37,29 @@ class Delegate : public menus::SimpleMenuModel::Delegate {
   mutable int enable_count_;
 };
 
+// Just like Delegate, except the items are treated as "dynamic" so updates to
+// the label/icon in the model are reflected in the menu.
+class DynamicDelegate : public Delegate {
+ public:
+  DynamicDelegate() : icon_(NULL) {}
+  virtual bool IsItemForCommandIdDynamic(int command_id) const { return true; }
+  virtual string16 GetLabelForCommandId(int command_id) const { return label_; }
+  virtual bool GetIconForCommandId(int command_id, SkBitmap* icon) const {
+    if (icon_) {
+      *icon = *icon_;
+      return true;
+    } else {
+      return false;
+    }
+  }
+  void SetDynamicLabel(string16 label) { label_ = label; }
+  void SetDynamicIcon(SkBitmap* icon) { icon_ = icon; }
+
+ private:
+  string16 label_;
+  SkBitmap* icon_;
+};
+
 TEST_F(MenuControllerTest, EmptyMenu) {
   Delegate delegate;
   menus::SimpleMenuModel model(&delegate);
@@ -195,3 +221,42 @@ TEST_F(MenuControllerTest, DefaultInitializer) {
   model.AddItem(4, ASCIIToUTF16("four"));
   EXPECT_EQ(3, [[menu menu] numberOfItems]);
 }
+
+// Test that menus with dynamic labels actually get updated.
+TEST_F(MenuControllerTest, Dynamic) {
+  DynamicDelegate delegate;
+
+  // Create a menu containing a single item whose label is "initial" and who has
+  // no icon.
+  string16 initial = ASCIIToUTF16("initial");
+  delegate.SetDynamicLabel(initial);
+  menus::SimpleMenuModel model(&delegate);
+  model.AddItem(1, ASCIIToUTF16("foo"));
+  scoped_nsobject<MenuController> menu(
+      [[MenuController alloc] initWithModel:&model useWithPopUpButtonCell:NO]);
+  EXPECT_EQ([[menu menu] numberOfItems], 1);
+  // Validate() simulates opening the menu - the item label/icon should be
+  // initialized after this so we can validate the menu contents.
+  Validate(menu.get(), [menu menu]);
+  NSMenuItem* item = [[menu menu] itemAtIndex:0];
+  // Item should have the "initial" label and no icon.
+  EXPECT_EQ(initial, base::SysNSStringToUTF16([item title]));
+  EXPECT_EQ(nil, [item image]);
+
+  // Now update the item to have a label of "second" and an icon.
+  string16 second = ASCIIToUTF16("second");
+  delegate.SetDynamicLabel(second);
+  SkBitmap* bitmap =
+      ResourceBundle::GetSharedInstance().GetBitmapNamed(IDR_THROBBER);
+  delegate.SetDynamicIcon(bitmap);
+  // Simulate opening the menu and validate that the item label + icon changes.
+  Validate(menu.get(), [menu menu]);
+  EXPECT_EQ(second, base::SysNSStringToUTF16([item title]));
+  EXPECT_TRUE([item image] != nil);
+
+  // Now get rid of the icon and make sure it goes away.
+  delegate.SetDynamicIcon(NULL);
+  Validate(menu.get(), [menu menu]);
+  EXPECT_EQ(second, base::SysNSStringToUTF16([item title]));
+  EXPECT_EQ(nil, [item image]);
+}
diff --git a/chrome/browser/ui/toolbar/back_forward_menu_model.cc b/chrome/browser/ui/toolbar/back_forward_menu_model.cc
index 5ea279f..390dce0 100644
--- a/chrome/browser/ui/toolbar/back_forward_menu_model.cc
+++ b/chrome/browser/ui/toolbar/back_forward_menu_model.cc
@@ -87,7 +87,7 @@ string16 BackForwardMenuModel::GetLabelAt(int index) const {
   return menu_text;
 }
 
-bool BackForwardMenuModel::IsLabelDynamicAt(int index) const {
+bool BackForwardMenuModel::IsItemDynamicAt(int index) const {
   // This object is only used for a single showing of a menu.
   return false;
 }
diff --git a/chrome/browser/ui/toolbar/back_forward_menu_model.h b/chrome/browser/ui/toolbar/back_forward_menu_model.h
index aba4688..1d669a0 100644
--- a/chrome/browser/ui/toolbar/back_forward_menu_model.h
+++ b/chrome/browser/ui/toolbar/back_forward_menu_model.h
@@ -48,7 +48,7 @@ class BackForwardMenuModel : public menus::MenuModel {
   virtual ItemType GetTypeAt(int index) const;
   virtual int GetCommandIdAt(int index) const;
   virtual string16 GetLabelAt(int index) const;
-  virtual bool IsLabelDynamicAt(int index) const;
+  virtual bool IsItemDynamicAt(int index) const;
   virtual bool GetAcceleratorAt(int index,
                                 menus::Accelerator* accelerator) const;
   virtual bool IsItemCheckedAt(int index) const;
diff --git a/chrome/browser/ui/toolbar/wrench_menu_model.cc b/chrome/browser/ui/toolbar/wrench_menu_model.cc
index 0efd669..cd8a114 100644
--- a/chrome/browser/ui/toolbar/wrench_menu_model.cc
+++ b/chrome/browser/ui/toolbar/wrench_menu_model.cc
@@ -221,9 +221,6 @@ WrenchMenuModel::WrenchMenuModel(menus::AcceleratorProvider* provider,
                  Source<Profile>(browser_->profile()));
   registrar_.Add(this, NotificationType::NAV_ENTRY_COMMITTED,
                  NotificationService::AllSources());
-  registrar_.Add(this,
-                 NotificationType::BACKGROUND_PAGE_TRACKER_CHANGED,
-                 NotificationService::AllSources());
 }
 
 WrenchMenuModel::~WrenchMenuModel() {
@@ -235,7 +232,7 @@ bool WrenchMenuModel::DoesCommandIdDismissMenu(int command_id) const {
   return command_id != IDC_ZOOM_MINUS && command_id != IDC_ZOOM_PLUS;
 }
 
-bool WrenchMenuModel::IsLabelForCommandIdDynamic(int command_id) const {
+bool WrenchMenuModel::IsItemForCommandIdDynamic(int command_id) const {
   return command_id == IDC_ZOOM_PERCENT_DISPLAY ||
 #if defined(OS_MACOSX)
          command_id == IDC_FULLSCREEN ||
@@ -244,6 +241,26 @@ bool WrenchMenuModel::IsLabelForCommandIdDynamic(int command_id) const {
          command_id == IDC_VIEW_BACKGROUND_PAGES;
 }
 
+bool WrenchMenuModel::GetIconForCommandId(int command_id,
+                                          SkBitmap* bitmap) const {
+  switch (command_id) {
+    case IDC_VIEW_BACKGROUND_PAGES: {
+      int num_pages = BackgroundPageTracker::GetInstance()->
+          GetUnacknowledgedBackgroundPageCount();
+      if (num_pages > 0) {
+        *bitmap = GetBackgroundPageIcon();
+        return true;
+      } else {
+        // No icon.
+        return false;
+      }
+    }
+    default:
+      // No icon for other dynamic menu items.
+      return false;
+  }
+}
+
 string16 WrenchMenuModel::GetLabelForCommandId(int command_id) const {
   switch (command_id) {
     case IDC_SYNC_BOOKMARKS:
@@ -350,18 +367,6 @@ void WrenchMenuModel::Observe(NotificationType type,
                               const NotificationSource& source,
                               const NotificationDetails& details) {
   switch (type.value) {
-    case NotificationType::BACKGROUND_PAGE_TRACKER_CHANGED: {
-      int num_pages = BackgroundPageTracker::GetInstance()->
-          GetUnacknowledgedBackgroundPageCount();
-      if (num_pages > 0) {
-        SetIcon(GetIndexOfCommandId(IDC_VIEW_BACKGROUND_PAGES),
-                GetBackgroundPageIcon());
-      } else {
-        // Just set a blank icon (no icon).
-        SetIcon(GetIndexOfCommandId(IDC_VIEW_BACKGROUND_PAGES), SkBitmap());
-      }
-      break;
-    }
     case NotificationType::ZOOM_LEVEL_CHANGED:
     case NotificationType::NAV_ENTRY_COMMITTED:
       UpdateZoomControls();
@@ -482,14 +487,6 @@ void WrenchMenuModel::Build() {
           *rb.GetBitmapNamed(IDR_CONFLICT_MENU));
 #endif
 
-  // Add an icon to the View Background Pages item if there are unacknowledged
-  // pages.
-  if (BackgroundPageTracker::GetInstance()->
-      GetUnacknowledgedBackgroundPageCount() > 0) {
-    SetIcon(GetIndexOfCommandId(IDC_VIEW_BACKGROUND_PAGES),
-            GetBackgroundPageIcon());
-  }
-
   AddItemWithStringId(IDC_HELP_PAGE, IDS_HELP_PAGE);
   if (browser_defaults::kShowExitMenuItem) {
     AddSeparator();
diff --git a/chrome/browser/ui/toolbar/wrench_menu_model.h b/chrome/browser/ui/toolbar/wrench_menu_model.h
index e984a4b..2690ea9 100644
--- a/chrome/browser/ui/toolbar/wrench_menu_model.h
+++ b/chrome/browser/ui/toolbar/wrench_menu_model.h
@@ -82,8 +82,9 @@ class WrenchMenuModel : public menus::SimpleMenuModel,
   virtual bool DoesCommandIdDismissMenu(int command_id) const;
 
   // Overridden for both ButtonMenuItemModel::Delegate and SimpleMenuModel:
-  virtual bool IsLabelForCommandIdDynamic(int command_id) const;
+  virtual bool IsItemForCommandIdDynamic(int command_id) const;
   virtual string16 GetLabelForCommandId(int command_id) const;
+  virtual bool GetIconForCommandId(int command_id, SkBitmap* icon) const;
   virtual void ExecuteCommand(int command_id);
   virtual bool IsCommandIdChecked(int command_id) const;
   virtual bool IsCommandIdEnabled(int command_id) const;
diff --git a/chrome/browser/ui/views/frame/browser_view.cc b/chrome/browser/ui/views/frame/browser_view.cc
index 769b59f..3f99af9 100644
--- a/chrome/browser/ui/views/frame/browser_view.cc
+++ b/chrome/browser/ui/views/frame/browser_view.cc
@@ -1527,7 +1527,7 @@ bool BrowserView::GetAcceleratorForCommandId(int command_id,
   return toolbar_->GetAcceleratorForCommandId(command_id, accelerator);
 }
 
-bool BrowserView::IsLabelForCommandIdDynamic(int command_id) const {
+bool BrowserView::IsItemForCommandIdDynamic(int command_id) const {
   return command_id == IDC_RESTORE_TAB;
 }
 
diff --git a/chrome/browser/ui/views/frame/browser_view.h b/chrome/browser/ui/views/frame/browser_view.h
index 402f4f2..584a9c1 100644
--- a/chrome/browser/ui/views/frame/browser_view.h
+++ b/chrome/browser/ui/views/frame/browser_view.h
@@ -360,7 +360,7 @@ class BrowserView : public BrowserBubbleHost,
   virtual bool IsCommandIdEnabled(int command_id) const;
   virtual bool GetAcceleratorForCommandId(int command_id,
                                           menus::Accelerator* accelerator);
-  virtual bool IsLabelForCommandIdDynamic(int command_id) const;
+  virtual bool IsItemForCommandIdDynamic(int command_id) const;
   virtual string16 GetLabelForCommandId(int command_id) const;
   virtual void ExecuteCommand(int command_id);