Add an IconKey.icon_effects field

Previously, the plan was to re-use the low bits of the IconKey.u_key to
represent icon post-processing effects such as desaturation to gray.

On further thought, we would also like to apply such effects to IconKeys
that already use the u_key to represent a resource ID, and do not use an
IncrementingIconKeyFactory.

It is therefore clearer to have an explicit field for icon effects.

In this commit, this new field is always zero. A follow-up commit will
use non-zero values.

BUG=826982

Change-Id: I6453a648ed2608a2af59d0d01e1fe9bf4af5c90f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1505741Reviewed-by: Dominick Ng <dominickn@chromium.org>
Commit-Queue: Nigel Tao <nigeltao@chromium.org>
Cr-Commit-Position: refs/heads/master@{#638358}

chrome/browser/apps/app_service/arc_apps.cc

@@ -327,11 +327,12 @@ void ArcApps::OnAppRemoved(const std::string& app_id) {
 
 void ArcApps::OnAppIconUpdated(const std::string& app_id,
                                const ArcAppIconDescriptor& descriptor) {
+  static constexpr uint32_t icon_effects = 0;
   apps::mojom::AppPtr app = apps::mojom::App::New();
   app->app_type = apps::mojom::AppType::kArc;
   app->app_id = app_id;
-  app->icon_key =
-      icon_key_factory_.MakeIconKey(apps::mojom::AppType::kArc, app_id);
+  app->icon_key = icon_key_factory_.MakeIconKey(apps::mojom::AppType::kArc,
+                                                app_id, icon_effects);
   Publish(std::move(app));
 }
 
@@ -445,8 +446,9 @@ apps::mojom::AppPtr ArcApps::Convert(const std::string& app_id,
   app->name = app_info.name;
   app->short_name = app->name;
 
-  app->icon_key =
-      icon_key_factory_.MakeIconKey(apps::mojom::AppType::kArc, app_id);
+  static constexpr uint32_t icon_effects = 0;
+  app->icon_key = icon_key_factory_.MakeIconKey(apps::mojom::AppType::kArc,
+                                                app_id, icon_effects);
 
   app->last_launch_time = app_info.last_launch_time;
   app->install_time = app_info.install_time;

chrome/browser/apps/app_service/built_in_chromeos_apps.cc

@@ -40,7 +40,7 @@ apps::mojom::AppPtr Convert(const app_list::InternalApp& internal_app) {
 
   app->icon_key = apps::mojom::IconKey::New(
       apps::mojom::AppType::kBuiltIn,
-      static_cast<uint64_t>(internal_app.icon_resource_id), std::string());
+      static_cast<uint64_t>(internal_app.icon_resource_id), std::string(), 0);
 
   app->last_launch_time = base::Time();
   app->install_time = base::Time();

chrome/browser/apps/app_service/crostini_apps.cc

@@ -221,6 +221,8 @@ apps::mojom::AppPtr CrostiniApps::Convert(
 apps::mojom::IconKeyPtr CrostiniApps::NewIconKey(const std::string& app_id) {
   DCHECK(!app_id.empty());
 
+  static constexpr uint32_t icon_effects = 0;
+
   // Treat the Crostini Terminal as a special case, loading an icon defined by
   // a resource instead of asking the Crostini VM (or the cache of previous
   // responses from the Crostini VM). Presumably this is for bootstrapping: the
@@ -229,10 +231,12 @@ apps::mojom::IconKeyPtr CrostiniApps::NewIconKey(const std::string& app_id) {
   // app and before bringing up an Crostini VM for the first time.
   if (app_id == crostini::kCrostiniTerminalId) {
     return apps::mojom::IconKey::New(apps::mojom::AppType::kCrostini,
-                                     IDR_LOGO_CROSTINI_TERMINAL, std::string());
+                                     IDR_LOGO_CROSTINI_TERMINAL, std::string(),
+                                     icon_effects);
   }
 
-  return icon_key_factory_.MakeIconKey(apps::mojom::AppType::kCrostini, app_id);
+  return icon_key_factory_.MakeIconKey(apps::mojom::AppType::kCrostini, app_id,
+                                       icon_effects);
 }
 
 void CrostiniApps::PublishAppID(const std::string& app_id,

chrome/browser/apps/app_service/extension_apps.cc

@@ -423,7 +423,9 @@ apps::mojom::AppPtr ExtensionApps::Convert(
   app->name = extension->name();
   app->short_name = extension->short_name();
 
-  app->icon_key = icon_key_factory_.MakeIconKey(app_type_, extension->id());
+  static constexpr uint32_t icon_effects = 0;
+  app->icon_key =
+      icon_key_factory_.MakeIconKey(app_type_, extension->id(), icon_effects);
 
   if (profile_) {
     auto* prefs = extensions::ExtensionPrefs::Get(profile_);

chrome/browser/apps/app_service/icon_key_util.cc

@@ -11,12 +11,8 @@ IncrementingIconKeyFactory::IncrementingIconKeyFactory() : u_key_(0) {}
 apps::mojom::IconKeyPtr IncrementingIconKeyFactory::MakeIconKey(
     apps::mojom::AppType app_type,
     const std::string& s_key,
-    uint8_t flags) {
-  // The flags occupy the low 8 bits.
-  u_key_ += 1 << 8;
-
-  return apps::mojom::IconKey::New(
-      app_type, u_key_ | static_cast<uint64_t>(flags), s_key);
+    uint32_t icon_effects) {
+  return apps::mojom::IconKey::New(app_type, ++u_key_, s_key, icon_effects);
 }
 
 }  // namespace apps_util

chrome/browser/apps/app_service/icon_key_util.h

@@ -22,17 +22,13 @@ namespace apps_util {
 // publish such IconKeys whenever an app's icon changes, even though the App ID
 // itself doesn't change, and App Service app subscribers will notice (and
 // reload) the new icon from the new (changed) icon key.
-//
-// The low 8 bits (a uint8_t) of the resultant IconKey's u_key are reserved for
-// caller-specific flags. For example, colorful/gray icons for enabled/disabled
-// states of the same app can be distinguished in one of those bits.
 class IncrementingIconKeyFactory {
  public:
   IncrementingIconKeyFactory();
 
   apps::mojom::IconKeyPtr MakeIconKey(apps::mojom::AppType app_type,
                                       const std::string& s_key,
-                                      uint8_t flags = 0);
+                                      uint32_t icon_effects);
 
  private:
   uint64_t u_key_;

chrome/services/app_service/app_service_impl_unittest.cc

@@ -196,7 +196,7 @@ TEST_F(AppServiceImplTest, PubSub) {
     pub0.load_icon_s_key = "-";
     pub1.load_icon_s_key = "-";
     pub2.load_icon_s_key = "-";
-    auto icon_key = apps::mojom::IconKey::New(app_type, 0, "o");
+    auto icon_key = apps::mojom::IconKey::New(app_type, 0, "o", 0);
     constexpr bool allow_placeholder_icon = false;
     impl.LoadIcon(
         std::move(icon_key), apps::mojom::IconCompression::kUncompressed,

chrome/services/app_service/public/cpp/app_update_unittest.cc

@@ -241,7 +241,7 @@ class AppUpdateTest : public testing::Test {
     // IconKey tests.
 
     if (state) {
-      auto x = apps::mojom::IconKey::New(app_type, 100, std::string());
+      auto x = apps::mojom::IconKey::New(app_type, 100, std::string(), 0);
       state->icon_key = x.Clone();
       expect_icon_key_ = x.Clone();
       expect_icon_key_changed_ = false;
@@ -249,7 +249,7 @@ class AppUpdateTest : public testing::Test {
     }
 
     if (delta) {
-      auto x = apps::mojom::IconKey::New(app_type, 0, "one hundred");
+      auto x = apps::mojom::IconKey::New(app_type, 0, "one hundred", 0);
       delta->icon_key = x.Clone();
       expect_icon_key_ = x.Clone();
       expect_icon_key_changed_ = true;

chrome/services/app_service/public/mojom/types.mojom

@@ -89,6 +89,9 @@ struct IconKey {
   // The semantics of u_key and s_key depend on the app_type.
   uint64 u_key;
   string s_key;
+  // A bitmask of icon post-processing effects, such as desaturation to gray
+  // and rounding the corners.
+  uint32 icon_effects;
 };
 
 enum IconCompression {