WebApps: Tests that Placeholder apps can be installed with shortcuts.

This is a follow up to https://crrev.com/c/1890394 adding a test.

Bug: 1019319
Change-Id: Icc3498a0975c5370299fe558b212cb4c5b8a8232
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1893604
Commit-Queue: Jay Harris <harrisjay@chromium.org>
Reviewed-by: Giovanni Ortuño Urquidi <ortuno@chromium.org>
Cr-Commit-Position: refs/heads/master@{#712361}

chrome/browser/web_applications/components/app_shortcut_manager.cc

@@ -11,25 +11,6 @@
 
 namespace web_app {
 
-namespace {
-
-void OnShortcutInfoRetrievedCreateShortcuts(
-    bool add_to_desktop,
-    CreateShortcutsCallback callback,
-    std::unique_ptr<ShortcutInfo> info) {
-  base::FilePath shortcut_data_dir = internals::GetShortcutDataDir(*info);
-
-  ShortcutLocations locations;
-  locations.on_desktop = add_to_desktop;
-  locations.applications_menu_location = APP_MENU_LOCATION_SUBDIR_CHROMEAPPS;
-
-  internals::ScheduleCreatePlatformShortcuts(
-      std::move(shortcut_data_dir), locations, SHORTCUT_CREATION_BY_USER,
-      std::move(info), std::move(callback));
-}
-
-}  // namespace
-
 AppShortcutManager::AppShortcutManager(Profile* profile) : profile_(profile) {}
 
 AppShortcutManager::~AppShortcutManager() {
@@ -57,17 +38,23 @@ bool AppShortcutManager::CanCreateShortcuts() const {
 #endif
 }
 
+void AppShortcutManager::SuppressShortcutsForTesting() {
+  suppress_shortcuts_for_testing_ = true;
+}
+
 void AppShortcutManager::CreateShortcuts(const AppId& app_id,
                                          bool add_to_desktop,
                                          CreateShortcutsCallback callback) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
   DCHECK(CanCreateShortcuts());
+
   GetShortcutInfoForApp(
-      app_id,
-      base::BindOnce(&OnShortcutInfoRetrievedCreateShortcuts, add_to_desktop,
-                     base::BindOnce(&AppShortcutManager::OnShortcutsCreated,
-                                    weak_ptr_factory_.GetWeakPtr(), app_id,
-                                    std::move(callback))));
+      app_id, base::BindOnce(
+                  &AppShortcutManager::OnShortcutInfoRetrievedCreateShortcuts,
+                  weak_ptr_factory_.GetWeakPtr(), add_to_desktop,
+                  base::BindOnce(&AppShortcutManager::OnShortcutsCreated,
+                                 weak_ptr_factory_.GetWeakPtr(), app_id,
+                                 std::move(callback))));
 }
 
 void AppShortcutManager::OnShortcutsCreated(const AppId& app_id,
@@ -81,4 +68,24 @@ void AppShortcutManager::OnShortcutsCreated(const AppId& app_id,
   std::move(callback).Run(success);
 }
 
+void AppShortcutManager::OnShortcutInfoRetrievedCreateShortcuts(
+    bool add_to_desktop,
+    CreateShortcutsCallback callback,
+    std::unique_ptr<ShortcutInfo> info) {
+  if (suppress_shortcuts_for_testing_) {
+    std::move(callback).Run(true);
+    return;
+  }
+
+  base::FilePath shortcut_data_dir = internals::GetShortcutDataDir(*info);
+
+  ShortcutLocations locations;
+  locations.on_desktop = add_to_desktop;
+  locations.applications_menu_location = APP_MENU_LOCATION_SUBDIR_CHROMEAPPS;
+
+  internals::ScheduleCreatePlatformShortcuts(
+      std::move(shortcut_data_dir), locations, SHORTCUT_CREATION_BY_USER,
+      std::move(info), std::move(callback));
+}
+
 }  // namespace web_app

chrome/browser/web_applications/components/app_shortcut_manager.h

@@ -37,6 +37,10 @@ class AppShortcutManager {
   void AddObserver(AppShortcutObserver* observer);
   void RemoveObserver(AppShortcutObserver* observer);
 
+  // Tells the AppShortcutManager that no shortcuts should actually be written
+  // to the disk.
+  void SuppressShortcutsForTesting();
+
   virtual bool CanCreateShortcuts() const;
 
   // virtual for testing.
@@ -61,6 +65,13 @@ class AppShortcutManager {
   Profile* profile() { return profile_; }
 
  private:
+  void OnShortcutInfoRetrievedCreateShortcuts(
+      bool add_to_desktop,
+      CreateShortcutsCallback callback,
+      std::unique_ptr<ShortcutInfo> info);
+
+  bool suppress_shortcuts_for_testing_ = false;
+
   AppRegistrar* registrar_ = nullptr;
   Profile* const profile_;
 

chrome/browser/web_applications/pending_app_manager_impl_browsertest.cc

@@ -14,6 +14,7 @@
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/web_applications/test/web_app_browsertest_util.h"
 #include "chrome/browser/web_applications/components/app_registrar.h"
+#include "chrome/browser/web_applications/components/app_shortcut_manager.h"
 #include "chrome/browser/web_applications/components/external_install_options.h"
 #include "chrome/browser/web_applications/components/externally_installed_web_app_prefs.h"
 #include "chrome/browser/web_applications/components/web_app_constants.h"
@@ -35,6 +36,11 @@ class PendingAppManagerImplBrowserTest : public InProcessBrowserTest {
         ->registrar();
   }
 
+  AppShortcutManager& shortcut_manager() {
+    return WebAppProviderBase::GetProviderBase(browser()->profile())
+        ->shortcut_manager();
+  }
+
   PendingAppManager& pending_app_manager() {
     return WebAppProviderBase::GetProviderBase(browser()->profile())
         ->pending_app_manager();
@@ -82,6 +88,32 @@ IN_PROC_BROWSER_TEST_F(PendingAppManagerImplBrowserTest, InstallSucceeds) {
   EXPECT_EQ("Manifest test app", registrar().GetAppShortName(app_id.value()));
 }
 
+// Installing a placeholder app with shortcuts should succeed.
+IN_PROC_BROWSER_TEST_F(PendingAppManagerImplBrowserTest,
+                       PlaceholderInstallSucceedsWithShortcuts) {
+  ASSERT_TRUE(embedded_test_server()->Start());
+  shortcut_manager().SuppressShortcutsForTesting();
+
+  GURL final_url =
+      embedded_test_server()->GetURL("/banners/manifest_test_page.html");
+  // Add a redirect, so a placeholder is installed.
+  GURL url(
+      embedded_test_server()->GetURL("/server-redirect?" + final_url.spec()));
+
+  ExternalInstallOptions options = CreateInstallOptions(url);
+  options.install_placeholder = true;
+  options.add_to_applications_menu = true;
+  options.add_to_desktop = true;
+  InstallApp(options);
+
+  EXPECT_EQ(InstallResultCode::kSuccessNewInstall, result_code_.value());
+  base::Optional<AppId> app_id =
+      ExternallyInstalledWebAppPrefs(browser()->profile()->GetPrefs())
+          .LookupAppId(url);
+  ASSERT_TRUE(app_id.has_value());
+  EXPECT_TRUE(registrar().IsPlaceholderApp(app_id.value()));
+}
+
 // Tests that the browser doesn't crash if it gets shutdown with a pending
 // installation.
 IN_PROC_BROWSER_TEST_F(PendingAppManagerImplBrowserTest,