Refactor KioskAppIconLoader to accept a callback instead of a Delegate

Change-Id: Icc3a6cc7f8bdea073a830e0001344f4166189bc7
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4161704
Reviewed-by: Jeroen Dhollander <jeroendh@google.com>
Reviewed-by: Roland Bock <rbock@google.com>
Commit-Queue: Ben Franz <bfranz@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1095567}

chrome/browser/ash/app_mode/arc/arc_kiosk_app_data.cc

@@ -7,10 +7,8 @@
 #include <utility>
 
 #include "base/logging.h"
-#include "base/path_service.h"
 #include "chrome/browser/ash/app_mode/arc/arc_kiosk_app_manager.h"
 #include "chrome/browser/browser_process.h"
-#include "chrome/common/chrome_paths.h"
 #include "components/prefs/pref_service.h"
 #include "components/prefs/scoped_user_pref_update.h"
 #include "content/public/browser/browser_thread.h"
@@ -44,7 +42,13 @@ bool ArcKioskAppData::LoadFromCache() {
   PrefService* local_state = g_browser_process->local_state();
   const base::Value::Dict& dict = local_state->GetDict(dictionary_name());
 
-  return LoadFromDictionary(dict);
+  if (!LoadFromDictionary(dict)) {
+    return false;
+  }
+
+  DecodeIcon(base::BindOnce(&ArcKioskAppData::OnIconLoadDone,
+                            weak_ptr_factory_.GetWeakPtr()));
+  return true;
 }
 
 void ArcKioskAppData::SetCache(const std::string& name,
@@ -65,16 +69,16 @@ void ArcKioskAppData::SetCache(const std::string& name,
   SaveToDictionary(dict_update);
 }
 
-void ArcKioskAppData::OnIconLoadSuccess(const gfx::ImageSkia& icon) {
+void ArcKioskAppData::OnIconLoadDone(absl::optional<gfx::ImageSkia> icon) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
   kiosk_app_icon_loader_.reset();
-  icon_ = icon;
-}
 
-void ArcKioskAppData::OnIconLoadFailure() {
-  kiosk_app_icon_loader_.reset();
-  LOG(ERROR) << "Icon Load Failure";
-  // Do nothing
+  if (!icon.has_value()) {
+    LOG(ERROR) << "Icon Load Failure";
+    return;
+  }
+
+  icon_ = icon.value();
 }
 
 }  // namespace ash

chrome/browser/ash/app_mode/arc/arc_kiosk_app_data.h

@@ -39,15 +39,15 @@ class ArcKioskAppData : public KioskAppDataBase {
   // Sets the cached data.
   void SetCache(const std::string& name, const gfx::ImageSkia& icon);
 
-  // Callbacks for KioskAppIconLoader.
-  void OnIconLoadSuccess(const gfx::ImageSkia& icon) override;
-  void OnIconLoadFailure() override;
-
  private:
+  void OnIconLoadDone(absl::optional<gfx::ImageSkia> icon);
+
   // Not cached, always provided in ctor.
   const std::string package_name_;
   const std::string activity_;
   const std::string intent_;
+
+  base::WeakPtrFactory<ArcKioskAppData> weak_ptr_factory_{this};
 };
 
 }  // namespace ash

chrome/browser/ash/app_mode/kiosk_app_data.cc

@@ -160,8 +160,9 @@ class KioskAppData::CrxLoader : public extensions::SandboxedUnpackerClient {
   void NotifyFinishedOnUIThread() {
     DCHECK_CURRENTLY_ON(BrowserThread::UI);
 
-    if (client_)
+    if (client_) {
       client_->OnCrxLoadFinished(this);
+    }
   }
 
   base::WeakPtr<KioskAppData> client_;
@@ -204,8 +205,9 @@ class KioskAppData::WebstoreDataParser
   ~WebstoreDataParser() override = default;
 
   void ReportFailure() {
-    if (client_)
+    if (client_) {
       client_->OnWebstoreParseFailure();
+    }
 
     delete this;
   }
@@ -235,8 +237,9 @@ class KioskAppData::WebstoreDataParser
       required_platform_version = temp->GetString();
     }
 
-    if (client_)
+    if (client_) {
       client_->OnWebstoreParseSuccess(icon, required_platform_version);
+    }
     delete this;
   }
   void OnWebstoreParseFailure(const std::string& id,
@@ -274,8 +277,9 @@ KioskAppData::~KioskAppData() = default;
 void KioskAppData::Load() {
   SetStatus(Status::kLoading);
 
-  if (LoadFromCache())
+  if (LoadFromCache()) {
     return;
+  }
 
   StartFetch();
 }
@@ -305,8 +309,9 @@ void KioskAppData::LoadFromInstalledApp(Profile* profile,
 }
 
 void KioskAppData::SetCachedCrx(const base::FilePath& crx_file) {
-  if (crx_file_ == crx_file)
+  if (crx_file_ == crx_file) {
     return;
+  }
 
   crx_file_ = crx_file;
   LoadFromCrx();
@@ -346,13 +351,15 @@ void KioskAppData::SetStatus(Status status) {
     status = Status::kLoaded;
   }
 
-  if (status_ == status)
+  if (status_ == status) {
     return;
+  }
 
   status_ = status;
 
-  if (!delegate_)
+  if (!delegate_) {
     return;
+  }
 
   switch (status_) {
     case Status::kInit:
@@ -375,17 +382,22 @@ bool KioskAppData::LoadFromCache() {
   PrefService* local_state = g_browser_process->local_state();
   const base::Value::Dict& dict = local_state->GetDict(dictionary_name());
 
-  if (!LoadFromDictionary(dict))
+  if (!LoadFromDictionary(dict)) {
     return false;
+  }
+
+  DecodeIcon(base::BindOnce(&KioskAppData::OnIconLoadDone,
+                            weak_factory_.GetWeakPtr()));
 
   const std::string app_key = std::string(kKeyApps) + '.' + app_id();
   const std::string required_platform_version_key =
       app_key + '.' + kKeyRequiredPlatformVersion;
 
   const std::string* maybe_required_platform_version =
       dict.FindStringByDottedPath(required_platform_version_key);
-  if (!maybe_required_platform_version)
+  if (!maybe_required_platform_version) {
     return false;
+  }
 
   required_platform_version_ = *maybe_required_platform_version;
   return true;
@@ -400,8 +412,9 @@ void KioskAppData::SetCache(const std::string& name,
   icon_.MakeThreadSafe();
 
   base::FilePath cache_dir;
-  if (delegate_)
+  if (delegate_) {
     delegate_->GetKioskAppIconCacheDir(&cache_dir);
+  }
 
   SaveIcon(icon, cache_dir);
 
@@ -430,17 +443,17 @@ void KioskAppData::OnExtensionIconLoaded(const gfx::Image& icon) {
   SetStatus(Status::kLoaded);
 }
 
-void KioskAppData::OnIconLoadSuccess(const gfx::ImageSkia& icon) {
+void KioskAppData::OnIconLoadDone(absl::optional<gfx::ImageSkia> icon) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   kiosk_app_icon_loader_.reset();
-  icon_ = icon;
-  SetStatus(Status::kLoaded);
-}
+  if (!icon.has_value()) {
+    // Re-fetch data from web store when failed to load cached data.
+    StartFetch();
+    return;
+  }
 
-void KioskAppData::OnIconLoadFailure() {
-  kiosk_app_icon_loader_.reset();
-  // Re-fetch data from web store when failed to load cached data.
-  StartFetch();
+  icon_ = icon.value();
+  SetStatus(Status::kLoaded);
 }
 
 // static
@@ -499,15 +512,19 @@ void KioskAppData::OnWebstoreResponseParseSuccess(
   webstore_fetcher_.reset();
 
   std::string manifest;
-  if (!CheckResponseKeyValue(*id, webstore_data, kManifestKey, &manifest))
+  if (!CheckResponseKeyValue(*id, webstore_data, kManifestKey, &manifest)) {
     return;
+  }
 
-  if (!CheckResponseKeyValue(*id, webstore_data, kLocalizedNameKey, &name_))
+  if (!CheckResponseKeyValue(*id, webstore_data, kLocalizedNameKey, &name_)) {
     return;
+  }
 
   std::string icon_url_string;
-  if (!CheckResponseKeyValue(*id, webstore_data, kIconUrlKey, &icon_url_string))
+  if (!CheckResponseKeyValue(*id, webstore_data, kIconUrlKey,
+                             &icon_url_string)) {
     return;
+  }
 
   GURL icon_url =
       extension_urls::GetWebstoreLaunchURL().Resolve(icon_url_string);
@@ -547,8 +564,9 @@ bool KioskAppData::CheckResponseKeyValue(const std::string& extension_id,
 }
 
 void KioskAppData::LoadFromCrx() {
-  if (crx_file_.empty())
+  if (crx_file_.empty()) {
     return;
+  }
 
   scoped_refptr<CrxLoader> crx_loader(
       new CrxLoader(weak_factory_.GetWeakPtr(), crx_file_));
@@ -558,24 +576,27 @@ void KioskAppData::LoadFromCrx() {
 void KioskAppData::OnCrxLoadFinished(const CrxLoader* crx_loader) {
   DCHECK(crx_loader);
 
-  if (crx_loader->crx_file() != crx_file_)
+  if (crx_loader->crx_file() != crx_file_) {
     return;
+  }
 
   if (!crx_loader->success()) {
     LOG(ERROR) << "Failed to load cached extension data for app_id="
                << app_id();
-    // If after unpacking the cached extension we received an error, schedule a
-    // redownload upon next session start(kiosk or login).
-    if (delegate_)
+    // If after unpacking the cached extension we received an error, schedule
+    // a redownload upon next session start(kiosk or login).
+    if (delegate_) {
       delegate_->OnExternalCacheDamaged(app_id());
+    }
 
     SetStatus(Status::kInit);
     return;
   }
 
   SkBitmap icon = crx_loader->icon();
-  if (icon.empty())
+  if (icon.empty()) {
     icon = *extensions::util::GetDefaultAppIcon().bitmap();
+  }
   SetCache(crx_loader->name(), icon, crx_loader->required_platform_version());
 
   SetStatus(Status::kLoaded);

chrome/browser/ash/app_mode/kiosk_app_data.h

@@ -21,7 +21,7 @@ class Profile;
 namespace extensions {
 class Extension;
 class WebstoreDataFetcher;
-}
+}  // namespace extensions
 
 namespace gfx {
 class Image;
@@ -31,7 +31,7 @@ namespace network {
 namespace mojom {
 class URLLoaderFactory;
 }
-}
+}  // namespace network
 
 namespace ash {
 
@@ -90,10 +90,6 @@ class KioskAppData : public KioskAppDataBase,
       const GURL& update_url,
       const std::string& required_platform_version);
 
-  // Callbacks for KioskAppIconLoader.
-  void OnIconLoadSuccess(const gfx::ImageSkia& icon) override;
-  void OnIconLoadFailure() override;
-
   // Tests do not always fake app data download.
   // This allows to ignore download errors.
   static void SetIgnoreKioskAppDataLoadFailuresForTesting(bool value);
@@ -148,6 +144,8 @@ class KioskAppData : public KioskAppDataBase,
 
   void OnCrxLoadFinished(const CrxLoader* crx_loader);
 
+  void OnIconLoadDone(absl::optional<gfx::ImageSkia> icon);
+
   KioskAppDataDelegate* delegate_;  // not owned.
   Status status_;
 

chrome/browser/ash/app_mode/kiosk_app_data_base.cc

@@ -12,6 +12,7 @@
 #include "base/task/task_traits.h"
 #include "base/task/thread_pool.h"
 #include "base/threading/scoped_blocking_call.h"
+#include "chrome/browser/ash/app_mode/kiosk_app_icon_loader.h"
 #include "chrome/browser/browser_process.h"
 #include "components/prefs/pref_service.h"
 #include "components/prefs/scoped_user_pref_update.h"
@@ -56,8 +57,9 @@ void RemoveDictionaryPath(base::Value::Dict& dict, base::StringPiece path) {
   if (delimiter_position != base::StringPiece::npos) {
     current_dictionary =
         dict.FindDictByDottedPath(current_path.substr(0, delimiter_position));
-    if (!current_dictionary)
+    if (!current_dictionary) {
       return;
+    }
     current_path = current_path.substr(delimiter_position + 1);
   }
   current_dictionary->Remove(current_path);
@@ -95,8 +97,7 @@ void KioskAppDataBase::SaveIconToDictionary(ScopedDictPrefUpdate& dict_update) {
   dict_update->SetByDottedPath(icon_path_key, icon_path_.value());
 }
 
-bool KioskAppDataBase::LoadFromDictionary(const base::Value::Dict& dict,
-                                          bool lazy_icon_load) {
+bool KioskAppDataBase::LoadFromDictionary(const base::Value::Dict& dict) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   const std::string app_key =
       std::string(KioskAppDataBase::kKeyApps) + '.' + app_id_;
@@ -105,8 +106,9 @@ bool KioskAppDataBase::LoadFromDictionary(const base::Value::Dict& dict,
 
   // If there is no title stored, do not stop, sometimes only icon is cached.
   const std::string* maybe_name = dict.FindStringByDottedPath(name_key);
-  if (maybe_name)
+  if (maybe_name) {
     name_ = *maybe_name;
+  }
 
   const std::string* icon_path_string =
       dict.FindStringByDottedPath(icon_path_key);
@@ -116,16 +118,13 @@ bool KioskAppDataBase::LoadFromDictionary(const base::Value::Dict& dict,
 
   icon_path_ = base::FilePath(*icon_path_string);
 
-  if (!lazy_icon_load) {
-    DecodeIcon();
-  }
-
   return true;
 }
 
-void KioskAppDataBase::DecodeIcon() {
+void KioskAppDataBase::DecodeIcon(KioskAppIconLoader::ResultCallback callback) {
   DLOG_IF(ERROR, icon_path_.empty()) << "Icon path is empty";
-  kiosk_app_icon_loader_ = std::make_unique<KioskAppIconLoader>(this);
+  kiosk_app_icon_loader_ =
+      std::make_unique<KioskAppIconLoader>(std::move(callback));
   kiosk_app_icon_loader_->Start(icon_path_);
 }