Wrench menu reorg phase 2

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 230 matching lines @@
     Build(browser);
   }
 
  private:
   void Build(Browser* browser) {
 #if defined(OS_CHROMEOS) && defined(OFFICIAL_BUILD)
     int help_string_id = IDS_GET_HELP;
 #else
     int help_string_id = IDS_HELP_PAGE;
 #endif
+// Don't display the about menu item on Chrome OS.

[Peter Kasting, 2015/06/17 22:59:43]

Nit: Say why not.

[edwardjung, 2015/06/18 16:52:05]

Done. Although there doesn't seem to a logical reason not to have a direct link
to about. I've asked around our team if it should be reinstated which would
simplify the code.

+#if !defined(OS_CHROMEOS)
+    AddItem(IDC_ABOUT, l10n_util::GetStringUTF16(IDS_ABOUT));
+#endif
     AddItemWithStringId(IDC_HELP_PAGE_VIA_MENU, help_string_id);
     if (browser_defaults::kShowHelpMenuItemIcon) {
       ui::ResourceBundle& rb = ResourceBundle::GetSharedInstance();
       SetIcon(GetIndexOfCommandId(IDC_HELP_PAGE_VIA_MENU),
               rb.GetNativeImageNamed(IDR_HELP_MENU));
     }
 
     AddItemWithStringId(IDC_FEEDBACK, IDS_FEEDBACK);
   }
 
@@ skipping 15 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);
+  AddItemWithStringId(IDC_SAVE_PAGE, IDS_SAVE_PAGE);
 
   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);
   } else if (show_create_shortcuts) {
     AddItemWithStringId(IDC_CREATE_SHORTCUTS, IDS_CREATE_SHORTCUTS);
   }
+  AddSeparator(ui::NORMAL_SEPARATOR);
+  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 defined(OS_CHROMEOS)
   AddItemWithStringId(IDC_TAKE_SCREENSHOT, IDS_TAKE_SCREENSHOT);
 #endif
 
   encoding_menu_model_.reset(new EncodingMenuModel(browser));
   AddSubMenuWithStringId(IDC_ENCODING_MENU, IDS_ENCODING_MENU,
                          encoding_menu_model_.get());
   AddSeparator(ui::NORMAL_SEPARATOR);
+

[Peter Kasting, 2015/06/17 22:59:43]

Nit: I can't figure out the logic of when this function adds blank lines. 
Before separators?  After?  Everywhere?  Nowhere?  None of these seems to
accurately describe it.  Pick a system and stick with it.

[edwardjung, 2015/06/18 16:52:05]

Reformatted. Blank lines added for each section of the menu structure described
in the function comment. Also reformatted WrenchMenuModel::Build in the same
manner.

   AddItemWithStringId(IDC_DEV_TOOLS, IDS_DEV_TOOLS);
-  AddItemWithStringId(IDC_VIEW_SOURCE, IDS_VIEW_SOURCE);

[Peter Kasting, 2015/06/17 22:59:43]

Why are we removing View Source?  I don't think that makes sense.

[edwardjung, 2015/06/18 16:52:05]

There's a lot more detail reorganisation in the bug thread but an overview:

We're trying to encourage Dev Tools as the entry point for viewing the source,
console and devices.  For Linux and Mac users, the OS level menu still has the
view source item. On all platforms view source is available from right clicking
on a page and the shortcut key. Most developers would use dev tools or know the
right click / shortcut.

Also the total usage of view source in the wrench menu and it was very small
percentage in the overall view source usage and wrench menu clicks.

Current 7 day view source UMA stats for all browsers:
3.32% of total View Source views came from the wrench menu item (54,208)
View source clicks accounts for 0.073% of all wrench menu items.

On Windows: 
3.41% of total view source visits (45,785).
0.067% of total menu clicks.

[Peter Kasting, 2015/06/18 17:16:58]

I'm still concerned about this.

* 3.5% of total View Source visits still seems like a reasonably high amount.
* The Dev Tools don't make it easy for novices like me to figure out how to
"view source", which I do actually need to do on a surprising number of
occasions.  I worry about this being disproportionately difficult for
non-developers.  I think we still want View Source to be easily accessible for
such users.
* Basic CHI principles argue against making a context menu option the only or
primary way of exposing a feature.
* View Source has a great deal of nostalgic and PR value.  Many people view it
as the capability which democratized the web.  Removing it from these menus
could hurt us on these fronts.

I'm normally a big fan of simplifying and streamlining menus, but given that
this is already buried in a submenu, I'm gravely concerned that the potential
costs of removing this particular item outweigh the benefits.  I'd like to not
do this change.  In the limit, I'd like to make it as a completely separate,
experiment-driven change, and collect sufficient stats to see whether users in
the experimental group figure out how to substitute another way of getting to
this, or whether overall usage decreases.  If usage decreases, that seems like
an argument against the change.

[edwardjung, 2015/06/18 17:42:27]

Copying comments here so that ainslie can see the thread.

I agree that it's not easy to navigate dev tools to view the actual source
versus the rendered DOM, I don't know if there could be improvements there. The
UI of dev tools is going through some change down the line but don't know if the
sources panel will get a make over.

I also do still use view source a lot as a lighter and quicker dev tools to for
quick checks, so also agree that for novices who don't know the shortcut, on
Windows / CrOS at least, we can't educate them once the menu item is removed.
 

-  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
 }
 
 ////////////////////////////////////////////////////////////////////////////////
 // WrenchMenuModel
 
@@ skipping 566 matching lines @@
       IsCommandIdVisible(IDC_UPGRADE_DIALOG))
     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);
 
   AddSeparator(ui::NORMAL_SEPARATOR);
 
-  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,
+    AddSubMenuWithStringId(IDC_RECENT_TABS_MENU, IDS_HISTORY_RECENT_TABS_MENU,
                            recent_tabs_sub_menu_model_.get());
   }
+  AddItemWithStringId(IDC_SHOW_DOWNLOADS, IDS_SHOW_DOWNLOADS);
 
   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());
   }
 
   CreateZoomMenu();
 
   AddItemWithStringId(IDC_PRINT, IDS_PRINT);
-  AddItemWithStringId(IDC_SAVE_PAGE, IDS_SAVE_PAGE);
+  // AddItemWithStringId(IDC_SAVE_PAGE, IDS_SAVE_PAGE);

[Peter Kasting, 2015/06/17 22:59:43]

In or out, but not in-but-commented-out.

[edwardjung, 2015/06/18 16:52:05]

Done.

   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.
@@ skipping 10 matching lines @@
         signin_ui_util::GetSignedInServiceErrors(
             browser_->profile()->GetOriginalProfile()).empty()) {
       AddItem(IDC_SHOW_SYNC_SETUP,
               l10n_util::GetStringFUTF16(
                   IDS_SYNC_MENU_PRE_SYNCED_LABEL,
                   l10n_util::GetStringUTF16(IDS_SHORT_PRODUCT_NAME)));
     }
   }
 #endif
 
-// 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());
+#elif !defined(OS_CHROMEOS)
+  // Display the about menu item for non Chrome OS and non Google builds.

[Peter Kasting, 2015/06/17 22:59:43]

Why do we have two blocks in this file both dedicated to adding this ABOUT item?

[edwardjung, 2015/06/18 16:52:05]

We moved the 'About' menu item under the help submenu (block above). However on
Chromium builds the help submenu is not displayed so reinstate the menu item at
the top level. 

I've added an extra comment to describe this.

+  AddItem(IDC_ABOUT, l10n_util::GetStringUTF16(IDS_ABOUT));
 #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 defined(OS_WIN)
@@ skipping 131 matching lines @@
                        ->GetZoomPercent();
   }
   zoom_label_ = l10n_util::GetStringFUTF16(
       IDS_ZOOM_PERCENT, base::IntToString16(zoom_percent));
 }
 
 void WrenchMenuModel::OnZoomLevelChanged(
     const content::HostZoomMap::ZoomLevelChange& change) {
   UpdateZoomControls();
 }