Show icons for Crostini Apps in the File Manager

This patch adds support for app icons when used as file handlers in the
file manager. As there is no place to store the CrostiniAppIcon
objects, a IconLoadWaiter is added which manually manages it's lifetime
and deletes itself after an icon is loaded (or fails to load).

This is a re-land of https://crrev.com/c/1127204, but with the longer
deletion time-out removed. It turns out the longer time-out is not
needed, and it was causing LSAN to detect a leak as the objects were
not cleaned up by test completion.

BUG=822513

Change-Id: Id04730ee2ea18f6f5004624471e40bcd13e9974b
Reviewed-on: https://chromium-review.googlesource.com/1149432Reviewed-by: Nicholas Verne <nverne@chromium.org>
Reviewed-by: Joel Hockey <joelhockey@chromium.org>
Commit-Queue: Timothy Loh <timloh@chromium.org>
Cr-Commit-Position: refs/heads/master@{#577822}

chrome/browser/chromeos/crostini/crostini_registry_service.cc

@@ -349,6 +349,10 @@ CrostiniRegistryService::CrostiniRegistryService(Profile* profile)
 
 CrostiniRegistryService::~CrostiniRegistryService() = default;
 
+base::WeakPtr<CrostiniRegistryService> CrostiniRegistryService::GetWeakPtr() {
+  return weak_ptr_factory_.GetWeakPtr();
+}
+
 // The code follows these steps to identify apps and returns the first match:
 // 1) Ignore windows if the App Id is prefixed by org.chromium.arc.
 // 2) If the Startup Id is set, look for a matching desktop file id.

chrome/browser/chromeos/crostini/crostini_registry_service.h

@@ -124,6 +124,8 @@ class CrostiniRegistryService : public KeyedService {
   explicit CrostiniRegistryService(Profile* profile);
   ~CrostiniRegistryService() override;
 
+  base::WeakPtr<CrostiniRegistryService> GetWeakPtr();
+
   // Returns a shelf app id for an exo window startup id or app id.
   //
   // First try to return a desktop file id matching the |window_startup_id|.

chrome/browser/chromeos/crostini/crostini_util.cc

@@ -9,6 +9,8 @@
 #include "base/files/file_path.h"
 #include "base/metrics/histogram_functions.h"
 #include "base/strings/string_util.h"
+#include "base/task_scheduler/post_task.h"
+#include "base/timer/timer.h"
 #include "chrome/browser/chromeos/crostini/crostini_app_launch_observer.h"
 #include "chrome/browser/chromeos/crostini/crostini_manager.h"
 #include "chrome/browser/chromeos/crostini/crostini_pref_names.h"
@@ -17,6 +19,7 @@
 #include "chrome/browser/chromeos/profiles/profile_helper.h"
 #include "chrome/browser/chromeos/virtual_machines/virtual_machines_util.h"
 #include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/ui/app_list/crostini/crostini_app_icon.h"
 #include "chrome/browser/ui/ash/launcher/chrome_launcher_controller.h"
 #include "chrome/browser/ui/ash/launcher/shelf_spinner_controller.h"
 #include "chrome/browser/ui/ash/launcher/shelf_spinner_item_controller.h"
@@ -116,6 +119,84 @@ void LaunchContainerApplication(
       base::BindOnce(OnContainerApplicationLaunched, app_id));
 }
 
+// Helper class for loading icons. The callback is called when all icons have
+// been loaded, or after a provided timeout, after which the object deletes
+// itself.
+// TODO(timloh): We should consider having a service, so multiple requests for
+// the same icon won't load the same image multiple times and only the first
+// request would incur the loading delay.
+class IconLoadWaiter : public CrostiniAppIcon::Observer {
+ public:
+  static void LoadIcons(
+      Profile* profile,
+      const std::vector<std::string>& app_ids,
+      int resource_size_in_dip,
+      ui::ScaleFactor scale_factor,
+      base::TimeDelta timeout,
+      base::OnceCallback<void(const std::vector<gfx::ImageSkia>&)> callback) {
+    new IconLoadWaiter(profile, app_ids, resource_size_in_dip, scale_factor,
+                       timeout, std::move(callback));
+  }
+
+ private:
+  IconLoadWaiter(
+      Profile* profile,
+      const std::vector<std::string>& app_ids,
+      int resource_size_in_dip,
+      ui::ScaleFactor scale_factor,
+      base::TimeDelta timeout,
+      base::OnceCallback<void(const std::vector<gfx::ImageSkia>&)> callback)
+      : callback_(std::move(callback)) {
+    for (const std::string& app_id : app_ids) {
+      icons_.push_back(std::make_unique<CrostiniAppIcon>(
+          profile, app_id, resource_size_in_dip, this));
+      icons_.back()->LoadForScaleFactor(scale_factor);
+    }
+
+    timeout_timer_.Start(FROM_HERE, timeout, this,
+                         &IconLoadWaiter::RunCallback);
+  }
+
+  // TODO(timloh): This is only called when an icon is found, so if any of the
+  // requested apps are missing an icon, we'll have to wait for the timeout. We
+  // should add an interface so we can avoid this.
+  void OnIconUpdated(CrostiniAppIcon* icon) override {
+    loaded_icons_++;
+    if (loaded_icons_ != icons_.size())
+      return;
+
+    timeout_timer_.AbandonAndStop();
+    RunCallback();
+  }
+
+  void Delete() {
+    DCHECK(!timeout_timer_.IsRunning());
+    delete this;
+  }
+
+  void RunCallback() {
+    DCHECK(callback_);
+    std::vector<gfx::ImageSkia> result;
+    for (const auto& icon : icons_)
+      result.emplace_back(icon->image_skia());
+    std::move(callback_).Run(result);
+
+    // If we're running the callback as loading has finished, we can't delete
+    // ourselves yet as it would destroy the CrostiniAppIcon which is calling
+    // into us right now.
+    base::SequencedTaskRunnerHandle::Get()->PostTask(
+        FROM_HERE,
+        base::BindOnce(&IconLoadWaiter::Delete, base::Unretained(this)));
+  }
+
+  std::vector<std::unique_ptr<CrostiniAppIcon>> icons_;
+  size_t loaded_icons_ = 0;
+
+  base::OneShotTimer timeout_timer_;
+
+  base::OnceCallback<void(const std::vector<gfx::ImageSkia>&)> callback_;
+};
+
 }  // namespace
 
 void SetCrostiniUIAllowedForTesting(bool enabled) {
@@ -236,6 +317,18 @@ void LaunchCrostiniApp(Profile* profile,
                      std::move(launch_closure)));
 }
 
+void LoadIcons(Profile* profile,
+               const std::vector<std::string>& app_ids,
+               int resource_size_in_dip,
+               ui::ScaleFactor scale_factor,
+               base::TimeDelta timeout,
+               base::OnceCallback<void(const std::vector<gfx::ImageSkia>&)>
+                   icons_loaded_callback) {
+  IconLoadWaiter::LoadIcons(profile, app_ids, resource_size_in_dip,
+                            scale_factor, timeout,
+                            std::move(icons_loaded_callback));
+}
+
 std::string CryptohomeIdForProfile(Profile* profile) {
   std::string id = chromeos::ProfileHelper::GetUserIdHashFromProfile(profile);
   // Empty id means we're running in a test.

chrome/browser/chromeos/crostini/crostini_util.h

@@ -8,11 +8,17 @@
 #include <string>
 
 #include "base/optional.h"
+#include "ui/base/resource/scale_factor.h"
 
 namespace base {
 class FilePath;
+class TimeDelta;
 }  // namespace base
 
+namespace gfx {
+class ImageSkia;
+}  // namespace gfx
+
 class Profile;
 
 // Enables/disables overriding IsCrostiniUIAllowedForProfile's normal
@@ -49,6 +55,17 @@ void LaunchCrostiniApp(Profile* profile,
                        int64_t display_id,
                        const std::vector<std::string>& files);
 
+// Convenience wrapper around CrostiniAppIconLoader. As requesting icons from
+// the container can be slow, we just use the default (penguin) icons after the
+// timeout elapses. Subsequent calls would get the correct icons once loaded.
+void LoadIcons(Profile* profile,
+               const std::vector<std::string>& app_ids,
+               int resource_size_in_dip,
+               ui::ScaleFactor scale_factor,
+               base::TimeDelta timeout,
+               base::OnceCallback<void(const std::vector<gfx::ImageSkia>&)>
+                   icons_loaded_callback);
+
 // Retrieves cryptohome_id from profile.
 std::string CryptohomeIdForProfile(Profile* profile);
 

chrome/browser/chromeos/file_manager/crostini_file_tasks.cc

@@ -31,6 +31,52 @@
 namespace file_manager {
 namespace file_tasks {
 
+namespace {
+
+constexpr base::TimeDelta kIconLoadTimeout =
+    base::TimeDelta::FromMilliseconds(100);
+constexpr size_t kIconSizeInDip = 16;
+
+GURL GeneratePNGDataUrl(const SkBitmap& sk_bitmap) {
+  std::vector<unsigned char> output;
+  gfx::PNGCodec::EncodeBGRASkBitmap(sk_bitmap, false /* discard_transparency */,
+                                    &output);
+  std::string encoded;
+  base::Base64Encode(
+      base::StringPiece(reinterpret_cast<const char*>(output.data()),
+                        output.size()),
+      &encoded);
+  return GURL("data:image/png;base64," + encoded);
+}
+
+void OnAppIconsLoaded(Profile* profile,
+                      const std::vector<std::string>& app_ids,
+                      ui::ScaleFactor scale_factor,
+                      std::vector<FullTaskDescriptor>* result_list,
+                      base::OnceClosure completion_closure,
+                      const std::vector<gfx::ImageSkia>& icons) {
+  DCHECK(!app_ids.empty());
+  DCHECK_EQ(app_ids.size(), icons.size());
+
+  float scale = ui::GetScaleForScaleFactor(scale_factor);
+
+  crostini::CrostiniRegistryService* registry_service =
+      crostini::CrostiniRegistryServiceFactory::GetForProfile(profile);
+  for (size_t i = 0; i < app_ids.size(); ++i) {
+    result_list->push_back(FullTaskDescriptor(
+        TaskDescriptor(app_ids[i], TASK_TYPE_CROSTINI_APP,
+                       kCrostiniAppActionID),
+        registry_service->GetRegistration(app_ids[i])->Name(),
+        extensions::api::file_manager_private::Verb::VERB_OPEN_WITH,
+        GeneratePNGDataUrl(icons[i].GetRepresentation(scale).sk_bitmap()),
+        false /* is_default */, false /* is_generic */));
+  }
+
+  std::move(completion_closure).Run();
+}
+
+}  // namespace
+
 void FindCrostiniTasks(Profile* profile,
                        const std::vector<extensions::EntryInfo>& entries,
                        std::vector<FullTaskDescriptor>* result_list,
@@ -44,6 +90,8 @@ void FindCrostiniTasks(Profile* profile,
   for (const extensions::EntryInfo& entry : entries)
     target_mime_types.insert(entry.mime_type);
 
+  std::vector<std::string> result_app_ids;
+
   crostini::CrostiniRegistryService* registry_service =
       crostini::CrostiniRegistryServiceFactory::GetForProfile(profile);
   for (const std::string& app_id : registry_service->GetRegisteredAppIds()) {
@@ -62,16 +110,20 @@ void FindCrostiniTasks(Profile* profile,
     }
     if (had_unsupported_mime_type)
       continue;
+    result_app_ids.push_back(app_id);
+  }
 
-    // TODO(timloh): Add support for Crostini icons
-    result_list->push_back(FullTaskDescriptor(
-        TaskDescriptor(app_id, TASK_TYPE_CROSTINI_APP, kCrostiniAppActionID),
-        registration.Name(),
-        extensions::api::file_manager_private::Verb::VERB_OPEN_WITH, GURL(),
-        false /* is_default */, false /* is_generic */));
+  if (result_app_ids.empty()) {
+    std::move(completion_closure).Run();
+    return;
   }
 
-  std::move(completion_closure).Run();
+  ui::ScaleFactor scale_factor = ui::GetSupportedScaleFactors().back();
+
+  LoadIcons(
+      profile, result_app_ids, kIconSizeInDip, scale_factor, kIconLoadTimeout,
+      base::BindOnce(OnAppIconsLoaded, profile, result_app_ids, scale_factor,
+                     result_list, std::move(completion_closure)));
 }
 
 void ExecuteCrostiniTask(

chrome/browser/ui/app_list/crostini/crostini_app_icon.cc

@@ -194,14 +194,12 @@ void CrostiniAppIcon::DecodeRequest::OnImageDecoded(const SkBitmap& bitmap) {
         resized_image, false /* discard_transparency */, &png_data);
   }
   if (encode_result) {
-    // Now save this so we can reload it later when needed.
-    crostini::CrostiniRegistryService* registry_service =
-        crostini::CrostiniRegistryServiceFactory::GetForProfile(
-            host_->profile());
-    DCHECK(registry_service);
+    if (!host_->registry_service_)
+      return;
 
+    // Now save this so we can reload it later when needed.
     const base::FilePath path =
-        registry_service->GetIconPath(host_->app_id(), scale_factor_);
+        host_->registry_service_->GetIconPath(host_->app_id(), scale_factor_);
     DCHECK(!path.empty());
     base::PostTaskWithTraits(
         FROM_HERE, {base::MayBlock(), base::TaskPriority::BACKGROUND},
@@ -231,7 +229,9 @@ CrostiniAppIcon::CrostiniAppIcon(Profile* profile,
                                  const std::string& app_id,
                                  int resource_size_in_dip,
                                  Observer* observer)
-    : profile_(profile),
+    : registry_service_(
+          crostini::CrostiniRegistryServiceFactory::GetForProfile(profile)
+              ->GetWeakPtr()),
       app_id_(app_id),
       resource_size_in_dip_(resource_size_in_dip),
       observer_(observer),
@@ -246,12 +246,10 @@ CrostiniAppIcon::CrostiniAppIcon(Profile* profile,
 CrostiniAppIcon::~CrostiniAppIcon() = default;
 
 void CrostiniAppIcon::LoadForScaleFactor(ui::ScaleFactor scale_factor) {
-  crostini::CrostiniRegistryService* registry_service =
-      crostini::CrostiniRegistryServiceFactory::GetForProfile(profile_);
-  DCHECK(registry_service);
+  DCHECK(registry_service_);
 
   const base::FilePath path =
-      registry_service->GetIconPath(app_id_, scale_factor);
+      registry_service_->GetIconPath(app_id_, scale_factor);
   DCHECK(!path.empty());
 
   base::PostTaskWithTraitsAndReplyWithResult(
@@ -263,14 +261,14 @@ void CrostiniAppIcon::LoadForScaleFactor(ui::ScaleFactor scale_factor) {
 
 void CrostiniAppIcon::MaybeRequestIcon(ui::ScaleFactor scale_factor) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
-  crostini::CrostiniRegistryService* registry_service =
-      crostini::CrostiniRegistryServiceFactory::GetForProfile(profile_);
-  DCHECK(registry_service);
+  // Fail safely if the icon outlives the Profile (and the Crostini Registry).
+  if (!registry_service_)
+    return;
 
   // CrostiniRegistryService notifies CrostiniAppModelBuilder via Observer when
   // icon is ready and CrostiniAppModelBuilder refreshes the icon of the
   // corresponding item by calling LoadScaleFactor.
-  registry_service->MaybeRequestIcon(app_id_, scale_factor);
+  registry_service_->MaybeRequestIcon(app_id_, scale_factor);
 }
 
 // static

chrome/browser/ui/app_list/crostini/crostini_app_icon.h

@@ -19,6 +19,10 @@ namespace base {
 class FilePath;
 }
 
+namespace crostini {
+class CrostiniRegistryService;
+}
+
 class Profile;
 
 // A class that provides an ImageSkia for UI code to use. It handles Crostini
@@ -44,7 +48,6 @@ class CrostiniAppIcon {
 
   const std::string& app_id() const { return app_id_; }
   const gfx::ImageSkia& image_skia() const { return image_skia_; }
-  Profile* profile() const { return profile_; }
 
   // Icon loading is performed in several steps. It is initiated by
   // LoadImageForScaleFactor request that specifies a required scale factor.
@@ -82,7 +85,7 @@ class CrostiniAppIcon {
   // Removed the corresponding |request| from our list.
   void DiscardDecodeRequest(DecodeRequest* request);
 
-  Profile* profile_;
+  base::WeakPtr<crostini::CrostiniRegistryService> registry_service_;
   const std::string app_id_;
   const int resource_size_in_dip_;
   Observer* const observer_;