Improve extension icon prediction

chrome/browser/extensions/extension_action_manager.cc

Index: chrome/browser/extensions/extension_action_manager.cc
diff --git a/chrome/browser/extensions/extension_action_manager.cc b/chrome/browser/extensions/extension_action_manager.cc
index b4799741373788db69ca8511c2a4e6cd1c9bd681..8587a8ba8d4c49f6328900c47cb9edade6a815c1 100644
--- a/chrome/browser/extensions/extension_action_manager.cc
+++ b/chrome/browser/extensions/extension_action_manager.cc
@@ -12,6 +12,8 @@
 #include "extensions/browser/extension_registry.h"
 #include "extensions/browser/extension_system.h"
 #include "extensions/browser/extensions_browser_client.h"
+#include "extensions/common/constants.h"
+#include "extensions/common/manifest_handlers/icons_handler.h"
 
 namespace extensions {
 
@@ -82,6 +84,40 @@ void ExtensionActionManager::OnExtensionUnloaded(
 
 namespace {
 
+// Loads resources missing from |action| (ie title, icons) from "icons" key of

[Devlin, 2014/07/25 16:46:55]

"i.e."
"the "icons" key"

[gpdavis, 2014/07/28 21:40:36]

Done.

+// |extension|'s manifest.
+void PopulateMissingValues(const Extension& extension,
+                           ExtensionAction* action) {
+  if (!action)

[Devlin, 2014/07/25 16:46:55]

Why are we passing in NULL actions in the first place?

[gpdavis, 2014/07/28 21:40:36]

You're right; we shouldn't need to check for this.  I had left it in since I was
moving this function call around, but as it sits now, it should never get a NULL
action.  Done.

+    return;
+
+  // If the title is missing from |action|, set it to |extension|'s name.
+  if (!action->HasTitle(ExtensionAction::kDefaultTabId))
+    action->SetTitle(ExtensionAction::kDefaultTabId, extension.name());
+
+  // Get largest available icon for |extension|. If no icon is found, there
+  // is nothing available to replace missing action icons with, so we return.
+  std::string icon_path = extensions::IconsInfo::GetIcons(&extension).Get(
+      extension_misc::EXTENSION_ICON_GIGANTOR,
+      ExtensionIconSet::MATCH_SMALLER);
+  if (icon_path.empty())
+    return;
+
+  scoped_ptr<ExtensionIconSet> default_icon(new ExtensionIconSet());
+  if (action->default_icon())
+    *default_icon = *action->default_icon();
+
+  // Replace any missing extension action icons with the largest icon
+  // retrieved from |extension|'s manifest.
+  for (size_t i = 0; i < extension_misc::kNumExtensionActionIconSizes; ++i) {
+    int size = extension_misc::kExtensionActionIconSizes[i];
+    if (default_icon->Get(size, ExtensionIconSet::MATCH_EXACTLY).empty())
+      default_icon->Add(size, icon_path);
+  }
+
+  action->set_default_icon(default_icon.Pass());
+}
+
 // Returns map[extension_id] if that entry exists. Otherwise, if
 // action_info!=NULL, creates an ExtensionAction from it, fills in the map, and
 // returns that.  Otherwise (action_info==NULL), returns NULL.
@@ -109,13 +145,15 @@ ExtensionAction* GetOrCreateOrNull(
   linked_ptr<ExtensionAction> action(new ExtensionAction(
       extension_id, action_type, *action_info));
   (*map)[extension_id] = action;
+  PopulateMissingValues(*service->GetExtensionById(extension_id, false),

[Devlin, 2014/07/25 16:46:55]

this is deprecated. Use ExtensionRegistry::enabled_extensions().GetByID().

[gpdavis, 2014/07/28 21:40:36]

Any idea why this deprecation isn't documented?  Done.

[not at google - send to devlin, 2014/07/28 21:55:36]

good point: https://codereview.chromium.org/426553010/

+                        action.get());
   return action.get();
 }
 
 }  // namespace
 
 ExtensionAction* ExtensionActionManager::GetPageAction(
-    const extensions::Extension& extension) const {
+    const Extension& extension) const {
   return GetOrCreateOrNull(&page_actions_, extension.id(),
                            ActionInfo::TYPE_PAGE,
                            ActionInfo::GetPageActionInfo(&extension),
@@ -123,15 +161,22 @@ ExtensionAction* ExtensionActionManager::GetPageAction(
 }
 
 ExtensionAction* ExtensionActionManager::GetBrowserAction(
-    const extensions::Extension& extension) const {
+    const Extension& extension) const {
   return GetOrCreateOrNull(&browser_actions_, extension.id(),
                            ActionInfo::TYPE_BROWSER,
                            ActionInfo::GetBrowserActionInfo(&extension),
                            profile_);
 }
 
+ExtensionAction* ExtensionActionManager::GetBestFitAction(

[Devlin, 2014/07/25 16:46:55]

I'm a bit confused.  I thought part of the purpose here was to be able to _get
an icon for an extension that didn't declare a page action or a browser action_.
 But here, we're just gonna return NULL if it doesn't have one of those two?

[gpdavis, 2014/07/28 21:40:36]

I'm actually a bit confused about the purpose of this as well.  Should this be
constructing an action and populating it with icons from the "icons" manifest
key if a page/browser action is not found?

[not at google - send to devlin, 2014/07/28 21:55:36]

Yes exactly.

one could imagine super-cool functionality here, like generating its icon from
the first letter of the extension's name or something.

but for now... it should try to get icons from the extension's icons, yes.

[gpdavis, 2014/07/29 00:18:01]

Can do.  A couple of design questions:

Script injection requests are always supposed to end up as page actions, right? 
My initial inclination was that GetBestFitAction would first try
GetBrowserAction, then GetPageAction, then fallback to creating a new action.

First of all, should the returned action be used directly, or just for
reference?  For example, the ActiveScriptController creates a new page action
and sets its default icons manually.  Instead, should it take the generated
action and run with it, or should it only use it for reference for the new
action it's creating?

Follow up:  if we do indeed use the action returned by GetBestFitAction, what if
the extension has a browser action and it requests script injection permissions?
 Then the returned action would be a browser action.

Finally, in regards to pointers, should I be using a scoped pointer within this
function, then use release() to return it?  If a new action is created, it's not
going to be stored anywhere within the ExtensionActionManager.

+    const Extension& extension) const {
+  if (ActionInfo::GetBrowserActionInfo(&extension))
+    return GetBrowserAction(extension);
+  return GetPageAction(extension);
+}
+
 ExtensionAction* ExtensionActionManager::GetSystemIndicator(
-    const extensions::Extension& extension) const {
+    const Extension& extension) const {
   // If it does not already exist, create the SystemIndicatorManager for the
   // given profile.  This could return NULL if the system indicator area is
   // unavailable on the current system.  If so, return NULL to signal that