[WebAppProvider] Fixing OS hooks to not be called for sync install

This patch adds a check for |locally_installed| in the post-finalize
step of the WebAppInstallTask callback chain. Instead of calling
InstallOsHooks, it skips that (as the web app is not locally installed).

This also updates the TestOsIntegrationManager to store a little more
data, and make the TestWebAppProviderCreator take a RepeatingCallback
to support multiple profiles.

TBR: loyso@chromium.org
Bug: 1143023
Change-Id: I18f3864e909980a892f6f669fa4029b098b43d01
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2505685
Commit-Queue: Daniel Murphy <dmurph@chromium.org>
Reviewed-by: Phillis Tang <phillis@chromium.org>
Cr-Commit-Position: refs/heads/master@{#824979}

chrome/browser/sync/test/integration/two_client_web_apps_bmo_sync_test.cc

@@ -2,6 +2,7 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+#include <memory>
 #include "base/bind.h"
 #include "base/macros.h"
 #include "base/strings/utf_string_conversions.h"
@@ -19,6 +20,8 @@
 #include "chrome/browser/web_applications/components/install_finalizer.h"
 #include "chrome/browser/web_applications/components/install_manager.h"
 #include "chrome/browser/web_applications/components/os_integration_manager.h"
+#include "chrome/browser/web_applications/test/test_os_integration_manager.h"
+#include "chrome/browser/web_applications/test/test_web_app_provider.h"
 #include "chrome/browser/web_applications/test/web_app_install_observer.h"
 #include "chrome/browser/web_applications/test/web_app_test.h"
 #include "chrome/browser/web_applications/web_app.h"
@@ -34,19 +37,28 @@
 namespace web_app {
 namespace {
 
+std::unique_ptr<KeyedService> CreateTestWebAppProvider(Profile* profile) {
+  auto provider = std::make_unique<TestWebAppProvider>(profile);
+  provider->SetOsIntegrationManager(
+      std::make_unique<TestOsIntegrationManager>(profile, nullptr, nullptr));
+  provider->Start();
+  DCHECK(provider);
+  return provider;
+}
+
 class TwoClientWebAppsBMOSyncTest : public SyncTest {
  public:
-  TwoClientWebAppsBMOSyncTest() : SyncTest(TWO_CLIENT) {}
+  TwoClientWebAppsBMOSyncTest()
+      : SyncTest(TWO_CLIENT),
+        test_web_app_provider_creator_(
+            std::make_unique<TestWebAppProviderCreator>(
+                base::BindRepeating(&CreateTestWebAppProvider))) {}
   ~TwoClientWebAppsBMOSyncTest() override = default;
 
   bool SetupClients() override {
     bool result = SyncTest::SetupClients();
     if (!result)
       return result;
-    // All of the tests need to have os integration suppressed & the
-    // WebAppProvider ready.
-    os_hooks_suppress_ =
-        web_app::OsIntegrationManager::ScopedSuppressOsHooksForTesting();
     for (Profile* profile : GetAllProfiles()) {
       auto* web_app_provider = WebAppProvider::Get(profile);
       web_app_provider->install_finalizer()
@@ -148,6 +160,11 @@ class TwoClientWebAppsBMOSyncTest : public SyncTest {
     return *web_app_registrar;
   }
 
+  TestOsIntegrationManager& GetOSIntegrationManager(Profile* profile) {
+    return reinterpret_cast<TestOsIntegrationManager&>(
+        WebAppProvider::Get(profile)->os_integration_manager());
+  }
+
   extensions::AppSorting* GetAppSorting(Profile* profile) {
     return extensions::ExtensionSystem::Get(profile)->app_sorting();
   }
@@ -167,7 +184,7 @@ class TwoClientWebAppsBMOSyncTest : public SyncTest {
   }
 
  private:
-  ScopedOsHooksSuppress os_hooks_suppress_;
+  std::unique_ptr<TestWebAppProviderCreator> test_web_app_provider_creator_;
 
   DISALLOW_COPY_AND_ASSIGN(TwoClientWebAppsBMOSyncTest);
 };
@@ -563,5 +580,55 @@ IN_PROC_BROWSER_TEST_F(TwoClientWebAppsBMOSyncTest, UninstallSynced) {
   EXPECT_TRUE(AllProfilesHaveSameWebAppIds());
 }
 
+IN_PROC_BROWSER_TEST_F(TwoClientWebAppsBMOSyncTest, NoShortcutsCreatedOnSync) {
+  ASSERT_TRUE(SetupSync());
+  ASSERT_TRUE(AllProfilesHaveSameWebAppIds());
+  ASSERT_TRUE(embedded_test_server()->Start());
+
+  // Install & uninstall on profile 0, and validate profile 1 sees it.
+  {
+    base::RunLoop loop;
+    base::RepeatingCallback<void(const AppId&)> on_installed_closure;
+    base::RepeatingCallback<void(const AppId&)> on_hooks_closure;
+#if defined(OS_CHROMEOS)
+    on_installed_closure = base::DoNothing();
+    on_hooks_closure = base::BindLambdaForTesting(
+        [&](const AppId& installed_app_id) { loop.Quit(); });
+#else
+    on_installed_closure = base::BindLambdaForTesting(
+        [&](const AppId& installed_app_id) { loop.Quit(); });
+    on_hooks_closure = base::BindLambdaForTesting(
+        [](const AppId& installed_app_id) { FAIL(); });
+#endif
+    WebAppInstallObserver app_listener(GetProfile(1));
+    app_listener.SetWebAppInstalledDelegate(on_installed_closure);
+    app_listener.SetWebAppInstalledWithOsHooksDelegate(on_hooks_closure);
+    InstallAppAsUserInitiated(GetProfile(0));
+    loop.Run();
+    EXPECT_TRUE(AllProfilesHaveSameWebAppIds());
+  }
+  EXPECT_EQ(
+      1u, GetOSIntegrationManager(GetProfile(0)).num_create_shortcuts_calls());
+#if defined(OS_CHROMEOS)
+  auto last_options =
+      GetOSIntegrationManager(GetProfile(1)).get_last_install_options();
+  EXPECT_TRUE(last_options.has_value());
+  OsHooksResults expected_os_hook_requests;
+  expected_os_hook_requests[OsHookType::kShortcuts] = true;
+  expected_os_hook_requests[OsHookType::kRunOnOsLogin] = false;
+  expected_os_hook_requests[OsHookType::kShortcutsMenu] = true;
+  expected_os_hook_requests[OsHookType::kFileHandlers] = true;
+  EXPECT_EQ(expected_os_hook_requests, last_options->os_hooks);
+  EXPECT_TRUE(last_options->add_to_desktop);
+  EXPECT_FALSE(last_options->add_to_quick_launch_bar);
+  EXPECT_EQ(
+      1u, GetOSIntegrationManager(GetProfile(1)).num_create_shortcuts_calls());
+#else
+  EXPECT_FALSE(GetOSIntegrationManager(GetProfile(1))
+                   .get_last_install_options()
+                   .has_value());
+#endif
+}
+
 }  // namespace
 }  // namespace web_app

chrome/browser/web_applications/components/os_integration_manager.cc

@@ -124,6 +124,10 @@ void OsIntegrationManager::InstallOsHooks(
           &OsHooksBarrierInfo::Run,
           base::Owned(new OsHooksBarrierInfo(std::move(callback))));
 
+  DCHECK(options.os_hooks[OsHookType::kShortcuts] ||
+         !options.os_hooks[OsHookType::kShortcutsMenu])
+      << "Cannot install shortcuts menu without installing shortcuts.";
+
   // TODO(ortuno): Make adding a shortcut to the applications menu independent
   // from adding a shortcut to desktop.
   if (options.os_hooks[OsHookType::kShortcuts] &&

chrome/browser/web_applications/pending_app_install_task.cc

@@ -267,6 +267,8 @@ void PendingAppInstallTask::OnWebAppInstalled(bool is_placeholder,
   InstallOsHooksOptions options;
   options.os_hooks[OsHookType::kShortcuts] =
       install_options_.add_to_applications_menu;
+  options.os_hooks[OsHookType::kShortcutsMenu] =
+      install_options_.add_to_applications_menu;
   options.add_to_desktop = install_options_.add_to_desktop;
   options.add_to_quick_launch_bar = install_options_.add_to_quick_launch_bar;
   options.os_hooks[OsHookType::kRunOnOsLogin] =
@@ -275,7 +277,6 @@ void PendingAppInstallTask::OnWebAppInstalled(bool is_placeholder,
   // TODO(crbug.com/1087219): Determine if |register_file_handlers| should be
   // configured from somewhere else rather than always true.
   options.os_hooks[OsHookType::kFileHandlers] = true;
-  options.os_hooks[OsHookType::kShortcutsMenu] = true;
 
   os_integration_manager_->InstallOsHooks(
       app_id,

chrome/browser/web_applications/test/test_os_integration_manager.cc

@@ -41,22 +41,23 @@ void TestOsIntegrationManager::InstallOsHooks(
     std::unique_ptr<WebApplicationInfo> web_app_info,
     InstallOsHooksOptions options) {
   OsHooksResults os_hooks_results{false};
-  os_hooks_results[OsHookType::kFileHandlers] = true;
-  os_hooks_results[OsHookType::kShortcutsMenu] = true;
+  last_options_ = options;
+
+  if (options.os_hooks[OsHookType::kFileHandlers])
+    os_hooks_results[OsHookType::kFileHandlers] = true;
 
   did_add_to_desktop_ = options.add_to_desktop;
 
   if (options.os_hooks[OsHookType::kShortcuts] && can_create_shortcuts_) {
     bool success = true;
+    ++num_create_shortcuts_calls_;
     auto it = next_create_shortcut_results_.find(app_id);
     if (it != next_create_shortcut_results_.end()) {
       success = it->second;
       next_create_shortcut_results_.erase(app_id);
     }
-    if (success) {
-      ++num_create_shortcuts_calls_;
+    if (success)
       os_hooks_results[OsHookType::kShortcutsMenu] = true;
-    }
   }
 
   if (options.os_hooks[OsHookType::kRunOnOsLogin]) {

chrome/browser/web_applications/test/test_os_integration_manager.h

@@ -58,6 +58,10 @@ class TestOsIntegrationManager : public OsIntegrationManager {
     return did_add_to_desktop_;
   }
 
+  base::Optional<InstallOsHooksOptions> get_last_install_options() const {
+    return last_options_;
+  }
+
   void SetNextCreateShortcutsResult(const AppId& app_id, bool success);
 
   void SetFileHandlerManager(
@@ -68,6 +72,7 @@ class TestOsIntegrationManager : public OsIntegrationManager {
   size_t num_register_run_on_os_login_calls_ = 0;
   size_t num_add_app_to_quick_launch_bar_calls_ = 0;
   base::Optional<bool> did_add_to_desktop_;
+  base::Optional<InstallOsHooksOptions> last_options_;
 
   bool can_create_shortcuts_ = true;
   std::map<AppId, bool> next_create_shortcut_results_;

chrome/browser/web_applications/test/test_system_web_app_installation.cc

@@ -92,18 +92,18 @@ TestSystemWebAppInstallation::TestSystemWebAppInstallation(SystemAppType type,
   }
 
   test_web_app_provider_creator_ = std::make_unique<TestWebAppProviderCreator>(
-      base::BindOnce(&TestSystemWebAppInstallation::CreateWebAppProvider,
-                     // base::Unretained is safe here. This callback is called
-                     // at TestingProfile::Init, which is at test startup.
-                     // TestSystemWebAppInstallation is intended to have the
-                     // same lifecycle as the test, it won't be destroyed before
-                     // the test finishes.
-                     base::Unretained(this), info));
+      base::BindRepeating(&TestSystemWebAppInstallation::CreateWebAppProvider,
+                          // base::Unretained is safe here. This callback is
+                          // called at TestingProfile::Init, which is at test
+                          // startup. TestSystemWebAppInstallation is intended
+                          // to have the same lifecycle as the test, it won't be
+                          // destroyed before the test finishes.
+                          base::Unretained(this), info));
 }
 
 TestSystemWebAppInstallation::TestSystemWebAppInstallation() {
   test_web_app_provider_creator_ = std::make_unique<
-      TestWebAppProviderCreator>(base::BindOnce(
+      TestWebAppProviderCreator>(base::BindRepeating(
       &TestSystemWebAppInstallation::CreateWebAppProviderWithNoSystemWebApps,
       // base::Unretained is safe here. This callback is called
       // at TestingProfile::Init, which is at test startup.

chrome/browser/web_applications/test/test_web_app_provider.cc

@@ -151,7 +151,7 @@ std::unique_ptr<KeyedService> TestWebAppProviderCreator::CreateWebAppProvider(
   Profile* profile = Profile::FromBrowserContext(context);
   if (!AreWebAppsEnabled(profile) || !callback_)
     return nullptr;
-  return std::move(callback_).Run(profile);
+  return callback_.Run(profile);
 }
 
 }  // namespace web_app

chrome/browser/web_applications/test/test_web_app_provider.h

@@ -83,9 +83,12 @@ class TestWebAppProvider : public WebAppProvider {
 // BrowserContextKeyedService initialization pipeline.
 class TestWebAppProviderCreator {
  public:
-  using CreateWebAppProviderCallback =
+  using OnceCreateWebAppProviderCallback =
       base::OnceCallback<std::unique_ptr<KeyedService>(Profile* profile)>;
+  using CreateWebAppProviderCallback =
+      base::RepeatingCallback<std::unique_ptr<KeyedService>(Profile* profile)>;
 
+  explicit TestWebAppProviderCreator(OnceCreateWebAppProviderCallback callback);
   explicit TestWebAppProviderCreator(CreateWebAppProviderCallback callback);
   ~TestWebAppProviderCreator();
 

chrome/browser/web_applications/web_app_install_task.cc

@@ -776,10 +776,23 @@ void WebAppInstallTask::OnInstallFinalizedCreateShortcuts(
     return;
   }
 
-  RecordAppBanner(web_contents(), web_app_info->start_url);
   RecordWebAppInstallationTimestamp(profile_->GetPrefs(), app_id,
                                     install_source_);
 
+  if (install_params_ && !install_params_->locally_installed) {
+    DCHECK(background_installation_);
+    DCHECK(!(install_params_->add_to_applications_menu ||
+             install_params_->add_to_desktop ||
+             install_params_->add_to_quick_launch_bar ||
+             install_params_->run_on_os_login))
+        << "Cannot create os hooks for a non-locally installed ";
+    CallInstallCallback(app_id, InstallResultCode::kSuccessNewInstall);
+    return;
+  }
+
+  // Only record the AppBanner stats for locally installed apps.
+  RecordAppBanner(web_contents(), web_app_info->start_url);
+
   InstallOsHooksOptions options;
 
   options.os_hooks[OsHookType::kShortcuts] = true;
@@ -791,23 +804,22 @@ void WebAppInstallTask::OnInstallFinalizedCreateShortcuts(
     options.add_to_quick_launch_bar = false;
 
   if (install_params_) {
+    DCHECK(install_params_->locally_installed);
     options.os_hooks[OsHookType::kShortcuts] =
         install_params_->add_to_applications_menu;
     options.add_to_desktop = install_params_->add_to_desktop;
     options.add_to_quick_launch_bar = install_params_->add_to_quick_launch_bar;
     options.os_hooks[OsHookType::kRunOnOsLogin] =
         install_params_->run_on_os_login;
+    options.os_hooks[OsHookType::kShortcutsMenu] =
+        install_params_->add_to_applications_menu;
+    // TODO(crbug.com/1087219): Determine if file handlers should be
+    // configured from somewhere else rather than always true.
+    options.os_hooks[OsHookType::kFileHandlers] = true;
   }
-
-  // TODO(crbug.com/1087219): Determine if file handlers should be
-  // configured from somewhere else rather than always true.
-  options.os_hooks[OsHookType::kFileHandlers] = true;
-  options.os_hooks[OsHookType::kShortcutsMenu] = true;
-
   auto hooks_created_callback =
       base::BindOnce(&WebAppInstallTask::OnOsHooksCreated, GetWeakPtr(),
                      web_app_info->open_as_window, app_id);
-
   os_integration_manager_->InstallOsHooks(app_id,
                                           std::move(hooks_created_callback),
                                           std::move(web_app_info), options);