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..d700d132c9cef3f120ab46fbc2fc45a8a088fd90 100644
--- a/chrome/browser/extensions/extension_action_manager.cc
+++ b/chrome/browser/extensions/extension_action_manager.cc
@@ -12,6 +12,8 @@
 #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 +84,41 @@ void ExtensionActionManager::OnExtensionUnloaded(
 
 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.
@@ -109,6 +146,8 @@ ExtensionAction* GetOrCreateOrNull(
   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();
 }
 
@@ -130,6 +169,14 @@ ExtensionAction* ExtensionActionManager::GetBrowserAction(
                            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