Restructure the wrench menu into an action based order

chrome/browser/ui/toolbar/wrench_menu_model.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/ui/toolbar/wrench_menu_model.h"
 
 #include <algorithm>
 #include <cmath>
 
 #include "base/command_line.h"
@@ skipping 221 matching lines @@
 
 void ToolsMenuModel::Build(Browser* browser) {
   bool show_create_shortcuts = true;
 #if defined(OS_CHROMEOS) || defined(OS_MACOSX)
   show_create_shortcuts = false;
 #elif defined(USE_ASH)
   if (browser->host_desktop_type() == chrome::HOST_DESKTOP_TYPE_ASH)
     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;
 #elif defined(OS_WIN)
     int string_id = IDS_ADD_TO_TASKBAR;
 #else
     int string_id = IDS_ADD_TO_DESKTOP;
 #endif
 #if defined(USE_ASH)
     if (browser->host_desktop_type() == chrome::HOST_DESKTOP_TYPE_ASH)
       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());
+  AddSeparator(ui::NORMAL_SEPARATOR);
+  AddItemWithStringId(IDC_DEV_TOOLS, IDS_DEV_TOOLS);
   AddItemWithStringId(IDC_VIEW_SOURCE, IDS_VIEW_SOURCE);
-  AddItemWithStringId(IDC_DEV_TOOLS, IDS_DEV_TOOLS);
   AddItemWithStringId(IDC_DEV_TOOLS_CONSOLE, IDS_DEV_TOOLS_CONSOLE);
   AddItemWithStringId(IDC_DEV_TOOLS_DEVICES, IDS_DEV_TOOLS_DEVICES);
 
 #if defined(ENABLE_PROFILING) && !defined(NO_TCMALLOC)
   AddSeparator(ui::NORMAL_SEPARATOR);
   AddCheckItemWithStringId(IDC_PROFILING_ENABLED, IDS_PROFILING_ENABLED);
 #endif
 }
 
 ////////////////////////////////////////////////////////////////////////////////
@@ skipping 529 matching lines @@
 }
 
 bool WrenchMenuModel::ShouldShowNewIncognitoWindowMenuItem() {
   if (browser_->profile()->IsGuestSession())
     return false;
 
   return IncognitoModePrefs::GetAvailability(browser_->profile()->GetPrefs()) !=
       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_,
                                                                  browser_,
                                                                  NULL));
     AddSubMenuWithStringId(IDC_RECENT_TABS_MENU, IDS_RECENT_TABS_MENU,
                            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
+
+  CreateZoomMenu();
+
+  AddItemWithStringId(IDC_PRINT, IDS_PRINT);
+  AddItemWithStringId(IDC_SAVE_PAGE, IDS_SAVE_PAGE);
+  AddItemWithStringId(IDC_FIND, IDS_FIND);
+  if (base::CommandLine::ForCurrentProcess()->HasSwitch(
+          switches::kEnableDomDistiller))
+    AddItemWithStringId(IDC_DISTILL_PAGE, IDS_DISTILL_PAGE);
+  tools_menu_model_.reset(new ToolsMenuModel(this, browser_));
+  AddSubMenuWithStringId(
+      IDC_MORE_TOOLS_MENU, IDS_MORE_TOOLS_MENU, tools_menu_model_.get());
 
   // 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);
-  }
-
-  AddItemWithStringId(IDC_SAVE_PAGE, IDS_SAVE_PAGE);
-  AddItemWithStringId(IDC_FIND, IDS_FIND);
-  AddItemWithStringId(IDC_PRINT, IDS_PRINT);
-
-  tools_menu_model_.reset(new ToolsMenuModel(this, browser_));
-  CreateZoomMenu();
-
-  AddItemWithStringId(IDC_SHOW_HISTORY, IDS_SHOW_HISTORY);
-  AddItemWithStringId(IDC_SHOW_DOWNLOADS, IDS_SHOW_DOWNLOADS);
-  AddSeparator(ui::NORMAL_SEPARATOR);
+  AddItemWithStringId(IDC_OPTIONS, IDS_SETTINGS);
 
 #if !defined(OS_CHROMEOS)
   if (!switches::IsNewAvatarMenu()) {
     // No "Sign in to Chromium..." menu item on ChromeOS.
     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));
 #endif
 
 #if defined(GOOGLE_CHROME_BUILD)
   help_menu_model_.reset(new HelpMenuModel(this, browser_));
   AddSubMenuWithStringId(IDC_HELP_MENU, IDS_HELP_MENU,
                          help_menu_model_.get());
 #endif
 
 #if defined(OS_CHROMEOS)
   if (base::CommandLine::ForCurrentProcess()->HasSwitch(
           chromeos::switches::kEnableRequestTabletSite))
     AddCheckItemWithStringId(IDC_TOGGLE_REQUEST_TABLET_SITE,
                              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
-
-  AddGlobalErrorMenuItems();
-
-  AddSeparator(ui::NORMAL_SEPARATOR);
-  AddSubMenuWithStringId(
-      IDC_MORE_TOOLS_MENU, IDS_MORE_TOOLS_MENU, tools_menu_model_.get());
+  // 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
 
   bool show_exit_menu = browser_defaults::kShowExitMenuItem;
 #if defined(OS_WIN)
   if (browser_->host_desktop_type() == chrome::HOST_DESKTOP_TYPE_ASH)
     show_exit_menu = false;
 #endif
 
   if (show_exit_menu) {
     AddSeparator(ui::NORMAL_SEPARATOR);
     AddItemWithStringId(IDC_EXIT, IDS_EXIT);
   }
 
   RemoveTrailingSeparators();

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

I think this call can be removed (and, since this is the only call to this
function AFAIK, the whole function can be removed).  As long as you remove the
separator I comment about above, I believe all codepaths result in a
non-separator item being last in the menu.

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

Removed, but didn't remove the function, a search in the codebase shows that
it's used by:
chrome/browser/ui/ash/launcher/launcher_application_menu_item_model.cc

https://cs.corp.google.com/#clankium/src/chrome/browser/ui/ash/launcher/launc...

[Peter Kasting, 2015/04/02 20:25:37]

Can you remove that instance too so we can kill this?  You're welcome to do it
as a followon CL if you'd prefer.  I believe changing that function as follows
should work:

void LauncherApplicationMenuItemModel::Build() {
  if (launcher_items_.empty())
    return;

  AddSeparator(ui::SPACING_SEPARATOR);
  for (...) {
    ...
  }

  AddSeparator(ui::SPACING_SEPARATOR);
}

In other words, add an initial early-return, remove the
RemoveTrailingSeparators() call, and make the final AddSeparator() call
unconditional.

   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
   // how update the menu if new errors are added.
   ui::ResourceBundle& rb = ui::ResourceBundle::GetSharedInstance();
   // GetSignedInServiceErrors() can modify the global error list, so call it
   // before iterating through that list below.
   std::vector<GlobalError*> signin_errors;
   signin_errors = signin_ui_util::GetSignedInServiceErrors(
       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;
     DCHECK(error);
     if (error->HasMenuItem()) {
 #if !defined(OS_CHROMEOS)
       // Don't add a signin error if it's already being displayed elsewhere.
       if (std::find(signin_errors.begin(), signin_errors.end(), error) !=
           signin_errors.end()) {
         MenuModel* model = this;
         int index = 0;
         if (MenuModel::GetModelAndIndexForCommandId(
                 IDC_SHOW_SIGNIN, &model, &index)) {
           continue;
         }
       }
 #endif
 
       AddItem(error->MenuItemCommandID(), error->MenuItemLabel());
       int icon_id = error->MenuItemIconResourceID();
       if (icon_id) {
         const gfx::Image& image = rb.GetNativeImageNamed(icon_id);
         SetIcon(GetIndexOfCommandId(error->MenuItemCommandID()),
                 image);
       }
+      menu_items_added = true;
     }
   }
+  return menu_items_added;
 }
 
 void WrenchMenuModel::CreateExtensionToolbarOverflowMenu() {
   // We only add the extensions overflow container if there are any icons that
   // aren't shown in the main container.
   if (!extensions::ExtensionToolbarModel::Get(browser_->profile())->
           all_icons_visible()) {
     AddItem(IDC_EXTENSIONS_OVERFLOW_MENU, base::string16());
     AddSeparator(ui::UPPER_SEPARATOR);
   }
@@ skipping 42 matching lines @@
                        ->GetZoomPercent();
   }
   zoom_label_ = l10n_util::GetStringFUTF16(
       IDS_ZOOM_PERCENT, base::IntToString16(zoom_percent));
 }
 
 void WrenchMenuModel::OnZoomLevelChanged(
     const content::HostZoomMap::ZoomLevelChange& change) {
   UpdateZoomControls();
 }