Extend contextMenus API to support browser/page actions

chrome/browser/extensions/extension_context_menu_model.cc

Index: chrome/browser/extensions/extension_context_menu_model.cc
diff --git a/chrome/browser/extensions/extension_context_menu_model.cc b/chrome/browser/extensions/extension_context_menu_model.cc
index e2774b619dbb4f86c402c70754e4ea68cbfc8ff5..5ec9ece08770b857637d4045f65d7b30d28c5fae 100644
--- a/chrome/browser/extensions/extension_context_menu_model.cc
+++ b/chrome/browser/extensions/extension_context_menu_model.cc
@@ -4,8 +4,11 @@
 
 #include "chrome/browser/extensions/extension_context_menu_model.h"
 
+#include "base/metrics/histogram.h"
 #include "base/prefs/pref_service.h"
 #include "base/strings/utf_string_conversions.h"
+#include "chrome/app/chrome_command_ids.h"
+#include "chrome/browser/browser_process.h"
 #include "chrome/browser/extensions/api/extension_action/extension_action_api.h"
 #include "chrome/browser/extensions/extension_action.h"
 #include "chrome/browser/extensions/extension_action_manager.h"
@@ -20,6 +23,7 @@
 #include "chrome/common/pref_names.h"
 #include "chrome/common/url_constants.h"
 #include "content/public/browser/web_contents.h"
+#include "content/public/common/context_menu_params.h"
 #include "extensions/browser/extension_prefs.h"
 #include "extensions/browser/extension_system.h"
 #include "extensions/browser/management_policy.h"
@@ -27,11 +31,14 @@
 #include "grit/chromium_strings.h"
 #include "grit/generated_resources.h"
 #include "ui/base/l10n/l10n_util.h"
+#include "ui/gfx/text_elider.h"

[Yoyo Zhou, 2014/06/27 23:37:57]

Is this used?

[gpdavis, 2014/06/28 00:05:41]

Ah, no, it isn't.  I had it included because of a TruncateString call that ended
up not being necessary.  I'll take care of that.

[gpdavis, 2014/06/30 21:11:11]

Done.

 
 using content::OpenURLParams;
 using content::Referrer;
 using content::WebContents;
 using extensions::Extension;
+using extensions::MenuItem;
+using extensions::MenuManager;
 
 ExtensionContextMenuModel::ExtensionContextMenuModel(const Extension* extension,
                                                      Browser* browser,
@@ -40,7 +47,12 @@ ExtensionContextMenuModel::ExtensionContextMenuModel(const Extension* extension,
       extension_id_(extension->id()),
       browser_(browser),
       profile_(browser->profile()),
-      delegate_(delegate) {
+      delegate_(delegate),
+      action_type_(GetActionType(extension)),
+      extension_items_(profile_,
+                       this,
+                       this,
+                       base::Bind(MenuItemMatchesContext, action_type_)) {
   InitMenu(extension);
 
   if (profile_->GetPrefs()->GetBoolean(prefs::kExtensionsUIDeveloperMode) &&
@@ -56,11 +68,19 @@ ExtensionContextMenuModel::ExtensionContextMenuModel(const Extension* extension,
       extension_id_(extension->id()),
       browser_(browser),
       profile_(browser->profile()),
-      delegate_(NULL) {
+      delegate_(NULL),
+      action_type_(GetActionType(extension)),
+      extension_items_(profile_,
+                       this,
+                       this,
+                       base::Bind(MenuItemMatchesContext, action_type_)) {
   InitMenu(extension);
 }
 
 bool ExtensionContextMenuModel::IsCommandIdChecked(int command_id) const {
+  if (command_id >= IDC_EXTENSIONS_CONTEXT_CUSTOM_FIRST &&
+      command_id <= IDC_EXTENSIONS_CONTEXT_CUSTOM_LAST)
+    return extension_items_.IsCommandIdChecked(command_id);
   return false;
 }
 
@@ -103,6 +123,15 @@ void ExtensionContextMenuModel::ExecuteCommand(int command_id,
   if (!extension)
     return;
 
+  if (command_id >= IDC_EXTENSIONS_CONTEXT_CUSTOM_FIRST &&
+      command_id <= IDC_EXTENSIONS_CONTEXT_CUSTOM_LAST) {
+    WebContents* web_contents =
+        browser_->tab_strip_model()->GetActiveWebContents();
+    content::ContextMenuParams *params = new content::ContextMenuParams();
+    extension_items_.ExecuteCommand(command_id, web_contents, *params);
+    return;
+  }
+
   switch (command_id) {
     case NAME: {
       OpenURLParams params(extensions::ManifestURL::GetHomepageURL(extension),
@@ -161,9 +190,6 @@ void ExtensionContextMenuModel::InitMenu(const Extension* extension) {
 
   extensions::ExtensionActionManager* extension_action_manager =
       extensions::ExtensionActionManager::Get(profile_);
-  extension_action_ = extension_action_manager->GetBrowserAction(*extension);
-  if (!extension_action_)
-    extension_action_ = extension_action_manager->GetPageAction(*extension);
 
   std::string extension_name = extension->name();
   // Ampersands need to be escaped to avoid being treated like
@@ -171,6 +197,8 @@ void ExtensionContextMenuModel::InitMenu(const Extension* extension) {
   base::ReplaceChars(extension_name, "&", "&&", &extension_name);
   AddItem(NAME, base::UTF8ToUTF16(extension_name));
   AddSeparator(ui::NORMAL_SEPARATOR);
+  AppendExtensionItems();
+  AddSeparator(ui::NORMAL_SEPARATOR);

[Yoyo Zhou, 2014/06/27 23:37:57]

We don't want to do this if there are no extension-provided items.

[gpdavis, 2014/06/28 00:05:41]

I'll go ahead and put the AddSeparator call into AppendExtensionItems.

[gpdavis, 2014/06/30 21:11:11]

Done.

   AddItemWithStringId(CONFIGURE, IDS_EXTENSIONS_OPTIONS_MENU_ITEM);
   AddItem(UNINSTALL, l10n_util::GetStringUTF16(IDS_EXTENSIONS_UNINSTALL));
   if (extension_action_manager->GetBrowserAction(*extension))
@@ -184,3 +212,82 @@ const Extension* ExtensionContextMenuModel::GetExtension() const {
       extensions::ExtensionSystem::Get(profile_)->extension_service();
   return extension_service->GetExtensionById(extension_id_, false);
 }
+
+void ExtensionContextMenuModel::AppendExtensionItems() {
+  extension_items_.Clear();
+  ExtensionService* service =
+      extensions::ExtensionSystem::Get(profile_)->extension_service();
+  if (!service)
+    return;  // In unit-tests, we may not have an ExtensionService.
+
+  MenuManager* menu_manager = MenuManager::Get(profile_);
+  if (!menu_manager)
+    return;
+
+  base::string16 *null_string = new base::string16();

[Yoyo Zhou, 2014/06/27 23:37:57]

This is a leak. Use EmptyString16 from string_util.

[gpdavis, 2014/06/28 00:05:41]

Oh, wonderful.  I was wondering what I should use as a null string.  Thanks.

[gpdavis, 2014/06/30 21:11:11]

Done.

+
+  // Get a list of extension id's that have context menu items, and sort by the

[Yoyo Zhou, 2014/06/27 23:37:57]

Wait a minute. Here's my understanding of the world:
ExtensionContextMenuModel is created when you right-click on an extension
surface (e.g. a page action).

Does it make sense for an extension to add context menu items for other
extension's items?

Arguably, I would say this should only add items from our own extension_. But if
you have a compelling reason otherwise, please say so.

[gpdavis, 2014/06/28 00:05:41]

I wasn't sure which way to do it, and since the change is minimal, I just chose
to do it this way.  I imagine situations in which an extension might want to
interact with another extension, so its context menu item would then show up on
all extensions' context menus instead of just its own.  But if we don't need to
accommodate for anything like that, then no problem.

I can make this only add items from the extension that was clicked on.  Should
be a simple modification to the filter function.

[Yoyo Zhou, 2014/06/28 00:29:44]

Yes, please. I don't think it makes sense for an extension to act on an
arbitrary other extension that you choose by its browser/page action.

[gpdavis, 2014/06/30 21:11:11]

Done.

+  // 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) {
+    const Extension* extension =
+        service->GetExtensionById(i->extension_id, false);
+    // Platform apps have their context menus created directly in
+    // AppendPlatformAppItems.
+    if (extension && !extension->is_platform_app()) {

[Yoyo Zhou, 2014/06/27 23:37:57]

I don't think platform apps can have extension UI surfaces that
ExtensionContextMenuModel gets created on.

[gpdavis, 2014/06/30 21:11:11]

Done.

+      base::string16 menu_title = extension_items_.GetTopLevelContextMenuTitle(
+          *i, *null_string);
+      map_ids[menu_title] = i->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, *null_string, &index);
+  }
+
+  UMA_HISTOGRAM_TIMES("Extensions.ContextMenus_BuildTime",

[Yoyo Zhou, 2014/06/27 23:37:57]

You can look at the comments in base/metrics/histogram.h to understand the
histograms here. We probably want to use new metric names, though, since these
are different. How about

Extensions.ActionContextMenus_BuildTime
Extensions.ActionContextMenus_ItemCount
?

[gpdavis, 2014/06/28 00:05:41]

Sounds good to me.  I'll take a look at that file and add these new metrics in.

[gpdavis, 2014/06/30 21:11:11]

Done.

+                      base::TimeTicks::Now() - begin);
+  UMA_HISTOGRAM_COUNTS("Extensions.ContextMenus_ItemCount", index);
+}
+
+ExtensionContextMenuModel::ActionType ExtensionContextMenuModel::GetActionType(

[Yoyo Zhou, 2014/06/27 23:37:57]

This is not a good style. A name like "GetActionType" suggests that it's a const
function, whereas this has the side effect of setting extension_action_. It'd be
better for this to be folded into InitMenu. There's no particular need to
initialize action_type_ and extension_items_ in the initializer list.

[gpdavis, 2014/06/28 00:27:02]

Oops-- I meant to address this comment but I must not have saved the draft.

I added this function because action_type_ needs to be initialized before
extension_items_ because it's necessary for the fourth parameter, the filter
function (MenuItemMatchesContext).  But if I have it set up in InitMenu, then I
can construct extension_items_ afterwards and this function will no longer be
necessary.

[gpdavis, 2014/06/30 21:11:11]

Done.

+    const Extension* extension) {
+  extensions::ExtensionActionManager* extension_action_manager =
+      extensions::ExtensionActionManager::Get(profile_);
+  extension_action_ = extension_action_manager->GetBrowserAction(*extension);
+  if (!extension_action_) {
+    extension_action_ = extension_action_manager->GetPageAction(*extension);
+    return PAGE_ACTION;
+  }
+  return BROWSER_ACTION;
+}
+
+bool ExtensionContextMenuModel::MenuItemMatchesContext(
+    ActionType type,
+    const extensions::MenuItem* item) {
+  MenuItem::ContextList contexts = item->contexts();
+
+  if (contexts.Contains(MenuItem::ALL))
+    return true;
+  if (contexts.Contains(MenuItem::PAGE_ACTION) && (type == PAGE_ACTION))
+    return true;
+  if (contexts.Contains(MenuItem::BROWSER_ACTION) && (type == BROWSER_ACTION))
+    return true;
+
+  return false;
+}