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..33304fcf44f13cda92ad6c801359ba5cdd53e1b4 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,17 +83,54 @@ 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) {
+  const int* kIconSizes = extension_misc::kExtensionActionIconSizes;
+  const size_t kNumIconSizes = extension_misc::kNumExtensionActionIconSizes;
+
+  // If the title is missing from |action|, set it to |extension|'s name.
+  if (action->GetTitle(ExtensionAction::kDefaultTabId).empty())
+    action->SetTitle(ExtensionAction::kDefaultTabId, extension.name());
+
+  scoped_ptr<ExtensionIconSet> default_icon(new ExtensionIconSet());
+  if (action->default_icon())
+    *default_icon = *action->default_icon();
+
+  const ExtensionIconSet& extension_icons =
+      extensions::IconsInfo::GetIcons(&extension);
+  std::string largest_icon = extensions::IconsInfo::GetIcons(&extension).Get(

[not at google - send to devlin, 2014/08/07 22:14:15]

instead of extensions::IconsInfo::GetIcons(&extension) use |extension_icons|?

[gpdavis, 2014/08/07 23:48:35]

Whoops-- I definitely meant to use that (that's why I cached it in the first
place).  Done.

+      extension_misc::EXTENSION_ICON_GIGANTOR,
+      ExtensionIconSet::MATCH_SMALLER);
+  int largest_icon_size = extension_icons.GetIconSizeFromPath(largest_icon);

[not at google - send to devlin, 2014/08/07 22:14:15]

move this inside the !largest_icon.empty() check.

[gpdavis, 2014/08/07 23:48:34]

Done.

+
+  if (!largest_icon.empty()) {
+    // Replace any missing extension action icons with the largest icon
+    // retrieved from |extension|'s manifest so long as the largest icon is
+    // larger than the current key.
+    for (size_t i = 0; i < kNumIconSizes; ++i) {
+      int size = kIconSizes[i];
+      if (default_icon->Get(size, ExtensionIconSet::MATCH_BIGGER).empty()
+          && largest_icon_size > size) {
+        default_icon->Add(size, largest_icon);
+      }
+    }

[not at google - send to devlin, 2014/08/07 22:14:15]

if you step back in this loop rather than forward you can early-exit, because
the icons will be in decreasing order of size.

[gpdavis, 2014/08/07 23:48:35]

Clever.  Will adjust unit tests (since previously it would fill them all in if
they were all missing, as it started at the bottom).

+    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,
+    const Extension* extension,
     ActionInfo::Type action_type,
     const ActionInfo* action_info,
     Profile* profile) {
   std::map<std::string, linked_ptr<ExtensionAction> >::const_iterator it =
-      map->find(extension_id);
+      map->find(extension->id());
   if (it != map->end())
     return it->second.get();
   if (!action_info)
@@ -101,37 +139,53 @@ 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))
+  if (!extension)
     return NULL;
 
   linked_ptr<ExtensionAction> action(new ExtensionAction(
-      extension_id, action_type, *action_info));
-  (*map)[extension_id] = action;
+      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 {
-  return GetOrCreateOrNull(&page_actions_, extension.id(),
+    const Extension& extension) const {
+  return GetOrCreateOrNull(&page_actions_, &extension,
                            ActionInfo::TYPE_PAGE,
                            ActionInfo::GetPageActionInfo(&extension),
                            profile_);
 }
 
 ExtensionAction* ExtensionActionManager::GetBrowserAction(
-    const extensions::Extension& extension) const {
-  return GetOrCreateOrNull(&browser_actions_, extension.id(),
+    const Extension& extension) const {
+  return GetOrCreateOrNull(&browser_actions_, &extension,
                            ActionInfo::TYPE_BROWSER,
                            ActionInfo::GetBrowserActionInfo(&extension),
                            profile_);
 }
 
+scoped_ptr<ExtensionAction> ExtensionActionManager::GetBestFitAction(
+    const Extension& extension,
+    ActionInfo::Type type) const {
+  const ActionInfo* info = ActionInfo::GetBrowserActionInfo(&extension);
+  if (!info)
+    info = ActionInfo::GetPageActionInfo(&extension);
+
+  // Create a new ExtensionAction of |type| with |extension|'s ActionInfo.
+  // If no ActionInfo exists for |extension|, create and return a new action
+  // with a blank ActionInfo.
+  // Populate any missing values from |extension|'s manifest.
+  scoped_ptr<ExtensionAction> new_action(new ExtensionAction(
+      extension.id(), type, info ? *info : ActionInfo()));
+  PopulateMissingValues(extension, new_action.get());
+  return new_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
@@ -139,7 +193,7 @@ ExtensionAction* ExtensionActionManager::GetSystemIndicator(
   if (!extensions::SystemIndicatorManagerFactory::GetForProfile(profile_))
     return NULL;
 
-  return GetOrCreateOrNull(&system_indicators_, extension.id(),
+  return GetOrCreateOrNull(&system_indicators_, &extension,
                            ActionInfo::TYPE_SYSTEM_INDICATOR,
                            ActionInfo::GetSystemIndicatorInfo(&extension),
                            profile_);