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
+// |extension|'s manifest.
+void PopulateMissingValues(const extensions::Extension& extension,

[not at google - send to devlin, 2014/07/24 00:41:26]

extensions:: unnecessary (you might as well do file-wide replacement)

[gpdavis, 2014/07/24 17:57:46]

Done.

+                           ExtensionAction* action) {
+  if (action) {

[not at google - send to devlin, 2014/07/24 00:41:26]

if anything, early-return here if it's null. but I think it's better to make
sure that it's only ever passed a non-NULL |action|. it's weird to pass in a
NULL out-param.

[gpdavis, 2014/07/24 17:57:45]

Done.

+    // 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;
+
+    const ExtensionIconSet* default_icon = action->default_icon();
+    ExtensionIconSet new_default_icon;
+    if (default_icon)
+      new_default_icon = *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 (new_default_icon.Get(size, ExtensionIconSet::MATCH_EXACTLY).empty())
+        new_default_icon.Add(size, icon_path);
+    }
+
+    action->set_default_icon(
+        make_scoped_ptr(new ExtensionIconSet(new_default_icon)));

[not at google - send to devlin, 2014/07/24 00:41:26]

to avoid all this copying you could assign new_default_icon to a scoped_ptr
right from the start. could you also clean up that slightly awkward
new_default_icon thing as well?

scoped_ptr<ExtensionIconSet> default_icon(new ExtensionIconSet());
if (action->default_icon())
  *default_icon = *action->default_icon();

[gpdavis, 2014/07/24 17:57:46]

Done.

+  }
+}
+
 // 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),
+                        action.get());

[not at google - send to devlin, 2014/07/24 00:41:26]

nice! but let's be conservative for now and just do this on GetBestFitAction.

[gpdavis, 2014/07/24 17:57:45]

Unfortunately, this needs to be done upon creation of the action, otherwise the
browser crashes via segfault upon reloading of an unpacked extension.  It took
me a while to figure out why it wasn't working, but I believe it has to do with
the fact that the default_icon is used for creation of the IconFactory when
action views are updated.  If we populate these values after the action has been
used to create the icon factory, theres a discrepancy between the default_icon
used for the factory and the one now attributed to the action.  However, if we
populate these values immediately after the action is created, and before its
used to create the factory, this doesn't happen, since reloading an extension
updates all the action views, which recreates all the icon factories.

Relevant code:
https://code.google.com/p/chromium/codesearch#chromium/src/extensions/browser...

The action's default_icon is passed in via reference, and when we change that
reference in the ExtensionAction in PopulateMissingValues, bad things happen.

Is there any particular reason you don't want to make this call upon creation? 
It seemed appropriate to update all of this before the ExtensionAction lets
loose its references, and the only way this call could do anything different the
second time is if the extension is reloaded, in which case it's recreated
anyway.

[not at google - send to devlin, 2014/07/24 18:01:46]

I think that's fine.

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

[not at google - send to devlin, 2014/07/24 00:41:26]

no else after return.

[gpdavis, 2014/07/24 17:57:46]

Done.

+    return GetPageAction(extension);
+}
+
 ExtensionAction* ExtensionActionManager::GetSystemIndicator(
     const extensions::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