Register PluginVM apps separate to crostini

Refactor CrostiniApps and GuestOsRegistryService so that PluginVM apps
are registered as type PluginVm by PluginVmApps.

When I recently changed the icon handling, it was a mistake to register
PluginVM apps as crostini.

Bug: b/158617133
Change-Id: I0877e75b9b087c6ece34cbe8642399b5cc2764c8
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2379407Reviewed-by: Nancy Wang <nancylingwang@chromium.org>
Reviewed-by: Timothy Loh <timloh@chromium.org>
Commit-Queue: Joel Hockey <joelhockey@chromium.org>
Cr-Commit-Position: refs/heads/master@{#802940}

chrome/browser/apps/app_service/crostini_apps.cc

@@ -111,15 +111,15 @@ void CrostiniApps::Connect(
     mojo::PendingRemote<apps::mojom::Subscriber> subscriber_remote,
     apps::mojom::ConnectOptionsPtr opts) {
   std::vector<apps::mojom::AppPtr> apps;
-  // Register all apps with app service. We will only show termina apps in
-  // launcher, shelf, etc, but app service will manage icons for all apps which
-  // can appear in FilesApp open-with.
-  for (const auto& pair : registry_->GetAllRegisteredApps()) {
-    const std::string& app_id = pair.first;
+
+  for (const auto& pair :
+       registry_->GetRegisteredApps(guest_os::GuestOsRegistryService::VmType::
+                                        ApplicationList_VmType_TERMINA)) {
     const guest_os::GuestOsRegistryService::Registration& registration =
         pair.second;
-    apps.push_back(Convert(app_id, registration, true));
+    apps.push_back(Convert(registration, /*new_icon_key=*/true));
   }
+
   mojo::Remote<apps::mojom::Subscriber> subscriber(
       std::move(subscriber_remote));
   subscriber->OnApps(std::move(apps));
@@ -132,34 +132,9 @@ void CrostiniApps::LoadIcon(const std::string& app_id,
                             int32_t size_hint_in_dip,
                             bool allow_placeholder_icon,
                             LoadIconCallback callback) {
-  if (icon_key) {
-    if (icon_key->resource_id != apps::mojom::IconKey::kInvalidResourceId) {
-      // The icon is a resource built into the Chrome OS binary.
-      constexpr bool is_placeholder_icon = false;
-      LoadIconFromResource(icon_type, size_hint_in_dip, icon_key->resource_id,
-                           is_placeholder_icon,
-                           static_cast<IconEffects>(icon_key->icon_effects),
-                           std::move(callback));
-      return;
-    } else {
-      auto scale_factor = apps_util::GetPrimaryDisplayUIScaleFactor();
-
-      // Try loading the icon from an on-disk cache. If that fails, fall back
-      // to LoadIconFromVM.
-      LoadIconFromFileWithFallback(
-          icon_type, size_hint_in_dip,
-          registry_->GetIconPath(app_id, scale_factor),
-          static_cast<IconEffects>(icon_key->icon_effects), std::move(callback),
-          base::BindOnce(&CrostiniApps::LoadIconFromVM,
-                         weak_ptr_factory_.GetWeakPtr(), app_id, icon_type,
-                         size_hint_in_dip, scale_factor,
-                         static_cast<IconEffects>(icon_key->icon_effects)));
-      return;
-    }
-  }
-
-  // On failure, we still run the callback, with the zero IconValue.
-  std::move(callback).Run(apps::mojom::IconValue::New());
+  registry_->LoadIcon(app_id, std::move(icon_key), icon_type, size_hint_in_dip,
+                      allow_placeholder_icon, IDR_LOGO_CROSTINI_DEFAULT_192,
+                      std::move(callback));
 }
 
 void CrostiniApps::Launch(const std::string& app_id,
@@ -231,17 +206,31 @@ void CrostiniApps::GetMenuModel(const std::string& app_id,
 
 void CrostiniApps::OnRegistryUpdated(
     guest_os::GuestOsRegistryService* registry_service,
+    guest_os::GuestOsRegistryService::VmType vm_type,
     const std::vector<std::string>& updated_apps,
     const std::vector<std::string>& removed_apps,
     const std::vector<std::string>& inserted_apps) {
+  if (vm_type != guest_os::GuestOsRegistryService::VmType::
+                     ApplicationList_VmType_TERMINA) {
+    return;
+  }
+
   for (const std::string& app_id : updated_apps) {
-    PublishAppID(app_id, PublishAppIDType::kUpdate);
+    if (auto registration = registry_->GetRegistration(app_id)) {
+      Publish(Convert(*registration, /*new_icon_key=*/false), subscribers_);
+    }
   }
   for (const std::string& app_id : removed_apps) {
-    PublishAppID(app_id, PublishAppIDType::kUninstall);
+    apps::mojom::AppPtr app = apps::mojom::App::New();
+    app->app_type = apps::mojom::AppType::kCrostini;
+    app->app_id = app_id;
+    app->readiness = apps::mojom::Readiness::kUninstalledByUser;
+    Publish(std::move(app), subscribers_);
   }
   for (const std::string& app_id : inserted_apps) {
-    PublishAppID(app_id, PublishAppIDType::kInstall);
+    if (auto registration = registry_->GetRegistration(app_id)) {
+      Publish(Convert(*registration, /*new_icon_key=*/true), subscribers_);
+    }
   }
 }
 
@@ -262,53 +251,13 @@ void CrostiniApps::OnCrostiniEnabledChanged() {
   Publish(std::move(app), subscribers_);
 }
 
-void CrostiniApps::LoadIconFromVM(const std::string app_id,
-                                  apps::mojom::IconType icon_type,
-                                  int32_t size_hint_in_dip,
-                                  ui::ScaleFactor scale_factor,
-                                  IconEffects icon_effects,
-                                  LoadIconCallback callback) {
-  registry_->RequestIcon(
-      app_id, scale_factor,
-      base::BindOnce(&CrostiniApps::OnLoadIconFromVM,
-                     weak_ptr_factory_.GetWeakPtr(), app_id, icon_type,
-                     size_hint_in_dip, icon_effects, std::move(callback)));
-}
-
-void CrostiniApps::OnLoadIconFromVM(const std::string app_id,
-                                    apps::mojom::IconType icon_type,
-                                    int32_t size_hint_in_dip,
-                                    IconEffects icon_effects,
-                                    LoadIconCallback callback,
-                                    std::string compressed_icon_data) {
-  if (compressed_icon_data.empty()) {
-    auto registration = registry_->GetRegistration(app_id);
-    if (crostini::IsUnmatchedCrostiniShelfAppId(app_id) ||
-        (registration && registration->VmType() ==
-                             guest_os::GuestOsRegistryService::VmType::
-                                 ApplicationList_VmType_TERMINA)) {
-      // Load default penguin for crostini. We must set is_placeholder_icon to
-      // false to stop endless recursive calls.
-      LoadIconFromResource(
-          icon_type, size_hint_in_dip, IDR_LOGO_CROSTINI_DEFAULT_192,
-          /*is_placeholder_icon=*/false, icon_effects, std::move(callback));
-    } else {
-      // Leave it for app service to get a default for Plugin VM.
-      std::move(callback).Run(apps::mojom::IconValue::New());
-    }
-  } else {
-    LoadIconFromCompressedData(icon_type, size_hint_in_dip, icon_effects,
-                               compressed_icon_data, std::move(callback));
-  }
-}
-
 apps::mojom::AppPtr CrostiniApps::Convert(
-    const std::string& app_id,
     const guest_os::GuestOsRegistryService::Registration& registration,
     bool new_icon_key) {
   apps::mojom::AppPtr app = PublisherBase::MakeApp(
-      apps::mojom::AppType::kCrostini, app_id, apps::mojom::Readiness::kReady,
-      registration.Name(), apps::mojom::InstallSource::kUser);
+      apps::mojom::AppType::kCrostini, registration.app_id(),
+      apps::mojom::Readiness::kReady, registration.Name(),
+      apps::mojom::InstallSource::kUser);
 
   const std::string& executable_file_name = registration.ExecutableFileName();
   if (!executable_file_name.empty()) {
@@ -319,7 +268,7 @@ apps::mojom::AppPtr CrostiniApps::Convert(
   }
 
   if (new_icon_key) {
-    app->icon_key = NewIconKey(app_id);
+    app->icon_key = NewIconKey(registration.app_id());
   }
 
   app->last_launch_time = registration.LastLaunchTime();
@@ -372,23 +321,4 @@ apps::mojom::IconKeyPtr CrostiniApps::NewIconKey(const std::string& app_id) {
   return icon_key_factory_.MakeIconKey(icon_effects);
 }
 
-void CrostiniApps::PublishAppID(const std::string& app_id,
-                                PublishAppIDType type) {
-  if (type == PublishAppIDType::kUninstall) {
-    apps::mojom::AppPtr app = apps::mojom::App::New();
-    app->app_type = apps::mojom::AppType::kCrostini;
-    app->app_id = app_id;
-    app->readiness = apps::mojom::Readiness::kUninstalledByUser;
-    Publish(std::move(app), subscribers_);
-    return;
-  }
-
-  base::Optional<guest_os::GuestOsRegistryService::Registration> registration =
-      registry_->GetRegistration(app_id);
-  if (registration.has_value()) {
-    Publish(Convert(app_id, *registration, type == PublishAppIDType::kInstall),
-            subscribers_);
-  }
-}
-
 }  // namespace apps

chrome/browser/apps/app_service/crostini_apps.h

@@ -42,12 +42,6 @@ class CrostiniApps : public KeyedService,
       Profile* profile);
 
  private:
-  enum class PublishAppIDType {
-    kInstall,
-    kUninstall,
-    kUpdate,
-  };
-
   void Initialize(const mojo::Remote<apps::mojom::AppService>& app_service);
 
   // apps::mojom::Publisher overrides.
@@ -75,6 +69,7 @@ class CrostiniApps : public KeyedService,
   // GuestOsRegistryService::Observer overrides.
   void OnRegistryUpdated(
       guest_os::GuestOsRegistryService* registry_service,
+      guest_os::GuestOsRegistryService::VmType vm_type,
       const std::vector<std::string>& updated_apps,
       const std::vector<std::string>& removed_apps,
       const std::vector<std::string>& inserted_apps) override;
@@ -84,26 +79,10 @@ class CrostiniApps : public KeyedService,
   // once it can support hiding apps.
   void OnCrostiniEnabledChanged();
 
-  void LoadIconFromVM(const std::string app_id,
-                      apps::mojom::IconType icon_type,
-                      int32_t size_hint_in_dip,
-                      ui::ScaleFactor scale_factor,
-                      IconEffects icon_effects,
-                      LoadIconCallback callback);
-
-  void OnLoadIconFromVM(const std::string app_id,
-                        apps::mojom::IconType icon_type,
-                        int32_t size_hint_in_dip,
-                        IconEffects icon_effects,
-                        LoadIconCallback callback,
-                        std::string compressed_icon_data);
-
   apps::mojom::AppPtr Convert(
-      const std::string& app_id,
       const guest_os::GuestOsRegistryService::Registration& registration,
       bool new_icon_key);
   apps::mojom::IconKeyPtr NewIconKey(const std::string& app_id);
-  void PublishAppID(const std::string& app_id, PublishAppIDType type);
 
   mojo::RemoteSet<apps::mojom::Subscriber> subscribers_;
 

chrome/browser/apps/app_service/plugin_vm_apps.cc

@@ -21,6 +21,7 @@
 #include "chrome/browser/chromeos/profiles/profile_helper.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/ui/webui/app_management/app_management.mojom.h"
+#include "chrome/common/chrome_features.h"
 #include "chrome/grit/chrome_unscaled_resources.h"
 #include "chrome/grit/generated_resources.h"
 #include "components/prefs/pref_service.h"
@@ -138,7 +139,7 @@ namespace apps {
 PluginVmApps::PluginVmApps(
     const mojo::Remote<apps::mojom::AppService>& app_service,
     Profile* profile)
-    : profile_(profile), permissions_observer_(this) {
+    : profile_(profile), registry_(nullptr), permissions_observer_(this) {
   // Don't show anything for non-primary profiles. We can't use
   // `IsPluginVmAllowedForProfile()` here because we still let the user
   // uninstall Plugin VM when it isn't allowed for some other reasons (e.g.
@@ -147,6 +148,12 @@ PluginVmApps::PluginVmApps(
     return;
   }
 
+  registry_ = guest_os::GuestOsRegistryServiceFactory::GetForProfile(profile_);
+  if (!registry_) {
+    return;
+  }
+  registry_->AddObserver(this);
+
   PublisherBase::Initialize(app_service, apps::mojom::AppType::kPluginVm);
 
   // Register for Plugin VM changes to policy and installed state, so that we
@@ -169,7 +176,11 @@ PluginVmApps::PluginVmApps(
   }
 }
 
-PluginVmApps::~PluginVmApps() = default;
+PluginVmApps::~PluginVmApps() {
+  if (registry_) {
+    registry_->RemoveObserver(this);
+  }
+}
 
 void PluginVmApps::Connect(
     mojo::PendingRemote<apps::mojom::Subscriber> subscriber_remote,
@@ -177,6 +188,14 @@ void PluginVmApps::Connect(
   std::vector<apps::mojom::AppPtr> apps;
   apps.push_back(GetPluginVmApp(profile_, is_allowed_));
 
+  for (const auto& pair :
+       registry_->GetRegisteredApps(guest_os::GuestOsRegistryService::VmType::
+                                        ApplicationList_VmType_PLUGIN_VM)) {
+    const guest_os::GuestOsRegistryService::Registration& registration =
+        pair.second;
+    apps.push_back(Convert(registration, /*new_icon_key=*/true));
+  }
+
   mojo::Remote<apps::mojom::Subscriber> subscriber(
       std::move(subscriber_remote));
   subscriber->OnApps(std::move(apps));
@@ -189,16 +208,10 @@ void PluginVmApps::LoadIcon(const std::string& app_id,
                             int32_t size_hint_in_dip,
                             bool allow_placeholder_icon,
                             LoadIconCallback callback) {
-  constexpr bool is_placeholder_icon = false;
-  if (icon_key &&
-      (icon_key->resource_id != apps::mojom::IconKey::kInvalidResourceId)) {
-    LoadIconFromResource(
-        icon_type, size_hint_in_dip, icon_key->resource_id, is_placeholder_icon,
-        static_cast<IconEffects>(icon_key->icon_effects), std::move(callback));
-    return;
-  }
-  // On failure, we still run the callback, with the zero IconValue.
-  std::move(callback).Run(apps::mojom::IconValue::New());
+  registry_->LoadIcon(app_id, std::move(icon_key), icon_type, size_hint_in_dip,
+                      allow_placeholder_icon,
+                      apps::mojom::IconKey::kInvalidResourceId,
+                      std::move(callback));
 }
 
 void PluginVmApps::Launch(const std::string& app_id,
@@ -268,6 +281,63 @@ void PluginVmApps::GetMenuModel(const std::string& app_id,
   std::move(callback).Run(std::move(menu_items));
 }
 
+void PluginVmApps::OnRegistryUpdated(
+    guest_os::GuestOsRegistryService* registry_service,
+    guest_os::GuestOsRegistryService::VmType vm_type,
+    const std::vector<std::string>& updated_apps,
+    const std::vector<std::string>& removed_apps,
+    const std::vector<std::string>& inserted_apps) {
+  if (vm_type != guest_os::GuestOsRegistryService::VmType::
+                     ApplicationList_VmType_PLUGIN_VM) {
+    return;
+  }
+
+  for (const std::string& app_id : updated_apps) {
+    if (auto registration = registry_->GetRegistration(app_id)) {
+      Publish(Convert(*registration, /*new_icon_key=*/false), subscribers_);
+    }
+  }
+  for (const std::string& app_id : removed_apps) {
+    apps::mojom::AppPtr app = apps::mojom::App::New();
+    app->app_type = apps::mojom::AppType::kPluginVm;
+    app->app_id = app_id;
+    app->readiness = apps::mojom::Readiness::kUninstalledByUser;
+    Publish(std::move(app), subscribers_);
+  }
+  for (const std::string& app_id : inserted_apps) {
+    if (auto registration = registry_->GetRegistration(app_id)) {
+      Publish(Convert(*registration, /*new_icon_key=*/true), subscribers_);
+    }
+  }
+}
+
+apps::mojom::AppPtr PluginVmApps::Convert(
+    const guest_os::GuestOsRegistryService::Registration& registration,
+    bool new_icon_key) {
+  apps::mojom::AppPtr app = PublisherBase::MakeApp(
+      apps::mojom::AppType::kPluginVm, registration.app_id(),
+      apps::mojom::Readiness::kReady, registration.Name(),
+      apps::mojom::InstallSource::kUser);
+
+  if (new_icon_key) {
+    auto icon_effects =
+        base::FeatureList::IsEnabled(features::kAppServiceAdaptiveIcon)
+            ? IconEffects::kCrOsStandardIcon
+            : IconEffects::kNone;
+    app->icon_key = icon_key_factory_.MakeIconKey(icon_effects);
+  }
+
+  app->last_launch_time = registration.LastLaunchTime();
+  app->install_time = registration.InstallTime();
+
+  app->show_in_launcher = apps::mojom::OptionalBool::kFalse;
+  app->show_in_search = apps::mojom::OptionalBool::kFalse;
+  app->show_in_shelf = apps::mojom::OptionalBool::kFalse;
+  app->show_in_management = apps::mojom::OptionalBool::kFalse;
+
+  return app;
+}
+
 void PluginVmApps::OnPluginVmAllowedChanged(bool is_allowed) {
   // Republish the Plugin VM app when policy changes have changed
   // its availability. Only changed fields need to be republished.

chrome/browser/apps/app_service/plugin_vm_apps.h

@@ -9,6 +9,8 @@
 #include <string>
 
 #include "base/scoped_observer.h"
+#include "chrome/browser/apps/app_service/icon_key_util.h"
+#include "chrome/browser/chromeos/guest_os/guest_os_registry_service.h"
 #include "chrome/browser/chromeos/plugin_vm/plugin_vm_manager.h"
 #include "chrome/browser/chromeos/plugin_vm/plugin_vm_util.h"
 #include "components/prefs/pref_change_registrar.h"
@@ -27,7 +29,8 @@ namespace apps {
 //
 // See components/services/app_service/README.md.
 class PluginVmApps : public apps::PublisherBase,
-                     public plugin_vm::PluginVmPermissionsObserver {
+                     public plugin_vm::PluginVmPermissionsObserver,
+                     public guest_os::GuestOsRegistryService::Observer {
  public:
   PluginVmApps(const mojo::Remote<apps::mojom::AppService>& app_service,
                Profile* profile);
@@ -61,6 +64,17 @@ class PluginVmApps : public apps::PublisherBase,
                     int64_t display_id,
                     GetMenuModelCallback callback) override;
 
+  // GuestOsRegistryService::Observer overrides.
+  void OnRegistryUpdated(
+      guest_os::GuestOsRegistryService* registry_service,
+      guest_os::GuestOsRegistryService::VmType vm_type,
+      const std::vector<std::string>& updated_apps,
+      const std::vector<std::string>& removed_apps,
+      const std::vector<std::string>& inserted_apps) override;
+
+  apps::mojom::AppPtr Convert(
+      const guest_os::GuestOsRegistryService::Registration& registration,
+      bool new_icon_key);
   void OnPluginVmAllowedChanged(bool is_allowed);
   void OnPluginVmConfiguredChanged();
   // plugin_vm::PluginVmPermissionsObserver
@@ -70,6 +84,9 @@ class PluginVmApps : public apps::PublisherBase,
   mojo::RemoteSet<apps::mojom::Subscriber> subscribers_;
 
   Profile* const profile_;
+  guest_os::GuestOsRegistryService* registry_;
+
+  apps_util::IncrementingIconKeyFactory icon_key_factory_;
 
   // Whether the Plugin VM app is allowed by policy.
   bool is_allowed_;

chrome/browser/chromeos/crostini/crostini_package_notification.cc

@@ -96,9 +96,14 @@ PackageOperationStatus CrostiniPackageNotification::GetOperationStatus() const {
 
 void CrostiniPackageNotification::OnRegistryUpdated(
     guest_os::GuestOsRegistryService* registry_service,
+    guest_os::GuestOsRegistryService::VmType vm_type,
     const std::vector<std::string>& updated_apps,
     const std::vector<std::string>& removed_apps,
     const std::vector<std::string>& inserted_apps) {
+  if (vm_type != guest_os::GuestOsRegistryService::VmType::
+                     ApplicationList_VmType_TERMINA) {
+    return;
+  }
   inserted_apps_.insert(inserted_apps.begin(), inserted_apps.end());
 }
 

chrome/browser/chromeos/crostini/crostini_package_notification.h

@@ -60,6 +60,7 @@ class CrostiniPackageNotification
   // GuestOsRegistryService::Observer:
   void OnRegistryUpdated(
       guest_os::GuestOsRegistryService* registry_service,
+      guest_os::GuestOsRegistryService::VmType vm_type,
       const std::vector<std::string>& updated_apps,
       const std::vector<std::string>& removed_apps,
       const std::vector<std::string>& inserted_apps) override;

chrome/browser/chromeos/guest_os/guest_os_registry_service.cc

@@ -17,6 +17,7 @@
 #include "base/time/clock.h"
 #include "base/time/default_clock.h"
 #include "base/time/time.h"
+#include "chrome/browser/apps/app_service/dip_px_util.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/chromeos/crostini/crostini_features.h"
 #include "chrome/browser/chromeos/crostini/crostini_manager.h"
@@ -668,6 +669,86 @@ base::FilePath GuestOsRegistryService::GetIconPath(
   }
 }
 
+void GuestOsRegistryService::LoadIcon(
+    const std::string& app_id,
+    apps::mojom::IconKeyPtr icon_key,
+    apps::mojom::IconType icon_type,
+    int32_t size_hint_in_dip,
+    bool allow_placeholder_icon,
+    int fallback_icon_resource_id,
+    apps::mojom::Publisher::LoadIconCallback callback) {
+  if (icon_key) {
+    if (icon_key->resource_id != apps::mojom::IconKey::kInvalidResourceId) {
+      // The icon is a resource built into the Chrome OS binary.
+      constexpr bool is_placeholder_icon = false;
+      apps::LoadIconFromResource(
+          icon_type, size_hint_in_dip, icon_key->resource_id,
+          is_placeholder_icon,
+          static_cast<apps::IconEffects>(icon_key->icon_effects),
+          std::move(callback));
+      return;
+    } else {
+      auto scale_factor = apps_util::GetPrimaryDisplayUIScaleFactor();
+
+      // Try loading the icon from an on-disk cache. If that fails, fall back
+      // to LoadIconFromVM.
+      apps::LoadIconFromFileWithFallback(
+          icon_type, size_hint_in_dip, GetIconPath(app_id, scale_factor),
+          static_cast<apps::IconEffects>(icon_key->icon_effects),
+          std::move(callback),
+          base::BindOnce(&GuestOsRegistryService::LoadIconFromVM,
+                         weak_ptr_factory_.GetWeakPtr(), app_id, icon_type,
+                         size_hint_in_dip, scale_factor,
+                         static_cast<apps::IconEffects>(icon_key->icon_effects),
+                         fallback_icon_resource_id));
+      return;
+    }
+  }
+
+  // On failure, we still run the callback, with the zero IconValue.
+  std::move(callback).Run(apps::mojom::IconValue::New());
+}
+
+void GuestOsRegistryService::LoadIconFromVM(
+    const std::string& app_id,
+    apps::mojom::IconType icon_type,
+    int32_t size_hint_in_dip,
+    ui::ScaleFactor scale_factor,
+    apps::IconEffects icon_effects,
+    int fallback_icon_resource_id,
+    apps::mojom::Publisher::LoadIconCallback callback) {
+  RequestIcon(app_id, scale_factor,
+              base::BindOnce(&GuestOsRegistryService::OnLoadIconFromVM,
+                             weak_ptr_factory_.GetWeakPtr(), app_id, icon_type,
+                             size_hint_in_dip, icon_effects,
+                             fallback_icon_resource_id, std::move(callback)));
+}
+
+void GuestOsRegistryService::OnLoadIconFromVM(
+    const std::string& app_id,
+    apps::mojom::IconType icon_type,
+    int32_t size_hint_in_dip,
+    apps::IconEffects icon_effects,
+    int fallback_icon_resource_id,
+    apps::mojom::Publisher::LoadIconCallback callback,
+    std::string compressed_icon_data) {
+  if (compressed_icon_data.empty()) {
+    if (fallback_icon_resource_id != apps::mojom::IconKey::kInvalidResourceId) {
+      // We load the fallback icon, but we tell AppsService that this is not
+      // a placeholder to avoid endless repeat calls since we don't expect to
+      // find a better icon than this any time soon.
+      apps::LoadIconFromResource(
+          icon_type, size_hint_in_dip, fallback_icon_resource_id,
+          /*is_placeholder_icon=*/false, icon_effects, std::move(callback));
+    } else {
+      std::move(callback).Run(apps::mojom::IconValue::New());
+    }
+  } else {
+    apps::LoadIconFromCompressedData(icon_type, size_hint_in_dip, icon_effects,
+                                     compressed_icon_data, std::move(callback));
+  }
+}
+
 void GuestOsRegistryService::RequestIcon(
     const std::string& app_id,
     ui::ScaleFactor scale_factor,
@@ -724,8 +805,10 @@ void GuestOsRegistryService::ClearApplicationList(
 
   std::vector<std::string> updated_apps;
   std::vector<std::string> inserted_apps;
-  for (Observer& obs : observers_)
-    obs.OnRegistryUpdated(this, updated_apps, removed_apps, inserted_apps);
+  for (Observer& obs : observers_) {
+    obs.OnRegistryUpdated(this, vm_type, updated_apps, removed_apps,
+                          inserted_apps);
+  }
 }
 
 void GuestOsRegistryService::UpdateApplicationList(
@@ -839,8 +922,10 @@ void GuestOsRegistryService::UpdateApplicationList(
   if (updated_apps.empty() && removed_apps.empty() && inserted_apps.empty())
     return;
 
-  for (Observer& obs : observers_)
-    obs.OnRegistryUpdated(this, updated_apps, removed_apps, inserted_apps);
+  for (Observer& obs : observers_) {
+    obs.OnRegistryUpdated(this, app_list.vm_type(), updated_apps, removed_apps,
+                          inserted_apps);
+  }
 }
 
 void GuestOsRegistryService::RemoveAppData(const std::string& app_id) {

chrome/browser/chromeos/guest_os/guest_os_registry_service.h

@@ -16,9 +16,11 @@
 #include "base/memory/weak_ptr.h"
 #include "base/observer_list.h"
 #include "base/values.h"
+#include "chrome/browser/apps/app_service/app_icon_factory.h"
 #include "chrome/browser/chromeos/crostini/crostini_simple_types.h"
 #include "chromeos/dbus/vm_applications/apps.pb.h"
 #include "components/keyed_service/core/keyed_service.h"
+#include "components/services/app_service/public/mojom/app_service.mojom.h"
 #include "ui/base/resource/scale_factor.h"
 
 class Profile;
@@ -116,6 +118,7 @@ class GuestOsRegistryService : public KeyedService {
     // last_launch_time field is updated.
     virtual void OnRegistryUpdated(
         guest_os::GuestOsRegistryService* registry_service,
+        VmType vm_type,
         const std::vector<std::string>& updated_apps,
         const std::vector<std::string>& removed_apps,
         const std::vector<std::string>& inserted_apps) {}
@@ -146,6 +149,36 @@ class GuestOsRegistryService : public KeyedService {
   base::FilePath GetIconPath(const std::string& app_id,
                              ui::ScaleFactor scale_factor) const;
 
+  // Attempts to load icon in the following order:
+  // 1/ Loads from resource if |icon_key->resource_id| is valid (non-zero).
+  // 2/ Looks up file cache.
+  // 3/ Fetches from VM.
+  // 4/ Uses |fallback_icon_resource_id| if it is valid (non-zero).
+  // 5/ Returns empty.
+  void LoadIcon(const std::string& app_id,
+                apps::mojom::IconKeyPtr icon_key,
+                apps::mojom::IconType icon_type,
+                int32_t size_hint_in_dip,
+                bool allow_placeholder_icon,
+                int fallback_icon_resource_id,
+                apps::mojom::Publisher::LoadIconCallback callback);
+
+  void LoadIconFromVM(const std::string& app_id,
+                      apps::mojom::IconType icon_type,
+                      int32_t size_hint_in_dip,
+                      ui::ScaleFactor scale_factor,
+                      apps::IconEffects icon_effects,
+                      int fallback_icon_resource_id,
+                      apps::mojom::Publisher::LoadIconCallback callback);
+
+  void OnLoadIconFromVM(const std::string& app_id,
+                        apps::mojom::IconType icon_type,
+                        int32_t size_hint_in_dip,
+                        apps::IconEffects icon_effects,
+                        int fallback_icon_resource_id,
+                        apps::mojom::Publisher::LoadIconCallback callback,
+                        std::string compressed_icon_data);
+
   // Fetches icons from container.
   void RequestIcon(const std::string& app_id,
                    ui::ScaleFactor scale_factor,

chrome/browser/chromeos/guest_os/guest_os_registry_service_unittest.cc

@@ -50,8 +50,9 @@ class GuestOsRegistryServiceTest : public testing::Test {
 
   class Observer : public GuestOsRegistryService::Observer {
    public:
-    MOCK_METHOD4(OnRegistryUpdated,
+    MOCK_METHOD5(OnRegistryUpdated,
                  void(GuestOsRegistryService*,
+                      GuestOsRegistryService::VmType,
                       const std::vector<std::string>&,
                       const std::vector<std::string>&,
                       const std::vector<std::string>&));
@@ -170,10 +171,13 @@ TEST_F(GuestOsRegistryServiceTest, Observer) {
 
   Observer observer;
   service()->AddObserver(&observer);
-  EXPECT_CALL(observer,
-              OnRegistryUpdated(
-                  service(), testing::IsEmpty(), testing::IsEmpty(),
-                  testing::UnorderedElementsAre(app_id_1, app_id_2, app_id_3)));
+  EXPECT_CALL(
+      observer,
+      OnRegistryUpdated(
+          service(),
+          GuestOsRegistryService::VmType::ApplicationList_VmType_TERMINA,
+          testing::IsEmpty(), testing::IsEmpty(),
+          testing::UnorderedElementsAre(app_id_1, app_id_2, app_id_3)));
   service()->UpdateApplicationList(app_list);
 
   // Rename desktop file for "app 2" to "app 4" (deletion+insertion)
@@ -181,10 +185,13 @@ TEST_F(GuestOsRegistryServiceTest, Observer) {
   // Rename name for "app 3" to "banana"
   app_list.mutable_apps(2)->mutable_name()->mutable_values(0)->set_value(
       "banana");
-  EXPECT_CALL(observer,
-              OnRegistryUpdated(service(), testing::ElementsAre(app_id_3),
-                                testing::ElementsAre(app_id_2),
-                                testing::ElementsAre(app_id_4)));
+  EXPECT_CALL(
+      observer,
+      OnRegistryUpdated(
+          service(),
+          GuestOsRegistryService::VmType::ApplicationList_VmType_TERMINA,
+          testing::ElementsAre(app_id_3), testing::ElementsAre(app_id_2),
+          testing::ElementsAre(app_id_4)));
   service()->UpdateApplicationList(app_list);
 }
 
@@ -199,17 +206,24 @@ TEST_F(GuestOsRegistryServiceTest, ObserverForPvmDefault) {
   service()->AddObserver(&observer);
 
   // Observers should be called when apps are added or updated.
-  EXPECT_CALL(observer, OnRegistryUpdated(
-                            service(), testing::IsEmpty(), testing::IsEmpty(),
-                            testing::UnorderedElementsAre(app_id_1)))
+  EXPECT_CALL(
+      observer,
+      OnRegistryUpdated(
+          service(),
+          GuestOsRegistryService::VmType::ApplicationList_VmType_PLUGIN_VM,
+          testing::IsEmpty(), testing::IsEmpty(),
+          testing::UnorderedElementsAre(app_id_1)))
       .Times(1);
   service()->UpdateApplicationList(app_list);
 
   // Observers should be called when apps are removed.
-  EXPECT_CALL(observer,
-              OnRegistryUpdated(service(), testing::IsEmpty(),
-                                testing::UnorderedElementsAre(app_id_1),
-                                testing::IsEmpty()))
+  EXPECT_CALL(
+      observer,
+      OnRegistryUpdated(
+          service(),
+          GuestOsRegistryService::VmType::ApplicationList_VmType_PLUGIN_VM,
+          testing::IsEmpty(), testing::UnorderedElementsAre(app_id_1),
+          testing::IsEmpty()))
       .Times(1);
   service()->ClearApplicationList(
       GuestOsRegistryService::VmType::ApplicationList_VmType_PLUGIN_VM,
@@ -287,9 +301,13 @@ TEST_F(GuestOsRegistryServiceTest, InstallAndLaunchTime) {
 
   Observer observer;
   service()->AddObserver(&observer);
-  EXPECT_CALL(observer, OnRegistryUpdated(service(), testing::IsEmpty(),
-                                          testing::IsEmpty(),
-                                          testing::ElementsAre(app_id)));
+  EXPECT_CALL(
+      observer,
+      OnRegistryUpdated(
+          service(),
+          GuestOsRegistryService::VmType::ApplicationList_VmType_TERMINA,
+          testing::IsEmpty(), testing::IsEmpty(),
+          testing::ElementsAre(app_id)));
   service()->UpdateApplicationList(app_list);
 
   base::Optional<GuestOsRegistryService::Registration> result =
@@ -301,7 +319,7 @@ TEST_F(GuestOsRegistryServiceTest, InstallAndLaunchTime) {
   // UpdateApplicationList with nothing changed. Times shouldn't be updated and
   // the observer shouldn't fire.
   test_clock_.Advance(base::TimeDelta::FromHours(1));
-  EXPECT_CALL(observer, OnRegistryUpdated(_, _, _, _)).Times(0);
+  EXPECT_CALL(observer, OnRegistryUpdated(_, _, _, _, _)).Times(0);
   service()->UpdateApplicationList(app_list);
   result = service()->GetRegistration(app_id);
   EXPECT_EQ(result->InstallTime(), install_time);
@@ -318,9 +336,13 @@ TEST_F(GuestOsRegistryServiceTest, InstallAndLaunchTime) {
   // The install time shouldn't change if fields change.
   test_clock_.Advance(base::TimeDelta::FromHours(1));
   app_list.mutable_apps(0)->set_no_display(true);
-  EXPECT_CALL(observer,
-              OnRegistryUpdated(service(), testing::ElementsAre(app_id),
-                                testing::IsEmpty(), testing::IsEmpty()));
+  EXPECT_CALL(
+      observer,
+      OnRegistryUpdated(
+          service(),
+          GuestOsRegistryService::VmType::ApplicationList_VmType_TERMINA,
+          testing::ElementsAre(app_id), testing::IsEmpty(),
+          testing::IsEmpty()));
   service()->UpdateApplicationList(app_list);
   result = service()->GetRegistration(app_id);
   EXPECT_EQ(result->InstallTime(), install_time);