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| (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 = extension_icons.Get(
+      extension_misc::EXTENSION_ICON_GIGANTOR,
+      ExtensionIconSet::MATCH_SMALLER);
+  int largest_icon_size;
+
+  if (!largest_icon.empty() &&
+      (largest_icon_size = extension_icons.GetIconSizeFromPath(largest_icon))) {

[not at google - send to devlin, 2014/08/08 00:21:07]

this looks kind of awkward, can't you declare that largest_icon_size variable
inside here? why the need to have the && part of the condition?

[gpdavis, 2014/08/08 01:03:58]

Oh, I misinterpreted what you said.  I thought you meant place it inside the
conditional statement.  Done.

+    // 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 (int i = kNumIconSizes - 1; i >= 0; --i) {
+      int size = kIconSizes[i];
+      if (default_icon->Get(size, ExtensionIconSet::MATCH_BIGGER).empty()
+          && largest_icon_size > size) {

[not at google - send to devlin, 2014/08/08 00:21:07]

this largest_icon_size>size check is funny - you're stepping down in sizes, so
if it's true on the first iteration it'll be true for all future iterations. you
might as well include it in the outer for-loop condition.

for (int i = kNumIconSizes - 1, size;
     i >= 0 && (size = kIconSizes[i]) < largest_icon_size;
     ++i) {
  ...
}

something like that should work?

[gpdavis, 2014/08/08 01:03:58]

What if the action has no icons, but the extension has a 24px icon?  We want the
24 to go into the "19" key, but not the "38" key.  An early termination of the
loop you're suggesting wouldn't work for that.

In fact, I just tweaked a test for this case.  This is the behavior we want,
right?

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

ah right, of course. now that it's a new day I think I know what I my poor brain
was trying to tell me yesterday, and it's what you wrote.

+        default_icon->Add(size, largest_icon);
+        break;
+      }
+    }
+    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)
     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))
+  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
   // the system indicator area is unusable.
   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_);
 }
 
 }  // namespace extensions