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..6d1f5d2f5b6426f6b62ba781d399191dbde9d4e2 100644
--- a/chrome/browser/extensions/extension_action_manager.cc
+++ b/chrome/browser/extensions/extension_action_manager.cc
@@ -6,12 +6,13 @@
 
 #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 {
 
@@ -82,6 +83,37 @@ void ExtensionActionManager::OnExtensionUnloaded(
 
 namespace {
 
+// Loads resources missing from |action| (i.e. title, icons) from the "icons"
+// key of |extension|'s manifest.
+void PopulateMissingValues(const Extension& extension,
+                           ExtensionAction* action) {
+  // 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;

[not at google - send to devlin, 2014/08/01 18:43:37]

much as it's tempting to early-return here I think it's a little confusing given
you conditionally execute code further up in the function.

[gpdavis, 2014/08/01 20:23:10]

Hmm.  How would you prefer this to be organized?  The goal here is to check the
title and update it if necessary, then if we find an icon that we can use to
replace any missing icons from the action's key, use it.  But if not, we
shouldn't make any attempts to change icon stuff.

+
+  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.
@@ -101,21 +133,22 @@ ExtensionAction* GetOrCreateOrNull(
   // 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))
+  const Extension* extension = ExtensionRegistry::Get(profile)

[not at google - send to devlin, 2014/08/01 18:43:37]

all call sites actually have access to an Extension so just pass that in rather
than |extension_id|.

[gpdavis, 2014/08/01 20:23:10]

I'll pass in a const Extension* since we have a null check on extension below.

+      ->enabled_extensions().GetByID(extension_id);
+  if (!extension)
     return NULL;
 
   linked_ptr<ExtensionAction> action(new ExtensionAction(
       extension_id, action_type, *action_info));
   (*map)[extension_id] = action;
+  PopulateMissingValues(*extension, 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 +156,33 @@ 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_);
 }
 
+scoped_ptr<ExtensionAction> ExtensionActionManager::GetBestFitAction(
+    const Extension& extension) const {
+  // Attempt to pull an existing action.
+  scoped_ptr<ExtensionAction> action(GetBrowserAction(extension));

[not at google - send to devlin, 2014/08/01 18:43:37]

I don't think this is right? the browser actions are owned by this class, and if
you pass ownership out it'll break.

I think given the comment below you'll want to copy these so that you can modify
the action type.

[gpdavis, 2014/08/01 20:23:10]

Ahh, okay, so this method shouldn't return a reference to this action, it should
return an entirely new action that can be manipulated and used?  I think that's
where my misunderstanding comes from here.  If that's correct, this makes a lot
more sense.

+  if (action.get())
+    return action.Pass();
+  action.reset(GetPageAction(extension));
+  if (action.get())
+    return action.Pass();
+
+  // If no action exists, create and return a page action.
+  // Populate any missing values from |extension|'s manifest.
+  action.reset(
+      new ExtensionAction(extension.id(), ActionInfo::TYPE_PAGE, ActionInfo()));

[not at google - send to devlin, 2014/08/01 18:43:37]

TYPE_PAGE isn't necessarily right. you'll need to pass the page type into this
method.

[gpdavis, 2014/08/01 20:23:10]

So we should pass in the type of action we'd like to get back?  If we find a
browser action, we should make a copy of it and change its type to page if
TYPE_PAGE is passed in?

[not at google - send to devlin, 2014/08/05 00:40:09]

yeah exactly.

[gpdavis, 2014/08/05 18:44:15]

Solid.  Patch set 6 has this change.

+  PopulateMissingValues(extension, action.get());
+  return action.Pass();
+}
+
 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