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).

BUG=822513

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

chrome/browser/chromeos/crostini/crostini_registry_service.cc

@@ -334,6 +334,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,9 @@
 #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/time/time.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 +20,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 +120,96 @@ 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. The waiter deletes itself after
+// all icons have been loaded, or a longer timeout. This longer timeout allows
+// icons that were loading to finish and save to disk if they retrieved an icon
+// from the container, while also ensuring these do not leak if the icons fail
+// to be retrieved.
+// TODO(timloh): This feels like a hack. We should consider having a service,
+// so multiple requests for the same icon won't load the same image multiple
+// times.
+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);
+  }
+
+  void OnIconUpdated(CrostiniAppIcon* icon) override {
+    loaded_icons_++;
+    if (!FinishedLoading())
+      return;
+
+    timeout_timer_.AbandonAndStop();
+    deletion_timer_.AbandonAndStop();
+    if (callback_)
+      RunCallback();
+    // Don't immediately delete 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)));
+  }
+
+  bool FinishedLoading() { return loaded_icons_ == icons_.size(); }
+
+  void Delete() {
+    DCHECK(!timeout_timer_.IsRunning());
+    DCHECK(!deletion_timer_.IsRunning());
+    delete this;
+  }
+
+  void RunCallback() {
+    std::vector<gfx::ImageSkia> result;
+    for (const auto& icon : icons_)
+      result.emplace_back(icon->image_skia());
+    std::move(callback_).Run(result);
+
+    if (!FinishedLoading()) {
+      // Leave this object alive for a bit longer so we can finish up requests.
+      deletion_timer_.Start(
+          FROM_HERE, base::TimeDelta::FromSeconds(kDeletionTimeoutInSeconds),
+          this, &IconLoadWaiter::Delete);
+    }
+  }
+
+  static constexpr int kDeletionTimeoutInSeconds = 10;
+
+  std::vector<std::unique_ptr<CrostiniAppIcon>> icons_;
+  size_t loaded_icons_ = 0;
+
+  base::OneShotTimer timeout_timer_;
+  base::OneShotTimer deletion_timer_;
+
+  base::OnceCallback<void(const std::vector<gfx::ImageSkia>&)> callback_;
+};
+
 }  // namespace
 
 void SetCrostiniUIAllowedForTesting(bool enabled) {
@@ -236,6 +330,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_;