Improve extension icon prediction

chrome/browser/extensions/extension_action_manager.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/extensions/extension_action_manager.h"
 
 #include "chrome/browser/extensions/api/system_indicator/system_indicator_manager_factory.h"
 #include "chrome/browser/extensions/extension_action.h"
 #include "chrome/browser/extensions/extension_service.h"
 #include "chrome/browser/profiles/profile.h"
 #include "components/keyed_service/content/browser_context_dependency_manager.h"
 #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 {
 
 namespace {
 
 // BrowserContextKeyedServiceFactory for ExtensionActionManager.
 class ExtensionActionManagerFactory : public BrowserContextKeyedServiceFactory {
  public:
   // BrowserContextKeyedServiceFactory implementation:
   static ExtensionActionManager* GetForProfile(Profile* profile) {
@@ skipping 50 matching lines @@
     content::BrowserContext* browser_context,
     const Extension* extension,
     UnloadedExtensionInfo::Reason reason) {
   page_actions_.erase(extension->id());
   browser_actions_.erase(extension->id());
   system_indicators_.erase(extension->id());
 }
 
 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.
 ExtensionAction* GetOrCreateOrNull(
     std::map<std::string, linked_ptr<ExtensionAction> >* map,
     const std::string& extension_id,
     ActionInfo::Type action_type,
     const ActionInfo* action_info,
     Profile* profile) {
   std::map<std::string, linked_ptr<ExtensionAction> >::const_iterator it =
       map->find(extension_id);
   if (it != map->end())
     return it->second.get();
   if (!action_info)
     return NULL;
 
   // Only create action info for enabled extensions.
   // This avoids bugs where actions are recreated just after being removed
   // in response to OnExtensionUnloaded().
   ExtensionService* service =
       ExtensionSystem::Get(profile)->extension_service();
   if (!service->GetExtensionById(extension_id, false))
     return NULL;
 
   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),
                            profile_);
 }
 
 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
   // the system indicator area is unusable.
   if (!extensions::SystemIndicatorManagerFactory::GetForProfile(profile_))
     return NULL;
 
   return GetOrCreateOrNull(&system_indicators_, extension.id(),
                            ActionInfo::TYPE_SYSTEM_INDICATOR,
                            ActionInfo::GetSystemIndicatorInfo(&extension),
                            profile_);
 }
 
 }  // namespace extensions