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 530 matching lines @@
 
 bool WrenchMenuModel::ShouldShowNewIncognitoWindowMenuItem() {
   if (browser_->profile()->IsGuestSession())
     return false;
 
   return IncognitoModePrefs::GetAvailability(browser_->profile()->GetPrefs()) !=
       IncognitoModePrefs::DISABLED;
 }
 
 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));
   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/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_,
                                                                  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
+
+  // 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);
+  }
+  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);
+  // 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()) {
     // 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;
+      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));
 #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);
 
+  // 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
-
-  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) {

[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
 
   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();
   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();
 }