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..fa1ea5385a4a91588c4b0dcfcb3ba31d839cc531 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);
 
@@ -826,6 +822,12 @@ bool WrenchMenuModel::ShouldShowNewIncognitoWindowMenuItem() {
 }
 
 void WrenchMenuModel::Build() {
+  bool add_separator = false;
+
+  if (extensions::FeatureSwitch::extension_action_redesign()->IsEnabled())
+    CreateExtensionToolbarOverflowMenu();
+
+  // Errors and warnings.
 #if defined(OS_WIN)
   AddItem(IDC_VIEW_INCOMPATIBILITIES,
       l10n_util::GetStringUTF16(IDS_VIEW_INCOMPATIBILITIES));
@@ -833,23 +835,36 @@ void WrenchMenuModel::Build() {
       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/03/30 19:37:02]

This seems strange.  Where is the UI for these enumerated modules going to live?
 If it's going to be the VIEW_INCOMPATIBILITIES icon, why is that controlling
whether we'll add a separator two items lower down?

[edwardjung, 2015/03/31 11:44:04]

I altered this from the previous code, which was using the enumerated models to
determine whether there are any module errors to add a separator.  I don't
believe the enumerated modules are used in the UI at all. 

When AddItem(IDC_VIEW_INCOMPATIBILITIES…) is called,
WrenchMenuModel::IsCommandIdVisible() is a called to do a check for the
incompatible modules and that determines if a menuitem is displayed. 

current code:
if (model->modules_to_notify_about() > 0 ||
    model->confirmed_bad_modules_detected() > 0)
  AddSeparator(ui::NORMAL_SEPARATOR);

So this code does the same check for module errors.

What I want to achieve is a single separator after all the errors are displayed,
rather than a separator after each error/warning which is how it is currently.
So the add_separator flag aggregates all the current separators to one.

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

[Peter Kasting, 2015/03/30 19:37:02]

Nit: No {}

[edwardjung, 2015/03/31 11:44:05]

Done.

+    AddItem(IDC_UPGRADE_DIALOG, GetUpgradeDialogMenuItemName());
+  }
 
+  if (AddGlobalErrorMenuItems())
+    add_separator = true;
+
+#if defined(OS_WIN)
+  SetIcon(GetIndexOfCommandId(IDC_VIEW_INCOMPATIBILITIES),

[Peter Kasting, 2015/03/30 19:37:02]

Why is this code here instead of being up with the other
IDC_VIEW_INCOMPATIBILITIES code?

[edwardjung, 2015/03/31 11:44:05]

Good point, didn't think since it was in a separate block before. Moved up. 

+          ui::ResourceBundle::GetSharedInstance().
+              GetNativeImageNamed(IDR_INPUT_ALERT_MENU));
+#endif
+
+  if (add_separator)
+    AddSeparator(ui::NORMAL_SEPARATOR);
+
+  // Tabs and windows.

[Peter Kasting, 2015/03/30 19:37:02]

If it really adds clarity to comment each section of the menu like this, then I
would consider breaking this long function into a series of smaller helpers with
descriptive names, which will accomplish the same thing more readably.

Otherwise I would probably omit these comments.

[edwardjung, 2015/03/31 11:44:05]

Point taken. I've added a comment at the top of the function.

   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);
+  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());
-  }
+  // Places previously been - History, bookmarks, recent tabs.

[Peter Kasting, 2015/03/30 19:37:02]

This description doesn't seem to apply to SHOW_DOWNLOADS?  It's also weird that
the comment lists items in a different order than the code.

[edwardjung, 2015/03/31 11:44:04]

Removed. 

+  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 +874,32 @@ 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();
+  // Page actions.
+  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)) {

[Peter Kasting, 2015/03/30 19:37:02]

Nit: No {}

[edwardjung, 2015/03/31 11:44:05]

Done.

     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();
+  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();
+
+  // Customisation and learn.

[Peter Kasting, 2015/03/30 19:37:02]

"Customization and learning"?  Though I don't know what "learning" (or "learn")
actually means here.

[edwardjung, 2015/03/31 11:44:05]

Removed. 

+  AddItemWithStringId(IDC_OPTIONS, IDS_SETTINGS);
 
 #if !defined(OS_CHROMEOS)
   if (!switches::IsNewAvatarMenu()) {
@@ -913,17 +907,21 @@ 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;
+      signin_errors = signin_ui_util::GetSignedInServiceErrors(
+           browser_->profile()->GetOriginalProfile());

[Peter Kasting, 2015/03/30 19:37:02]

Nit: Since you don't actually care about the errors, just whether there are
some, I'd inline this call below and omit the temp.

[edwardjung, 2015/03/31 11:44:05]

Done.

+
+      if (signin_errors.size() == 0) {

[Peter Kasting, 2015/03/30 19:37:02]

Nit: Use .empty()

[edwardjung, 2015/03/31 11:44:05]

Done.

+        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));

[Peter Kasting, 2015/03/30 19:37:02]

Nit: Existing wrapping isn't actually style-compliant.  How about this:

        AddItem(IDC_SHOW_SYNC_SETUP,
                l10n_util::GetStringFUTF16(
                    IDS_SYNC_MENU_PRE_SYNCED_LABEL,
                    l10n_util::GetStringUTF16(IDS_SHORT_PRODUCT_NAME)));

[edwardjung, 2015/03/31 11:44:05]

Done.

+      }
     }
   }
 #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 +940,36 @@ void WrenchMenuModel::Build() {
                              IDS_TOGGLE_REQUEST_TABLET_SITE);
 #endif
 
-  if (browser_defaults::kShowUpgradeMenuItem)
-    AddItem(IDC_UPGRADE_DIALOG, GetUpgradeDialogMenuItemName());
+  AddSeparator(ui::NORMAL_SEPARATOR);
 
+  // Exit / relaunch.
 #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) {

[Peter Kasting, 2015/03/30 19:37:02]

Nit: No {}

[edwardjung, 2015/03/31 11:44:04]

Done.

+        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 +986,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 +999,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 +1025,10 @@ void WrenchMenuModel::AddGlobalErrorMenuItems() {
         SetIcon(GetIndexOfCommandId(error->MenuItemCommandID()),
                 image);
       }
+      menu_items_added = true;
     }
   }
+  return menu_items_added;
 }
 
 void WrenchMenuModel::CreateExtensionToolbarOverflowMenu() {