Don't have ArcApps load icons from disk cache

The disk cache format (e.g. file names) is really a private
implementation detail of the ArcAppListPrefs and ArcAppIcon classes.
ArcAppIcon already consults the disk cache. The ArcApps class has been
recently changed to use ArcAppIcon's, via an ArcIconOnceLoader.

This commit is a small behavior change when the App Service is enabled,
if the icon file was already in the cache. Before, there will be no icon
("an empty, transparent rectangle") until the file was decoded. After,
there will be a placeholder icon (IDR_APP_DEFAULT_ICON) until the file
was decoded. Either way, the "until the file was decoded" time
(involving file I/O) can be visible to the naked eye, as a flash of the
empty or placeholder icon until the real icon is loaded. But it isn't as
long as e.g. the time taken to spin up an Android VM. If the icon file
wasn't in the cache, IDR_APP_DEFAULT_ICON would be shown either way.

Whether or not the "empty icon before real icon" or
"IDR_APP_DEFAULT_ICON before real icon" behavior is preferable, the
behavior after this commit (IDR_APP_DEFAULT_ICON) is more consistent
when comparing the Android app icon behavior with the App Service
enabled or disabled, although when enabled, less consistent when
comparing Android and Linux (Crostini) app icon behavior.

Anyway, keeping it an ArcAppIcon implementation detail is also arguably
cleaner code. It's certainly a little less code (this commit is mostly
deletion), and removes a "mapped app id" TODO.

BUG=826982

Change-Id: I11f9f5d27061896005def458fe89f890e338e348
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1774005Reviewed-by: Dominick Ng <dominickn@chromium.org>
Commit-Queue: Nigel Tao <nigeltao@chromium.org>
Cr-Commit-Position: refs/heads/master@{#691854}

chrome/browser/apps/app_service/arc_apps.cc

@@ -189,35 +189,32 @@ void ArcApps::LoadIcon(const std::string& app_id,
                        int32_t size_hint_in_dip,
                        bool allow_placeholder_icon,
                        LoadIconCallback callback) {
-  if (icon_key) {
-    // Treat the Play Store as a special case, loading an icon defined by a
-    // resource instead of asking the Android VM (or the cache of previous
-    // responses from the Android VM). Presumably this is for bootstrapping:
-    // the Play Store icon (the UI for enabling and installing Android apps)
-    // should be showable even before the user has installed their first
-    // Android app and before bringing up an Android VM for the first time.
-    if (app_id == arc::kPlayStoreAppId) {
-      LoadPlayStoreIcon(icon_compression, size_hint_in_dip,
-                        static_cast<IconEffects>(icon_key->icon_effects),
-                        std::move(callback));
-      return;
-    }
-
-    // Try loading the icon from an on-disk cache. If that fails, fall back to
-    // LoadIconFromVM.
-    LoadIconFromFileWithFallback(
-        icon_compression, size_hint_in_dip,
-        GetCachedIconFilePath(app_id, size_hint_in_dip),
-        static_cast<IconEffects>(icon_key->icon_effects), std::move(callback),
-        base::BindOnce(&ArcApps::LoadIconFromVM, weak_ptr_factory_.GetWeakPtr(),
-                       app_id, icon_compression, size_hint_in_dip,
-                       allow_placeholder_icon,
-                       static_cast<IconEffects>(icon_key->icon_effects)));
+  if (!icon_key) {
+    std::move(callback).Run(apps::mojom::IconValue::New());
     return;
   }
-
-  // On failure, we still run the callback, with the zero IconValue.
-  std::move(callback).Run(apps::mojom::IconValue::New());
+  IconEffects icon_effects = static_cast<IconEffects>(icon_key->icon_effects);
+
+  // Treat the Play Store as a special case, loading an icon defined by a
+  // resource instead of asking the Android VM (or the cache of previous
+  // responses from the Android VM). Presumably this is for bootstrapping:
+  // the Play Store icon (the UI for enabling and installing Android apps)
+  // should be showable even before the user has installed their first
+  // Android app and before bringing up an Android VM for the first time.
+  if (app_id == arc::kPlayStoreAppId) {
+    LoadPlayStoreIcon(icon_compression, size_hint_in_dip, icon_effects,
+                      std::move(callback));
+  } else if (allow_placeholder_icon) {
+    constexpr bool is_placeholder_icon = true;
+    LoadIconFromResource(icon_compression, size_hint_in_dip,
+                         IDR_APP_DEFAULT_ICON, is_placeholder_icon,
+                         icon_effects, std::move(callback));
+  } else {
+    arc_icon_once_loader_.LoadIcon(
+        app_id, size_hint_in_dip, icon_compression,
+        base::BindOnce(&OnArcAppIconCompletelyLoaded, icon_compression,
+                       size_hint_in_dip, icon_effects, std::move(callback)));
+  }
 }
 
 void ArcApps::Launch(const std::string& app_id,
@@ -398,41 +395,6 @@ void ArcApps::OnPackageListInitialRefreshed() {
   }
 }
 
-const base::FilePath ArcApps::GetCachedIconFilePath(const std::string& app_id,
-                                                    int32_t size_hint_in_dip) {
-  ArcAppListPrefs* prefs = ArcAppListPrefs::Get(profile_);
-  if (!prefs) {
-    return base::FilePath();
-  }
-  // TODO(crbug.com/826982): process the app_id argument like the private
-  // GetAppFromAppOrGroupId function and the ArcAppIcon::mapped_app_id_ field
-  // in arc_app_icon.cc?
-  return prefs->GetIconPath(
-      app_id,
-      ArcAppIconDescriptor(size_hint_in_dip,
-                           apps_util::GetPrimaryDisplayUIScaleFactor()));
-}
-
-void ArcApps::LoadIconFromVM(const std::string app_id,
-                             apps::mojom::IconCompression icon_compression,
-                             int32_t size_hint_in_dip,
-                             bool allow_placeholder_icon,
-                             IconEffects icon_effects,
-                             LoadIconCallback callback) {
-  if (allow_placeholder_icon) {
-    constexpr bool is_placeholder_icon = true;
-    LoadIconFromResource(icon_compression, size_hint_in_dip,
-                         IDR_APP_DEFAULT_ICON, is_placeholder_icon,
-                         icon_effects, std::move(callback));
-    return;
-  }
-
-  arc_icon_once_loader_.LoadIcon(
-      app_id, size_hint_in_dip, icon_compression,
-      base::BindOnce(&OnArcAppIconCompletelyLoaded, icon_compression,
-                     size_hint_in_dip, icon_effects, std::move(callback)));
-}
-
 void ArcApps::LoadPlayStoreIcon(apps::mojom::IconCompression icon_compression,
                                 int32_t size_hint_in_dip,
                                 IconEffects icon_effects,

chrome/browser/apps/app_service/arc_apps.h

@@ -9,7 +9,6 @@
 #include <vector>
 
 #include "base/callback_forward.h"
-#include "base/files/file_path.h"
 #include "base/macros.h"
 #include "base/memory/weak_ptr.h"
 #include "chrome/browser/apps/app_service/app_icon_factory.h"
@@ -84,14 +83,6 @@ class ArcApps : public KeyedService,
       const arc::mojom::ArcPackageInfo& package_info) override;
   void OnPackageListInitialRefreshed() override;
 
-  const base::FilePath GetCachedIconFilePath(const std::string& app_id,
-                                             int32_t size_hint_in_dip);
-  void LoadIconFromVM(const std::string app_id,
-                      apps::mojom::IconCompression icon_compression,
-                      int32_t size_hint_in_dip,
-                      bool allow_placeholder_icon,
-                      IconEffects icon_effects,
-                      LoadIconCallback callback);
   void LoadPlayStoreIcon(apps::mojom::IconCompression icon_compression,
                          int32_t size_hint_in_dip,
                          IconEffects icon_effects,