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..1c8510461d3b798e41a97d8478a71ad0f52bf83e 100644
--- a/chrome/browser/extensions/extension_context_menu_model.cc
+++ b/chrome/browser/extensions/extension_context_menu_model.cc
@@ -4,8 +4,12 @@
 
 #include "chrome/browser/extensions/extension_context_menu_model.h"
 
+#include "base/metrics/histogram.h"
 #include "base/prefs/pref_service.h"
+#include "base/strings/string_util.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 +24,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"
@@ -32,6 +37,8 @@ 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,8 @@ ExtensionContextMenuModel::ExtensionContextMenuModel(const Extension* extension,
       extension_id_(extension->id()),
       browser_(browser),
       profile_(browser->profile()),
-      delegate_(delegate) {
+      delegate_(delegate),
+      action_type_(NO_ACTION) {
   InitMenu(extension);
 
   if (profile_->GetPrefs()->GetBoolean(prefs::kExtensionsUIDeveloperMode) &&
@@ -56,11 +64,15 @@ ExtensionContextMenuModel::ExtensionContextMenuModel(const Extension* extension,
       extension_id_(extension->id()),
       browser_(browser),
       profile_(browser->profile()),
-      delegate_(NULL) {
+      delegate_(NULL),
+      action_type_(NO_ACTION) {
   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 +115,16 @@ 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();

[Devlin, 2014/07/01 20:54:23]

content::ContextMenuParams* params (note asterisk placement).

[Devlin, 2014/07/01 20:54:24]

Gah!  Leak!

[gpdavis, 2014/07/02 01:17:46]

I'm ashamed of all the leaks you've found in my code.  Nice catch.  I'll make a
private params_.

+    params->extension_id = extension->id();
+    extension_items_->ExecuteCommand(command_id, web_contents, *params);
+    return;
+  }
+
   switch (command_id) {
     case NAME: {
       OpenURLParams params(extensions::ManifestURL::GetHomepageURL(extension),
@@ -162,14 +184,27 @@ 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_)
+  if (!extension_action_) {
     extension_action_ = extension_action_manager->GetPageAction(*extension);
+    if (extension_action_)
+      action_type_ = PAGE_ACTION;
+  } else {
+    action_type_ = BROWSER_ACTION;
+  }
+
+  extension_items_.reset(new extensions::ContextMenuMatcher(profile_,

[Devlin, 2014/07/01 20:54:24]

nit: fix param wrapping.

[gpdavis, 2014/07/02 01:17:46]

Done.

+                         this,
+                         this,
+                         base::Bind(MenuItemMatchesExtension,
+                                    action_type_,
+                                    extension)));
 
   std::string extension_name = extension->name();
   // Ampersands need to be escaped to avoid being treated like
   // mnemonics in the menu.
   base::ReplaceChars(extension_name, "&", "&&", &extension_name);
   AddItem(NAME, base::UTF8ToUTF16(extension_name));
+  AppendExtensionItems();
   AddSeparator(ui::NORMAL_SEPARATOR);
   AddItemWithStringId(CONFIGURE, IDS_EXTENSIONS_OPTIONS_MENU_ITEM);
   AddItem(UNINSTALL, l10n_util::GetStringUTF16(IDS_EXTENSIONS_UNINSTALL));
@@ -184,3 +219,72 @@ const Extension* ExtensionContextMenuModel::GetExtension() const {
       extensions::ExtensionSystem::Get(profile_)->extension_service();
   return extension_service->GetExtensionById(extension_id_, false);
 }
+
+void ExtensionContextMenuModel::AppendExtensionItems() {

[Devlin, 2014/07/01 20:54:24]

The fact that 90% of this is verbatim copied code makes me wonder if we
shouldn't have it in a common location... but we'll worry about that later.

+  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;
+
+  // Get a list of extension id's that have context menu items, and sort by the

[Devlin, 2014/07/01 20:54:24]

extension ids, not extension id's.

[gpdavis, 2014/07/02 01:17:46]

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

[Devlin, 2014/07/01 20:54:24]

nit: I don't know why, but we seem to have the pattern of having iterators be
called iter or it (I prefer iter), and only using i as an integer or size_t

[gpdavis, 2014/07/02 01:17:46]

Done.

+       i != ids.end();
+       ++i) {
+    const Extension* extension =
+        service->GetExtensionById(i->extension_id, false);

[Devlin, 2014/07/01 20:54:23]

ExtensionService::GetExtensionById is deprecated.

[gpdavis, 2014/07/02 01:17:46]

It is?  I don't see any indication of that in the comments.

What should I use instead?

[Devlin, 2014/07/02 17:25:21]

Huh.  There should be a comment there.
Instead, use ExtensionRegistry::enabled_extensions().GetByID(id);

[gpdavis, 2014/07/03 01:46:58]

Done.

+    if (extension) {
+      base::string16 menu_title = extension_items_->GetTopLevelContextMenuTitle(
+          *i, base::EmptyString16());
+      map_ids[menu_title] = i->extension_id;
+      sorted_menu_titles.push_back(menu_title);
+    }
+  }
+  if (sorted_menu_titles.empty())
+    return;
+
+  AddSeparator(ui::NORMAL_SEPARATOR);
+
+  const std::string app_locale = g_browser_process->GetApplicationLocale();

[Devlin, 2014/07/01 20:54:24]

Make this a const &, not just const.

[gpdavis, 2014/07/02 01:17:46]

Done.  Can you explain exactly why this ought to be the case here?  This code
was borrowed, and I didn't touch this line.  Such is the case for a lot of these
nits ;)

[Devlin, 2014/07/02 17:25:21]

I believe it :)  There's no guarantee that the code existing in the codebase is
the best code.

The reason to have it be a const & is to avoid having to make a copy of it. 
Since you only persist it for the current function, you can guarantee it won't
go away, and a const & is fine and saves a little bit of time.

+  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, base::EmptyString16(), &index);

[Devlin, 2014/07/01 20:54:23]

See comment for base::EmptyString16 where its declared.

[gpdavis, 2014/07/02 01:17:46]

I was following yoyo's suggestion to use this.  How would I go about doing this
another way without running into a memory leak?  I need to pass in an empty
string by reference.

[Devlin, 2014/07/02 17:25:21]

Yeah, we should only use base::EmptyString[16] when we need to be sure the
reference isn't gonna go anywhere.  In this case, you're just passing in a
string by const &, and it only does that to avoid copying it (as you want to do
above).  So for this, it's perfectly fine to just pass in base::string16(), as
in:
extension_items_->AppendExtensionItems(
    extension_key, base::string16(), &index);

[gpdavis, 2014/07/03 01:46:58]

Done.

+  }
+
+  UMA_HISTOGRAM_TIMES("Extensions.ActionContextMenus_BuildTime",

[Devlin, 2014/07/01 20:54:23]

If you don't add these to histograms.xml, we'll never see the data.

[gpdavis, 2014/07/02 01:17:46]

How exactly do I add items to this file without blowing things up?

[Devlin, 2014/07/02 17:25:20]

Did something blow up?  Huh... usually that doesn't happen ;)

You should be able to just add a new histogram definition along with the rest
(in alphabetical order), and it should be fine.  If it's whining that it's not
pretty-printed you could a) try to find it yourself out of stubbornness (my
approach), or b) do the smart thing and run the pretty_print.py script in the
same folder.  If it's not compiling anymore, it likely just means that a bracket
or quote is mismatched somewhere.  If it's something else, then I dunno - I'd
have to see the explosion. :)

[gpdavis, 2014/07/03 01:46:58]

Well, it didn't actually blow up because I was apprehensive about adding things
to it.  I can't find any instances of ContextMenus_BuildTime (which was the
original metric from where this code was borrowed), so I'm kind of puzzled.  Not
really sure how to go about adding this metric if I can't find where the old one
that I'm modeling it off of is.

[Devlin, 2014/07/07 20:20:57]

If there's no entry in histograms.xml, then we don't care about the data and
aren't even gathering it.  Read: clearly no one cares about this data, and we
can safely remove it (and not add any analogous data) :)

[gpdavis, 2014/07/09 02:13:53]

Sounds good to me!  I'll go ahead and remove the unused entry from the file that
this code was borrowed from.  Done.

+                      base::TimeTicks::Now() - begin);
+  UMA_HISTOGRAM_COUNTS("Extensions.ActionContextMenus_ItemCount", index);
+}
+
+bool ExtensionContextMenuModel::MenuItemMatchesExtension(
+    ActionType type,
+    const Extension* extension,
+    const extensions::MenuItem* item) {
+  if (type == NO_ACTION || item->extension_id() != extension->id())
+    return false;
+
+  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;
+}