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..eeefe0310daabdd903e781c891d3dcff506aac60 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,65 @@ void ExtensionActionManager::OnExtensionUnloaded(
 
 namespace {
 
+const int* kIconSizes = extension_misc::kExtensionActionIconSizes;

[not at google - send to devlin, 2014/08/07 16:03:37]

should this be const int kIconSizes[] = ...? or does the compiler not like that.
I can never remember.

[gpdavis, 2014/08/07 18:30:59]

Well, I was trying to avoid copying the array by just creating a pointer to the
start of the array.  Either way would work, and if the array implementation is
preferred, I could do that.  I just figured this was ever so marginally better
because it functions as an array anyway *shrug*

[not at google - send to devlin, 2014/08/07 19:03:28]

I don't think using [] copies the array.

[gpdavis, 2014/08/07 21:28:18]

You were right about the compiler not liking it:
const int kIconSizes[] = extension_misc::kExtensionActionIconSizes;

"Array initializer must be an initializer list"

I thought perhaps that would copy the array contents over.  In any case, I think
the pointer is what we're looking for.

+const int kNumIcons = extension_misc::kNumExtensionActionIconSizes;

[not at google - send to devlin, 2014/08/07 16:03:37]

not much point making these scoped outside of the only function which uses them.

(and this should really be kNumIconSizes)
(and it should be a size_t)

[gpdavis, 2014/08/07 18:30:59]

Done.

+
+// 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->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();
+
+  // If the action has a 38px icon and no 19px icon, add the 38px icon path
+  // for the 19px key and return;

[not at google - send to devlin, 2014/08/07 16:03:37]

why this? seems very specific to 19 vs 38 px icons.

I would say that a more general approach is: (I think, if that's what you're
going for):

for (size_t i = 0; i < kNumIconSizes; ++i) {
  int i_icon_size = kIconSizes[i];
  if (!default_icon->Get(i_icon_size).empty())
    continue;
  // Find the smallest icon which can be cleanly scaled down to this one.
  for (size_t j = i + 1; j < kNumIconSizes; ++j) {
    int j_icon_size = jIconSizes[j];
    if (j_icon_size % i_icon_size != 0)
      continue;
    const std::string& path = default_icon->Get(j_icon_size);
    if (path.empty())
      continue;
      default_icon->Add(
          i_icon_size, path, ExtensionIconSet::MATCH_EXACTLY);
      break;
    }
  }
}

but isn't all of this what MATCH_BIGGER in Get() is supposed to do for you
automatically? maybe I'm misunderstanding.

[gpdavis, 2014/08/07 18:30:59]

It certainly is specific to 19 and 38px icons.

Essentially the goal here was to condense a rather convoluted set of logic as
much as possible.  Perhaps a discussion on how exactly we want to approach these
situations is worthwhile.  What I've implemented (and correct me if any of these
assumptions are incorrect):

1. We don't want to replace missing action icons with extension icons if there
is another viable action icon to use instead.  The only time this manifests
itself currently is when there's a 38px icon but no 19px icon; in this case, we
scale down the 38, and we're done because 19 and 38 have icons.

2. I was under the impression that we want to use the largest extension icon for
substitution if case 1) doesn't happen.  But what if the extension's largest
icon is 24px?  We could use that for 19 (right?), but we couldn't use it for 38.
 This is what the last for loop is for.


So my approach was:

Check condition 1) by seeing if 38 exists but 19 doesn't, in which case we set
19 to 38's icon and return (since all action icons are now filled).

Otherwise, we retrieve the largest extension icon we can find, and if we find
one, we go through the action icons keys, replacing anything that satisfies the
conditions: the icon must be missing from the action key, and the resolution of
the key must be smaller than the largest extension icon we have.


This could certainly be restructured as a loop that goes through each action
icon and makes the determination on what to do.  But I noticed that modulo
operation, and I'm wondering if there's more to scaling icons than I thought.  I
was under the impression that any -larger- icon is a viable option, but it looks
like you're suggesting that the resolution ought to be a multiple.

So how about this.  Loop through each action icon.  If the icon is missing,
first search any remaining larger action icons for one that has a resolution
that modulos cleanly (which covers the case I described above, since the action
icons are 19 and 38).  If we can't find one, then we consult the extension icons
in the same way: find a larger icon that has a resolution that divides cleanly. 
I could use the array of extension icon sizes for this.  My only concern is that
the sets of icon sizes don't match up; under these conditions, the only
extension icon that will be chosen for 19px would be the 19px icon, since none
of the other resolutions modulo with 19 cleanly.

Sorry for the long-winded response.  This should be easy implementation if I get
solidify exactly what we ought to be doing here.  What should the rules be for
resizing icons?

[not at google - send to devlin, 2014/08/07 19:03:28]

Yes the modulo thing was so that we get cleaner icon scaling, but some kind of
icon is better than no icon (provided the resolution isn't crappy). but really
it seems like based on kExtensionIconSizes vs kExtensionActionIconSizes there is
actually no clean conversion.

the 38 --> 19 conversion should already be happening with that Get() method,
right? so what would be wrong with a super simple algorithm, find the largest
extension icon, and if it's larger than the required extension action size,
assign it?

[gpdavis, 2014/08/07 21:28:18]

This is essentially what I have, except I'm passing in MATCH_EXACTLY, so that
conversion does not happen in Get().  But the 38px icon is already pulled for
page and browser actions if the 19px icon doesn't exist.  So then in that case,
we don't really need to replace the 19px icon at all, right?

I think I understand what you're saying now.  Loop through all action icons,
getting with MATCH_BIGGER, and anything that doesn't exist should get replaced
with the largest extension icon we can find, so long as it's larger than the
action icon we're considering.  Sounds reasonable to me.

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

Yeah that's what I'm saying.

Specifics of 19 and 38 shouldn't come into it, there's already a constant
defined somewhere which lists them.

+  if (default_icon->Get(kIconSizes[0],
+                        ExtensionIconSet::MATCH_EXACTLY).empty() &&
+      !default_icon->Get(kIconSizes[1],
+                         ExtensionIconSet::MATCH_EXACTLY).empty()) {
+    default_icon->Add(kIconSizes[0], default_icon->Get(kIconSizes[1],
+        ExtensionIconSet::MATCH_EXACTLY));
+  } else {
+    // Get largest available icon for |extension|.
+    const ExtensionIconSet& extension_icons =
+        extensions::IconsInfo::GetIcons(&extension);
+    std::string largest_icon = extension_icons.Get(
+        extension_misc::EXTENSION_ICON_GIGANTOR,
+        ExtensionIconSet::MATCH_SMALLER);
+
+    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 < kNumIcons; ++i) {
+        int size = kIconSizes[i];
+        if (default_icon->Get(size, ExtensionIconSet::MATCH_EXACTLY).empty()
+            && extension_icons.GetIconSizeFromPath(largest_icon) > size)
+          default_icon->Add(size, largest_icon);

[not at google - send to devlin, 2014/08/07 16:03:37]

{} around this.

[gpdavis, 2014/08/07 18:30:59]

Done.

+      }
+
+    }
+  }
+
+  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 +150,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 +204,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_);