Extend contextMenus API to support browser/page actions

chrome/browser/renderer_context_menu/render_view_context_menu.cc

 // Copyright 2014 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/renderer_context_menu/render_view_context_menu.h"
 
 #include <algorithm>
 #include <set>
 #include <utility>
 
 #include "apps/app_load_service.h"
 #include "base/command_line.h"
 #include "base/logging.h"
 #include "base/metrics/histogram.h"
 #include "base/prefs/pref_member.h"
 #include "base/prefs/pref_service.h"
 #include "base/stl_util.h"
 #include "base/strings/string_util.h"
 #include "base/strings/stringprintf.h"
 #include "base/strings/utf_string_conversions.h"
-#include "base/time/time.h"
 #include "chrome/app/chrome_command_ids.h"
 #include "chrome/browser/app_mode/app_mode_utils.h"
 #include "chrome/browser/autocomplete/autocomplete_classifier.h"
 #include "chrome/browser/autocomplete/autocomplete_classifier_factory.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/chrome_notification_types.h"
 #include "chrome/browser/custom_handlers/protocol_handler_registry_factory.h"
 #include "chrome/browser/devtools/devtools_window.h"
 #include "chrome/browser/download/download_service.h"
 #include "chrome/browser/download/download_service_factory.h"
@@ skipping 524 matching lines @@
     return;
 
   base::string16 printable_selection_text = PrintableSelectionText();
   EscapeAmpersands(&printable_selection_text);
 
   // Get a list of extension id's that have context menu items, and sort by the
   // top level context menu title of the extension.
   std::set<MenuItem::ExtensionKey> ids = menu_manager->ExtensionIds();
   std::vector<base::string16> sorted_menu_titles;
   std::map<base::string16, std::string> map_ids;
-  for (std::set<MenuItem::ExtensionKey>::iterator i = ids.begin();
-       i != ids.end();
-       ++i) {
+  for (std::set<MenuItem::ExtensionKey>::iterator iter = ids.begin();
+       iter != ids.end();
+       ++iter) {
     const Extension* extension =
-        service->GetExtensionById(i->extension_id, false);
+        service->GetExtensionById(iter->extension_id, false);
     // Platform apps have their context menus created directly in
     // AppendPlatformAppItems.
     if (extension && !extension->is_platform_app()) {
       base::string16 menu_title = extension_items_.GetTopLevelContextMenuTitle(
-          *i, printable_selection_text);
-      map_ids[menu_title] = i->extension_id;
+          *iter, printable_selection_text);
+      map_ids[menu_title] = iter->extension_id;
       sorted_menu_titles.push_back(menu_title);
     }
   }
   if (sorted_menu_titles.empty())
     return;
 
   const std::string app_locale = g_browser_process->GetApplicationLocale();
   l10n_util::SortStrings16(app_locale, &sorted_menu_titles);
 
   int index = 0;
-  base::TimeTicks begin = base::TimeTicks::Now();
-  for (size_t i = 0; i < sorted_menu_titles.size(); ++i) {
-    const std::string& id = map_ids[sorted_menu_titles[i]];
-    const MenuItem::ExtensionKey extension_key(id);
-    extension_items_.AppendExtensionItems(
-        extension_key, printable_selection_text, &index);
+  for (std::vector<base::string16>::iterator iter = sorted_menu_titles.begin();

[brettw, 2014/08/04 20:17:47]

It's not clear to me why you rewrote this loop. To me, the new code seems
strictly worse than the old code, and even if it wasn't, this isn't the type of
thing that's worth doing.

[Devlin, 2014/08/04 20:23:02]

Greg probably did this based off my comments on similar code in extensions/ (I
prefer this way because it's marginally faster to loop through the vector with
an iter than to dereference all the different indices).  But we don't need to
change old code to match, and OWNERS preference takes priority.

[gpdavis, 2014/08/04 20:52:32]

Yep, that was it.  Since Devlin suggested these changes to code I borrowed from
here, I thought I'd change it here as well (made a note of it in an earlier
patch set).

I'll go ahead and put this back to the way it was.

+       iter != sorted_menu_titles.end();
+       ++iter) {
+    const std::string& id = map_ids[*iter];
+    extension_items_.AppendExtensionItems(MenuItem::ExtensionKey(id),
+                                          printable_selection_text,
+                                          &index,
+                                          false);  // is_action_menu
   }
-
-  UMA_HISTOGRAM_TIMES("Extensions.ContextMenus_BuildTime",
-                      base::TimeTicks::Now() - begin);
-  UMA_HISTOGRAM_COUNTS("Extensions.ContextMenus_ItemCount", index);
 }
 
 void RenderViewContextMenu::AppendCurrentExtensionItems() {
   // Avoid appending extension related items when |extension| is null.
   // For Panel, this happens when the panel is navigated to a url outside of the
   // extension's package.
   const Extension* extension = GetExtension();
   if (extension) {
     // Only add extension items from this extension.
     int index = 0;
     const MenuItem::ExtensionKey key(
         extension->id(), WebViewGuest::GetViewInstanceId(source_web_contents_));
-    extension_items_.AppendExtensionItems(
-        key, PrintableSelectionText(), &index);
+    extension_items_.AppendExtensionItems(key,
+                                          PrintableSelectionText(),
+                                          &index,
+                                          false);  // is_action_menu
   }
 }
 
 void RenderViewContextMenu::InitMenu() {
   if (content_type_->SupportsGroup(ContextMenuContentType::ITEM_GROUP_CUSTOM)) {
     AppendCustomItems();
 
     const bool has_selection = !params_.selection_text.empty();
     if (has_selection) {
       // We will add more items if there's a selection, so add a separator.
@@ skipping 1413 matching lines @@
   source_web_contents_->GetRenderViewHost()->
       ExecuteMediaPlayerActionAtLocation(location, action);
 }
 
 void RenderViewContextMenu::PluginActionAt(
     const gfx::Point& location,
     const WebPluginAction& action) {
   source_web_contents_->GetRenderViewHost()->
       ExecutePluginActionAtLocation(location, action);
 }