The "Update Chrome" menu item should appear in addition to the About menu.

It should not replace it. This patch modifications to the GTK and Cocoa ports to make the update chrome item appear when an update is available. On win/chromeos, the menu item is always there but disabled, since I'm having some problems figuring out the views custom menu implementation.

BUG=46221
TEST=The upgrade item should now appear under instead of replacing the about command.

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

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

11 files changed, 74 insertions, 44 deletions

diff --git a/app/menus/menu_model.cc b/app/menus/menu_model.cc
index 63acbf6..cbce228 100644
--- a/app/menus/menu_model.cc
+++ b/app/menus/menu_model.cc
@@ -6,6 +6,10 @@
 
 namespace menus {
 
+bool MenuModel::IsVisibleAt(int index) const {
+  return true;
+}
+
 bool MenuModel::GetModelAndIndexForCommandId(int command_id,
                                              MenuModel** model, int* index) {
   int item_count = (*model)->GetItemCount();
diff --git a/app/menus/menu_model.h b/app/menus/menu_model.h
index c91efc9..1cb356a 100644
--- a/app/menus/menu_model.h
+++ b/app/menus/menu_model.h
@@ -94,6 +94,9 @@ class MenuModel {
   // Returns the enabled state of the item at the specified index.
   virtual bool IsEnabledAt(int index) const = 0;
 
+  // Returns true if the menu item is visible.
+  virtual bool IsVisibleAt(int index) const;
+
   // Returns the model for the submenu at the specified index.
   virtual MenuModel* GetSubmenuModelAt(int index) const = 0;
 
diff --git a/app/menus/simple_menu_model.cc b/app/menus/simple_menu_model.cc
index 6a40f75..b645b09 100644
--- a/app/menus/simple_menu_model.cc
+++ b/app/menus/simple_menu_model.cc
@@ -24,6 +24,10 @@ struct SimpleMenuModel::Item {
 ////////////////////////////////////////////////////////////////////////////////
 // SimpleMenuModel::Delegate, public:
 
+bool SimpleMenuModel::Delegate::IsCommandIdVisible(int command_id) const {
+  return true;
+}
+
 bool SimpleMenuModel::Delegate::IsLabelForCommandIdDynamic(
     int command_id) const {
   return false;
@@ -250,6 +254,14 @@ bool SimpleMenuModel::IsEnabledAt(int index) const {
   return delegate_->IsCommandIdEnabled(command_id);
 }
 
+bool SimpleMenuModel::IsVisibleAt(int index) const {
+  int command_id = GetCommandIdAt(index);
+  if (!delegate_ || command_id == kSeparatorId ||
+      items_.at(FlipIndex(index)).button_model)
+    return true;
+  return delegate_->IsCommandIdVisible(command_id);
+}
+
 void SimpleMenuModel::HighlightChangedTo(int index) {
   if (delegate_)
     delegate_->CommandIdHighlighted(GetCommandIdAt(index));
@@ -264,6 +276,10 @@ MenuModel* SimpleMenuModel::GetSubmenuModelAt(int index) const {
   return items_.at(FlipIndex(index)).submenu;
 }
 
+int SimpleMenuModel::FlipIndex(int index) const {
+  return index;
+}
+
 ////////////////////////////////////////////////////////////////////////////////
 // SimpleMenuModel, Private:
 
diff --git a/app/menus/simple_menu_model.h b/app/menus/simple_menu_model.h
index c196225..149791c 100644
--- a/app/menus/simple_menu_model.h
+++ b/app/menus/simple_menu_model.h
@@ -26,6 +26,7 @@ class SimpleMenuModel : public MenuModel {
     // Methods for determining the state of specific command ids.
     virtual bool IsCommandIdChecked(int command_id) const = 0;
     virtual bool IsCommandIdEnabled(int command_id) const = 0;
+    virtual bool IsCommandIdVisible(int command_id) const;
 
     // Gets the accelerator for the specified command id. Returns true if the
     // command id has a valid accelerator, false otherwise.
@@ -107,6 +108,7 @@ class SimpleMenuModel : public MenuModel {
   virtual bool GetIconAt(int index, SkBitmap* icon) const;
   virtual menus::ButtonMenuItemModel* GetButtonMenuItemAt(int index) const;
   virtual bool IsEnabledAt(int index) const;
+  virtual bool IsVisibleAt(int index) const;
   virtual void HighlightChangedTo(int index);
   virtual void ActivatedAt(int index);
   virtual MenuModel* GetSubmenuModelAt(int index) const;
@@ -117,7 +119,7 @@ class SimpleMenuModel : public MenuModel {
   // forcing customers to insert things backwards, we return the indices
   // backwards instead. That's what this method is for. By default, it just
   // returns what it's passed.
-  virtual int FlipIndex(int index) const { return index; }
+  virtual int FlipIndex(int index) const;
 
   Delegate* delegate() { return delegate_; }
 
diff --git a/chrome/app/chrome_dll_resource.h b/chrome/app/chrome_dll_resource.h
index 2c37401..42bbdf4 100644
--- a/chrome/app/chrome_dll_resource.h
+++ b/chrome/app/chrome_dll_resource.h
@@ -208,6 +208,7 @@
 #define IDC_MANAGE_EXTENSIONS           40022
 #define IDC_AUTOFILL_DEFAULT            40023
 #define IDC_DEV_TOOLS_INSPECT           40025
+#define IDC_UPGRADE_DIALOG              40026
 
 // Spell-check
 // Insert any additional suggestions before _LAST; these have to be consecutive.
diff --git a/chrome/browser/browser.cc b/chrome/browser/browser.cc
index 02fe3ea..eab1ea4 100644
--- a/chrome/browser/browser.cc
+++ b/chrome/browser/browser.cc
@@ -2168,12 +2168,8 @@ void Browser::ExecuteCommandWithDisposition(
     case IDC_VIEW_PASSWORDS:        OpenPasswordManager();            break;
     case IDC_CLEAR_BROWSING_DATA:   OpenClearBrowsingDataDialog();    break;
     case IDC_IMPORT_SETTINGS:       OpenImportSettingsDialog();       break;
-    case IDC_ABOUT:
-      if (Singleton<UpgradeDetector>::get()->notify_upgrade())
-        OpenUpdateChromeDialog();
-      else
-        OpenAboutChromeDialog();
-      break;
+    case IDC_ABOUT:                 OpenAboutChromeDialog();          break;
+    case IDC_UPGRADE_DIALOG:        OpenUpdateChromeDialog();         break;
     case IDC_HELP_PAGE:             OpenHelpTab();                    break;
 #if defined(OS_CHROMEOS)
     case IDC_SYSTEM_OPTIONS:        OpenSystemOptionsDialog();        break;
@@ -3326,6 +3322,10 @@ void Browser::InitCommandState() {
   // Show various bits of UI
   command_updater_.UpdateCommandEnabled(IDC_CLEAR_BROWSING_DATA, normal_window);
 
+  // The upgrade entry should always be enabled. Whether it is visible is a
+  // separate matter determined on menu show.
+  command_updater_.UpdateCommandEnabled(IDC_UPGRADE_DIALOG, true);
+
   // Initialize other commands whose state changes based on fullscreen mode.
   UpdateCommandsForFullscreenMode(false);
 }
diff --git a/chrome/browser/cocoa/menu_controller.mm b/chrome/browser/cocoa/menu_controller.mm
index a5b8256..4e0aa16 100644
--- a/chrome/browser/cocoa/menu_controller.mm
+++ b/chrome/browser/cocoa/menu_controller.mm
@@ -143,6 +143,7 @@
     BOOL checked = model->IsItemCheckedAt(modelIndex);
     DCHECK([(id)item isKindOfClass:[NSMenuItem class]]);
     [(id)item setState:(checked ? NSOnState : NSOffState)];
+    [(id)item setHidden:(!model->IsVisibleAt(modelIndex))];
     if (model->IsLabelDynamicAt(modelIndex)) {
       NSString* label =
           l10n_util::FixUpWindowsStyleLabel(model->GetLabelAt(modelIndex));
diff --git a/chrome/browser/gtk/menu_gtk.cc b/chrome/browser/gtk/menu_gtk.cc
index 80c20c4..129623e 100644
--- a/chrome/browser/gtk/menu_gtk.cc
+++ b/chrome/browser/gtk/menu_gtk.cc
@@ -214,22 +214,31 @@ GtkWidget* MenuGtk::AppendSeparator() {
 }
 
 GtkWidget* MenuGtk::AppendMenuItem(int command_id, GtkWidget* menu_item) {
-  return AppendMenuItemToMenu(command_id, menu_item, menu_, true);
+  return AppendMenuItemToMenu(command_id, NULL, menu_item, menu_, true);
 }
 
-GtkWidget* MenuGtk::AppendMenuItemToMenu(int command_id,
+GtkWidget* MenuGtk::AppendMenuItemToMenu(int index,
+                                         menus::MenuModel* model,
                                          GtkWidget* menu_item,
                                          GtkWidget* menu,
                                          bool connect_to_activate) {
   // Native menu items do their own thing, so only selectively listen for the
   // activate signal.
   if (connect_to_activate) {
-    SetMenuItemID(menu_item, command_id);
+    SetMenuItemID(menu_item, index);
     g_signal_connect(menu_item, "activate",
                      G_CALLBACK(OnMenuItemActivated), this);
   }
 
-  gtk_widget_show(menu_item);
+  // AppendMenuItemToMenu is used both internally when we control menu creation
+  // from a model (where the model can choose to hide certain menu items), and
+  // with immediate commands which don't provide the option.
+  if (model) {
+    if (model->IsVisibleAt(index))
+      gtk_widget_show(menu_item);
+  } else {
+    gtk_widget_show(menu_item);
+  }
   gtk_menu_shell_append(GTK_MENU_SHELL(menu), menu_item);
   return menu_item;
 }
@@ -361,7 +370,7 @@ void MenuGtk::BuildSubmenuFromModel(menus::MenuModel* model, GtkWidget* menu) {
     }
 
     g_object_set_data(G_OBJECT(menu_item), "model", model);
-    AppendMenuItemToMenu(i, menu_item, menu, connect_to_activate);
+    AppendMenuItemToMenu(i, model, menu_item, menu, connect_to_activate);
 
     if (model->IsLabelDynamicAt(i)) {
       g_signal_connect(menu, "show", G_CALLBACK(OnSubmenuShow),
@@ -603,6 +612,11 @@ void MenuGtk::SetMenuItemInfo(GtkWidget* widget, gpointer userdata) {
   if (GTK_IS_MENU_ITEM(widget)) {
     gtk_widget_set_sensitive(widget, model->IsEnabledAt(id));
 
+    if (model->IsVisibleAt(id))
+      gtk_widget_show(widget);
+    else
+      gtk_widget_hide(widget);
+
     GtkWidget* submenu = gtk_menu_item_get_submenu(GTK_MENU_ITEM(widget));
     if (submenu) {
       gtk_container_foreach(GTK_CONTAINER(submenu), &SetMenuItemInfo,
diff --git a/chrome/browser/gtk/menu_gtk.h b/chrome/browser/gtk/menu_gtk.h
index 7f8e7ec..7ad315f 100644
--- a/chrome/browser/gtk/menu_gtk.h
+++ b/chrome/browser/gtk/menu_gtk.h
@@ -63,7 +63,8 @@ class MenuGtk {
                                           const std::string& label);
   GtkWidget* AppendSeparator();
   GtkWidget* AppendMenuItem(int command_id, GtkWidget* menu_item);
-  GtkWidget* AppendMenuItemToMenu(int command_id,
+  GtkWidget* AppendMenuItemToMenu(int index,
+                                  menus::MenuModel* model,
                                   GtkWidget* menu_item,
                                   GtkWidget* menu,
                                   bool connect_to_activate);
diff --git a/chrome/browser/wrench_menu_model.cc b/chrome/browser/wrench_menu_model.cc
index 3958546..abea856 100644
--- a/chrome/browser/wrench_menu_model.cc
+++ b/chrome/browser/wrench_menu_model.cc
@@ -194,17 +194,14 @@ WrenchMenuModel::~WrenchMenuModel() {
 
 bool WrenchMenuModel::IsLabelForCommandIdDynamic(int command_id) const {
   return command_id == IDC_ZOOM_PERCENT_DISPLAY ||
-         command_id == IDC_SYNC_BOOKMARKS ||
 #if defined(OS_MACOSX)
          command_id == IDC_FULLSCREEN ||
 #endif
-         command_id == IDC_ABOUT;
+         command_id == IDC_SYNC_BOOKMARKS;
 }
 
 string16 WrenchMenuModel::GetLabelForCommandId(int command_id) const {
   switch (command_id) {
-    case IDC_ABOUT:
-      return GetAboutEntryMenuLabel();
     case IDC_SYNC_BOOKMARKS:
       return GetSyncMenuLabel();
     case IDC_ZOOM_PERCENT_DISPLAY:
@@ -246,6 +243,12 @@ bool WrenchMenuModel::IsCommandIdEnabled(int command_id) const {
   return browser_->command_updater()->IsCommandEnabled(command_id);
 }
 
+bool WrenchMenuModel::IsCommandIdVisible(int command_id) const {
+  if (command_id == IDC_UPGRADE_DIALOG)
+    return Singleton<UpgradeDetector>::get()->notify_upgrade();
+  return true;
+}
+
 bool WrenchMenuModel::GetAcceleratorForCommandId(
       int command_id,
       menus::Accelerator* accelerator) {
@@ -366,24 +369,18 @@ void WrenchMenuModel::Build() {
                            IDS_TAB_CXMENU_USE_VERTICAL_TABS);
 #endif
 
-  // TODO(erg): This entire section needs to be reworked and is out of scope of
-  // the first cleanup patch I'm doing. Part 1 (crbug.com/47320) is moving most
-  // of the logic from each platform specific UI code into the model here. Part
-  // 2 (crbug.com/46221) is normalizing the about menu item/hidden update menu
-  // item behaviour across the three platforms.
-
-  // On Mac, there is no About item unless it is replaced with the update
-  // available notification.
-  if (browser_defaults::kShowAboutMenuItem ||
-      Singleton<UpgradeDetector>::get()->notify_upgrade()) {
-    AddItem(IDC_ABOUT,
-            l10n_util::GetStringFUTF16(
-                IDS_ABOUT,
-                l10n_util::GetStringUTF16(IDS_PRODUCT_NAME)));
-    // ResourceBundle& rb = ResourceBundle::GetSharedInstance();
-    // SetIcon(GetIndexOfCommandId(IDC_ABOUT),
-    //                *rb.GetBitmapNamed(IDR_UPDATE_AVAILABLE));
+  // On Mac, there is no About item.
+  if (browser_defaults::kShowAboutMenuItem) {
+    AddItem(IDC_ABOUT, l10n_util::GetStringFUTF16(
+        IDS_ABOUT, l10n_util::GetStringUTF16(IDS_PRODUCT_NAME)));
   }
+
+  AddItem(IDC_UPGRADE_DIALOG, l10n_util::GetStringFUTF16(
+      IDS_UPDATE_NOW, l10n_util::GetStringUTF16(IDS_PRODUCT_NAME)));
+  ResourceBundle& rb = ResourceBundle::GetSharedInstance();
+  SetIcon(GetIndexOfCommandId(IDC_UPGRADE_DIALOG),
+          *rb.GetBitmapNamed(IDR_UPDATE_AVAILABLE));
+
   AddItemWithStringId(IDC_HELP_PAGE, IDS_HELP_PAGE);
   if (browser_defaults::kShowExitMenuItem) {
     AddSeparator();
@@ -443,12 +440,3 @@ string16 WrenchMenuModel::GetSyncMenuLabel() const {
   return sync_ui_util::GetSyncMenuLabel(
       browser_->profile()->GetOriginalProfile()->GetProfileSyncService());
 }
-
-string16 WrenchMenuModel::GetAboutEntryMenuLabel() const {
-  if (Singleton<UpgradeDetector>::get()->notify_upgrade()) {
-    return l10n_util::GetStringFUTF16(
-        IDS_UPDATE_NOW, l10n_util::GetStringUTF16(IDS_PRODUCT_NAME));
-  }
-  return l10n_util::GetStringFUTF16(
-      IDS_ABOUT, l10n_util::GetStringUTF16(IDS_PRODUCT_NAME));
-}
diff --git a/chrome/browser/wrench_menu_model.h b/chrome/browser/wrench_menu_model.h
index b3e2298..e344646 100644
--- a/chrome/browser/wrench_menu_model.h
+++ b/chrome/browser/wrench_menu_model.h
@@ -83,6 +83,7 @@ class WrenchMenuModel : public menus::SimpleMenuModel,
   virtual void ExecuteCommand(int command_id);
   virtual bool IsCommandIdChecked(int command_id) const;
   virtual bool IsCommandIdEnabled(int command_id) const;
+  virtual bool IsCommandIdVisible(int command_id) const;
   virtual bool GetAcceleratorForCommandId(
       int command_id,
       menus::Accelerator* accelerator);
@@ -123,7 +124,6 @@ class WrenchMenuModel : public menus::SimpleMenuModel,
   double GetZoom(bool* enable_increment, bool* enable_decrement);
 
   string16 GetSyncMenuLabel() const;
-  string16 GetAboutEntryMenuLabel() const;
 
   // Models for the special menu items with buttons.
   scoped_ptr<menus::ButtonMenuItemModel> edit_menu_item_model_;