desktop-pwas: Add option to skip shortcut creation in PendingAppManager

WebAppPolicyManager would like to skip shortcut creation because there
is a separate policy to create them. So we add that option to AppInfo
and plumb it all the way down to BookmarkAppHelper

Bug: 876175
Change-Id: I09f460efe7c3809856bde77ebe4570de6ae8845e
Reviewed-on: https://chromium-review.googlesource.com/1186220
Commit-Queue: Giovanni Ortuño Urquidi <ortuno@chromium.org>
Reviewed-by: Dominick Ng <dominickn@chromium.org>
Cr-Commit-Position: refs/heads/master@{#585788}

chrome/browser/extensions/bookmark_app_helper.cc

@@ -603,25 +603,28 @@ void BookmarkAppHelper::FinishInstallation(const Extension* extension) {
 // On Mac, shortcuts are automatically created for hosted apps when they are
 // installed, so there is no need to create them again.
 #if !defined(OS_MACOSX)
+  if (create_shortcuts_) {
 #if !defined(OS_CHROMEOS)
-  web_app::ShortcutLocations creation_locations;
+    web_app::ShortcutLocations creation_locations;
 #if defined(OS_LINUX) || defined(OS_WIN)
-  creation_locations.on_desktop = true;
+    creation_locations.on_desktop = true;
 #else
-  creation_locations.on_desktop = false;
+    creation_locations.on_desktop = false;
 #endif
-  creation_locations.applications_menu_location =
-      web_app::APP_MENU_LOCATION_SUBDIR_CHROMEAPPS;
-  creation_locations.in_quick_launch_bar = false;
-  web_app::CreateShortcuts(web_app::SHORTCUT_CREATION_BY_USER,
-                           creation_locations, current_profile, extension);
+    creation_locations.applications_menu_location =
+        web_app::APP_MENU_LOCATION_SUBDIR_CHROMEAPPS;
+    creation_locations.in_quick_launch_bar = false;
+
+    web_app::CreateShortcuts(web_app::SHORTCUT_CREATION_BY_USER,
+                             creation_locations, current_profile, extension);
 #else
-  // ChromeLauncherController does not exist in unit tests.
-  if (ChromeLauncherController::instance()) {
-    ChromeLauncherController::instance()->shelf_model()->PinAppWithID(
-        extension->id());
-  }
+    // ChromeLauncherController does not exist in unit tests.
+    if (ChromeLauncherController::instance()) {
+      ChromeLauncherController::instance()->shelf_model()->PinAppWithID(
+          extension->id());
+    }
 #endif  // !defined(OS_CHROMEOS)
+  }
 
   // Reparent the tab into an app window immediately when opening as a window.
   if (!silent_install &&

chrome/browser/extensions/bookmark_app_helper.h

@@ -96,6 +96,11 @@ class BookmarkAppHelper : public content::NotificationObserver {
   // If called, the installed extension will be considered default installed.
   void set_is_default_app() { is_default_app_ = true; }
 
+  // If called, desktop shortcuts will not be created.
+  void set_skip_shortcut_creation() { create_shortcuts_ = false; }
+
+  bool create_shortcuts() const { return create_shortcuts_; }
+
  protected:
   // Protected methods for testing.
 
@@ -155,6 +160,8 @@ class BookmarkAppHelper : public content::NotificationObserver {
 
   bool is_default_app_ = false;
 
+  bool create_shortcuts_ = true;
+
   // The mechanism via which the app creation was triggered.
   WebappInstallSource install_source_;
 

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

@@ -82,7 +82,9 @@ void WebAppPolicyManager::RefreshPolicyInstalledApps() {
     else
       container = PendingAppManager::LaunchContainer::kTab;
 
-    apps_to_install.emplace_back(GURL(url.GetString()), container);
+    // There is a separate policy to create shortcuts/pin apps to shelf.
+    apps_to_install.emplace_back(GURL(url.GetString()), container,
+                                 false /* create_shortcuts */);
   }
   pending_app_manager_->InstallApps(std::move(apps_to_install),
                                     base::DoNothing());

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

@@ -102,9 +102,11 @@ TEST_F(WebAppPolicyManagerTest, TwoForceInstalledApps) {
 
   std::vector<PendingAppManager::AppInfo> expected_apps_to_install;
   expected_apps_to_install.emplace_back(
-      GURL(kUrl1), PendingAppManager::LaunchContainer::kWindow);
+      GURL(kUrl1), PendingAppManager::LaunchContainer::kWindow,
+      false /* create_shortcuts */);
   expected_apps_to_install.emplace_back(
-      GURL(kUrl2), PendingAppManager::LaunchContainer::kTab);
+      GURL(kUrl2), PendingAppManager::LaunchContainer::kTab,
+      false /* create_shortcuts */);
 
   EXPECT_EQ(apps_to_install, expected_apps_to_install);
 }

chrome/browser/web_applications/components/pending_app_manager.cc

@@ -9,8 +9,12 @@
 
 namespace web_app {
 
-PendingAppManager::AppInfo::AppInfo(GURL url, LaunchContainer launch_container)
-    : url(std::move(url)), launch_container(launch_container) {}
+PendingAppManager::AppInfo::AppInfo(GURL url,
+                                    LaunchContainer launch_container,
+                                    bool create_shortcuts)
+    : url(std::move(url)),
+      launch_container(launch_container),
+      create_shortcuts(create_shortcuts) {}
 
 PendingAppManager::AppInfo::AppInfo(PendingAppManager::AppInfo&& other) =
     default;
@@ -19,15 +23,16 @@ PendingAppManager::AppInfo::~AppInfo() = default;
 
 std::unique_ptr<PendingAppManager::AppInfo> PendingAppManager::AppInfo::Clone()
     const {
-  auto other = std::make_unique<AppInfo>(url, launch_container);
+  auto other =
+      std::make_unique<AppInfo>(url, launch_container, create_shortcuts);
   DCHECK_EQ(*this, *other);
   return other;
 }
 
 bool PendingAppManager::AppInfo::operator==(
     const PendingAppManager::AppInfo& other) const {
-  return std::tie(url, launch_container) ==
-         std::tie(other.url, other.launch_container);
+  return std::tie(url, launch_container, create_shortcuts) ==
+         std::tie(other.url, other.launch_container, other.create_shortcuts);
 }
 
 PendingAppManager::PendingAppManager() = default;
@@ -37,7 +42,8 @@ PendingAppManager::~PendingAppManager() = default;
 std::ostream& operator<<(std::ostream& out,
                          const PendingAppManager::AppInfo& app_info) {
   return out << "url: " << app_info.url << "\n launch_container: "
-             << static_cast<int32_t>(app_info.launch_container);
+             << static_cast<int32_t>(app_info.launch_container)
+             << "\n create_shortcuts: " << app_info.create_shortcuts;
 }
 
 }  // namespace web_app

chrome/browser/web_applications/components/pending_app_manager.h

@@ -36,7 +36,9 @@ class PendingAppManager {
   };
 
   struct AppInfo {
-    AppInfo(GURL url, LaunchContainer launch_container);
+    AppInfo(GURL url,
+            LaunchContainer launch_container,
+            bool create_shortcuts = true);
     AppInfo(AppInfo&& other);
     ~AppInfo();
 
@@ -44,8 +46,9 @@ class PendingAppManager {
 
     bool operator==(const AppInfo& other) const;
 
-    GURL url;
-    LaunchContainer launch_container;
+    const GURL url;
+    const LaunchContainer launch_container;
+    const bool create_shortcuts;
 
     DISALLOW_COPY_AND_ASSIGN(AppInfo);
   };

chrome/browser/web_applications/extensions/bookmark_app_installation_task.cc

@@ -104,6 +104,10 @@ void BookmarkAppInstallationTask::OnGetWebApplicationInfo(
   // is plumbed through this class.
   helper_ = helper_factory_.Run(profile_, *web_app_info, web_contents,
                                 WebappInstallSource::MENU_BROWSER_TAB);
+
+  if (!app_info_.create_shortcuts)
+    helper_->set_skip_shortcut_creation();
+
   helper_->Create(base::Bind(&BookmarkAppInstallationTask::OnInstalled,
                              weak_ptr_factory_.GetWeakPtr(),
                              base::Passed(&result_callback)));

chrome/browser/web_applications/extensions/bookmark_app_installation_task_unittest.cc

@@ -55,6 +55,13 @@ class TestBookmarkAppHelper : public BookmarkAppHelper {
       : BookmarkAppHelper(profile, web_app_info, contents, install_source) {}
   ~TestBookmarkAppHelper() override {}
 
+  void CompleteInstallation() {
+    CompleteInstallableCheck();
+    content::RunAllTasksUntilIdle();
+    CompleteIconDownload();
+    content::RunAllTasksUntilIdle();
+  }
+
   void CompleteInstallableCheck() {
     blink::Manifest manifest;
     InstallableData data = {
@@ -78,6 +85,37 @@ class TestBookmarkAppHelper : public BookmarkAppHelper {
   DISALLOW_COPY_AND_ASSIGN(TestBookmarkAppHelper);
 };
 
+// All BookmarkAppDataRetriever operations are async, so this class posts tasks
+// when running callbacks to simulate async behavior in tests as well.
+class TestDataRetriever : public BookmarkAppDataRetriever {
+ public:
+  explicit TestDataRetriever(std::unique_ptr<WebApplicationInfo> web_app_info)
+      : web_app_info_(std::move(web_app_info)) {}
+
+  ~TestDataRetriever() override = default;
+
+  void GetWebApplicationInfo(content::WebContents* web_contents,
+                             GetWebApplicationInfoCallback callback) override {
+    DCHECK(web_contents);
+    base::ThreadTaskRunnerHandle::Get()->PostTask(
+        FROM_HERE,
+        base::BindOnce(std::move(callback), std::move(web_app_info_)));
+  }
+
+  void GetIcons(const GURL& app_url,
+                const std::vector<GURL>& icon_urls,
+                GetIconsCallback callback) override {
+    base::ThreadTaskRunnerHandle::Get()->PostTask(
+        FROM_HERE, base::BindOnce(std::move(callback),
+                                  std::vector<WebApplicationInfo::IconInfo>()));
+  }
+
+ private:
+  std::unique_ptr<WebApplicationInfo> web_app_info_;
+
+  DISALLOW_COPY_AND_ASSIGN(TestDataRetriever);
+};
+
 class BookmarkAppInstallationTaskTest : public ChromeRenderViewHostTestHarness {
  public:
   BookmarkAppInstallationTaskTest() = default;
@@ -100,6 +138,16 @@ class BookmarkAppInstallationTaskTest : public ChromeRenderViewHostTestHarness {
   }
 
  protected:
+  void SetTestingFactories(BookmarkAppInstallationTask* task,
+                           const GURL& app_url) {
+    WebApplicationInfo info;
+    info.app_url = app_url;
+    info.title = base::UTF8ToUTF16(kWebAppTitle);
+    task->SetDataRetrieverForTesting(std::make_unique<TestDataRetriever>(
+        std::make_unique<WebApplicationInfo>(std::move(info))));
+    task->SetBookmarkAppHelperFactoryForTesting(helper_factory());
+  }
+
   BookmarkAppInstallationTask::BookmarkAppHelperFactory helper_factory() {
     return base::BindRepeating(
         &BookmarkAppInstallationTaskTest::CreateTestBookmarkAppHelper,
@@ -135,37 +183,6 @@ class BookmarkAppInstallationTaskTest : public ChromeRenderViewHostTestHarness {
   DISALLOW_COPY_AND_ASSIGN(BookmarkAppInstallationTaskTest);
 };
 
-// All BookmarkAppDataRetriever operations are async, so this class posts tasks
-// when running callbacks to simulate async behavior in tests as well.
-class TestDataRetriever : public BookmarkAppDataRetriever {
- public:
-  explicit TestDataRetriever(std::unique_ptr<WebApplicationInfo> web_app_info)
-      : web_app_info_(std::move(web_app_info)) {}
-
-  ~TestDataRetriever() override = default;
-
-  void GetWebApplicationInfo(content::WebContents* web_contents,
-                             GetWebApplicationInfoCallback callback) override {
-    DCHECK(web_contents);
-    base::ThreadTaskRunnerHandle::Get()->PostTask(
-        FROM_HERE,
-        base::BindOnce(std::move(callback), std::move(web_app_info_)));
-  }
-
-  void GetIcons(const GURL& app_url,
-                const std::vector<GURL>& icon_urls,
-                GetIconsCallback callback) override {
-    base::ThreadTaskRunnerHandle::Get()->PostTask(
-        FROM_HERE, base::BindOnce(std::move(callback),
-                                  std::vector<WebApplicationInfo::IconInfo>()));
-  }
-
- private:
-  std::unique_ptr<WebApplicationInfo> web_app_info_;
-
-  DISALLOW_COPY_AND_ASSIGN(TestDataRetriever);
-};
-
 class TestInstaller : public BookmarkAppInstaller {
  public:
   explicit TestInstaller(Profile* profile, bool succeeds)
@@ -285,12 +302,7 @@ TEST_F(BookmarkAppInstallationTaskTest,
       web_app::PendingAppManager::AppInfo(
           app_url, web_app::PendingAppManager::LaunchContainer::kWindow));
 
-  WebApplicationInfo info;
-  info.app_url = app_url;
-  info.title = base::UTF8ToUTF16(kWebAppTitle);
-  task->SetDataRetrieverForTesting(std::make_unique<TestDataRetriever>(
-      std::make_unique<WebApplicationInfo>(std::move(info))));
-  task->SetBookmarkAppHelperFactoryForTesting(helper_factory());
+  SetTestingFactories(task.get(), app_url);
 
   task->InstallWebAppOrShortcutFromWebContents(
       web_contents(),
@@ -316,12 +328,7 @@ TEST_F(BookmarkAppInstallationTaskTest,
       web_app::PendingAppManager::AppInfo(
           app_url, web_app::PendingAppManager::LaunchContainer::kWindow));
 
-  WebApplicationInfo info;
-  info.app_url = app_url;
-  info.title = base::UTF8ToUTF16(kWebAppTitle);
-  task->SetDataRetrieverForTesting(std::make_unique<TestDataRetriever>(
-      std::make_unique<WebApplicationInfo>(std::move(info))));
-  task->SetBookmarkAppHelperFactoryForTesting(helper_factory());
+  SetTestingFactories(task.get(), app_url);
 
   task->InstallWebAppOrShortcutFromWebContents(
       web_contents(),
@@ -338,4 +345,29 @@ TEST_F(BookmarkAppInstallationTaskTest,
   EXPECT_FALSE(app_installed());
 }
 
+TEST_F(BookmarkAppInstallationTaskTest,
+       WebAppOrShortcutFromContents_NoShortcuts) {
+  const GURL app_url(kWebAppUrl);
+
+  web_app::PendingAppManager::AppInfo app_info(
+      app_url, web_app::PendingAppManager::LaunchContainer::kWindow,
+      false /* create_shortcuts */);
+  auto task = std::make_unique<BookmarkAppInstallationTask>(
+      profile(), std::move(app_info));
+
+  SetTestingFactories(task.get(), app_url);
+
+  task->InstallWebAppOrShortcutFromWebContents(
+      web_contents(),
+      base::BindOnce(&BookmarkAppInstallationTaskTest::OnInstallationTaskResult,
+                     base::Unretained(this), base::DoNothing().Once()));
+  content::RunAllTasksUntilIdle();
+
+  test_helper().CompleteInstallation();
+
+  EXPECT_TRUE(app_installed());
+
+  EXPECT_FALSE(test_helper().create_shortcuts());
+}
+
 }  // namespace extensions

chrome/browser/web_applications/extensions/pending_bookmark_app_manager_browsertest.cc

@@ -26,13 +26,18 @@ IN_PROC_BROWSER_TEST_F(PendingBookmarkAppManagerBrowserTest, InstallSucceeds) {
   ASSERT_TRUE(embedded_test_server()->Start());
   base::RunLoop run_loop;
   std::string app_id;
+  web_app::PendingAppManager::AppInfo app_info(
+      embedded_test_server()->GetURL("/banners/manifest_test_page.html"),
+      web_app::PendingAppManager::LaunchContainer::kWindow);
 
   web_app::WebAppProvider::Get(browser()->profile())
       ->pending_app_manager()
       .Install(web_app::PendingAppManager::AppInfo(
                    embedded_test_server()->GetURL(
                        "/banners/manifest_test_page.html"),
-                   web_app::PendingAppManager::LaunchContainer::kWindow),
+                   web_app::PendingAppManager::LaunchContainer::kWindow,
+                   false /* create_shortcuts */),  // Avoid creating real
+                                                   // shortcuts in tests.
                base::BindLambdaForTesting(
                    [&run_loop, &app_id](const GURL& provided_url,
                                         const std::string& id) {