Garbage-collect default-installed web apps

This uninstalls (on log in) those web apps that were previously
automatically installed due to being listed in an external .json file,
but are no longer listed.

This does for default-installed web apps what
https://chromium-review.googlesource.com/1195293 "policy: Uninstall apps
that are no longer forced installed" does for policy-installed web apps.

Bug: 855281
Change-Id: I4125e60f4a6b9c6033d78ca54d0005be45d06e82
Reviewed-on: https://chromium-review.googlesource.com/1198644Reviewed-by: Dominick Ng <dominickn@chromium.org>
Reviewed-by: Kyle Horimoto <khorimoto@chromium.org>
Commit-Queue: Nigel Tao <nigeltao@chromium.org>
Cr-Commit-Position: refs/heads/master@{#592651}

chrome/browser/chromeos/multidevice_setup/android_sms_app_helper_delegate_impl_unittest.cc

@@ -55,7 +55,7 @@ TEST_F(AndroidSmsAppHelperDelegateImplTest, TestInstallMessagesApp) {
       web_app::PendingAppManager::LaunchContainer::kWindow,
       web_app::PendingAppManager::InstallSource::kInternal));
   EXPECT_EQ(expected_apps_to_install,
-            test_pending_app_manager()->installed_apps());
+            test_pending_app_manager()->install_requests());
 }
 
 }  // namespace multidevice_setup

chrome/browser/web_applications/bookmark_apps/policy/web_app_policy_manager.cc

@@ -8,7 +8,6 @@
 #include <vector>
 
 #include "base/bind.h"
-#include "base/stl_util.h"
 #include "base/values.h"
 #include "build/build_config.h"
 #include "chrome/browser/profiles/profile.h"
@@ -71,22 +70,12 @@ void WebAppPolicyManager::InitChangeRegistrarAndRefreshPolicyInstalledApps() {
       base::BindRepeating(&WebAppPolicyManager::RefreshPolicyInstalledApps,
                           weak_ptr_factory_.GetWeakPtr()));
 
-  // Populate `last_app_urls_` with the current policy-installed apps so that
-  // we can uninstall any apps that are no longer in the policy.
-  last_app_urls_ = ExtensionIdsMap::GetInstalledAppUrls(
-      profile_, PendingAppManager::InstallSource::kExternalPolicy);
-
-  // Sort so that we can later use base::STLSetDifference.
-  std::sort(last_app_urls_.begin(), last_app_urls_.end());
-
   RefreshPolicyInstalledApps();
 }
 
 void WebAppPolicyManager::RefreshPolicyInstalledApps() {
   const base::Value* web_apps =
       pref_service_->GetList(prefs::kWebAppInstallForceList);
-
-  std::vector<GURL> app_urls;
   std::vector<PendingAppManager::AppInfo> apps_to_install;
   for (const base::Value& info : web_apps->GetList()) {
     const base::Value& url = *info.FindKey(kUrlKey);
@@ -109,18 +98,11 @@ void WebAppPolicyManager::RefreshPolicyInstalledApps() {
         GURL(url.GetString()), container,
         web_app::PendingAppManager::InstallSource::kExternalPolicy,
         false /* create_shortcuts */);
-    app_urls.emplace_back(url.GetString());
   }
 
-  pending_app_manager_->InstallApps(std::move(apps_to_install),
-                                    base::DoNothing());
-  std::sort(app_urls.begin(), app_urls.end());
-
-  auto apps_to_uninstall =
-      base::STLSetDifference<std::vector<GURL>>(last_app_urls_, app_urls);
-  pending_app_manager_->UninstallApps(std::move(apps_to_uninstall),
-                                      base::DoNothing());
-  last_app_urls_.swap(app_urls);
+  pending_app_manager_->SynchronizeInstalledApps(
+      std::move(apps_to_install),
+      PendingAppManager::InstallSource::kExternalPolicy);
 }
 
 }  // namespace web_app

chrome/browser/web_applications/bookmark_apps/policy/web_app_policy_manager.h

@@ -51,11 +51,6 @@ class WebAppPolicyManager {
 
   PrefChangeRegistrar pref_change_registrar_;
 
-  // URLs of the apps that this class last installed. Populated with the current
-  // policy-installed apps at start up and replaced every time this class calls
-  // PendingAppManager::InstallApps().
-  std::vector<GURL> last_app_urls_;
-
   base::WeakPtrFactory<WebAppPolicyManager> weak_ptr_factory_{this};
 
   DISALLOW_COPY_AND_ASSIGN(WebAppPolicyManager);

chrome/browser/web_applications/bookmark_apps/policy/web_app_policy_manager_unittest.cc

@@ -97,6 +97,7 @@ class WebAppPolicyManagerTest : public ChromeRenderViewHostTestHarness {
   }
 
   void SimulatePreviouslyInstalledApp(
+      TestPendingAppManager* pending_app_manager,
       GURL url,
       PendingAppManager::InstallSource install_source) {
     std::string id = GenerateFakeExtensionId(url);
@@ -105,6 +106,8 @@ class WebAppPolicyManagerTest : public ChromeRenderViewHostTestHarness {
 
     ExtensionIdsMap extension_ids_map(profile()->GetPrefs());
     extension_ids_map.Insert(url, id, install_source);
+
+    pending_app_manager->SimulatePreviouslyInstalledApp(url, install_source);
   }
 
  private:
@@ -117,7 +120,7 @@ TEST_F(WebAppPolicyManagerTest, NoForceInstalledAppsPrefValue) {
                                              pending_app_manager.get());
   base::RunLoop().RunUntilIdle();
 
-  const auto& apps_to_install = pending_app_manager->installed_apps();
+  const auto& apps_to_install = pending_app_manager->install_requests();
   EXPECT_TRUE(apps_to_install.empty());
 }
 
@@ -130,7 +133,7 @@ TEST_F(WebAppPolicyManagerTest, NoForceInstalledApps) {
                                              pending_app_manager.get());
   base::RunLoop().RunUntilIdle();
 
-  const auto& apps_to_install = pending_app_manager->installed_apps();
+  const auto& apps_to_install = pending_app_manager->install_requests();
   EXPECT_TRUE(apps_to_install.empty());
 }
 
@@ -146,7 +149,7 @@ TEST_F(WebAppPolicyManagerTest, TwoForceInstalledApps) {
                                              pending_app_manager.get());
   base::RunLoop().RunUntilIdle();
 
-  const auto& apps_to_install = pending_app_manager->installed_apps();
+  const auto& apps_to_install = pending_app_manager->install_requests();
 
   std::vector<PendingAppManager::AppInfo> expected_apps_to_install;
   expected_apps_to_install.push_back(GetWindowedAppInfo());
@@ -165,7 +168,7 @@ TEST_F(WebAppPolicyManagerTest, ForceInstallAppWithNoForcedLaunchContainer) {
                                              pending_app_manager.get());
   base::RunLoop().RunUntilIdle();
 
-  const auto& apps_to_install = pending_app_manager->installed_apps();
+  const auto& apps_to_install = pending_app_manager->install_requests();
 
   std::vector<PendingAppManager::AppInfo> expected_apps_to_install;
   expected_apps_to_install.push_back(GetDefaultContainerAppInfo());
@@ -184,7 +187,7 @@ TEST_F(WebAppPolicyManagerTest, DynamicRefresh) {
                                              pending_app_manager.get());
   base::RunLoop().RunUntilIdle();
 
-  const auto& apps_to_install = pending_app_manager->installed_apps();
+  const auto& apps_to_install = pending_app_manager->install_requests();
 
   std::vector<PendingAppManager::AppInfo> expected_apps_to_install;
   expected_apps_to_install.push_back(GetWindowedAppInfo());
@@ -204,13 +207,18 @@ TEST_F(WebAppPolicyManagerTest, DynamicRefresh) {
 }
 
 TEST_F(WebAppPolicyManagerTest, UninstallAppInstalledInPreviousSession) {
+  auto pending_app_manager = std::make_unique<TestPendingAppManager>();
+
   // Simulate two policy apps and a regular app that were installed in the
   // previous session.
   SimulatePreviouslyInstalledApp(
-      GURL(kWindowedUrl), PendingAppManager::InstallSource::kExternalPolicy);
+      pending_app_manager.get(), GURL(kWindowedUrl),
+      PendingAppManager::InstallSource::kExternalPolicy);
   SimulatePreviouslyInstalledApp(
-      GURL(kTabbedUrl), PendingAppManager::InstallSource::kExternalPolicy);
-  SimulatePreviouslyInstalledApp(GURL(kDefaultContainerUrl),
+      pending_app_manager.get(), GURL(kTabbedUrl),
+      PendingAppManager::InstallSource::kExternalPolicy);
+  SimulatePreviouslyInstalledApp(pending_app_manager.get(),
+                                 GURL(kDefaultContainerUrl),
                                  PendingAppManager::InstallSource::kInternal);
 
   // Push a policy with only one of the apps.
@@ -219,7 +227,6 @@ TEST_F(WebAppPolicyManagerTest, UninstallAppInstalledInPreviousSession) {
   profile()->GetPrefs()->Set(prefs::kWebAppInstallForceList,
                              std::move(first_list));
 
-  auto pending_app_manager = std::make_unique<TestPendingAppManager>();
   WebAppPolicyManager web_app_policy_manager(profile(),
                                              pending_app_manager.get());
   base::RunLoop().RunUntilIdle();
@@ -227,11 +234,11 @@ TEST_F(WebAppPolicyManagerTest, UninstallAppInstalledInPreviousSession) {
   // We should only try to install the app in the policy.
   std::vector<PendingAppManager::AppInfo> expected_apps_to_install;
   expected_apps_to_install.push_back(GetWindowedAppInfo());
-  EXPECT_EQ(pending_app_manager->installed_apps(), expected_apps_to_install);
+  EXPECT_EQ(pending_app_manager->install_requests(), expected_apps_to_install);
 
   // We should try to uninstall the app that is no longer in the policy.
   EXPECT_EQ(std::vector<GURL>({GURL(kTabbedUrl)}),
-            pending_app_manager->uninstalled_apps());
+            pending_app_manager->uninstall_requests());
 }
 
 // Tests that we correctly uninstall an app that we installed in the same
@@ -250,7 +257,7 @@ TEST_F(WebAppPolicyManagerTest, UninstallAppInstalledInCurrentSession) {
                              std::move(first_list));
   base::RunLoop().RunUntilIdle();
 
-  const auto& apps_to_install = pending_app_manager->installed_apps();
+  const auto& apps_to_install = pending_app_manager->install_requests();
 
   std::vector<PendingAppManager::AppInfo> expected_apps_to_install;
   expected_apps_to_install.push_back(GetWindowedAppInfo());
@@ -272,7 +279,7 @@ TEST_F(WebAppPolicyManagerTest, UninstallAppInstalledInCurrentSession) {
   EXPECT_EQ(apps_to_install, expected_apps_to_install);
 
   EXPECT_EQ(std::vector<GURL>({GURL(kTabbedUrl)}),
-            pending_app_manager->uninstalled_apps());
+            pending_app_manager->uninstall_requests());
 }
 
 }  // namespace web_app

chrome/browser/web_applications/components/BUILD.gn

@@ -70,6 +70,7 @@ source_set("unit_tests") {
   testonly = true
 
   sources = [
+    "pending_app_manager_unittest.cc",
     "web_app_helpers_unittest.cc",
     "web_app_icon_downloader_unittest.cc",
     "web_app_icon_generator_unittest.cc",
@@ -84,6 +85,7 @@ source_set("unit_tests") {
 
   deps = [
     ":components",
+    ":test_support",
     "//base/test:test_support",
     "//chrome/app/theme:theme_resources",
     "//chrome/browser/web_applications:web_app_group",

chrome/browser/web_applications/components/pending_app_manager.cc

@@ -4,9 +4,13 @@
 
 #include "chrome/browser/web_applications/components/pending_app_manager.h"
 
+#include <algorithm>
 #include <memory>
 #include <utility>
 
+#include "base/bind_helpers.h"
+#include "base/stl_util.h"
+
 namespace web_app {
 
 PendingAppManager::AppInfo::AppInfo(GURL url,
@@ -42,6 +46,29 @@ PendingAppManager::PendingAppManager() = default;
 
 PendingAppManager::~PendingAppManager() = default;
 
+void PendingAppManager::SynchronizeInstalledApps(
+    std::vector<AppInfo> desired_apps,
+    InstallSource install_source) {
+  DCHECK(std::all_of(desired_apps.begin(), desired_apps.end(),
+                     [&install_source](const AppInfo& app_info) {
+                       return app_info.install_source == install_source;
+                     }));
+
+  std::vector<GURL> current_urls = GetInstalledAppUrls(install_source);
+  std::sort(current_urls.begin(), current_urls.end());
+
+  std::vector<GURL> desired_urls;
+  for (const auto& info : desired_apps) {
+    desired_urls.emplace_back(info.url);
+  }
+  std::sort(desired_urls.begin(), desired_urls.end());
+
+  UninstallApps(
+      base::STLSetDifference<std::vector<GURL>>(current_urls, desired_urls),
+      base::DoNothing());
+  InstallApps(std::move(desired_apps), base::DoNothing());
+}
+
 std::ostream& operator<<(std::ostream& out,
                          const PendingAppManager::AppInfo& app_info) {
   return out << "url: " << app_info.url << "\n launch_container: "

chrome/browser/web_applications/components/pending_app_manager.h

@@ -70,16 +70,7 @@ class PendingAppManager {
   // that it otherwise doesn't recognize.
   //
   // In practice, every kExternalXxx enum definition should correspond to
-  // exactly one place in the code where InstallUninstallApps is called.
-  //
-  // TODO(nigeltao): PendingAppManager::InstallUninstallApps will land in
-  // https://crrev.com/c/1198644 "Garbage-collect default-installed web apps"
-  // which is on hold precisely because, as written, there is no distinction
-  // between kInternal and kExternalDefault, so that
-  // WebAppProvider::OnScanForExternalWebApps (which will correspond to
-  // kExternalDefault after this commit) will inadvertently uninstall the
-  // AndroidSmsAppHelperDelegateImpl::InstallAndroidSmsApp app (which will
-  // correspond to kInternal).
+  // exactly one place in the code where SynchronizeInstalledApps is called.
   enum class InstallSource {
     // Do not remove or re-order the names, only append to the end. Their
     // integer values are persisted in the preferences.
@@ -88,13 +79,13 @@ class PendingAppManager {
     // Installed by default on the system, such as "all such-and-such make and
     // model Chromebooks should have this app installed".
     //
-    // The corresponding InstallUninstallApps call site is in
+    // The corresponding SynchronizeInstalledApps call site is in
     // WebAppProvider::OnScanForExternalWebApps.
     kExternalDefault = 1,
     // Installed by sys-admin policy, such as "all example.com employees should
     // have this app installed".
     //
-    // The corresponding InstallUninstallApps call site is in
+    // The corresponding SynchronizeInstalledApps call site is in
     // WebAppPolicyManager::RefreshPolicyInstalledApps.
     kExternalPolicy = 2,
   };
@@ -150,6 +141,21 @@ class PendingAppManager {
   virtual void UninstallApps(std::vector<GURL> apps_to_uninstall,
                              const UninstallCallback& callback) = 0;
 
+  // Returns the URLs of those apps installed from |install_source|.
+  virtual std::vector<GURL> GetInstalledAppUrls(
+      InstallSource install_source) const = 0;
+
+  // Installs |desired_apps| and uninstalls any apps in
+  // GetInstalledAppUrls(install_source) that are not in |desired_apps|'s URLs.
+  //
+  // All apps in |desired_apps| should have |install_source| as their source.
+  //
+  // Note that this returns after queueing work (installation and
+  // uninstallation) to be done. It does not wait until that work is complete.
+  void SynchronizeInstalledApps(std::vector<AppInfo> desired_apps,
+                                InstallSource install_source);
+
+ private:
   DISALLOW_COPY_AND_ASSIGN(PendingAppManager);
 };
 

chrome/browser/web_applications/components/pending_app_manager_unittest.cc

+// Copyright 2018 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "chrome/browser/web_applications/components/pending_app_manager.h"
+
+#include <algorithm>
+#include <sstream>
+#include <vector>
+
+#include "chrome/browser/web_applications/components/test_pending_app_manager.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace web_app {
+
+class PendingAppManagerTest : public testing::Test {
+ protected:
+  void Sync(std::vector<GURL> urls) {
+    pending_app_manager_.ResetCounts();
+
+    std::vector<PendingAppManager::AppInfo> app_infos;
+    for (const auto& url : urls) {
+      app_infos.emplace_back(url, PendingAppManager::LaunchContainer::kWindow,
+                             PendingAppManager::InstallSource::kInternal);
+    }
+    pending_app_manager_.SynchronizeInstalledApps(
+        std::move(app_infos), PendingAppManager::InstallSource::kInternal);
+  }
+
+  void Expect(int deduped_install_count,
+              int deduped_uninstall_count,
+              std::vector<GURL> installed_app_urls) {
+    EXPECT_EQ(deduped_install_count,
+              pending_app_manager_.deduped_install_count());
+    EXPECT_EQ(deduped_uninstall_count,
+              pending_app_manager_.deduped_uninstall_count());
+
+    std::vector<GURL> urls = pending_app_manager_.GetInstalledAppUrls(
+        PendingAppManager::InstallSource::kInternal);
+    std::sort(urls.begin(), urls.end());
+    EXPECT_EQ(installed_app_urls, urls);
+  }
+
+  TestPendingAppManager pending_app_manager_;
+};
+
+TEST_F(PendingAppManagerTest, SynchronizeInstalledApps) {
+  GURL a("https://a.example.com/");
+  GURL b("https://b.example.com/");
+  GURL c("https://c.example.com/");
+  GURL d("https://d.example.com/");
+  GURL e("https://e.example.com/");
+
+  Sync(std::vector<GURL>{a, b, d});
+  Expect(3, 0, std::vector<GURL>{a, b, d});
+
+  Sync(std::vector<GURL>{b, e});
+  Expect(1, 2, std::vector<GURL>{b, e});
+
+  Sync(std::vector<GURL>{e});
+  Expect(0, 1, std::vector<GURL>{e});
+
+  Sync(std::vector<GURL>{c});
+  Expect(1, 1, std::vector<GURL>{c});
+
+  Sync(std::vector<GURL>{e, a, d});
+  Expect(3, 1, std::vector<GURL>{a, d, e});
+
+  Sync(std::vector<GURL>{c, a, b, d, e});
+  Expect(2, 0, std::vector<GURL>{a, b, c, d, e});
+
+  Sync(std::vector<GURL>{});
+  Expect(0, 5, std::vector<GURL>{});
+
+  // The remaining code tests duplicate inputs.
+
+  Sync(std::vector<GURL>{b, a, b, c});
+  Expect(3, 0, std::vector<GURL>{a, b, c});
+
+  Sync(std::vector<GURL>{e, a, e, e, e, a});
+  Expect(1, 2, std::vector<GURL>{a, e});
+
+  Sync(std::vector<GURL>{b, c, d});
+  Expect(3, 2, std::vector<GURL>{b, c, d});
+
+  Sync(std::vector<GURL>{a, a, a, a, a, a});
+  Expect(1, 3, std::vector<GURL>{a});
+
+  Sync(std::vector<GURL>{});
+  Expect(0, 1, std::vector<GURL>{});
+}
+
+}  // namespace web_app

chrome/browser/web_applications/components/test_pending_app_manager.cc

@@ -4,6 +4,7 @@
 
 #include "chrome/browser/web_applications/components/test_pending_app_manager.h"
 
+#include <string>
 #include <utility>
 
 #include "base/callback.h"
@@ -12,17 +13,30 @@
 
 namespace web_app {
 
-TestPendingAppManager::TestPendingAppManager() = default;
+TestPendingAppManager::TestPendingAppManager()
+    : deduped_install_count_(0), deduped_uninstall_count_(0) {}
 
 TestPendingAppManager::~TestPendingAppManager() = default;
 
+void TestPendingAppManager::SimulatePreviouslyInstalledApp(
+    const GURL& url,
+    InstallSource install_source) {
+  installed_apps_[url] = install_source;
+}
+
 void TestPendingAppManager::Install(AppInfo app_to_install,
                                     OnceInstallCallback callback) {
   // TODO(nigeltao): Add error simulation when error codes are added to the API.
 
-  const GURL url(app_to_install.url);
-  installed_apps_.push_back(std::move(app_to_install));
-  std::move(callback).Run(url, InstallResultCode::kSuccess);
+  auto i = installed_apps_.find(app_to_install.url);
+  if (i == installed_apps_.end()) {
+    installed_apps_[app_to_install.url] = app_to_install.install_source;
+    deduped_install_count_++;
+  }
+
+  install_requests_.push_back(std::move(app_to_install));
+  std::move(callback).Run(install_requests().back().url,
+                          InstallResultCode::kSuccess);
 }
 
 void TestPendingAppManager::InstallApps(
@@ -32,13 +46,29 @@ void TestPendingAppManager::InstallApps(
     Install(std::move(app), callback);
 }
 
-void TestPendingAppManager::UninstallApps(std::vector<GURL> apps_to_uninstall,
+void TestPendingAppManager::UninstallApps(std::vector<GURL> urls_to_uninstall,
                                           const UninstallCallback& callback) {
-  for (auto& app : apps_to_uninstall) {
-    const GURL url(app);
-    uninstalled_apps_.push_back(std::move(app));
+  for (auto& url : urls_to_uninstall) {
+    auto i = installed_apps_.find(url);
+    if (i != installed_apps_.end()) {
+      installed_apps_.erase(i);
+      deduped_uninstall_count_++;
+    }
+
+    uninstall_requests_.push_back(url);
     callback.Run(url, true /* succeeded */);
   }
 }
 
+std::vector<GURL> TestPendingAppManager::GetInstalledAppUrls(
+    InstallSource install_source) const {
+  std::vector<GURL> urls;
+  for (auto& it : installed_apps_) {
+    if (it.second == install_source) {
+      urls.emplace_back(it.first);
+    }
+  }
+  return urls;
+}
+
 }  // namespace web_app

chrome/browser/web_applications/components/test_pending_app_manager.h

@@ -5,7 +5,7 @@
 #ifndef CHROME_BROWSER_WEB_APPLICATIONS_COMPONENTS_TEST_PENDING_APP_MANAGER_H_
 #define CHROME_BROWSER_WEB_APPLICATIONS_COMPONENTS_TEST_PENDING_APP_MANAGER_H_
 
-#include <string>
+#include <map>
 #include <vector>
 
 #include "base/macros.h"
@@ -18,21 +18,46 @@ class TestPendingAppManager : public PendingAppManager {
  public:
   TestPendingAppManager();
   ~TestPendingAppManager() override;
-  const std::vector<AppInfo>& installed_apps() const { return installed_apps_; }
-  const std::vector<GURL>& uninstalled_apps() const {
-    return uninstalled_apps_;
+
+  // The foo_requests methods may return duplicates, if the underlying
+  // InstallApps or UninstallApps arguments do. The deduped_foo_count methods
+  // only count new installs or new uninstalls.
+
+  const std::vector<AppInfo>& install_requests() const {
+    return install_requests_;
+  }
+  const std::vector<GURL>& uninstall_requests() const {
+    return uninstall_requests_;
   }
 
+  int deduped_install_count() const { return deduped_install_count_; }
+  int deduped_uninstall_count() const { return deduped_uninstall_count_; }
+
+  void ResetCounts() {
+    deduped_install_count_ = 0;
+    deduped_uninstall_count_ = 0;
+  }
+
+  void SimulatePreviouslyInstalledApp(const GURL& url,
+                                      InstallSource install_source);
+
   // PendingAppManager:
   void Install(AppInfo app_to_install, OnceInstallCallback callback) override;
   void InstallApps(std::vector<AppInfo> apps_to_install,
                    const RepeatingInstallCallback& callback) override;
-  void UninstallApps(std::vector<GURL> apps_to_uninstall,
+  void UninstallApps(std::vector<GURL> urls_to_uninstall,
                      const UninstallCallback& callback) override;
+  std::vector<GURL> GetInstalledAppUrls(
+      InstallSource install_source) const override;
 
  private:
-  std::vector<AppInfo> installed_apps_;
-  std::vector<GURL> uninstalled_apps_;
+  std::vector<AppInfo> install_requests_;
+  std::vector<GURL> uninstall_requests_;
+
+  int deduped_install_count_;
+  int deduped_uninstall_count_;
+
+  std::map<GURL, InstallSource> installed_apps_;
 
   DISALLOW_COPY_AND_ASSIGN(TestPendingAppManager);
 };

chrome/browser/web_applications/extensions/pending_bookmark_app_manager.cc

@@ -75,14 +75,6 @@ void PendingBookmarkAppManager::Install(AppInfo app_to_install,
                      weak_ptr_factory_.GetWeakPtr()));
 }
 
-// TODO(nigeltao/ortuno): clarify whether the apps_to_install is relative or
-// absolute: in C++ terminology, an analogy is += versus = operators. Should we
-// install these apps *in addition to* what's already installed, or should we
-// make the list of installed apps is identical to the argument? If the former,
-// we also need a way to tell the PendingBookmarkAppManager to uninstall apps.
-// If the latter, we also need to pass the install source (a Manifest::Location
-// or something similar), so that "the list of policy-installed apps" doesn't
-// interfere with "the list of default-installed apps".
 void PendingBookmarkAppManager::InstallApps(
     std::vector<AppInfo> apps_to_install,
     const RepeatingInstallCallback& callback) {
@@ -131,6 +123,12 @@ void PendingBookmarkAppManager::UninstallApps(
   }
 }
 
+std::vector<GURL> PendingBookmarkAppManager::GetInstalledAppUrls(
+    InstallSource install_source) const {
+  return web_app::ExtensionIdsMap::GetInstalledAppUrls(profile_,
+                                                       install_source);
+}
+
 void PendingBookmarkAppManager::SetFactoriesForTesting(
     WebContentsFactory web_contents_factory,
     TaskFactory task_factory) {

chrome/browser/web_applications/extensions/pending_bookmark_app_manager.h

@@ -52,6 +52,8 @@ class PendingBookmarkAppManager final : public web_app::PendingAppManager,
                    const RepeatingInstallCallback& callback) override;
   void UninstallApps(std::vector<GURL> apps_to_uninstall,
                      const UninstallCallback& callback) override;
+  std::vector<GURL> GetInstalledAppUrls(
+      InstallSource install_source) const override;
 
   void SetFactoriesForTesting(WebContentsFactory web_contents_factory,
                               TaskFactory task_factory);

chrome/browser/web_applications/web_app_provider.cc

@@ -69,7 +69,8 @@ void WebAppProvider::Observe(int type,
 
 void WebAppProvider::OnScanForExternalWebApps(
     std::vector<web_app::PendingAppManager::AppInfo> app_infos) {
-  pending_app_manager_->InstallApps(std::move(app_infos), base::DoNothing());
+  pending_app_manager_->SynchronizeInstalledApps(
+      std::move(app_infos), PendingAppManager::InstallSource::kExternalDefault);
 }
 
 }  // namespace web_app