desktop-pwas: Change BookmarkAppInstallationTask to use a BookmarkAppHelper

Use BookmarkAppHelper to perform all necessary steps to install a
Bookmark App based on a WebContents.

Bug: 864904

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

chrome/browser/web_applications/extensions/bookmark_app_installation_task.cc

@@ -9,11 +9,31 @@
 
 #include "base/bind.h"
 #include "base/callback.h"
+#include "chrome/browser/extensions/bookmark_app_helper.h"
+#include "chrome/browser/favicon/favicon_utils.h"
+#include "chrome/browser/installable/installable_manager.h"
+#include "chrome/browser/ssl/security_state_tab_helper.h"
 #include "chrome/browser/web_applications/extensions/bookmark_app_data_retriever.h"
 #include "chrome/browser/web_applications/extensions/bookmark_app_installer.h"
+#include "chrome/common/web_application_info.h"
+#include "content/public/browser/browser_thread.h"
+#include "extensions/common/extension.h"
 
 namespace extensions {
 
+namespace {
+
+std::unique_ptr<BookmarkAppHelper> BookmarkAppHelperCreateWrapper(
+    Profile* profile,
+    const WebApplicationInfo& web_app_info,
+    content::WebContents* web_contents,
+    WebappInstallSource install_source) {
+  return std::make_unique<BookmarkAppHelper>(profile, web_app_info,
+                                             web_contents, install_source);
+}
+
+}  // namespace
+
 BookmarkAppInstallationTask::Result::Result(ResultCode code,
                                             const std::string& app_id)
     : code(code), app_id(app_id) {
@@ -24,8 +44,39 @@ BookmarkAppInstallationTask::Result::Result(Result&&) = default;
 
 BookmarkAppInstallationTask::Result::~Result() = default;
 
+// static
+void BookmarkAppInstallationTask::CreateTabHelpers(
+    content::WebContents* web_contents) {
+  InstallableManager::CreateForWebContents(web_contents);
+  SecurityStateTabHelper::CreateForWebContents(web_contents);
+  favicon::CreateContentFaviconDriverForWebContents(web_contents);
+}
+
+BookmarkAppInstallationTask::BookmarkAppInstallationTask(Profile* profile)
+    : profile_(profile),
+      helper_factory_(base::BindRepeating(&BookmarkAppHelperCreateWrapper)),
+      data_retriever_(std::make_unique<BookmarkAppDataRetriever>()),
+      installer_(std::make_unique<BookmarkAppInstaller>(profile)) {}
+
 BookmarkAppInstallationTask::~BookmarkAppInstallationTask() = default;
 
+void BookmarkAppInstallationTask::InstallWebAppOrShortcutFromWebContents(
+    content::WebContents* web_contents,
+    ResultCallback callback) {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+
+  data_retriever().GetWebApplicationInfo(
+      web_contents,
+      base::BindOnce(&BookmarkAppInstallationTask::OnGetWebApplicationInfo,
+                     weak_ptr_factory_.GetWeakPtr(), std::move(callback),
+                     web_contents));
+}
+
+void BookmarkAppInstallationTask::SetBookmarkAppHelperFactoryForTesting(
+    BookmarkAppHelperFactory helper_factory) {
+  helper_factory_ = helper_factory;
+}
+
 void BookmarkAppInstallationTask::SetDataRetrieverForTesting(
     std::unique_ptr<BookmarkAppDataRetriever> data_retriever) {
   data_retriever_ = std::move(data_retriever);
@@ -36,8 +87,32 @@ void BookmarkAppInstallationTask::SetInstallerForTesting(
   installer_ = std::move(installer);
 }
 
-BookmarkAppInstallationTask::BookmarkAppInstallationTask(Profile* profile)
-    : data_retriever_(std::make_unique<BookmarkAppDataRetriever>()),
-      installer_(std::make_unique<BookmarkAppInstaller>(profile)) {}
+void BookmarkAppInstallationTask::OnGetWebApplicationInfo(
+    ResultCallback result_callback,
+    content::WebContents* web_contents,
+    std::unique_ptr<WebApplicationInfo> web_app_info) {
+  if (!web_app_info) {
+    std::move(result_callback)
+        .Run(Result(ResultCode::kGetWebApplicationInfoFailed, std::string()));
+    return;
+  }
+
+  // TODO(crbug.com/864904): Use an appropriate install source once source
+  // is plumbed through this class.
+  helper_ = helper_factory_.Run(profile_, *web_app_info, web_contents,
+                                WebappInstallSource::MENU_BROWSER_TAB);
+  helper_->Create(base::Bind(&BookmarkAppInstallationTask::OnInstalled,
+                             weak_ptr_factory_.GetWeakPtr(),
+                             base::Passed(&result_callback)));
+}
+
+void BookmarkAppInstallationTask::OnInstalled(
+    ResultCallback result_callback,
+    const Extension* extension,
+    const WebApplicationInfo& web_app_info) {
+  std::move(result_callback)
+      .Run(extension ? Result(ResultCode::kSuccess, extension->id())
+                     : Result(ResultCode::kInstallationFailed, std::string()));
+}
 
 }  // namespace extensions

chrome/browser/web_applications/extensions/bookmark_app_installation_task.h

@@ -8,15 +8,24 @@
 #include <memory>
 #include <string>
 
-#include "base/callback_forward.h"
+#include "base/callback.h"
 #include "base/macros.h"
+#include "base/memory/weak_ptr.h"
 
 class Profile;
+enum class WebappInstallSource;
+struct WebApplicationInfo;
+
+namespace content {
+class WebContents;
+}
 
 namespace extensions {
 
 class BookmarkAppDataRetriever;
+class BookmarkAppHelper;
 class BookmarkAppInstaller;
+class Extension;
 
 // Class to install a BookmarkApp-based Shortcut or WebApp from a WebContents
 // or WebApplicationInfo. Can only be called from the UI thread.
@@ -41,23 +50,55 @@ class BookmarkAppInstallationTask {
   };
 
   using ResultCallback = base::OnceCallback<void(Result)>;
+  using BookmarkAppHelperFactory =
+      base::RepeatingCallback<std::unique_ptr<BookmarkAppHelper>(
+          Profile*,
+          const WebApplicationInfo&,
+          content::WebContents*,
+          WebappInstallSource)>;
+
+  // Ensures the tab helpers necessary for installing an app are present.
+  static void CreateTabHelpers(content::WebContents* web_contents);
+
+  explicit BookmarkAppInstallationTask(Profile* profile);
 
   virtual ~BookmarkAppInstallationTask();
 
+  virtual void InstallWebAppOrShortcutFromWebContents(
+      content::WebContents* web_contents,
+      ResultCallback callback);
+
+  void SetBookmarkAppHelperFactoryForTesting(
+      BookmarkAppHelperFactory helper_factory);
   void SetDataRetrieverForTesting(
       std::unique_ptr<BookmarkAppDataRetriever> data_retriever);
   void SetInstallerForTesting(std::unique_ptr<BookmarkAppInstaller> installer);
 
  protected:
-  explicit BookmarkAppInstallationTask(Profile* profile);
-
   BookmarkAppDataRetriever& data_retriever() { return *data_retriever_; }
   BookmarkAppInstaller& installer() { return *installer_; }
 
  private:
+  void OnGetWebApplicationInfo(
+      ResultCallback result_callback,
+      content::WebContents* web_contents,
+      std::unique_ptr<WebApplicationInfo> web_app_info);
+  void OnInstalled(ResultCallback result_callback,
+                   const Extension* extension,
+                   const WebApplicationInfo& web_app_info);
+
+  Profile* profile_;
+
+  // We temporarily use a BookmarkAppHelper until the WebApp and WebShortcut
+  // installation tasks reach feature parity with BookmarkAppHelper.
+  std::unique_ptr<BookmarkAppHelper> helper_;
+  BookmarkAppHelperFactory helper_factory_;
+
   std::unique_ptr<BookmarkAppDataRetriever> data_retriever_;
   std::unique_ptr<BookmarkAppInstaller> installer_;
 
+  base::WeakPtrFactory<BookmarkAppInstallationTask> weak_ptr_factory_{this};
+
   DISALLOW_COPY_AND_ASSIGN(BookmarkAppInstallationTask);
 };
 

chrome/browser/web_applications/extensions/bookmark_app_installation_task_unittest.cc

@@ -4,28 +4,35 @@
 
 #include "chrome/browser/web_applications/extensions/bookmark_app_installation_task.h"
 
+#include <map>
 #include <memory>
 #include <utility>
 #include <vector>
 
 #include "base/bind.h"
+#include "base/bind_helpers.h"
 #include "base/callback.h"
 #include "base/macros.h"
 #include "base/optional.h"
 #include "base/run_loop.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/threading/thread_task_runner_handle.h"
+#include "chrome/browser/extensions/bookmark_app_helper.h"
+#include "chrome/browser/extensions/extension_service.h"
 #include "chrome/browser/extensions/test_extension_system.h"
+#include "chrome/browser/installable/installable_data.h"
 #include "chrome/browser/web_applications/extensions/bookmark_app_data_retriever.h"
 #include "chrome/browser/web_applications/extensions/bookmark_app_installer.h"
 #include "chrome/browser/web_applications/extensions/bookmark_app_shortcut_installation_task.h"
 #include "chrome/common/web_application_info.h"
 #include "chrome/test/base/chrome_render_view_host_test_harness.h"
 #include "chrome/test/base/testing_profile.h"
+#include "content/public/test/test_utils.h"
 #include "content/public/test/web_contents_tester.h"
 #include "extensions/common/extension_id.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "third_party/blink/public/common/manifest/manifest.h"
+#include "third_party/skia/include/core/SkBitmap.h"
 
 namespace extensions {
 
@@ -34,11 +41,43 @@ using ResultCode = BookmarkAppInstallationTask::ResultCode;
 
 namespace {
 
-const char kWebAppUrl[] = "https://foo.example";
 const char kWebAppTitle[] = "Foo Title";
+const char kWebAppUrl[] = "https://foo.example";
 
 }  // namespace
 
+class TestBookmarkAppHelper : public BookmarkAppHelper {
+ public:
+  TestBookmarkAppHelper(Profile* profile,
+                        WebApplicationInfo web_app_info,
+                        content::WebContents* contents,
+                        WebappInstallSource install_source)
+      : BookmarkAppHelper(profile, web_app_info, contents, install_source) {}
+  ~TestBookmarkAppHelper() override {}
+
+  void CompleteInstallableCheck() {
+    blink::Manifest manifest;
+    InstallableData data = {
+        NO_MANIFEST, GURL(),  &manifest, GURL(), nullptr,
+        GURL(),      nullptr, false,     false,
+    };
+    BookmarkAppHelper::OnDidPerformInstallableCheck(data);
+  }
+
+  void CompleteIconDownload() {
+    BookmarkAppHelper::OnIconsDownloaded(
+        true, std::map<GURL, std::vector<SkBitmap>>());
+  }
+
+  void FailIconDownload() {
+    BookmarkAppHelper::OnIconsDownloaded(
+        false, std::map<GURL, std::vector<SkBitmap>>());
+  }
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(TestBookmarkAppHelper);
+};
+
 class BookmarkAppInstallationTaskTest : public ChromeRenderViewHostTestHarness {
  public:
   BookmarkAppInstallationTaskTest() = default;
@@ -61,15 +100,36 @@ class BookmarkAppInstallationTaskTest : public ChromeRenderViewHostTestHarness {
   }
 
  protected:
+  BookmarkAppInstallationTask::BookmarkAppHelperFactory helper_factory() {
+    return base::BindRepeating(
+        &BookmarkAppInstallationTaskTest::CreateTestBookmarkAppHelper,
+        base::Unretained(this));
+  }
+
   bool app_installed() {
     bool app_installed = app_installation_result_->code == ResultCode::kSuccess;
     EXPECT_NE(app_installed, app_installation_result_->app_id.empty());
     return app_installed;
   }
 
+  TestBookmarkAppHelper& test_helper() { return *test_helper_; }
+
   const Result& app_installation_result() { return *app_installation_result_; }
 
  private:
+  std::unique_ptr<BookmarkAppHelper> CreateTestBookmarkAppHelper(
+      Profile* profile,
+      const WebApplicationInfo& web_app_info,
+      content::WebContents* web_contents,
+      WebappInstallSource install_source) {
+    auto helper = std::make_unique<TestBookmarkAppHelper>(
+        profile, web_app_info, web_contents, install_source);
+    test_helper_ = helper.get();
+    return helper;
+  }
+
+  TestBookmarkAppHelper* test_helper_;
+
   std::unique_ptr<Result> app_installation_result_;
 
   DISALLOW_COPY_AND_ASSIGN(BookmarkAppInstallationTaskTest);
@@ -216,4 +276,54 @@ TEST_F(BookmarkAppInstallationTaskTest,
             app_installation_result().code);
 }
 
+TEST_F(BookmarkAppInstallationTaskTest,
+       WebAppOrShortcutFromContents_InstallationSucceeds) {
+  auto task = std::make_unique<BookmarkAppInstallationTask>(profile());
+  WebApplicationInfo info;
+  info.app_url = GURL(kWebAppUrl);
+  info.title = base::UTF8ToUTF16(kWebAppTitle);
+  task->SetDataRetrieverForTesting(std::make_unique<TestDataRetriever>(
+      std::make_unique<WebApplicationInfo>(std::move(info))));
+  task->SetBookmarkAppHelperFactoryForTesting(helper_factory());
+
+  task->InstallWebAppOrShortcutFromWebContents(
+      web_contents(),
+      base::BindOnce(&BookmarkAppInstallationTaskTest::OnInstallationTaskResult,
+                     base::Unretained(this), base::DoNothing().Once()));
+  content::RunAllTasksUntilIdle();
+
+  test_helper().CompleteInstallableCheck();
+  content::RunAllTasksUntilIdle();
+
+  test_helper().CompleteIconDownload();
+  content::RunAllTasksUntilIdle();
+
+  EXPECT_TRUE(app_installed());
+}
+
+TEST_F(BookmarkAppInstallationTaskTest,
+       WebAppOrShortcutFromContents_InstallationFails) {
+  auto task = std::make_unique<BookmarkAppInstallationTask>(profile());
+  WebApplicationInfo info;
+  info.app_url = GURL(kWebAppUrl);
+  info.title = base::UTF8ToUTF16(kWebAppTitle);
+  task->SetDataRetrieverForTesting(std::make_unique<TestDataRetriever>(
+      std::make_unique<WebApplicationInfo>(std::move(info))));
+  task->SetBookmarkAppHelperFactoryForTesting(helper_factory());
+
+  task->InstallWebAppOrShortcutFromWebContents(
+      web_contents(),
+      base::BindOnce(&BookmarkAppInstallationTaskTest::OnInstallationTaskResult,
+                     base::Unretained(this), base::DoNothing().Once()));
+  content::RunAllTasksUntilIdle();
+
+  test_helper().CompleteInstallableCheck();
+  content::RunAllTasksUntilIdle();
+
+  test_helper().FailIconDownload();
+  content::RunAllTasksUntilIdle();
+
+  EXPECT_FALSE(app_installed());
+}
+
 }  // namespace extensions

chrome/browser/web_applications/extensions/pending_bookmark_app_manager.cc

@@ -11,7 +11,6 @@
 
 #include "base/threading/thread_task_runner_handle.h"
 #include "chrome/browser/profiles/profile.h"
-#include "chrome/browser/web_applications/extensions/bookmark_app_shortcut_installation_task.h"
 #include "content/public/browser/navigation_controller.h"
 #include "content/public/browser/web_contents.h"
 
@@ -27,9 +26,7 @@ std::unique_ptr<content::WebContents> WebContentsCreateWrapper(
 
 std::unique_ptr<BookmarkAppInstallationTask> InstallationTaskCreateWrapper(
     Profile* profile) {
-  // TODO(crbug.com/864904): Use an installation task that can install Web Apps
-  // and Web Shortcuts.
-  return std::make_unique<BookmarkAppShortcutInstallationTask>(profile);
+  return std::make_unique<BookmarkAppInstallationTask>(profile);
 }
 
 }  // namespace
@@ -116,8 +113,11 @@ void PendingBookmarkAppManager::MaybeStartNextInstallation() {
 }
 
 void PendingBookmarkAppManager::CreateWebContentsIfNecessary() {
-  if (!web_contents_)
-    web_contents_ = web_contents_factory_.Run(profile_);
+  if (web_contents_)
+    return;
+
+  web_contents_ = web_contents_factory_.Run(profile_);
+  BookmarkAppInstallationTask::CreateTabHelpers(web_contents_.get());
 }
 
 void PendingBookmarkAppManager::OnInstalled(
@@ -155,15 +155,13 @@ void PendingBookmarkAppManager::DidFinishLoad(
 
   Observe(nullptr);
   current_installation_task_ = task_factory_.Run(profile_);
-  static_cast<BookmarkAppShortcutInstallationTask*>(
-      current_installation_task_.get())
-      ->InstallFromWebContents(
-          web_contents_.get(),
-          base::BindOnce(&PendingBookmarkAppManager::OnInstalled,
-                         // Safe because the installation task will not run its
-                         // callback after being deleted and this class owns the
-                         // task.
-                         base::Unretained(this)));
+  current_installation_task_->InstallWebAppOrShortcutFromWebContents(
+      web_contents_.get(),
+      base::BindOnce(&PendingBookmarkAppManager::OnInstalled,
+                     // Safe because the installation task will not run its
+                     // callback after being deleted and this class owns the
+                     // task.
+                     base::Unretained(this)));
 }
 
 void PendingBookmarkAppManager::DidFailLoad(

chrome/browser/web_applications/extensions/pending_bookmark_app_manager_browsertest.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/extensions/pending_bookmark_app_manager.h"
+
+#include "base/bind.h"
+#include "base/callback.h"
+#include "base/run_loop.h"
+#include "base/test/bind_test_util.h"
+#include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/ui/browser.h"
+#include "chrome/browser/web_applications/web_app_provider.h"
+#include "chrome/test/base/in_process_browser_test.h"
+#include "extensions/browser/extension_registry.h"
+#include "extensions/common/extension.h"
+#include "net/test/embedded_test_server/embedded_test_server.h"
+
+namespace extensions {
+
+class PendingBookmarkAppManagerBrowserTest : public InProcessBrowserTest {};
+
+// Basic integration test to make sure the whole flow works. Each step in the
+// flow is unit tested separately.
+IN_PROC_BROWSER_TEST_F(PendingBookmarkAppManagerBrowserTest, InstallSucceeds) {
+  ASSERT_TRUE(embedded_test_server()->Start());
+  base::RunLoop run_loop;
+  std::string app_id;
+
+  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),
+               base::BindLambdaForTesting(
+                   [&run_loop, &app_id](const GURL& provided_url,
+                                        const std::string& id) {
+                     app_id = id;
+                     run_loop.QuitClosure().Run();
+                   }));
+  run_loop.Run();
+
+  const Extension* app = ExtensionRegistry::Get(browser()->profile())
+                             ->enabled_extensions()
+                             .GetByID(app_id);
+  ASSERT_TRUE(app);
+  EXPECT_EQ("Manifest test app", app->name());
+}
+
+}  // namespace extensions

chrome/browser/web_applications/extensions/pending_bookmark_app_manager_unittest.cc

@@ -15,7 +15,7 @@
 #include "base/test/bind_test_util.h"
 #include "chrome/browser/extensions/test_extension_system.h"
 #include "chrome/browser/web_applications/components/pending_app_manager.h"
-#include "chrome/browser/web_applications/extensions/bookmark_app_shortcut_installation_task.h"
+#include "chrome/browser/web_applications/extensions/bookmark_app_installation_task.h"
 #include "chrome/test/base/chrome_render_view_host_test_harness.h"
 #include "chrome/test/base/testing_profile.h"
 #include "content/public/test/web_contents_tester.h"
@@ -50,14 +50,13 @@ web_app::PendingAppManager::AppInfo GetQuxAppInfo() {
 
 }  // namespace
 
-class TestBookmarkAppShortcutInstallationTask
-    : public BookmarkAppShortcutInstallationTask {
+class TestBookmarkAppInstallationTask : public BookmarkAppInstallationTask {
  public:
-  TestBookmarkAppShortcutInstallationTask(Profile* profile, bool succeeds)
-      : BookmarkAppShortcutInstallationTask(profile), succeeds_(succeeds) {}
-  ~TestBookmarkAppShortcutInstallationTask() override = default;
+  TestBookmarkAppInstallationTask(Profile* profile, bool succeeds)
+      : BookmarkAppInstallationTask(profile), succeeds_(succeeds) {}
+  ~TestBookmarkAppInstallationTask() override = default;
 
-  void InstallFromWebContents(
+  void InstallWebAppOrShortcutFromWebContents(
       content::WebContents* web_contents,
       BookmarkAppInstallationTask::ResultCallback callback) override {
     std::move(callback).Run(
@@ -72,7 +71,7 @@ class TestBookmarkAppShortcutInstallationTask
  private:
   bool succeeds_;
 
-  DISALLOW_COPY_AND_ASSIGN(TestBookmarkAppShortcutInstallationTask);
+  DISALLOW_COPY_AND_ASSIGN(TestBookmarkAppInstallationTask);
 };
 
 class PendingBookmarkAppManagerTest : public ChromeRenderViewHostTestHarness {
@@ -111,14 +110,12 @@ class PendingBookmarkAppManagerTest : public ChromeRenderViewHostTestHarness {
 
   std::unique_ptr<BookmarkAppInstallationTask> CreateSuccessfulInstallationTask(
       Profile* profile) {
-    return std::make_unique<TestBookmarkAppShortcutInstallationTask>(profile,
-                                                                     true);
+    return std::make_unique<TestBookmarkAppInstallationTask>(profile, true);
   }
 
   std::unique_ptr<BookmarkAppInstallationTask> CreateFailingInstallationTask(
       Profile* profile) {
-    return std::make_unique<TestBookmarkAppShortcutInstallationTask>(profile,
-                                                                     false);
+    return std::make_unique<TestBookmarkAppInstallationTask>(profile, false);
   }
 
   void InstallCallback(const GURL& url, const std::string& app_id) {

chrome/test/BUILD.gn

@@ -935,6 +935,7 @@ test("browser_tests") {
       "../browser/ui/zoom/zoom_controller_browsertest.cc",
       "../browser/unload_browsertest.cc",
       "../browser/usb/usb_browsertest.cc",
+      "../browser/web_applications/extensions/pending_bookmark_app_manager_browsertest.cc",
       "../browser/web_bluetooth_browsertest.cc",
       "../common/mac/app_mode_chrome_locator_browsertest.mm",
       "../common/mac/mock_launchd.cc",