Fix crostini handling of allow_placeholder_icon

Currently the crostini app icon handler returns an error when
|allow_placeholder_icon| is true. This should instead cause the
handler to refraim from returning any value until the real icon has
been loaded from the VM.

Bug: 1088393
Change-Id: I6dcb874193ccea91fd539c633940673275466835
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2226106
Commit-Queue: Fergus Dall <sidereal@google.com>
Reviewed-by: Dominick Ng <dominickn@chromium.org>
Cr-Commit-Position: refs/heads/master@{#779246}

chrome/browser/apps/app_service/app_icon_factory.cc

@@ -292,6 +292,7 @@ class IconLoadingPipeline : public base::RefCounted<IconLoadingPipeline> {
 
   // The image file must be compressed using the default encoding.
   void LoadCompressedIconFromFile(const base::FilePath& path);
+  void LoadIconFromCompressedData(const std::string& compressed_icon_data);
 
   void LoadIconFromResource(int icon_resource);
 
@@ -439,6 +440,17 @@ void IconLoadingPipeline::LoadCompressedIconFromFile(
                          base::WrapRefCounted(this))));
 }
 
+void IconLoadingPipeline::LoadIconFromCompressedData(
+    const std::string& compressed_icon_data) {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+  std::vector<uint8_t> data(compressed_icon_data.begin(),
+                            compressed_icon_data.end());
+  CompressedDataToImageSkiaCallback(
+      base::BindOnce(&IconLoadingPipeline::MaybeApplyEffectsAndComplete,
+                     base::WrapRefCounted(this)))
+      .Run(std::move(data));
+}
+
 void IconLoadingPipeline::LoadIconFromResource(int icon_resource) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
   if (icon_resource == kInvalidIconResource) {
@@ -716,6 +728,22 @@ void LoadIconFromFileWithFallback(
   icon_loader->LoadCompressedIconFromFile(path);
 }
 
+void LoadIconFromCompressedData(
+    apps::mojom::IconCompression icon_compression,
+    int size_hint_in_dip,
+    IconEffects icon_effects,
+    const std::string& compressed_icon_data,
+    apps::mojom::Publisher::LoadIconCallback callback) {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+  constexpr bool is_placeholder_icon = false;
+
+  scoped_refptr<IconLoadingPipeline> icon_loader =
+      base::MakeRefCounted<IconLoadingPipeline>(
+          icon_compression, size_hint_in_dip, is_placeholder_icon, icon_effects,
+          kInvalidIconResource, std::move(callback));
+  icon_loader->LoadIconFromCompressedData(compressed_icon_data);
+}
+
 void LoadIconFromResource(apps::mojom::IconCompression icon_compression,
                           int size_hint_in_dip,
                           int resource_id,

chrome/browser/apps/app_service/app_icon_factory.h

@@ -81,6 +81,14 @@ void LoadIconFromFileWithFallback(
     base::OnceCallback<void(apps::mojom::Publisher::LoadIconCallback)>
         fallback);
 
+// Creates an icon with the specified effects from |compressed_icon_data|.
+void LoadIconFromCompressedData(
+    apps::mojom::IconCompression icon_compression,
+    int size_hint_in_dip,
+    IconEffects icon_effects,
+    const std::string& compressed_icon_data,
+    apps::mojom::Publisher::LoadIconCallback callback);
+
 // Loads an icon from a compiled-into-the-binary resource, with a resource_id
 // named IDR_XXX, for some value of XXX.
 void LoadIconFromResource(apps::mojom::IconCompression icon_compression,

chrome/browser/apps/app_service/crostini_apps.cc

@@ -19,6 +19,7 @@
 #include "chrome/grit/chrome_unscaled_resources.h"
 #include "chrome/grit/generated_resources.h"
 #include "components/prefs/pref_change_registrar.h"
+#include "mojo/public/cpp/bindings/callback_helpers.h"
 #include "ui/display/display.h"
 #include "ui/display/screen.h"
 #include "ui/strings/grit/ui_strings.h"
@@ -244,12 +245,26 @@ void CrostiniApps::OnRegistryUpdated(
 }
 
 void CrostiniApps::OnAppIconUpdated(const std::string& app_id,
-                                    ui::ScaleFactor scale_factor) {
+                                    ui::ScaleFactor scale_factor,
+                                    const std::string& compressed_icon_data) {
   apps::mojom::AppPtr app = apps::mojom::App::New();
   app->app_type = apps::mojom::AppType::kCrostini;
   app->app_id = app_id;
   app->icon_key = NewIconKey(app_id);
-  Publish(std::move(app), subscribers_);
+
+  auto range = app_icon_callbacks_.equal_range(app_id);
+  for (auto it = range.first; it != range.second; it++) {
+    apps::mojom::IconCompression icon_compression;
+    int32_t size_hint_in_dip;
+    IconEffects icon_effects;
+    LoadIconCallback callback;
+    std::tie(icon_compression, size_hint_in_dip, icon_effects, callback) =
+        std::move(it->second);
+
+    LoadIconFromCompressedData(icon_compression, size_hint_in_dip, icon_effects,
+                               compressed_icon_data, std::move(callback));
+  }
+  app_icon_callbacks_.erase(range.first, range.second);
 }
 
 void CrostiniApps::OnCrostiniEnabledChanged() {
@@ -276,29 +291,31 @@ void CrostiniApps::LoadIconFromVM(const std::string app_id,
                                   ui::ScaleFactor scale_factor,
                                   IconEffects icon_effects,
                                   LoadIconCallback callback) {
-  if (!allow_placeholder_icon) {
-    // Treat this as failure. We still run the callback, with a nullptr to
-    // indicate failure.
-    std::move(callback).Run(nullptr);
-    return;
+  if (allow_placeholder_icon) {
+    // If a placeholder icon is allowed, pass back the crostini penguin while we
+    // load the real icon from the VM.
+    constexpr bool is_placeholder_icon = true;
+    LoadIconFromResource(icon_compression, size_hint_in_dip,
+                         IDR_LOGO_CROSTINI_DEFAULT_192, is_placeholder_icon,
+                         icon_effects, std::move(callback));
+  } else {
+    // If we don't pass back a fallback icon, we need to store the callback to
+    // use later.
+    LoadIconCallback wrapped_callback =
+        mojo::WrapCallbackWithDefaultInvokeIfNotRun(
+            std::move(callback), apps::mojom::IconValue::New());
+    app_icon_callbacks_.emplace(
+        app_id,
+        std::forward_as_tuple(std::move(icon_compression), size_hint_in_dip,
+                              icon_effects, std::move(wrapped_callback)));
   }
 
-  // Provide a placeholder icon.
-  constexpr bool is_placeholder_icon = true;
-  LoadIconFromResource(icon_compression, size_hint_in_dip,
-                       IDR_LOGO_CROSTINI_DEFAULT_192, is_placeholder_icon,
-                       icon_effects, std::move(callback));
-
   // Ask the VM to load the icon (and write a cached copy to the file system).
   // The "Maybe" is because multiple requests for the same icon will be merged,
-  // calling OnAppIconUpdated only once. In OnAppIconUpdated, we'll publish a
-  // new IconKey, and subscribers can re-schedule new LoadIcon calls, with new
+  // calling OnAppIconUpdated only once. In OnAppIconUpdated, the cached
+  // callbacks will be run, we'll publish a new IconKey, and subscribers who
+  // received a placeholder icon can re-schedule new LoadIcon calls, with new
   // LoadIconCallback's, that will pick up that cached copy.
-  //
-  // TODO(crbug.com/826982): add a safeguard to prevent an infinite loop where
-  // OnAppIconUpdated somehow doesn't write the cached icon file where we
-  // expect, leading to another MaybeRequestIcon call, leading to another
-  // OnAppIconUpdated call, leading to another MaybeRequestIcon call, etc.
   registry_->MaybeRequestIcon(app_id, scale_factor);
 }
 

chrome/browser/apps/app_service/crostini_apps.h

@@ -78,7 +78,8 @@ class CrostiniApps : public KeyedService,
       const std::vector<std::string>& removed_apps,
       const std::vector<std::string>& inserted_apps) override;
   void OnAppIconUpdated(const std::string& app_id,
-                        ui::ScaleFactor scale_factor) override;
+                        ui::ScaleFactor scale_factor,
+                        const std::string& compressed_icon_data) override;
 
   // Registers and unregisters terminal with AppService.
   // TODO(crbug.com/1028898): Move this code into System Apps
@@ -111,6 +112,14 @@ class CrostiniApps : public KeyedService,
 
   bool crostini_enabled_;
 
+  // A map from app_ids to callbacks waiting on those apps to load an icon.
+  std::multimap<std::string,
+                std::tuple<apps::mojom::IconCompression,
+                           int32_t,
+                           IconEffects,
+                           LoadIconCallback>>
+      app_icon_callbacks_;
+
   base::WeakPtrFactory<CrostiniApps> weak_ptr_factory_{this};
 
   DISALLOW_COPY_AND_ASSIGN(CrostiniApps);

chrome/browser/chromeos/guest_os/guest_os_registry_service.cc

@@ -899,17 +899,20 @@ void GuestOsRegistryService::OnContainerAppIcon(
       FROM_HERE, {base::MayBlock(), base::TaskPriority::BEST_EFFORT},
       base::BindOnce(&InstallIconFromFileThread, icon_path, icons[0].content),
       base::BindOnce(&GuestOsRegistryService::OnIconInstalled,
-                     weak_ptr_factory_.GetWeakPtr(), app_id, scale_factor));
+                     weak_ptr_factory_.GetWeakPtr(), app_id, scale_factor,
+                     icons[0].content));
 }
 
-void GuestOsRegistryService::OnIconInstalled(const std::string& app_id,
-                                             ui::ScaleFactor scale_factor,
-                                             bool success) {
+void GuestOsRegistryService::OnIconInstalled(
+    const std::string& app_id,
+    ui::ScaleFactor scale_factor,
+    const std::string& compressed_icon_data,
+    bool success) {
   if (!success)
     return;
 
   for (Observer& obs : observers_)
-    obs.OnAppIconUpdated(app_id, scale_factor);
+    obs.OnAppIconUpdated(app_id, scale_factor, compressed_icon_data);
 }
 
 void GuestOsRegistryService::MigrateTerminal() const {

chrome/browser/chromeos/guest_os/guest_os_registry_service.h

@@ -127,7 +127,8 @@ class GuestOsRegistryService : public KeyedService {
     // Called when an icon has been installed for the specified app so loading
     // of that icon should be requested again.
     virtual void OnAppIconUpdated(const std::string& app_id,
-                                  ui::ScaleFactor scale_factor) {}
+                                  ui::ScaleFactor scale_factor,
+                                  const std::string& compressed_icon_data) {}
 
    protected:
     virtual ~Observer() = default;
@@ -203,6 +204,7 @@ class GuestOsRegistryService : public KeyedService {
   // Callback for our internal call for saving out icon data.
   void OnIconInstalled(const std::string& app_id,
                        ui::ScaleFactor scale_factor,
+                       const std::string& compressed_icon_data,
                        bool success);
   // Removes all the icons installed for an application.
   void RemoveAppData(const std::string& app_id);