Allow arbitrary sizes for extension icons (and bookmark app icons)

BUG=564926

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

Cr-Commit-Position: refs/heads/master@{#363669}

chrome/browser/apps/drive/drive_app_converter.cc

@@ -117,16 +117,11 @@ void DriveAppConverter::Start() {
   web_app_.title = base::UTF8ToUTF16(drive_app_info_.app_name);
   web_app_.app_url = drive_app_info_.create_url;
 
-  const std::set<int> allowed_sizes(extension_misc::kExtensionIconSizes,
-                                    extension_misc::kExtensionIconSizes +
-                                        extension_misc::kNumExtensionIconSizes);
   std::set<int> pending_sizes;
   for (size_t i = 0; i < drive_app_info_.app_icons.size(); ++i) {
     const int icon_size = drive_app_info_.app_icons[i].first;
-    if (allowed_sizes.find(icon_size) == allowed_sizes.end() ||
-        pending_sizes.find(icon_size) != pending_sizes.end()) {
+    if (pending_sizes.find(icon_size) != pending_sizes.end())
       continue;
-    }
 
     pending_sizes.insert(icon_size);
     const GURL& icon_url = drive_app_info_.app_icons[i].second;

chrome/browser/apps/drive/drive_app_converter_browsertest.cc

@@ -71,9 +71,9 @@ class DriveAppConverterTest : public ExtensionBrowserTest {
   drive::DriveAppInfo GetTestDriveApp() {
     // Define four icons. icon1.png is 16x16 and good to use. icon2.png is
     // 16x16 but claims to be 32x32 and should be dropped. icon3.png is 66x66
-    // and not a valid extension icon size and should be dropped too. The forth
-    // one is icon2.png with 16x16 but should be ignored because 16x16 already
-    // has icon1.png as its resource.
+    // and not a typical icon size but it should be allowed. The fourth one is
+    // icon2.png with 16x16 but should be ignored because 16x16 already has
+    // icon1.png as its resource.
     drive::DriveAppInfo::IconList app_icons;
     app_icons.push_back(std::make_pair(16, GetTestUrl("extensions/icon1.png")));
     app_icons.push_back(std::make_pair(32, GetTestUrl("extensions/icon2.png")));
@@ -117,7 +117,11 @@ IN_PROC_BROWSER_TEST_F(DriveAppConverterTest, GoodApp) {
   EXPECT_EQ(extensions::LAUNCH_CONTAINER_TAB,
             AppLaunchInfo::GetLaunchContainer(app));
   EXPECT_EQ(0u, app->permissions_data()->active_permissions().apis().size());
-  EXPECT_EQ(1u, extensions::IconsInfo::GetIcons(app).map().size());
+  const ExtensionIconSet& icons = extensions::IconsInfo::GetIcons(app);
+  EXPECT_EQ(2u, icons.map().size());
+  EXPECT_FALSE(icons.Get(16, ExtensionIconSet::MATCH_EXACTLY).empty());
+  EXPECT_TRUE(icons.Get(32, ExtensionIconSet::MATCH_EXACTLY).empty());
+  EXPECT_FALSE(icons.Get(66, ExtensionIconSet::MATCH_EXACTLY).empty());
 
   const Extension* installed = extensions::ExtensionSystem::Get(profile())
                                    ->extension_service()

chrome/browser/extensions/bookmark_app_helper.cc

@@ -435,17 +435,16 @@ BookmarkAppHelper::ResizeIconsAndGenerateMissing(
     std::vector<BookmarkAppHelper::BitmapAndSource> icons,
     std::set<int> sizes_to_generate,
     WebApplicationInfo* web_app_info) {
-  // Add the downloaded icons. Extensions only allow certain icon sizes. First
-  // populate icons that match the allowed sizes exactly and then downscale
-  // remaining icons to the closest allowed size that doesn't yet have an icon.
-  std::set<int> allowed_sizes(extension_misc::kExtensionIconSizes,
-                              extension_misc::kExtensionIconSizes +
-                                  extension_misc::kNumExtensionIconSizes);
-
-  // If there are icons that don't match the accepted icon sizes, find the
-  // closest bigger icon to the accepted sizes and resize the icon to it.
+  // Resize provided icons to make sure we have versions for each size in
+  // |sizes_to_generate|.
   std::map<int, BitmapAndSource> resized_bitmaps(
-      ConstrainBitmapsToSizes(icons, allowed_sizes));
+      ConstrainBitmapsToSizes(icons, sizes_to_generate));
+
+  // Also add all provided icon sizes.
+  for (const BitmapAndSource& icon : icons) {
+    if (resized_bitmaps.find(icon.bitmap.width()) == resized_bitmaps.end())
+      resized_bitmaps.insert(std::make_pair(icon.bitmap.width(), icon));
+  }
 
   // Determine the color that will be used for the icon's background. For this
   // the dominant color of the first icon found is used.
@@ -777,18 +776,15 @@ void GetWebApplicationInfoFromApp(
   web_app_info.title = base::UTF8ToUTF16(extension->non_localized_name());
   web_app_info.description = base::UTF8ToUTF16(extension->description());
 
+  const ExtensionIconSet& icon_set = extensions::IconsInfo::GetIcons(extension);
   std::vector<extensions::ImageLoader::ImageRepresentation> info_list;
-  for (size_t i = 0; i < extension_misc::kNumExtensionIconSizes; ++i) {
-    int size = extension_misc::kExtensionIconSizes[i];
+  for (const auto& iter : icon_set.map()) {
     extensions::ExtensionResource resource =
-        extensions::IconsInfo::GetIconResource(
-            extension, size, ExtensionIconSet::MATCH_EXACTLY);
+        extension->GetResource(iter.second);
     if (!resource.empty()) {
       info_list.push_back(extensions::ImageLoader::ImageRepresentation(
-          resource,
-          extensions::ImageLoader::ImageRepresentation::ALWAYS_RESIZE,
-          gfx::Size(size, size),
-          ui::SCALE_FACTOR_100P));
+          resource, extensions::ImageLoader::ImageRepresentation::ALWAYS_RESIZE,
+          gfx::Size(iter.first, iter.first), ui::SCALE_FACTOR_100P));
     }
   }
 

chrome/common/extensions/api/extension_action/action_info.cc

@@ -91,7 +91,7 @@ scoped_ptr<ActionInfo> ActionInfo::Load(const Extension* extension,
     const base::DictionaryValue* icons_value = NULL;
     std::string default_icon;
     if (dict->GetDictionary(keys::kPageActionDefaultIcon, &icons_value)) {
-      if (!manifest_handler_helpers::LoadAllIconsFromDictionary(
+      if (!manifest_handler_helpers::LoadIconsFromDictionary(
               icons_value, &result->default_icon, error)) {
         return scoped_ptr<ActionInfo>();
       }

chrome/common/extensions/manifest_tests/extension_manifests_icons_unittest.cc

@@ -26,17 +26,9 @@ TEST_F(IconsManifestTest, NormalizeIconPaths) {
                                 ExtensionIconSet::MATCH_EXACTLY));
 }
 
-TEST_F(IconsManifestTest, InvalidIconSizes) {
+TEST_F(IconsManifestTest, IconSizes) {
   scoped_refptr<extensions::Extension> extension(
-      LoadAndExpectSuccess("init_ignored_icon_size.json"));
-  EXPECT_EQ("",
-            IconsInfo::GetIcons(extension.get())
-                .Get(300, ExtensionIconSet::MATCH_EXACTLY));
-}
-
-TEST_F(IconsManifestTest, ValidIconSizes) {
-  scoped_refptr<extensions::Extension> extension(
-      LoadAndExpectSuccess("init_valid_icon_size.json"));
+      LoadAndExpectSuccess("init_icon_size.json"));
   const ExtensionIconSet& icons = IconsInfo::GetIcons(extension.get());
 
   EXPECT_EQ("16.png", icons.Get(extension_misc::EXTENSION_ICON_BITTY,
@@ -53,6 +45,10 @@ TEST_F(IconsManifestTest, ValidIconSizes) {
                                  ExtensionIconSet::MATCH_EXACTLY));
   EXPECT_EQ("512.png", icons.Get(extension_misc::EXTENSION_ICON_GIGANTOR,
                                  ExtensionIconSet::MATCH_EXACTLY));
+
+  // Any old size will be accepted.
+  EXPECT_EQ("300.png", IconsInfo::GetIcons(extension.get())
+                           .Get(300, ExtensionIconSet::MATCH_EXACTLY));
 }
 
 }  // namespace extensions

chrome/test/data/extensions/manifest_tests/init_icon_size.json

@@ -8,6 +8,7 @@
     "48": "48.png",
     "128": "128.png",
     "256": "256.png",
+    "300": "300.png",
     "512": "512.png"
   }
 }

chrome/test/data/extensions/manifest_tests/init_ignored_icon_size.json

@@ -1,7 +0,0 @@
-{
-  "name": "my extension",
-  "version": "1.0.0.0",
-  "icons": {
-    "300": "300.png"
-  }
-}

extensions/common/constants.cc

@@ -61,20 +61,6 @@ const char kMimeTypePng[] = "image/png";
 
 namespace extension_misc {
 
-const int kExtensionIconSizes[] = {EXTENSION_ICON_GIGANTOR,     // 512
-                                   EXTENSION_ICON_EXTRA_LARGE,  // 256
-                                   EXTENSION_ICON_LARGE,        // 128
-                                   EXTENSION_ICON_MEDIUM,       // 48
-                                   EXTENSION_ICON_SMALL,        // 32
-                                   EXTENSION_ICON_SMALLISH,     // 24
-                                   EXTENSION_ICON_BITTY,        // 16
-                                   // Additional 2x resources to load.
-                                   2 * EXTENSION_ICON_MEDIUM,  // 96
-                                   2 * EXTENSION_ICON_SMALL    // 64
-};
-
-const size_t kNumExtensionIconSizes = arraysize(kExtensionIconSizes);
-
 const char kPdfExtensionId[] = "mhjfbmdgcfjbbpaeojofohoefgiehjai";
 const char kQuickOfficeComponentExtensionId[] =
     "bpmcpldpdmajfigpchkicefoigmkfalc";

extensions/common/constants.h

@@ -190,10 +190,6 @@ enum ExtensionIcons {
   EXTENSION_ICON_INVALID = 0,
 };
 
-// List of sizes for extension icons that can be defined in the manifest.
-extern const int kExtensionIconSizes[];
-extern const size_t kNumExtensionIconSizes;
-
 // The extension id of the PDF extension.
 extern const char kPdfExtensionId[];
 

extensions/common/manifest_handler_helpers.cc

@@ -32,37 +32,10 @@ bool NormalizeAndValidatePath(std::string* path) {
 }
 
 bool LoadIconsFromDictionary(const base::DictionaryValue* icons_value,
-                             const int* icon_sizes,
-                             size_t num_icon_sizes,
                              ExtensionIconSet* icons,
                              base::string16* error) {
   DCHECK(icons);
-  for (size_t i = 0; i < num_icon_sizes; ++i) {
-    std::string key = base::IntToString(icon_sizes[i]);
-    if (icons_value->HasKey(key)) {
-      std::string icon_path;
-      if (!icons_value->GetString(key, &icon_path)) {
-        *error = ErrorUtils::FormatErrorMessageUTF16(
-            errors::kInvalidIconPath, key);
-        return false;
-      }
-
-      if (!NormalizeAndValidatePath(&icon_path)) {
-        *error = ErrorUtils::FormatErrorMessageUTF16(
-            errors::kInvalidIconPath, key);
-        return false;
-      }
-
-      icons->Add(icon_sizes[i], icon_path);
-    }
-  }
-  return true;
-}
-
-bool LoadAllIconsFromDictionary(const base::DictionaryValue* icons_value,
-                                ExtensionIconSet* icons,
-                                base::string16* error) {
-  DCHECK(icons);
+  DCHECK(error);
   for (base::DictionaryValue::Iterator iterator(*icons_value);
        !iterator.IsAtEnd(); iterator.Advance()) {
     int size = 0;

extensions/common/manifest_handler_helpers.h

@@ -24,20 +24,12 @@ namespace manifest_handler_helpers {
 bool NormalizeAndValidatePath(std::string* path);
 
 // Loads icon paths defined in dictionary |icons_value| into ExtensionIconSet
-// |icons|. |icons_value| is a dictionary value {icon size -> icon path}. Icons
-// in |icons_value| whose size is not in |icon_sizes| will be ignored.
+// |icons|. |icons_value| is a dictionary value {icon size -> icon path}.
 // Returns success. If load fails, |error| will be set.
 bool LoadIconsFromDictionary(const base::DictionaryValue* icons_value,
-                             const int* icon_sizes,
-                             size_t num_icon_sizes,
                              ExtensionIconSet* icons,
                              base::string16* error);
 
-// As above, but loads all icons in |icons_value|.
-bool LoadAllIconsFromDictionary(const base::DictionaryValue* icons_value,
-                                ExtensionIconSet* icons,
-                                base::string16* error);
-
 }  // namespace manifest_handler_helpers
 }  // namespace extensions
 

extensions/common/manifest_handlers/icons_handler.cc

@@ -64,11 +64,7 @@ bool IconsHandler::Parse(Extension* extension, base::string16* error) {
   }
 
   if (!manifest_handler_helpers::LoadIconsFromDictionary(
-          icons_dict,
-          extension_misc::kExtensionIconSizes,
-          extension_misc::kNumExtensionIconSizes,
-          &icons_info->icons,
-          error)) {
+          icons_dict, &icons_info->icons, error)) {
     return false;
   }