Improve extension icon prediction

BUG=394920

Review URL: https://codereview.chromium.org/415813003

Cr-Commit-Position: refs/heads/master@{#289062}
git-svn-id: svn://svn.chromium.org/chrome/trunk/src@289062 0039d316-1c4b-4281-b951-d872f2087c98

chrome/browser/extensions/active_script_controller.cc

@@ -11,9 +11,11 @@
 #include "base/stl_util.h"
 #include "chrome/browser/extensions/active_tab_permission_granter.h"
 #include "chrome/browser/extensions/extension_action.h"
+#include "chrome/browser/extensions/extension_action_manager.h"
 #include "chrome/browser/extensions/extension_util.h"
 #include "chrome/browser/extensions/location_bar_controller.h"
 #include "chrome/browser/extensions/tab_helper.h"
+#include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/sessions/session_id.h"
 #include "chrome/common/extensions/api/extension_action/action_info.h"
 #include "content/public/browser/navigation_controller.h"
@@ -110,20 +112,11 @@ ExtensionAction* ActiveScriptController::GetActionForExtension(
   if (existing != active_script_actions_.end())
     return existing->second.get();
 
-  linked_ptr<ExtensionAction> action(new ExtensionAction(
-      extension->id(), ActionInfo::TYPE_PAGE, ActionInfo()));
-  action->SetTitle(ExtensionAction::kDefaultTabId, extension->name());
+  linked_ptr<ExtensionAction> action(ExtensionActionManager::Get(
+      Profile::FromBrowserContext(web_contents()->GetBrowserContext()))
+      ->GetBestFitAction(*extension, ActionInfo::TYPE_PAGE).release());
   action->SetIsVisible(ExtensionAction::kDefaultTabId, true);
 
-  const ActionInfo* action_info = ActionInfo::GetPageActionInfo(extension);
-  if (!action_info)
-    action_info = ActionInfo::GetBrowserActionInfo(extension);
-
-  if (action_info && !action_info->default_icon.empty()) {
-    action->set_default_icon(
-        make_scoped_ptr(new ExtensionIconSet(action_info->default_icon)));
-  }
-
   active_script_actions_[extension->id()] = action;
   return action.get();
 }

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,55 @@ 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 = extension_icons.Get(
+      extension_misc::EXTENSION_ICON_GIGANTOR,
+      ExtensionIconSet::MATCH_SMALLER);
+
+  if (!largest_icon.empty()) {
+    int largest_icon_size = extension_icons.GetIconSizeFromPath(largest_icon);
+    // 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) {
+        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)
@@ -101,37 +140,55 @@ 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 (!ExtensionRegistry::Get(profile)
+      ->enabled_extensions().Contains(extension.id())) {
     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 +196,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_);

chrome/browser/extensions/extension_action_manager.h

@@ -9,6 +9,7 @@
 #include <string>
 
 #include "base/scoped_observer.h"
+#include "chrome/common/extensions/api/extension_action/action_info.h"
 #include "components/keyed_service/core/keyed_service.h"
 #include "extensions/browser/extension_registry_observer.h"
 
@@ -41,6 +42,13 @@ class ExtensionActionManager : public KeyedService,
   ExtensionAction* GetSystemIndicator(
       const extensions::Extension& extension) const;
 
+  // Gets the best fit ExtensionAction for the given |extension|. This takes
+  // into account |extension|'s browser or page actions, if any, along with its
+  // name and any declared icons.
+  scoped_ptr<ExtensionAction> GetBestFitAction(
+      const extensions::Extension& extension,
+      extensions::ActionInfo::Type type) const;
+
  private:
   // Implement ExtensionRegistryObserver.
   virtual void OnExtensionUnloaded(content::BrowserContext* browser_context,

chrome/browser/extensions/extension_action_manager_unittest.cc

+// Copyright 2014 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 "base/strings/string_number_conversions.h"
+#include "chrome/browser/extensions/extension_action.h"
+#include "chrome/test/base/testing_profile.h"
+#include "extensions/browser/extension_registry.h"
+#include "extensions/common/extension_builder.h"
+#include "extensions/common/manifest_handlers/icons_handler.h"
+#include "extensions/common/value_builder.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace extensions {
+
+namespace {
+
+const char kBrowserAction[] = "browser_action";
+const char kPageAction[] = "page_action";
+
+}  // namespace
+
+class ExtensionActionManagerTest : public testing::Test {
+ public:
+  ExtensionActionManagerTest();
+
+ protected:
+  // Build an extension, populating |action_type| key with |action|, and
+  // "icons" key with |extension_icons|.
+  scoped_refptr<Extension> BuildExtension(DictionaryBuilder& extension_icons,
+                                          DictionaryBuilder& action,
+                                          const char* action_type);
+
+  // Returns true if |action|'s title matches |extension|'s name.
+  bool TitlesMatch(const Extension& extension, const ExtensionAction& action);
+
+  // Returns true if |action|'s icon for size |action_key| matches
+  // |extension|'s icon for size |extension_key|;
+  bool IconsMatch(const Extension& extension,
+                  int extension_key,
+                  const ExtensionAction& action,
+                  int action_key);
+
+  // Returns the appropriate action for |extension| according to |action_type|.
+  ExtensionAction* GetAction(const char* action_type,
+                             const Extension& extension);
+
+  // Tests that values that are missing from the |action_type| key are properly
+  // populated with values from the other keys in the manifest (e.g.
+  // "default_icon" key of |action_type| is populated with "icons" key).
+  void TestPopulateMissingValues(const char* action_type);
+
+  ExtensionActionManager* manager() { return manager_; }
+
+ private:
+  ExtensionRegistry* registry_;
+  int curr_id_;
+  ExtensionActionManager* manager_;
+  scoped_ptr<TestingProfile> profile_;
+};
+
+ExtensionActionManagerTest::ExtensionActionManagerTest()
+    : curr_id_(0),
+      profile_(new TestingProfile) {
+  registry_ = ExtensionRegistry::Get(profile_.get());
+  manager_ = ExtensionActionManager::Get(profile_.get());
+}
+
+scoped_refptr<Extension> ExtensionActionManagerTest::BuildExtension(
+    DictionaryBuilder& extension_icons,
+    DictionaryBuilder& action,
+    const char* action_type) {
+  std::string id = base::IntToString(curr_id_++);
+  scoped_refptr<Extension> extension = ExtensionBuilder()
+      .SetManifest(DictionaryBuilder().Set("version", "1")
+          .Set("manifest_version", 2)
+          .Set("icons", extension_icons)
+          .Set(action_type, action)
+          .Set("name",
+              std::string("Test Extension").append(id)))
+      .SetID(id)
+      .Build();
+  registry_->AddEnabled(extension);
+  return extension;
+}
+
+bool ExtensionActionManagerTest::TitlesMatch(const Extension& extension,
+                                             const ExtensionAction& action) {
+  return action.GetTitle(ExtensionAction::kDefaultTabId) == extension.name();
+}
+
+bool ExtensionActionManagerTest::IconsMatch(const Extension& extension,
+                                            int extension_key,
+                                            const ExtensionAction& action,
+                                            int action_key) {
+  return action.default_icon()->Get(action_key,
+                                    ExtensionIconSet::MATCH_EXACTLY) ==
+      IconsInfo::GetIcons(&extension).Get(extension_key,
+                                          ExtensionIconSet::MATCH_EXACTLY);
+}
+
+ExtensionAction* ExtensionActionManagerTest::GetAction(
+    const char* action_type,
+    const Extension& extension) {
+  return (action_type == kBrowserAction) ?
+      manager_->GetBrowserAction(extension) :
+      manager_->GetPageAction(extension);
+}
+
+void ExtensionActionManagerTest::TestPopulateMissingValues(
+    const char* action_type) {
+  // Test that the largest icon from the extension's "icons" key is chosen as a
+  // replacement for missing action default_icons keys. "19" should not be
+  // replaced because "38" can always be used in its place.
+  scoped_refptr<Extension> extension = BuildExtension(
+      DictionaryBuilder().Set("48", "icon48.png")
+                         .Set("128", "icon128.png"),
+      DictionaryBuilder().Pass(),
+      action_type);
+
+  ASSERT_TRUE(extension.get());
+  const ExtensionAction* action = GetAction(action_type, *extension);
+  ASSERT_TRUE(action);
+
+  ASSERT_TRUE(TitlesMatch(*extension, *action));
+  ASSERT_TRUE(IconsMatch(*extension, 128, *action, 38));
+
+  // Test that the action's missing default_icons are not replaced with smaller
+  // icons.
+  extension = BuildExtension(
+      DictionaryBuilder().Set("24", "icon24.png"),
+      DictionaryBuilder().Pass(),
+      action_type);
+
+  ASSERT_TRUE(extension.get());
+  action = GetAction(action_type, *extension);
+  ASSERT_TRUE(action);
+
+  ASSERT_TRUE(IconsMatch(*extension, 24, *action, 19));
+  ASSERT_FALSE(IconsMatch(*extension, 24, *action, 38));
+
+  // Test that an action's 19px icon is not replaced if a 38px action icon
+  // exists.
+  extension = BuildExtension(
+      DictionaryBuilder().Set("128", "icon128.png"),
+      DictionaryBuilder().Set("default_icon", DictionaryBuilder()
+                                              .Set("38", "action38.png")),
+      action_type);
+
+  ASSERT_TRUE(extension.get());
+  action = GetAction(action_type, *extension);
+  ASSERT_TRUE(action);
+
+  ASSERT_FALSE(IconsMatch(*extension, 128, *action, 19));
+
+  // Test that existing default_icons and default_title are not replaced.
+  extension = BuildExtension(
+      DictionaryBuilder().Set("128", "icon128.png"),
+      DictionaryBuilder().Set("default_title", "Action!")
+                         .Set("default_icon", DictionaryBuilder()
+                                              .Set("19", "action19.png")
+                                              .Set("38", "action38.png")),
+      action_type);
+
+  ASSERT_TRUE(extension.get());
+  action = GetAction(action_type, *extension);
+  ASSERT_TRUE(action);
+
+  ASSERT_FALSE(TitlesMatch(*extension, *action));
+  ASSERT_FALSE(IconsMatch(*extension, 128, *action, 19));
+  ASSERT_FALSE(IconsMatch(*extension, 128, *action, 38));
+}
+
+namespace {
+
+TEST_F(ExtensionActionManagerTest, PopulateBrowserAction) {
+  TestPopulateMissingValues(kBrowserAction);
+}
+
+TEST_F(ExtensionActionManagerTest, PopulatePageAction) {
+  TestPopulateMissingValues(kPageAction);
+}
+
+TEST_F(ExtensionActionManagerTest, GetBestFitActionTest) {
+  // Create an extension with page action defaults.
+  scoped_refptr<Extension> extension = BuildExtension(
+      DictionaryBuilder().Set("48", "icon48.png"),
+      DictionaryBuilder().Set("default_title", "Action!")
+                         .Set("default_icon", DictionaryBuilder()
+                                              .Set("38", "action38.png")),
+      kPageAction);
+  ASSERT_TRUE(extension.get());
+
+  // Get a "best fit" browser action for |extension|.
+  scoped_ptr<ExtensionAction> action =
+      manager()->GetBestFitAction(*extension, ActionInfo::TYPE_BROWSER);
+  ASSERT_TRUE(action.get());
+  ASSERT_EQ(action->action_type(), ActionInfo::TYPE_BROWSER);
+
+  // |action|'s title and default icon should match |extension|'s page action's.
+  ASSERT_EQ(action->GetTitle(ExtensionAction::kDefaultTabId), "Action!");
+  ASSERT_EQ(action->default_icon()->Get(38, ExtensionIconSet::MATCH_EXACTLY),
+            "action38.png");
+
+  // Create a new extension without page action defaults.
+  extension = BuildExtension(
+      DictionaryBuilder().Set("48", "icon48.png"),
+      DictionaryBuilder().Pass(),
+      kPageAction);
+  ASSERT_TRUE(extension.get());
+
+  action = manager()->GetBestFitAction(*extension, ActionInfo::TYPE_BROWSER);
+
+  // Now these values match because |extension| does not have page action
+  // defaults.
+  ASSERT_TRUE(TitlesMatch(*extension, *action));
+  ASSERT_TRUE(IconsMatch(*extension, 48, *action, 38));
+}
+
+}  // namespace
+}  // namespace extensions

chrome/chrome_tests_unit.gypi

@@ -938,6 +938,7 @@
         'browser/extensions/extension_api_unittest.cc',
         'browser/extensions/extension_api_unittest.h',
         'browser/extensions/extension_action_icon_factory_unittest.cc',
+        'browser/extensions/extension_action_manager_unittest.cc',
         'browser/extensions/extension_action_unittest.cc',
         'browser/extensions/extension_context_menu_model_unittest.cc',
         'browser/extensions/extension_creator_filter_unittest.cc',