Make ApkWebAppInstallService handle upgrades to and from web apps.

An ARC app may begin as a non-web app, but then be upgraded to be a web
app. The opposite may also happen: an ARC app may be a web app, and then
be upgraded to not be a web app. For each of these scenarios, we need to
ensure that the browser-side web app is appropriately installed or
uninstalled.

This CL adds a new override of ArcAppListPrefs::OnPackageModified to
listen for apps that are upgraded into or from being web apps, and
trigger the appropriate installation or uninstallation. A new test is
added to verify the behaviour.

BUG=944972

Change-Id: I9e8cd04c0ceb87ca678eac0896ebdf6dde48938b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1536041Reviewed-by: Ben Wells <benwells@chromium.org>
Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Commit-Queue: Dominick Ng <dominickn@chromium.org>
Cr-Commit-Position: refs/heads/master@{#647107}

chrome/browser/chromeos/apps/apk_web_app_installer_browsertest.cc

@@ -91,8 +91,18 @@ class ApkWebAppInstallerBrowserTest
 
   void TearDownOnMainThread() override { DisableArc(); }
 
-  arc::mojom::ArcPackageInfoPtr GetPackage(const std::string& package_name,
-                                           const std::string& app_title) {
+  arc::mojom::ArcPackageInfoPtr GetWebAppPackage(
+      const std::string& package_name,
+      const std::string& app_title) {
+    auto package = GetArcAppPackage(package_name, app_title);
+    package->web_app_info = GetWebAppInfo(app_title);
+
+    return package;
+  }
+
+  arc::mojom::ArcPackageInfoPtr GetArcAppPackage(
+      const std::string& package_name,
+      const std::string& app_title) {
     auto package = arc::mojom::ArcPackageInfo::New();
     package->package_name = package_name;
     package->package_version = 1;
@@ -101,8 +111,6 @@ class ApkWebAppInstallerBrowserTest
     package->sync = true;
     package->system = false;
 
-    package->web_app_info = GetWebAppInfo(app_title);
-
     return package;
   }
 
@@ -144,6 +152,14 @@ class ApkWebAppInstallerBrowserTest
       std::move(quit_closure_).Run();
   }
 
+  void Reset() {
+    removed_package_ = "";
+    installed_extension_ = nullptr;
+    uninstalled_extension_ = nullptr;
+    reason_ = extensions::UNINSTALL_REASON_FOR_TESTING;
+    is_update_ = base::nullopt;
+  }
+
  protected:
   ArcAppListPrefs* arc_app_list_prefs_ = nullptr;
   std::unique_ptr<arc::FakeAppInstance> app_instance_;
@@ -173,7 +189,7 @@ IN_PROC_BROWSER_TEST_F(ApkWebAppInstallerBrowserTest, InstallAndUninstall) {
   observer.Add(extensions::ExtensionRegistry::Get(browser()->profile()));
   ApkWebAppService* service = ApkWebAppService::Get(browser()->profile());
   service->SetArcAppListPrefsForTesting(arc_app_list_prefs_);
-  app_instance_->SendPackageAdded(GetPackage(kPackageName, kAppTitle));
+  app_instance_->SendPackageAdded(GetWebAppPackage(kPackageName, kAppTitle));
 
   WaitForQuit();
 
@@ -199,7 +215,7 @@ IN_PROC_BROWSER_TEST_F(ApkWebAppInstallerBrowserTest, PackageListRefreshed) {
   ApkWebAppService* service = ApkWebAppService::Get(browser()->profile());
   service->SetArcAppListPrefsForTesting(arc_app_list_prefs_);
   std::vector<arc::mojom::ArcPackageInfoPtr> packages;
-  packages.push_back(GetPackage(kPackageName, kAppTitle));
+  packages.push_back(GetWebAppPackage(kPackageName, kAppTitle));
   app_instance_->SendRefreshPackageList(std::move(packages));
 
   WaitForQuit();
@@ -238,13 +254,13 @@ IN_PROC_BROWSER_TEST_F(ApkWebAppInstallerDelayedArcStartBrowserTest,
 
   // Start up ARC and set the package to be installed.
   EnableArc();
-  app_instance_->SendPackageAdded(GetPackage(kPackageName, kAppTitle));
+  app_instance_->SendPackageAdded(GetWebAppPackage(kPackageName, kAppTitle));
 
   // Trigger a package refresh, which should call to ARC to remove the package.
   arc_app_list_prefs_->AddObserver(this);
   service->SetArcAppListPrefsForTesting(arc_app_list_prefs_);
   std::vector<arc::mojom::ArcPackageInfoPtr> packages;
-  packages.push_back(GetPackage(kPackageName, kAppTitle));
+  packages.push_back(GetWebAppPackage(kPackageName, kAppTitle));
   app_instance_->SendRefreshPackageList(std::move(packages));
 
   EXPECT_EQ(kPackageName, removed_package_);
@@ -253,4 +269,46 @@ IN_PROC_BROWSER_TEST_F(ApkWebAppInstallerDelayedArcStartBrowserTest,
   DisableArc();
 }
 
+// Test an upgrade that becomes a web app and then stops being a web app.
+IN_PROC_BROWSER_TEST_F(ApkWebAppInstallerBrowserTest,
+                       UpgradeToWebAppAndToArcApp) {
+  ScopedObserver<extensions::ExtensionRegistry,
+                 extensions::ExtensionRegistryObserver>
+      observer(this);
+  observer.Add(extensions::ExtensionRegistry::Get(browser()->profile()));
+  ApkWebAppService* service = ApkWebAppService::Get(browser()->profile());
+  service->SetArcAppListPrefsForTesting(arc_app_list_prefs_);
+  app_instance_->SendPackageAdded(GetArcAppPackage(kPackageName, kAppTitle));
+
+  EXPECT_FALSE(installed_extension_);
+  EXPECT_FALSE(uninstalled_extension_);
+
+  // Send a second package added call from ARC, upgrading the package to a web
+  // app.
+  app_instance_->SendPackageAdded(GetWebAppPackage(kPackageName, kAppTitle));
+  WaitForQuit();
+
+  EXPECT_TRUE(installed_extension_);
+  EXPECT_EQ(kAppTitle, installed_extension_->name());
+  EXPECT_FALSE(is_update_.value());
+
+  // Send an package added call from ARC, upgrading the package to not be a
+  // web app. The extension should be synchronously uninstalled.
+  app_instance_->SendPackageAdded(GetArcAppPackage(kPackageName, kAppTitle));
+
+  EXPECT_TRUE(uninstalled_extension_);
+  EXPECT_EQ(kAppTitle, uninstalled_extension_->name());
+  EXPECT_EQ(extensions::UNINSTALL_REASON_ARC, reason_);
+
+  Reset();
+
+  // Upgrade the package to a web app again and make sure it is installed again.
+  app_instance_->SendPackageAdded(GetWebAppPackage(kPackageName, kAppTitle));
+  WaitForQuit();
+
+  EXPECT_TRUE(installed_extension_);
+  EXPECT_EQ(kAppTitle, installed_extension_->name());
+  EXPECT_FALSE(is_update_.value());
+}
+
 }  // namespace chromeos

chrome/browser/chromeos/apps/apk_web_app_service.cc

@@ -113,9 +113,41 @@ void ApkWebAppService::Shutdown() {
 
 void ApkWebAppService::OnPackageInstalled(
     const arc::mojom::ArcPackageInfo& package_info) {
-  if (package_info.web_app_info.is_null())
+  // This method is called when a) new packages are installed, and b) existing
+  // packages are updated. In (b), there are two cases to handle: the package
+  // could previously have been an Android app and has now become a web app, and
+  // vice-versa.
+  DictionaryPrefUpdate web_apps_to_apks(profile_->GetPrefs(),
+                                        kWebAppToApkDictPref);
+
+  // Search the pref dict for any |web_app_id| that has a value matching the
+  // provided package name.
+  std::string web_app_id;
+  for (const auto& it : web_apps_to_apks->DictItems()) {
+    const base::Value* v =
+        it.second.FindKeyOfType(kPackageNameKey, base::Value::Type::STRING);
+
+    if (v && (v->GetString() == package_info.package_name)) {
+      web_app_id = it.first;
+      break;
+    }
+  }
+
+  bool was_previously_web_app = !web_app_id.empty();
+  bool is_now_web_app = !package_info.web_app_info.is_null();
+
+  // The previous and current states match. Nothing to do.
+  if (is_now_web_app == was_previously_web_app)
+    return;
+
+  if (was_previously_web_app) {
+    // The package was a web app, but now isn't. Remove the web app.
+    OnPackageRemoved(package_info.package_name, true /* uninstalled */);
     return;
+  }
 
+  // The package is a web app but we don't have a corresponding browser-side
+  // artifact. Install it.
   auto* instance = ARC_GET_INSTANCE_FOR_METHOD(
       arc_app_list_prefs_->app_connection_holder(), RequestPackageIcon);
   if (!instance)

chrome/browser/ui/app_list/arc/arc_app_list_prefs.h

@@ -169,7 +169,10 @@ class ArcAppListPrefs : public KeyedService,
 
     virtual void OnNotificationsEnabledChanged(
         const std::string& package_name, bool enabled) {}
-    // Notifies that package has been installed.
+    // Notifies that package has been installed. This may be called in two
+    // cases:
+    // a) the package is being newly installed
+    // b) the package is already installed, and is being updated
     virtual void OnPackageInstalled(
         const arc::mojom::ArcPackageInfo& package_info) {}
     // Notifies that package has been modified.