Restructure the wrench menu into an action based order

chrome/browser/ui/toolbar/wrench_menu_model.cc

Index: chrome/browser/ui/toolbar/wrench_menu_model.cc
diff --git a/chrome/browser/ui/toolbar/wrench_menu_model.cc b/chrome/browser/ui/toolbar/wrench_menu_model.cc
index c7a0991892fdfbb61b75cdc9d89b944b5b816d20..1f2f939e8d5556d56324cdd054fb5e09b6ca8871 100644
--- a/chrome/browser/ui/toolbar/wrench_menu_model.cc
+++ b/chrome/browser/ui/toolbar/wrench_menu_model.cc
@@ -239,6 +239,12 @@ void ToolsMenuModel::Build(Browser* browser) {
     show_create_shortcuts = false;
 #endif
 
+  AddItemWithStringId(IDC_CLEAR_BROWSING_DATA, IDS_CLEAR_BROWSING_DATA);
+  AddItemWithStringId(IDC_MANAGE_EXTENSIONS, IDS_SHOW_EXTENSIONS);
+
+  if (chrome::CanOpenTaskManager())
+    AddItemWithStringId(IDC_TASK_MANAGER, IDS_TASK_MANAGER);
+
   if (extensions::util::IsNewBookmarkAppsEnabled()) {
 #if defined(OS_MACOSX)
     int string_id = IDS_ADD_TO_APPLICATIONS;
@@ -252,30 +258,20 @@ void ToolsMenuModel::Build(Browser* browser) {
       string_id = IDS_ADD_TO_SHELF;
 #endif
     AddItemWithStringId(IDC_CREATE_HOSTED_APP, string_id);
-    AddSeparator(ui::NORMAL_SEPARATOR);
   } else if (show_create_shortcuts) {
     AddItemWithStringId(IDC_CREATE_SHORTCUTS, IDS_CREATE_SHORTCUTS);
-    AddSeparator(ui::NORMAL_SEPARATOR);
   }
 
-  AddItemWithStringId(IDC_MANAGE_EXTENSIONS, IDS_SHOW_EXTENSIONS);
-
-  if (chrome::CanOpenTaskManager())
-    AddItemWithStringId(IDC_TASK_MANAGER, IDS_TASK_MANAGER);
-
-  AddItemWithStringId(IDC_CLEAR_BROWSING_DATA, IDS_CLEAR_BROWSING_DATA);
-
 #if defined(OS_CHROMEOS)
   AddItemWithStringId(IDC_TAKE_SCREENSHOT, IDS_TAKE_SCREENSHOT);
 #endif
 
-  AddSeparator(ui::NORMAL_SEPARATOR);
-
   encoding_menu_model_.reset(new EncodingMenuModel(browser));
   AddSubMenuWithStringId(IDC_ENCODING_MENU, IDS_ENCODING_MENU,
                          encoding_menu_model_.get());
-  AddItemWithStringId(IDC_VIEW_SOURCE, IDS_VIEW_SOURCE);
+  AddSeparator(ui::NORMAL_SEPARATOR);
   AddItemWithStringId(IDC_DEV_TOOLS, IDS_DEV_TOOLS);
+  AddItemWithStringId(IDC_VIEW_SOURCE, IDS_VIEW_SOURCE);
   AddItemWithStringId(IDC_DEV_TOOLS_CONSOLE, IDS_DEV_TOOLS_CONSOLE);
   AddItemWithStringId(IDC_DEV_TOOLS_DEVICES, IDS_DEV_TOOLS_DEVICES);
 
@@ -825,31 +821,55 @@ bool WrenchMenuModel::ShouldShowNewIncognitoWindowMenuItem() {
       IncognitoModePrefs::DISABLED;
 }
 
+// Note: When adding new menu items please place under an appropriate section.
+// Menu is organised as follows:
+// - Global browser errors and warnings.

[Peter Kasting, 2015/04/01 21:06:28]

Nit: Should probably add "- Extension toolbar overflow items." atop this list.

+// - Tabs and windows.
+// - Places previously been e.g. History, bookmarks, recent tabs.
+// - Page actions e.g. zoom, edit, find, print.
+// - Learn about the browser and global customisation e.g. settings, help.
+// - Browser relaunch, quit.
 void WrenchMenuModel::Build() {
+  bool add_separator = false;

[Peter Kasting, 2015/04/01 21:06:29]

Nit: This can be eliminated; see further notes below.

+
+  if (extensions::FeatureSwitch::extension_action_redesign()->IsEnabled())
+    CreateExtensionToolbarOverflowMenu();
+
 #if defined(OS_WIN)
   AddItem(IDC_VIEW_INCOMPATIBILITIES,
       l10n_util::GetStringUTF16(IDS_VIEW_INCOMPATIBILITIES));
+  SetIcon(GetIndexOfCommandId(IDC_VIEW_INCOMPATIBILITIES),
+          ui::ResourceBundle::GetSharedInstance().
+              GetNativeImageNamed(IDR_INPUT_ALERT_MENU));
+
   EnumerateModulesModel* model =
       EnumerateModulesModel::GetInstance();
   if (model->modules_to_notify_about() > 0 ||
       model->confirmed_bad_modules_detected() > 0)
-    AddSeparator(ui::NORMAL_SEPARATOR);
+    add_separator = true;

[Peter Kasting, 2015/04/01 21:06:28]

Instead of checking the EnumerateModulesModel directly, let's make the
visibility determination using
"IsCommandIdVisible(IDC_VIEW_INCOMPATIBILITIES);".

This will be simpler, and more future-proof if we later change the conditions
under which this item is shown.

This also will enable you to remove the #if defined(OS_WIN) around this block,
as on non-Windows, the item will always be invisible and thus harmless to add.

[edwardjung, 2015/04/02 13:13:28]

Nice, I like simplicity. Done.

As IDS_VIEW_INCOMPATIBILITIES was wrapped in a win only block I've made
modifications to the generated_resources.grd too.

 #endif
 
-  if (extensions::FeatureSwitch::extension_action_redesign()->IsEnabled())
-    CreateExtensionToolbarOverflowMenu();
+  if (browser_defaults::kShowUpgradeMenuItem) {

[Peter Kasting, 2015/04/01 21:06:28]

Nit: Move the check of this variable into IsCommandIdVisible().  This eliminates
the conditional here and is necessary for a simplification I suggest below.

[edwardjung, 2015/04/02 13:13:28]

Done.

+    AddItem(IDC_UPGRADE_DIALOG, GetUpgradeDialogMenuItemName());
+    if (UpgradeDetector::GetInstance()->notify_upgrade())

[Peter Kasting, 2015/04/01 21:06:28]

Similarly to above, this should call IsCommandIdVisible() instead of duplicating
that function's functionality.

[edwardjung, 2015/04/02 13:13:28]

Done.

+      add_separator = true;
+  }
+
+  if (AddGlobalErrorMenuItems())

[Peter Kasting, 2015/04/01 21:06:28]

You can eliminate |add_separator| entirely and shorten this function by moving
the IsCommandIdVisible() calls I suggest above down to here.  (Note that this
requires moving the kShowUpgradeMenuItem check into IsCommandIdVisible().)

This simply becomes:

  if (AddGlobalErrorMenuItems() ||
      IsCommandIdVisible(IDC_VIEW_INCOMPATIBILITIES) ||
      IsCommandIdVisible(IDC_UPGRADE_DIALOG))
    AddSeparator(ui::NORMAL_SEPARATOR);

[edwardjung, 2015/04/02 13:13:28]

Done.

+    add_separator = true;
+
+  if (add_separator)
+    AddSeparator(ui::NORMAL_SEPARATOR);
 
   AddItemWithStringId(IDC_NEW_TAB, IDS_NEW_TAB);
   AddItemWithStringId(IDC_NEW_WINDOW, IDS_NEW_WINDOW);
 
   if (ShouldShowNewIncognitoWindowMenuItem())
     AddItemWithStringId(IDC_NEW_INCOGNITO_WINDOW, IDS_NEW_INCOGNITO_WINDOW);

[Peter Kasting, 2015/04/01 21:06:28]

Nit: I'd probably remove the blank line above this conditional and add one just
below it.

[edwardjung, 2015/04/02 13:13:28]

Done.

+  AddSeparator(ui::NORMAL_SEPARATOR);
 
-  if (!browser_->profile()->IsGuestSession()) {
-    bookmark_sub_menu_model_.reset(new BookmarkSubMenuModel(this, browser_));
-    AddSubMenuWithStringId(IDC_BOOKMARKS_MENU, IDS_BOOKMARKS_MENU,
-                           bookmark_sub_menu_model_.get());
-  }
+  AddItemWithStringId(IDC_SHOW_HISTORY, IDS_SHOW_HISTORY);
+  AddItemWithStringId(IDC_SHOW_DOWNLOADS, IDS_SHOW_DOWNLOADS);
 
   if (!browser_->profile()->IsOffTheRecord()) {
     recent_tabs_sub_menu_model_.reset(new RecentTabsSubMenuModel(provider_,
@@ -859,53 +879,29 @@ void WrenchMenuModel::Build() {
                            recent_tabs_sub_menu_model_.get());
   }
 
-#if defined(OS_WIN)
-  base::win::Version min_version_for_ash_mode = base::win::VERSION_WIN8;
-  // Windows 7 ASH mode is only supported in DEBUG for now.
-#if !defined(NDEBUG)
-  min_version_for_ash_mode = base::win::VERSION_WIN7;
-#endif
-  // Windows 8 can support ASH mode using WARP, but Windows 7 requires a working
-  // GPU compositor.
-  if ((base::win::GetVersion() >= min_version_for_ash_mode &&
-      content::GpuDataManager::GetInstance()->CanUseGpuBrowserCompositor()) ||
-      (base::win::GetVersion() >= base::win::VERSION_WIN8)) {
-    if (browser_->host_desktop_type() == chrome::HOST_DESKTOP_TYPE_ASH) {
-      // ASH/Metro mode, add the 'Relaunch Chrome in desktop mode'.
-      AddSeparator(ui::NORMAL_SEPARATOR);
-      AddItemWithStringId(IDC_WIN_DESKTOP_RESTART, IDS_WIN_DESKTOP_RESTART);
-    } else {
-      // In Windows 8 desktop, add the 'Relaunch Chrome in Windows 8 mode'.
-      // In Windows 7 desktop, add the 'Relaunch Chrome in Windows ASH mode'
-      AddSeparator(ui::NORMAL_SEPARATOR);
-      if (base::win::GetVersion() >= base::win::VERSION_WIN8) {
-        AddItemWithStringId(IDC_WIN8_METRO_RESTART, IDS_WIN8_METRO_RESTART);
-      } else {
-        AddItemWithStringId(IDC_WIN_CHROMEOS_RESTART, IDS_WIN_CHROMEOS_RESTART);
-      }
-    }
+  if (!browser_->profile()->IsGuestSession()) {
+    bookmark_sub_menu_model_.reset(new BookmarkSubMenuModel(this, browser_));
+    AddSubMenuWithStringId(IDC_BOOKMARKS_MENU, IDS_BOOKMARKS_MENU,
+                           bookmark_sub_menu_model_.get());
   }
-#endif
 
-  // Append the full menu including separators. The final separator only gets
-  // appended when this is a touch menu - otherwise it would get added twice.
-  CreateCutCopyPasteMenu();
-
-  if (base::CommandLine::ForCurrentProcess()->HasSwitch(
-          switches::kEnableDomDistiller)) {
-    AddItemWithStringId(IDC_DISTILL_PAGE, IDS_DISTILL_PAGE);
-  }
+  CreateZoomMenu();
 
+  AddItemWithStringId(IDC_PRINT, IDS_PRINT);
   AddItemWithStringId(IDC_SAVE_PAGE, IDS_SAVE_PAGE);
   AddItemWithStringId(IDC_FIND, IDS_FIND);
-  AddItemWithStringId(IDC_PRINT, IDS_PRINT);
-
+  if (base::CommandLine::ForCurrentProcess()->HasSwitch(
+          switches::kEnableDomDistiller))
+    AddItemWithStringId(IDC_DISTILL_PAGE, IDS_DISTILL_PAGE);
   tools_menu_model_.reset(new ToolsMenuModel(this, browser_));
-  CreateZoomMenu();
+  AddSubMenuWithStringId(
+      IDC_MORE_TOOLS_MENU, IDS_MORE_TOOLS_MENU, tools_menu_model_.get());
 
-  AddItemWithStringId(IDC_SHOW_HISTORY, IDS_SHOW_HISTORY);
-  AddItemWithStringId(IDC_SHOW_DOWNLOADS, IDS_SHOW_DOWNLOADS);
-  AddSeparator(ui::NORMAL_SEPARATOR);
+  // Append the full menu including separators. The final separator only gets
+  // appended when this is a touch menu - otherwise it would get added twice.
+  CreateCutCopyPasteMenu();
+
+  AddItemWithStringId(IDC_OPTIONS, IDS_SETTINGS);
 
 #if !defined(OS_CHROMEOS)
   if (!switches::IsNewAvatarMenu()) {
@@ -913,17 +909,22 @@ void WrenchMenuModel::Build() {
     SigninManager* signin = SigninManagerFactory::GetForProfile(
         browser_->profile()->GetOriginalProfile());
     if (signin && signin->IsSigninAllowed()) {
-      const base::string16 short_product_name =
-          l10n_util::GetStringUTF16(IDS_SHORT_PRODUCT_NAME);
-      AddItem(IDC_SHOW_SYNC_SETUP, l10n_util::GetStringFUTF16(
-          IDS_SYNC_MENU_PRE_SYNCED_LABEL, short_product_name));
-      AddSeparator(ui::NORMAL_SEPARATOR);
+      // Check for sign in errors.
+      std::vector<GlobalError*> signin_errors;

[Peter Kasting, 2015/04/01 21:06:28]

This temp is unused and can be removed.

[edwardjung, 2015/04/02 13:13:28]

Done.

+
+      if (signin_ui_util::GetSignedInServiceErrors(
+              browser_->profile()->GetOriginalProfile()).empty()) {
+        const base::string16 short_product_name =

[Peter Kasting, 2015/04/01 21:06:28]

This temp is unused and can be removed.

[edwardjung, 2015/04/02 13:13:28]

Done.

+            l10n_util::GetStringUTF16(IDS_SHORT_PRODUCT_NAME);
+        AddItem(IDC_SHOW_SYNC_SETUP,
+                l10n_util::GetStringFUTF16(
+                    IDS_SYNC_MENU_PRE_SYNCED_LABEL,
+                    l10n_util::GetStringUTF16(IDS_SHORT_PRODUCT_NAME)));
+      }
     }
   }
 #endif
 
-  AddItemWithStringId(IDC_OPTIONS, IDS_SETTINGS);
-
 // On ChromeOS we don't want the about menu option.
 #if !defined(OS_CHROMEOS)
   AddItem(IDC_ABOUT, l10n_util::GetStringUTF16(IDS_ABOUT));
@@ -942,20 +943,34 @@ void WrenchMenuModel::Build() {
                              IDS_TOGGLE_REQUEST_TABLET_SITE);
 #endif
 
-  if (browser_defaults::kShowUpgradeMenuItem)
-    AddItem(IDC_UPGRADE_DIALOG, GetUpgradeDialogMenuItemName());
+  AddSeparator(ui::NORMAL_SEPARATOR);

[Peter Kasting, 2015/04/01 21:06:28]

This line should be removed.

* If we try to add the "restart in metro" items in the big #if OS_WIN block just
below, they add their own separator, so this will result in a double separator.
* If instead we try to show the exit menu, that also adds its own separator, so
again, this will result in a double separator.
* If we do neither of these, this becomes a trailing separator, and will be
removed anyway by the RemoveTrailingSeparators() call.

[edwardjung, 2015/04/02 13:13:28]

Good point. Done.

 
 #if defined(OS_WIN)
-  SetIcon(GetIndexOfCommandId(IDC_VIEW_INCOMPATIBILITIES),
-          ui::ResourceBundle::GetSharedInstance().
-              GetNativeImageNamed(IDR_INPUT_ALERT_MENU));
+  base::win::Version min_version_for_ash_mode = base::win::VERSION_WIN8;
+  // Windows 7 ASH mode is only supported in DEBUG for now.
+#if !defined(NDEBUG)
+  min_version_for_ash_mode = base::win::VERSION_WIN7;
+#endif
+  // Windows 8 can support ASH mode using WARP, but Windows 7 requires a working
+  // GPU compositor.
+  if ((base::win::GetVersion() >= min_version_for_ash_mode &&
+      content::GpuDataManager::GetInstance()->CanUseGpuBrowserCompositor()) ||
+      (base::win::GetVersion() >= base::win::VERSION_WIN8)) {
+    if (browser_->host_desktop_type() == chrome::HOST_DESKTOP_TYPE_ASH) {
+      // ASH/Metro mode, add the 'Relaunch Chrome in desktop mode'.
+      AddSeparator(ui::NORMAL_SEPARATOR);
+      AddItemWithStringId(IDC_WIN_DESKTOP_RESTART, IDS_WIN_DESKTOP_RESTART);
+    } else {
+      // In Windows 8 desktop, add the 'Relaunch Chrome in Windows 8 mode'.
+      // In Windows 7 desktop, add the 'Relaunch Chrome in Windows ASH mode'
+      AddSeparator(ui::NORMAL_SEPARATOR);
+      if (base::win::GetVersion() >= base::win::VERSION_WIN8)
+        AddItemWithStringId(IDC_WIN8_METRO_RESTART, IDS_WIN8_METRO_RESTART);
+      else
+        AddItemWithStringId(IDC_WIN_CHROMEOS_RESTART, IDS_WIN_CHROMEOS_RESTART);
+    }
+  }
 #endif
-
-  AddGlobalErrorMenuItems();
-
-  AddSeparator(ui::NORMAL_SEPARATOR);
-  AddSubMenuWithStringId(
-      IDC_MORE_TOOLS_MENU, IDS_MORE_TOOLS_MENU, tools_menu_model_.get());
 
   bool show_exit_menu = browser_defaults::kShowExitMenuItem;
 #if defined(OS_WIN)
@@ -972,7 +987,7 @@ void WrenchMenuModel::Build() {
   uma_action_recorded_ = false;
 }
 
-void WrenchMenuModel::AddGlobalErrorMenuItems() {
+bool WrenchMenuModel::AddGlobalErrorMenuItems() {
   // TODO(sail): Currently we only build the wrench menu once per browser
   // window. This means that if a new error is added after the menu is built
   // it won't show in the existing wrench menu. To fix this we need to some
@@ -985,6 +1000,7 @@ void WrenchMenuModel::AddGlobalErrorMenuItems() {
       browser_->profile()->GetOriginalProfile());
   const GlobalErrorService::GlobalErrorList& errors =
       GlobalErrorServiceFactory::GetForProfile(browser_->profile())->errors();
+  bool menu_items_added = false;
   for (GlobalErrorService::GlobalErrorList::const_iterator
        it = errors.begin(); it != errors.end(); ++it) {
     GlobalError* error = *it;
@@ -1010,8 +1026,10 @@ void WrenchMenuModel::AddGlobalErrorMenuItems() {
         SetIcon(GetIndexOfCommandId(error->MenuItemCommandID()),
                 image);
       }
+      menu_items_added = true;
     }
   }
+  return menu_items_added;
 }
 
 void WrenchMenuModel::CreateExtensionToolbarOverflowMenu() {