Let ArcAppIcon serve compressed icons

Bug: 826982
Change-Id: If549cb46f4f806e80b0fac5b5c3ba19b1855bac1
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1767289
Commit-Queue: Nigel Tao <nigeltao@chromium.org>
Reviewed-by: Yury Khmel <khmel@chromium.org>
Reviewed-by: Dominick Ng <dominickn@chromium.org>
Cr-Commit-Position: refs/heads/master@{#691436}

chrome/browser/apps/app_service/arc_apps.cc

@@ -41,6 +41,7 @@
 namespace {
 
 void OnArcAppIconCompletelyLoaded(
+    apps::mojom::IconCompression icon_compression,
     int32_t size_hint_in_dip,
     apps::IconEffects icon_effects,
     apps::mojom::Publisher::LoadIconCallback callback,
@@ -51,12 +52,28 @@ void OnArcAppIconCompletelyLoaded(
   }
 
   apps::mojom::IconValuePtr iv = apps::mojom::IconValue::New();
-  iv->icon_compression = apps::mojom::IconCompression::kUncompressed;
-  iv->uncompressed = icon->image_skia();
+  iv->icon_compression = icon_compression;
   iv->is_placeholder_icon = false;
 
-  if (icon_effects != apps::IconEffects::kNone) {
-    apps::ApplyIconEffects(icon_effects, size_hint_in_dip, &iv->uncompressed);
+  if (icon_compression == apps::mojom::IconCompression::kUncompressed) {
+    iv->uncompressed = icon->image_skia();
+    if (icon_effects != apps::IconEffects::kNone) {
+      apps::ApplyIconEffects(icon_effects, size_hint_in_dip, &iv->uncompressed);
+    }
+  } else {
+    auto& compressed_images = icon->compressed_images();
+    auto iter =
+        compressed_images.find(apps_util::GetPrimaryDisplayUIScaleFactor());
+    if (iter == compressed_images.end()) {
+      std::move(callback).Run(apps::mojom::IconValue::New());
+      return;
+    }
+    const std::string& data = iter->second;
+    iv->compressed = std::vector<uint8_t>(data.begin(), data.end());
+    if (icon_effects != apps::IconEffects::kNone) {
+      // TODO(crbug.com/988321): decompress the image, apply icon effects then
+      // re-compress.
+    }
   }
 
   std::move(callback).Run(std::move(iv));
@@ -513,18 +530,17 @@ void ArcApps::LoadIconFromVM(const std::string app_id,
     return;
   }
 
-  // TODO(crbug.com/826982): drop the "kUncompressed only" condition, and
-  // always use the arc_icon_once_loader_, once the ArcAppIcon class supports
-  // providing *compressed* icons (i.e. PNG-encoded bytes, not RGBA pixels).
+  // TODO(crbug.com/826982): drop the "not kUnknown" condition, and
+  // always use the arc_icon_once_loader_.
   //
   // Once that happens, we can delete the rest of this function, the LoadIcon0
   // code, the pending_load_icon_calls_ mechanism and any mention in this file
   // (or its .h) of arc::ConnectionHolder or arc::ConnectionObserver.
-  if (icon_compression == apps::mojom::IconCompression::kUncompressed) {
+  if (icon_compression != apps::mojom::IconCompression::kUnknown) {
     arc_icon_once_loader_.LoadIcon(
-        app_id, size_hint_in_dip,
-        base::BindOnce(&OnArcAppIconCompletelyLoaded, size_hint_in_dip,
-                       icon_effects, std::move(callback)));
+        app_id, size_hint_in_dip, icon_compression,
+        base::BindOnce(&OnArcAppIconCompletelyLoaded, icon_compression,
+                       size_hint_in_dip, icon_effects, std::move(callback)));
     return;
   }
 

chrome/browser/apps/app_service/arc_icon_once_loader.cc

@@ -2,20 +2,21 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#include <utility>
-
 #include "chrome/browser/apps/app_service/arc_icon_once_loader.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/ui/app_list/arc/arc_app_icon.h"
 
 namespace apps {
 
-// A part of an ArcIconOnceLoader, for a specific size_in_dip. This two-level
-// structure (an ArcIconOnceLoader contains multiple SizeSpecificLoader
-// instances) is needed because each ArcAppIcon is for a specific size_in_dip.
+// A part of an ArcIconOnceLoader, for a specific size_in_dip and
+// icon_compression. This two-level structure (an ArcIconOnceLoader contains
+// multiple SizeSpecificLoader instances) is needed because each ArcAppIcon is
+// for a specific size_in_dip and compressed-ness.
 class ArcIconOnceLoader::SizeSpecificLoader : public ArcAppIcon::Observer {
  public:
-  SizeSpecificLoader(Profile* profile, int32_t size_in_dip);
+  SizeSpecificLoader(Profile* profile,
+                     int32_t size_in_dip,
+                     apps::mojom::IconCompression icon_compression);
   ~SizeSpecificLoader() override;
 
   void LoadIcon(const std::string& app_id,
@@ -29,8 +30,10 @@ class ArcIconOnceLoader::SizeSpecificLoader : public ArcAppIcon::Observer {
  private:
   Profile* const profile_;
   const int32_t size_in_dip_;
+  const apps::mojom::IconCompression icon_compression_;
 
-  // Maps App IDs to their icon loaders (for a specific size in DIPs).
+  // Maps App IDs to their icon loaders (for a specific size_in_dip and
+  // icon_compression).
   std::map<std::string, std::unique_ptr<ArcAppIcon>> icons_;
 
   // Maps App IDs to callbacks to run when an icon is completely loaded.
@@ -39,9 +42,13 @@ class ArcIconOnceLoader::SizeSpecificLoader : public ArcAppIcon::Observer {
   DISALLOW_COPY_AND_ASSIGN(SizeSpecificLoader);
 };
 
-ArcIconOnceLoader::SizeSpecificLoader::SizeSpecificLoader(Profile* profile,
-                                                          int32_t size_in_dip)
-    : profile_(profile), size_in_dip_(size_in_dip) {}
+ArcIconOnceLoader::SizeSpecificLoader::SizeSpecificLoader(
+    Profile* profile,
+    int32_t size_in_dip,
+    apps::mojom::IconCompression icon_compression)
+    : profile_(profile),
+      size_in_dip_(size_in_dip),
+      icon_compression_(icon_compression) {}
 
 ArcIconOnceLoader::SizeSpecificLoader::~SizeSpecificLoader() {
   for (auto& kv_pair : callbacks_) {
@@ -64,12 +71,14 @@ void ArcIconOnceLoader::SizeSpecificLoader::LoadIcon(
   if (iter != icons_.end()) {
     return;
   }
+  bool compressed =
+      icon_compression_ == apps::mojom::IconCompression::kCompressed;
   iter = icons_
              .insert(std::make_pair(
-                 app_id, std::make_unique<ArcAppIcon>(profile_, app_id,
-                                                      size_in_dip_, this)))
+                 app_id, std::make_unique<ArcAppIcon>(
+                             profile_, app_id, size_in_dip_, this, compressed)))
              .first;
-  iter->second->image_skia().EnsureRepsForSupportedScales();
+  iter->second->LoadSupportedScaleFactors();
 }
 
 void ArcIconOnceLoader::SizeSpecificLoader::Remove(const std::string& app_id) {
@@ -137,13 +146,15 @@ void ArcIconOnceLoader::StopObserving(ArcAppListPrefs* prefs) {
 void ArcIconOnceLoader::LoadIcon(
     const std::string& app_id,
     int32_t size_in_dip,
+    apps::mojom::IconCompression icon_compression,
     base::OnceCallback<void(ArcAppIcon*)> callback) {
-  auto iter = size_specific_loaders_.find(size_in_dip);
+  auto key = std::make_pair(size_in_dip, icon_compression);
+  auto iter = size_specific_loaders_.find(key);
   if (iter == size_specific_loaders_.end()) {
     iter = size_specific_loaders_
                .insert(std::make_pair(
-                   size_in_dip,
-                   std::make_unique<SizeSpecificLoader>(profile_, size_in_dip)))
+                   key, std::make_unique<SizeSpecificLoader>(
+                            profile_, size_in_dip, icon_compression)))
                .first;
   }
   iter->second->LoadIcon(app_id, std::move(callback));
@@ -158,9 +169,14 @@ void ArcIconOnceLoader::OnAppRemoved(const std::string& app_id) {
 void ArcIconOnceLoader::OnAppIconUpdated(
     const std::string& app_id,
     const ArcAppIconDescriptor& descriptor) {
-  auto iter = size_specific_loaders_.find(descriptor.dip_size);
-  if (iter != size_specific_loaders_.end()) {
-    iter->second->Reload(app_id, descriptor.scale_factor);
+  for (int i = 0; i < 2; i++) {
+    auto icon_compression = i ? apps::mojom::IconCompression::kCompressed
+                              : apps::mojom::IconCompression::kUncompressed;
+    auto iter = size_specific_loaders_.find(
+        std::make_pair(descriptor.dip_size, icon_compression));
+    if (iter != size_specific_loaders_.end()) {
+      iter->second->Reload(app_id, descriptor.scale_factor);
+    }
   }
 }
 

chrome/browser/apps/app_service/arc_icon_once_loader.h

@@ -8,11 +8,13 @@
 #include <map>
 #include <memory>
 #include <string>
+#include <utility>
 
 #include "base/callback_forward.h"
 #include "base/macros.h"
 #include "chrome/browser/ui/app_list/arc/arc_app_icon_descriptor.h"
 #include "chrome/browser/ui/app_list/arc/arc_app_list_prefs.h"
+#include "chrome/services/app_service/public/mojom/types.mojom.h"
 
 class ArcAppIcon;
 class Profile;
@@ -41,6 +43,7 @@ class ArcIconOnceLoader : public ArcAppListPrefs::Observer {
   // loaded.
   void LoadIcon(const std::string& app_id,
                 int32_t size_in_dip,
+                apps::mojom::IconCompression icon_compression,
                 base::OnceCallback<void(ArcAppIcon*)> callback);
 
   // ArcAppListPrefs::Observer overrides.
@@ -51,8 +54,11 @@ class ArcIconOnceLoader : public ArcAppListPrefs::Observer {
  private:
   class SizeSpecificLoader;
 
+  using SizeAndCompression = std::pair<int32_t, apps::mojom::IconCompression>;
+
   Profile* const profile_;
-  std::map<int32_t, std::unique_ptr<SizeSpecificLoader>> size_specific_loaders_;
+  std::map<SizeAndCompression, std::unique_ptr<SizeSpecificLoader>>
+      size_specific_loaders_;
 
   DISALLOW_COPY_AND_ASSIGN(ArcIconOnceLoader);
 };

chrome/browser/ui/app_list/arc/arc_app_icon.cc

@@ -210,13 +210,13 @@ void ArcAppIcon::DecodeRequest::OnImageDecoded(const SkBitmap& bitmap) {
               << "x" << bitmap.height() << ". Expected " << expected_dim << ".";
       host_->MaybeRequestIcon(descriptor_.scale_factor);
     } else {
-      host_->Update(descriptor_.scale_factor,
-                    skia::ImageOperations::Resize(
-                        bitmap, skia::ImageOperations::RESIZE_BEST,
-                        expected_dim, expected_dim));
+      host_->UpdateUncompressed(descriptor_.scale_factor,
+                                skia::ImageOperations::Resize(
+                                    bitmap, skia::ImageOperations::RESIZE_BEST,
+                                    expected_dim, expected_dim));
     }
   } else {
-    host_->Update(descriptor_.scale_factor, bitmap);
+    host_->UpdateUncompressed(descriptor_.scale_factor, bitmap);
   }
 
   host_->DiscardDecodeRequest(this);
@@ -248,17 +248,22 @@ bool ArcAppIcon::IsSafeDecodingDisabledForTesting() {
 ArcAppIcon::ArcAppIcon(content::BrowserContext* context,
                        const std::string& app_id,
                        int resource_size_in_dip,
-                       Observer* observer)
+                       Observer* observer,
+                       bool serve_compressed_icons)
     : context_(context),
       app_id_(app_id),
       mapped_app_id_(GetAppFromAppOrGroupId(context, app_id)),
       resource_size_in_dip_(resource_size_in_dip),
-      observer_(observer) {
+      observer_(observer),
+      serve_compressed_icons_(serve_compressed_icons) {
   CHECK(observer_ != nullptr);
-  auto source = std::make_unique<Source>(weak_ptr_factory_.GetWeakPtr(),
-                                         resource_size_in_dip);
-  gfx::Size resource_size(resource_size_in_dip, resource_size_in_dip);
-  image_skia_ = gfx::ImageSkia(std::move(source), resource_size);
+
+  if (!serve_compressed_icons_) {
+    auto source = std::make_unique<Source>(weak_ptr_factory_.GetWeakPtr(),
+                                           resource_size_in_dip);
+    gfx::Size resource_size(resource_size_in_dip, resource_size_in_dip);
+    image_skia_ = gfx::ImageSkia(std::move(source), resource_size);
+  }
 
   const std::vector<ui::ScaleFactor>& scale_factors =
       ui::GetSupportedScaleFactors();
@@ -268,6 +273,19 @@ ArcAppIcon::ArcAppIcon(content::BrowserContext* context,
 ArcAppIcon::~ArcAppIcon() {
 }
 
+void ArcAppIcon::LoadSupportedScaleFactors() {
+  if (serve_compressed_icons_) {
+    for (auto scale_factor : incomplete_scale_factors_)
+      LoadForScaleFactor(scale_factor);
+  } else {
+    // Calling GetRepresentation indirectly calls LoadForScaleFactor but also
+    // first initializes image_skia_ with the placeholder icons (e.g.
+    // IDR_APP_DEFAULT_ICON), via ArcAppIcon::Source::GetImageForScale.
+    for (auto scale_factor : incomplete_scale_factors_)
+      image_skia_.GetRepresentation(ui::GetScaleForScaleFactor(scale_factor));
+  }
+}
+
 bool ArcAppIcon::EverySupportedScaleFactorIsLoaded() const {
   return incomplete_scale_factors_.empty();
 }
@@ -360,6 +378,12 @@ void ArcAppIcon::OnIconRead(
     MaybeRequestIcon(read_result->scale_factor);
 
   if (!read_result->unsafe_icon_data.empty()) {
+    if (serve_compressed_icons_) {
+      UpdateCompressed(read_result->scale_factor,
+                       std::move(read_result->unsafe_icon_data));
+      return;
+    }
+
     decode_requests_.emplace_back(std::make_unique<DecodeRequest>(
         weak_ptr_factory_.GetWeakPtr(),
         ArcAppIconDescriptor(resource_size_in_dip_, read_result->scale_factor),
@@ -383,7 +407,8 @@ void ArcAppIcon::OnIconRead(
   }
 }
 
-void ArcAppIcon::Update(ui::ScaleFactor scale_factor, const SkBitmap& bitmap) {
+void ArcAppIcon::UpdateUncompressed(ui::ScaleFactor scale_factor,
+                                    const SkBitmap& bitmap) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
 
   gfx::ImageSkiaRep image_rep(bitmap, ui::GetScaleForScaleFactor(scale_factor));
@@ -398,6 +423,14 @@ void ArcAppIcon::Update(ui::ScaleFactor scale_factor, const SkBitmap& bitmap) {
   observer_->OnIconUpdated(this);
 }
 
+void ArcAppIcon::UpdateCompressed(ui::ScaleFactor scale_factor,
+                                  std::string data) {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+  compressed_images_[scale_factor] = std::move(data);
+  incomplete_scale_factors_.erase(scale_factor);
+  observer_->OnIconUpdated(this);
+}
+
 void ArcAppIcon::DiscardDecodeRequest(DecodeRequest* request) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
 

chrome/browser/ui/app_list/arc/arc_app_icon.h

@@ -5,6 +5,7 @@
 #ifndef CHROME_BROWSER_UI_APP_LIST_ARC_ARC_APP_ICON_H_
 #define CHROME_BROWSER_UI_APP_LIST_ARC_ARC_APP_ICON_H_
 
+#include <map>
 #include <memory>
 #include <set>
 #include <string>
@@ -51,15 +52,30 @@ class ArcAppIcon {
   ArcAppIcon(content::BrowserContext* context,
              const std::string& app_id,
              int resource_size_in_dip,
-             Observer* observer);
+             Observer* observer,
+             bool serve_compressed_icons = false);
   ~ArcAppIcon();
 
+  // Starts loading the icon at every supported scale factor. The |observer_|
+  // will be notified as progress is made. "Supported" is in the same sense as
+  // ui::GetSupportedScaleFactors().
+  void LoadSupportedScaleFactors();
+
   // Whether every supported scale factor was successfully loaded. "Supported"
   // is in the same sense as ui::GetSupportedScaleFactors().
   bool EverySupportedScaleFactorIsLoaded() const;
 
   const std::string& app_id() const { return app_id_; }
-  const gfx::ImageSkia& image_skia() const { return image_skia_; }
+  // Valid if the |serve_compressed_icons_| is false.
+  const gfx::ImageSkia& image_skia() const {
+    DCHECK(!serve_compressed_icons_);
+    return image_skia_;
+  }
+  // Valid if the |serve_compressed_icons_| is true.
+  const std::map<ui::ScaleFactor, std::string>& compressed_images() const {
+    DCHECK(serve_compressed_icons_);
+    return compressed_images_;
+  }
 
   // Disables async safe decoding requests when unit tests are executed. This is
   // done to avoid two problems. Problems come because icons are decoded at a
@@ -105,7 +121,8 @@ class ArcAppIcon {
       const base::FilePath& path,
       const base::FilePath& default_app_path);
   void OnIconRead(std::unique_ptr<ArcAppIcon::ReadResult> read_result);
-  void Update(ui::ScaleFactor scale_factor, const SkBitmap& bitmap);
+  void UpdateUncompressed(ui::ScaleFactor scale_factor, const SkBitmap& bitmap);
+  void UpdateCompressed(ui::ScaleFactor scale_factor, std::string data);
   void DiscardDecodeRequest(DecodeRequest* request);
 
   content::BrowserContext* const context_;
@@ -115,8 +132,10 @@ class ArcAppIcon {
   const std::string mapped_app_id_;
   const int resource_size_in_dip_;
   Observer* const observer_;
+  const bool serve_compressed_icons_;
 
   gfx::ImageSkia image_skia_;
+  std::map<ui::ScaleFactor, std::string> compressed_images_;
   std::set<ui::ScaleFactor> incomplete_scale_factors_;
 
   // Contains pending image decode requests.

chrome/browser/ui/app_list/arc/arc_app_icon_loader.cc

@@ -36,7 +36,7 @@ void ArcAppIconLoader::FetchImage(const std::string& app_id) {
   // |icon_size_in_dip_| differs from this size, re-scale is required.
   std::unique_ptr<ArcAppIcon> icon =
       std::make_unique<ArcAppIcon>(profile(), app_id, icon_size_in_dip(), this);
-  icon->image_skia().EnsureRepsForSupportedScales();
+  icon->LoadSupportedScaleFactors();
   icon_map_[app_id] = std::move(icon);
   UpdateImage(app_id);
 }