Extensions: IconVariants: Prefer icon_variants over icons

Doc:
https://docs.google.com/document/d/1fY2sS0JucA87rj0itmcUqJG7HUxiBfLwTnzqt7J28SQ/edit?usp=sharing

Bug: chromium:344639840
Change-Id: Id76437b15316c72aac11da1be2c39cc0aec41713
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6288312
Reviewed-by: David Bertoni <dbertoni@chromium.org>
Commit-Queue: Solomon Kinard <solomonkinard@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1424221}

extensions/common/icons/extension_icon_variant.h

@@ -37,6 +37,16 @@ class ExtensionIconVariant {
     return diagnostics_;
   }
 
+  using Size = short;
+  using Path = std::string;
+
+  // Getters.
+  const std::optional<Path>& GetAny() const { return any_; }
+  const std::set<ColorScheme>& GetColorSchemes() const {
+    return color_schemes_;
+  }
+  const base::flat_map<Size, Path>& GetSizes() const { return sizes_; }
+
  private:
   // Helper methods that add to `this` object if the parameter is valid.
   void MaybeAddColorSchemes(const base::Value& value);
@@ -46,9 +56,6 @@ class ExtensionIconVariant {
   // Either `any` or `<size>` keys must have at least one value.
   bool IsValid() const;
 
-  using Size = short;
-  using Path = std::string;
-
   // The any key can have a path that's for any size.
   std::optional<Path> any_;
 

extensions/common/icons/extension_icon_variants.cc

@@ -51,7 +51,7 @@ void ExtensionIconVariants::Parse(const base::Value::List* list) {
 }
 
 bool ExtensionIconVariants::IsEmpty() const {
-  return list_.size() == 0;
+  return list_.empty();
 }
 
 void ExtensionIconVariants::Add(

extensions/common/icons/extension_icon_variants.h

@@ -9,12 +9,13 @@
 #include <vector>
 
 #include "base/values.h"
+#include "extensions/common/icons/extension_icon_set.h"
 #include "extensions/common/icons/extension_icon_variant.h"
 #include "extensions/common/icons/extension_icon_variants_diagnostics.h"
 
 namespace extensions {
 
-// Representation of the `icon_variants` key anywhere in manifest.json. It could
+// Representation of the `icon_variants` key defined in manifest.json. It could
 // be a top level key or a subkey of `action`.
 class ExtensionIconVariants {
  public:
@@ -37,6 +38,8 @@ class ExtensionIconVariants {
     return diagnostics_;
   }
 
+  const std::vector<ExtensionIconVariant>& GetList() const { return list_; }
+
  private:
   std::vector<ExtensionIconVariant> list_;
 

extensions/common/manifest_handlers/extension_manifests_icon_variants_unittest.cc

@@ -5,14 +5,27 @@
 #include "base/strings/stringprintf.h"
 #include "base/test/scoped_feature_list.h"
 #include "base/version_info/channel.h"
+#include "extensions/common/constants.h"
 #include "extensions/common/extension_features.h"
 #include "extensions/common/features/feature_channel.h"
+#include "extensions/common/icons/extension_icon_set.h"
 #include "extensions/common/manifest_constants.h"
+#include "extensions/common/manifest_handlers/icons_handler.h"
 #include "extensions/common/manifest_test.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace extensions {
 
+using IconVariantsFeatureFreeManifestTest = ManifestTest;
+
+// Test that icon_variants doesn't create an error when the feature isn't
+// enabled, even in the event of warnings.
+TEST_F(IconVariantsFeatureFreeManifestTest, Warnings) {
+  LoadAndExpectWarnings("icon_variants.json",
+                        {"'icon_variants' requires canary channel or newer, "
+                         "but this is the stable channel."});
+}
+
 class IconVariantsManifestTest : public ManifestTest {
  public:
   IconVariantsManifestTest() {
@@ -165,14 +178,26 @@ TEST_F(IconVariantsManifestTest, Errors) {
   }
 }
 
-using IconVariantsFeatureFreeManifestTest = ManifestTest;
+TEST_F(IconVariantsManifestTest, PreferIconVariantsOverIcons) {
+  ManifestData manifest_data = ManifestData::FromJSON(
+      R"({
+        "name": "test",
+        "version": "1",
+        "manifest_version": 3,
+        "icons": {
+          "16": "icons.16.png"
+        },
+        "icon_variants": [{
+          "16": "icon_variants.16.png"
+        }]
+      })");
+  scoped_refptr<extensions::Extension> extension(
+      LoadAndExpectSuccess(manifest_data));
+  const ExtensionIconSet& icons = IconsInfo::GetIcons(extension.get());
 
-// Test that icon_variants doesn't create an error, even in the event of
-// warnings.
-TEST_F(IconVariantsFeatureFreeManifestTest, Warnings) {
-  LoadAndExpectWarnings("icon_variants.json",
-                        {"'icon_variants' requires canary channel or newer, "
-                         "but this is the stable channel."});
+  EXPECT_EQ("icon_variants.16.png",
+            icons.Get(extension_misc::EXTENSION_ICON_BITTY,
+                      ExtensionIconSet::Match::kExactly));
 }
 
 }  // namespace extensions

extensions/common/manifest_handlers/icon_variants_handler.cc

@@ -7,6 +7,7 @@
 #include <memory>
 #include <string>
 
+#include "base/lazy_instance.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/values.h"
 #include "extensions/common/api/icon_variants.h"
@@ -14,9 +15,15 @@
 #include "extensions/common/icons/extension_icon_variants.h"
 #include "extensions/common/icons/extension_icon_variants_diagnostics.h"
 #include "extensions/common/manifest_constants.h"
+#include "ui/gfx/color_utils.h"
 
 namespace extensions {
 
+namespace {
+static base::LazyInstance<ExtensionIconSet>::DestructorAtExit g_empty_icon_set =
+    LAZY_INSTANCE_INITIALIZER;
+}  // namespace
+
 IconVariantsInfo::IconVariantsInfo() = default;
 IconVariantsInfo::~IconVariantsInfo() = default;
 
@@ -94,6 +101,40 @@ const IconVariantsInfo* IconVariantsInfo::GetIconVariants(
       extension->GetManifestData(IconVariantsManifestKeys::kIconVariants));
 }
 
+void IconVariantsInfo::InitializeIconSets() {
+  for (const auto& icon_variant : icon_variants->GetList()) {
+    const auto color_schemes = icon_variant.GetColorSchemes();
+    const auto sizes = icon_variant.GetSizes();
+    // TODO(crbug.com/344639840): Support any, e.g. any = icon_variant.GetAny();
+
+    for (const auto& size : sizes) {
+      // Add the size path pair to both extension icon sets if unspecified.
+      if (color_schemes.empty()) {
+        dark_.Add(size.first, size.second);
+        light_.Add(size.first, size.second);
+        continue;
+      }
+
+      if (color_schemes.contains(ExtensionIconVariant::ColorScheme::kDark)) {
+        dark_.Add(size.first, size.second);
+      }
+
+      if (color_schemes.contains(ExtensionIconVariant::ColorScheme::kLight)) {
+        light_.Add(size.first, size.second);
+      }
+    }
+  }
+}
+
+const ExtensionIconSet& IconVariantsInfo::Get() const {
+  if (!icon_variants) {
+    g_empty_icon_set.Get();
+  }
+
+  // TODO(crbug.com/344639840): Determine the current browser theme color.
+  return light_;
+}
+
 // TODO(crbug.com/41419485): Add more test coverage for warnings and errors.
 bool IconVariantsHandler::Parse(Extension* extension, std::u16string* error) {
   DCHECK(extension);
@@ -146,6 +187,7 @@ bool IconVariantsHandler::Parse(Extension* extension, std::u16string* error) {
   std::unique_ptr<IconVariantsInfo> icon_variants_info(
       std::make_unique<IconVariantsInfo>());
   icon_variants_info->icon_variants = std::move(icon_variants);
+  icon_variants_info->InitializeIconSets();
 
   extension->SetManifestData(keys::kIconVariants,
                              std::move(icon_variants_info));
@@ -156,7 +198,7 @@ bool IconVariantsHandler::Validate(
     const Extension* extension,
     std::string* error,
     std::vector<InstallWarning>* warnings) const {
-  // TODO(crbug.com/41419485): Validate icons.
+  // TODO(crbug.com/41419485): Validate icon existence.
   return true;
 }
 

extensions/common/manifest_handlers/icon_variants_handler.h

@@ -18,14 +18,25 @@ struct IconVariantsInfo : public Extension::ManifestData {
   IconVariantsInfo();
   ~IconVariantsInfo() override;
 
-  // A map of mode to ExtensionIconSet, which represents the declared icons.
-  std::unique_ptr<ExtensionIconVariants> icon_variants;
-
-  // Returns the icon set for the given `extension`.
+  // Returns whether `icon_variants` are defined for the given `extension`.
   static bool HasIconVariants(const Extension* extension);
 
   // Get IconVariants for the given `extension`, if they exist.
   static const IconVariantsInfo* GetIconVariants(const Extension* extension);
+
+  // Retrieve a matching ExtensionIconSet.
+  const ExtensionIconSet& Get() const;
+
+  // Data structure for `icon_variants`, based on icon_variants.idl.
+  std::unique_ptr<ExtensionIconVariants> icon_variants;
+
+  // Populate member variable extension sets from `icon_variants`.
+  void InitializeIconSets();
+
+ private:
+  // Allow for easy access to the theme-related sets
+  ExtensionIconSet dark_;
+  ExtensionIconSet light_;
 };
 
 // Parses the "icon_variants" manifest key.

extensions/common/manifest_handlers/icons_handler.cc

@@ -17,6 +17,7 @@
 #include "extensions/common/image_util.h"
 #include "extensions/common/manifest_constants.h"
 #include "extensions/common/manifest_handler_helpers.h"
+#include "extensions/common/manifest_handlers/icon_variants_handler.h"
 #include "extensions/strings/grit/extensions_strings.h"
 #include "ui/gfx/geometry/size.h"
 
@@ -30,6 +31,13 @@ static base::LazyInstance<ExtensionIconSet>::DestructorAtExit g_empty_icon_set =
 // static
 const ExtensionIconSet& IconsInfo::GetIcons(const Extension* extension) {
   DCHECK(extension);
+  // Prefer `icon_variants` over `icons`.
+  const IconVariantsInfo* icon_variants_info =
+      IconVariantsInfo::GetIconVariants(extension);
+  if (icon_variants_info) {
+    return icon_variants_info->Get();
+  }
+
   IconsInfo* info = static_cast<IconsInfo*>(
       extension->GetManifestData(keys::kIcons));
   return info ? info->icons : g_empty_icon_set.Get();