WebApp: Use InstallableManager to fetch blink manifest data.

Use UpdateWebAppInfoFromManifest to merge the manifest into info.

In next CLs:
1) Add scope and theme_color into WebApp.
2) Download icons and write them to disk.

Bug: 901226
Change-Id: I3439983ef06c8c1bed5223ba2127a8620f1e4b55
Reviewed-on: https://chromium-review.googlesource.com/c/1319391
Commit-Queue: Alexey Baskakov <loyso@chromium.org>
Reviewed-by: Giovanni Ortuño Urquidi <ortuno@chromium.org>
Cr-Commit-Position: refs/heads/master@{#605929}

chrome/browser/web_applications/components/web_app_constants.h

@@ -33,7 +33,8 @@ enum class InstallResultCode {
   kFailedUnknownReason = 2,
   kGetWebApplicationInfoFailed = 3,
   kPreviouslyUninstalled = 4,
-  kMaxValue = kPreviouslyUninstalled,
+  kWebContentsDestroyed = 5,
+  kMaxValue = kWebContentsDestroyed,
 };
 
 // Where an app was installed from. This affects what flags will be used when

chrome/browser/web_applications/test/web_app_test.cc

@@ -4,10 +4,17 @@
 
 #include "chrome/browser/web_applications/test/web_app_test.h"
 
+#include "content/public/test/web_contents_tester.h"
+
 namespace web_app {
 
 WebAppTest::WebAppTest() = default;
 
 WebAppTest::~WebAppTest() = default;
 
+void WebAppTest::SetUp() {
+  ChromeRenderViewHostTestHarness::SetUp();
+  SetContents(CreateTestWebContents());
+}
+
 }  // namespace web_app

chrome/browser/web_applications/test/web_app_test.h

@@ -15,6 +15,8 @@ class WebAppTest : public ChromeRenderViewHostTestHarness {
   WebAppTest();
   ~WebAppTest() override;
 
+  void SetUp() override;
+
  private:
   DISALLOW_COPY_AND_ASSIGN(WebAppTest);
 };

chrome/browser/web_applications/web_app_install_manager.cc

@@ -7,9 +7,12 @@
 #include "base/callback.h"
 #include "base/logging.h"
 #include "base/strings/utf_string_conversions.h"
+#include "chrome/browser/installable/installable_data.h"
+#include "chrome/browser/installable/installable_manager.h"
 #include "chrome/browser/web_applications/components/web_app_constants.h"
 #include "chrome/browser/web_applications/components/web_app_data_retriever.h"
 #include "chrome/browser/web_applications/components/web_app_helpers.h"
+#include "chrome/browser/web_applications/components/web_app_install_utils.h"
 #include "chrome/browser/web_applications/web_app.h"
 #include "chrome/browser/web_applications/web_app_registrar.h"
 #include "chrome/browser/web_applications/web_app_utils.h"
@@ -37,15 +40,16 @@ bool WebAppInstallManager::CanInstallWebApp(
 void WebAppInstallManager::InstallWebApp(content::WebContents* web_contents,
                                          bool force_shortcut_app,
                                          OnceInstallCallback install_callback) {
-  // TODO(loyso): Use force_shortcut_app flag during installation.
-
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
 
+  force_shortcut_app_ = force_shortcut_app;
+  web_contents_ = web_contents;
+  install_callback_ = std::move(install_callback);
+
   data_retriever_->GetWebApplicationInfo(
       web_contents,
       base::BindOnce(&WebAppInstallManager::OnGetWebApplicationInfo,
-                     weak_ptr_factory_.GetWeakPtr(),
-                     std::move(install_callback)));
+                     weak_ptr_factory_.GetWeakPtr()));
 }
 
 void WebAppInstallManager::SetDataRetrieverForTesting(
@@ -53,34 +57,82 @@ void WebAppInstallManager::SetDataRetrieverForTesting(
   data_retriever_ = std::move(data_retriever);
 }
 
+void WebAppInstallManager::CallInstallCallback(const AppId& app_id,
+                                               InstallResultCode code) {
+  // Reset current installation process arguments.
+  force_shortcut_app_ = false;
+  web_contents_ = nullptr;
+  web_app_info_.reset();
+
+  DCHECK(install_callback_);
+  std::move(install_callback_).Run(app_id, code);
+}
+
+void WebAppInstallManager::ReturnError(InstallResultCode code) {
+  CallInstallCallback(AppId(), code);
+}
+
 void WebAppInstallManager::OnGetWebApplicationInfo(
-    OnceInstallCallback install_callback,
     std::unique_ptr<WebApplicationInfo> web_app_info) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
 
-  if (!web_app_info) {
-    std::move(install_callback)
-        .Run(AppId(), InstallResultCode::kGetWebApplicationInfoFailed);
-    return;
-  }
+  if (!web_app_info)
+    return ReturnError(InstallResultCode::kGetWebApplicationInfoFailed);
+
+  // TODO(loyso): Observe WebContents lifetime instead.
+  if (web_contents_->IsBeingDestroyed())
+    return ReturnError(InstallResultCode::kWebContentsDestroyed);
+
+  web_app_info_ = std::move(web_app_info);
+
+  // TODO(loyso): Consider to merge InstallableManager into WebAppDataRetriever.
+  InstallableManager* installable_manager =
+      InstallableManager::FromWebContents(web_contents_);
+  DCHECK(installable_manager);
+
+  // TODO(crbug.com/829232) Unify with other calls to GetData.
+  InstallableParams params;
+  params.check_eligibility = true;
+  params.valid_primary_icon = true;
+  params.valid_manifest = true;
+  params.has_worker = true;
+  // Do not wait_for_worker. OnDidPerformInstallableCheck is always invoked.
+  installable_manager->GetData(
+      params,
+      base::BindRepeating(&WebAppInstallManager::OnDidPerformInstallableCheck,
+                          weak_ptr_factory_.GetWeakPtr()));
+}
+
+void WebAppInstallManager::OnDidPerformInstallableCheck(
+    const InstallableData& data) {
+  DCHECK(data.manifest_url.is_valid() || data.manifest->IsEmpty());
+
+  if (web_contents_->IsBeingDestroyed())
+    return ReturnError(InstallResultCode::kWebContentsDestroyed);
+
+  const ForInstallableSite for_installable_site =
+      data.error_code == NO_ERROR_DETECTED && !force_shortcut_app_
+          ? ForInstallableSite::kYes
+          : ForInstallableSite::kNo;
+
+  UpdateWebAppInfoFromManifest(*data.manifest, web_app_info_.get(),
+                               for_installable_site);
 
   // TODO(loyso): Implement installation logic from BookmarkAppHelper:
-  // - InstallableManager to get InstallableData.
-  // - UpdateWebAppInfoFromManifest.
   // - UpdateShareTargetInPrefs.
   // - WebAppIconDownloader.
   // etc
 
-  const AppId app_id = GenerateAppIdFromURL(web_app_info->app_url);
+  const AppId app_id = GenerateAppIdFromURL(web_app_info_->app_url);
   auto web_app = std::make_unique<WebApp>(app_id);
 
-  web_app->SetName(base::UTF16ToUTF8(web_app_info->title));
-  web_app->SetDescription(base::UTF16ToUTF8(web_app_info->description));
-  web_app->SetLaunchUrl(web_app_info->app_url.spec());
+  web_app->SetName(base::UTF16ToUTF8(web_app_info_->title));
+  web_app->SetDescription(base::UTF16ToUTF8(web_app_info_->description));
+  web_app->SetLaunchUrl(web_app_info_->app_url.spec());
 
   registrar_->RegisterApp(std::move(web_app));
 
-  std::move(install_callback).Run(app_id, InstallResultCode::kSuccess);
+  CallInstallCallback(app_id, InstallResultCode::kSuccess);
 }
 
 }  // namespace web_app

chrome/browser/web_applications/web_app_install_manager.h

@@ -5,12 +5,20 @@
 #ifndef CHROME_BROWSER_WEB_APPLICATIONS_WEB_APP_INSTALL_MANAGER_H_
 #define CHROME_BROWSER_WEB_APPLICATIONS_WEB_APP_INSTALL_MANAGER_H_
 
+#include <memory>
+
+#include "base/callback.h"
 #include "base/memory/weak_ptr.h"
 #include "chrome/browser/web_applications/components/install_manager.h"
 
 class Profile;
+struct InstallableData;
 struct WebApplicationInfo;
 
+namespace content {
+class WebContents;
+}
+
 namespace web_app {
 
 class WebAppDataRetriever;
@@ -31,9 +39,21 @@ class WebAppInstallManager final : public InstallManager {
       std::unique_ptr<WebAppDataRetriever> data_retriever);
 
  private:
+  void CallInstallCallback(const AppId& app_id, InstallResultCode code);
+  void ReturnError(InstallResultCode code);
+
   void OnGetWebApplicationInfo(
-      OnceInstallCallback install_callback,
       std::unique_ptr<WebApplicationInfo> web_app_info);
+  void OnDidPerformInstallableCheck(const InstallableData& data);
+
+  // Forces the creation of a shortcut app instead of a PWA even if installation
+  // is available.
+  bool force_shortcut_app_ = false;
+
+  // Arguments, valid during installation process:
+  OnceInstallCallback install_callback_;
+  content::WebContents* web_contents_;
+  std::unique_ptr<WebApplicationInfo> web_app_info_;
 
   Profile* profile_;
   WebAppRegistrar* registrar_;

chrome/browser/web_applications/web_app_install_manager_unittest.cc

@@ -4,10 +4,16 @@
 
 #include "chrome/browser/web_applications/web_app_install_manager.h"
 
+#include <memory>
+
 #include "base/callback.h"
 #include "base/run_loop.h"
+#include "base/strings/nullable_string16.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/test/bind_test_util.h"
+#include "chrome/browser/installable/installable_data.h"
+#include "chrome/browser/installable/installable_manager.h"
+#include "chrome/browser/ssl/security_state_tab_helper.h"
 #include "chrome/browser/web_applications/components/web_app_constants.h"
 #include "chrome/browser/web_applications/test/test_data_retriever.h"
 #include "chrome/browser/web_applications/test/web_app_test.h"
@@ -20,48 +26,118 @@
 
 namespace web_app {
 
-class WebAppInstallManagerTest : public WebAppTest {};
+class TestInstallableManager : public InstallableManager {
+ public:
+  explicit TestInstallableManager(content::WebContents* web_contents)
+      : InstallableManager(web_contents) {}
+
+  static TestInstallableManager* CreateForWebContents(
+      content::WebContents* web_contents) {
+    auto manager = std::make_unique<TestInstallableManager>(web_contents);
+    TestInstallableManager* result = manager.get();
+    web_contents->SetUserData(UserDataKey(), std::move(manager));
+    return result;
+  }
+
+  void SetData(std::unique_ptr<InstallableData> data) {
+    data_ = std::move(data);
+  }
+
+  void GetData(const InstallableParams& params,
+               const InstallableCallback& callback) override {
+    callback.Run(*std::move(data_));
+  }
+
+  std::unique_ptr<InstallableData> data_;
+};
+
+class WebAppInstallManagerTest : public WebAppTest {
+ public:
+  void SetUp() override {
+    WebAppTest::SetUp();
+
+    registrar_ = std::make_unique<WebAppRegistrar>();
+    install_manager_ =
+        std::make_unique<WebAppInstallManager>(profile(), registrar_.get());
+  }
+
+  void CreateRendererAppInfo(const GURL& url,
+                             const std::string name,
+                             const std::string description) {
+    auto web_app_info = std::make_unique<WebApplicationInfo>();
+
+    web_app_info->app_url = url;
+    web_app_info->title = base::UTF8ToUTF16(name);
+    web_app_info->description = base::UTF8ToUTF16(description);
+
+    install_manager_->SetDataRetrieverForTesting(
+        std::make_unique<TestDataRetriever>(std::move(web_app_info)));
+  }
+
+  void CreateDefaultInstallableManager() {
+    InstallableManager::CreateForWebContents(web_contents());
+    // Required by InstallableManager.
+    // Causes eligibility check to return NOT_FROM_SECURE_ORIGIN for GetData.
+    SecurityStateTabHelper::CreateForWebContents(web_contents());
+  }
+
+  void CreateTestInstallableManager(const GURL& manifest_url,
+                                    blink::Manifest* manifest) {
+    const InstallableStatusCode installable_code = NO_ERROR_DETECTED;
+    const bool valid_manifest = true;
+    const bool has_worker = true;
+
+    // Not used in WebAppInstallManager:
+    const GURL icon_url;
+    const std::unique_ptr<SkBitmap> icon;
+
+    auto installable_data = std::make_unique<InstallableData>(
+        installable_code, manifest_url, manifest, icon_url, icon.get(),
+        icon_url, icon.get(), valid_manifest, has_worker);
+
+    TestInstallableManager* installable_manager =
+        TestInstallableManager::CreateForWebContents(web_contents());
+    installable_manager->SetData(std::move(installable_data));
+  }
+
+  static base::NullableString16 ToNullableUTF16(const std::string& str) {
+    return base::NullableString16(base::UTF8ToUTF16(str), false);
+  }
+
+ protected:
+  std::unique_ptr<WebAppRegistrar> registrar_;
+  std::unique_ptr<WebAppInstallManager> install_manager_;
+};
 
 TEST_F(WebAppInstallManagerTest, InstallFromWebContents) {
   EXPECT_EQ(true, AllowWebAppInstallation(profile()));
 
-  auto registrar = std::make_unique<WebAppRegistrar>();
-  auto manager =
-      std::make_unique<WebAppInstallManager>(profile(), registrar.get());
-
-  auto web_app_info = std::make_unique<WebApplicationInfo>();
-
   const GURL url = GURL("https://example.com/path");
   const std::string name = "Name";
   const std::string description = "Description";
-
   const AppId app_id = GenerateAppIdFromURL(url);
 
-  web_app_info->app_url = url;
-  web_app_info->title = base::UTF8ToUTF16(name);
-  web_app_info->description = base::UTF8ToUTF16(description);
-  manager->SetDataRetrieverForTesting(
-      std::make_unique<TestDataRetriever>(std::move(web_app_info)));
+  CreateRendererAppInfo(url, name, description);
+  CreateDefaultInstallableManager();
 
   base::RunLoop run_loop;
   bool callback_called = false;
   const bool force_shortcut_app = false;
 
-  manager->InstallWebApp(
+  install_manager_->InstallWebApp(
       web_contents(), force_shortcut_app,
       base::BindLambdaForTesting(
           [&](const AppId& installed_app_id, InstallResultCode code) {
             EXPECT_EQ(InstallResultCode::kSuccess, code);
             EXPECT_EQ(app_id, installed_app_id);
             callback_called = true;
-            base::ThreadTaskRunnerHandle::Get()->PostTask(
-                FROM_HERE, run_loop.QuitClosure());
+            run_loop.Quit();
           }));
   run_loop.Run();
 
   EXPECT_TRUE(callback_called);
 
-  WebApp* web_app = registrar->GetAppById(app_id);
+  WebApp* web_app = registrar_->GetAppById(app_id);
   EXPECT_NE(nullptr, web_app);
 
   EXPECT_EQ(app_id, web_app->app_id());
@@ -71,30 +147,71 @@ TEST_F(WebAppInstallManagerTest, InstallFromWebContents) {
 }
 
 TEST_F(WebAppInstallManagerTest, GetWebApplicationInfoFailed) {
-  auto registrar = std::make_unique<WebAppRegistrar>();
-  auto manager =
-      std::make_unique<WebAppInstallManager>(profile(), registrar.get());
+  install_manager_->SetDataRetrieverForTesting(
+      std::make_unique<TestDataRetriever>(
+          std::unique_ptr<WebApplicationInfo>()));
 
-  manager->SetDataRetrieverForTesting(std::make_unique<TestDataRetriever>(
-      std::unique_ptr<WebApplicationInfo>()));
+  CreateDefaultInstallableManager();
 
   base::RunLoop run_loop;
   bool callback_called = false;
   const bool force_shortcut_app = false;
 
-  manager->InstallWebApp(
+  install_manager_->InstallWebApp(
       web_contents(), force_shortcut_app,
       base::BindLambdaForTesting(
           [&](const AppId& installed_app_id, InstallResultCode code) {
             EXPECT_EQ(InstallResultCode::kGetWebApplicationInfoFailed, code);
             EXPECT_EQ(AppId(), installed_app_id);
             callback_called = true;
-            base::ThreadTaskRunnerHandle::Get()->PostTask(
-                FROM_HERE, run_loop.QuitClosure());
+            run_loop.Quit();
+          }));
+  run_loop.Run();
+
+  EXPECT_TRUE(callback_called);
+}
+
+// TODO(loyso): Convert more tests from bookmark_app_helper_unittest.cc
+TEST_F(WebAppInstallManagerTest, InstallableCheck) {
+  const std::string renderer_description = "RendererDescription";
+  CreateRendererAppInfo(GURL("https://example.com/path"), "RendererName",
+                        renderer_description);
+
+  const GURL manifest_start_url = GURL("https://example.com/start");
+  const AppId app_id = GenerateAppIdFromURL(manifest_start_url);
+  const std::string manifest_name = "Name from Manifest";
+
+  blink::Manifest manifest;
+  manifest.short_name = ToNullableUTF16("Short Name from Manifest");
+  manifest.name = ToNullableUTF16(manifest_name);
+  manifest.start_url = GURL(manifest_start_url);
+
+  CreateTestInstallableManager(GURL("https://example.com/manifest"), &manifest);
+
+  base::RunLoop run_loop;
+  bool callback_called = false;
+  const bool force_shortcut_app = false;
+
+  install_manager_->InstallWebApp(
+      web_contents(), force_shortcut_app,
+      base::BindLambdaForTesting(
+          [&](const AppId& installed_app_id, InstallResultCode code) {
+            EXPECT_EQ(InstallResultCode::kSuccess, code);
+            EXPECT_EQ(app_id, installed_app_id);
+            callback_called = true;
+            run_loop.Quit();
           }));
   run_loop.Run();
 
   EXPECT_TRUE(callback_called);
+
+  WebApp* web_app = registrar_->GetAppById(app_id);
+  EXPECT_NE(nullptr, web_app);
+
+  EXPECT_EQ(app_id, web_app->app_id());
+  EXPECT_EQ(manifest_name, web_app->name());
+  EXPECT_EQ(manifest_start_url.spec(), web_app->launch_url());
+  EXPECT_EQ(renderer_description, web_app->description());
 }
 
 }  // namespace web_app

tools/metrics/histograms/enums.xml

@@ -52331,6 +52331,7 @@ Full version information for the fingerprint enum values:
   <int value="2" label="FailedUnknownReason"/>
   <int value="3" label="GetWebApplicationInfoFailed"/>
   <int value="4" label="PreviouslyUninstalled"/>
+  <int value="5" label="WebContentsDestroyed"/>
 </enum>
 
 <enum name="WebAppInstallSource">