desktop-pwas: Add option to install a placeholder app

Also adds test to PendingBookmarkAppManager and
BookmarkAppInstallationTask unit tests.

If for some reason we can't load the URL for an app, we will install
a "placeholder app" if the client specified that option. The placeholder
app has the default Chrome Apps icon, the URL as the name of the app,
and the launch container specified by the client.

Bug: 844278
Change-Id: Ia29ff33ffed05b6f7f89e7f439b55db8346e72b5
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1546740
Commit-Queue: Giovanni Ortuño Urquidi <ortuno@chromium.org>
Reviewed-by: Alexey Baskakov <loyso@chromium.org>
Cr-Commit-Position: refs/heads/master@{#647092}

chrome/browser/web_applications/components/install_options.h

@@ -62,6 +62,13 @@ struct InstallOptions {
 
   // Whether the app should be reinstalled even if it is already installed.
   bool always_update = false;
+
+  // Whether a placeholder app should be installed if we fail to retrieve the
+  // metadata for the app. A placeholder app uses:
+  //  - The default Chrome App icon for the icon
+  //  - |url| as the start_url
+  //  - |url| as the app name
+  bool install_placeholder = false;
 };
 
 std::ostream& operator<<(std::ostream& out,

chrome/browser/web_applications/extensions/bookmark_app_installation_task.cc

@@ -4,19 +4,22 @@
 
 #include "chrome/browser/web_applications/extensions/bookmark_app_installation_task.h"
 
+#include <memory>
+#include <utility>
+
 #include "base/bind.h"
 #include "base/callback.h"
+#include "base/strings/utf_string_conversions.h"
 #include "chrome/browser/favicon/favicon_utils.h"
 #include "chrome/browser/installable/installable_manager.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/ssl/security_state_tab_helper.h"
+#include "chrome/browser/web_applications/components/install_finalizer.h"
 #include "chrome/browser/web_applications/components/install_manager.h"
 #include "chrome/browser/web_applications/components/web_app_constants.h"
 #include "chrome/browser/web_applications/components/web_app_provider_base.h"
+#include "chrome/common/web_application_info.h"
 #include "content/public/browser/browser_thread.h"
-#include "extensions/browser/extension_registry.h"
-#include "extensions/common/constants.h"
-#include "extensions/common/extension.h"
 
 namespace extensions {
 
@@ -41,9 +44,11 @@ void BookmarkAppInstallationTask::CreateTabHelpers(
 
 BookmarkAppInstallationTask::BookmarkAppInstallationTask(
     Profile* profile,
+    std::unique_ptr<web_app::InstallFinalizer> install_finalizer,
     web_app::InstallOptions install_options)
     : profile_(profile),
       extension_ids_map_(profile_->GetPrefs()),
+      install_finalizer_(std::move(install_finalizer)),
       install_options_(std::move(install_options)) {}
 
 BookmarkAppInstallationTask::~BookmarkAppInstallationTask() = default;
@@ -60,22 +65,46 @@ void BookmarkAppInstallationTask::Install(content::WebContents* web_contents,
   provider->install_manager().InstallWebAppWithOptions(
       web_contents, install_options_,
       base::BindOnce(&BookmarkAppInstallationTask::OnWebAppInstalled,
-                     weak_ptr_factory_.GetWeakPtr(),
+                     weak_ptr_factory_.GetWeakPtr(), false /* is_placeholder */,
                      std::move(result_callback)));
 }
 
+void BookmarkAppInstallationTask::InstallPlaceholder(ResultCallback callback) {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+
+  WebApplicationInfo web_app_info;
+  web_app_info.title = base::UTF8ToUTF16(install_options_.url.spec());
+  web_app_info.app_url = install_options_.url;
+
+  switch (install_options_.launch_container) {
+    case web_app::LaunchContainer::kDefault:
+    case web_app::LaunchContainer::kTab:
+      web_app_info.open_as_window = false;
+      break;
+    case web_app::LaunchContainer::kWindow:
+      web_app_info.open_as_window = true;
+      break;
+  }
+
+  install_finalizer_->FinalizeInstall(
+      web_app_info,
+      base::BindOnce(&BookmarkAppInstallationTask::OnWebAppInstalled,
+                     weak_ptr_factory_.GetWeakPtr(), true /* is_placeholder */,
+                     std::move(callback)));
+}
+
 void BookmarkAppInstallationTask::OnWebAppInstalled(
+    bool is_placeholder,
     ResultCallback result_callback,
     const web_app::AppId& app_id,
     web_app::InstallResultCode code) {
-  ExtensionRegistry* registry = ExtensionRegistry::Get(profile_);
-  const Extension* extension = registry->enabled_extensions().GetByID(app_id);
-
-  if (code == web_app::InstallResultCode::kSuccess && extension) {
-    extension_ids_map_.Insert(install_options_.url, extension->id(),
+  if (code == web_app::InstallResultCode::kSuccess) {
+    extension_ids_map_.Insert(install_options_.url, app_id,
                               install_options_.install_source);
+    extension_ids_map_.SetIsPlaceholder(install_options_.url, is_placeholder);
+
     std::move(result_callback)
-        .Run(Result(web_app::InstallResultCode::kSuccess, extension->id()));
+        .Run(Result(web_app::InstallResultCode::kSuccess, app_id));
   } else {
     std::move(result_callback).Run(Result(code, base::nullopt));
   }

chrome/browser/web_applications/extensions/bookmark_app_installation_task.h

@@ -5,6 +5,8 @@
 #ifndef CHROME_BROWSER_WEB_APPLICATIONS_EXTENSIONS_BOOKMARK_APP_INSTALLATION_TASK_H_
 #define CHROME_BROWSER_WEB_APPLICATIONS_EXTENSIONS_BOOKMARK_APP_INSTALLATION_TASK_H_
 
+#include <memory>
+
 #include "base/callback.h"
 #include "base/macros.h"
 #include "base/memory/weak_ptr.h"
@@ -21,6 +23,7 @@ class WebContents;
 
 namespace web_app {
 enum class InstallResultCode;
+class InstallFinalizer;
 }
 
 namespace extensions {
@@ -52,18 +55,23 @@ class BookmarkAppInstallationTask {
   // for |profile|. |install_options| will be used to decide some of the
   // properties of the installed app e.g. open in a tab vs. window, installed by
   // policy, etc.
-  explicit BookmarkAppInstallationTask(Profile* profile,
-                                       web_app::InstallOptions install_options);
+  explicit BookmarkAppInstallationTask(
+      Profile* profile,
+      std::unique_ptr<web_app::InstallFinalizer> install_finalizer,
+      web_app::InstallOptions install_options);
 
   virtual ~BookmarkAppInstallationTask();
 
   virtual void Install(content::WebContents* web_contents,
                        ResultCallback result_callback);
 
+  virtual void InstallPlaceholder(ResultCallback result_callback);
+
   const web_app::InstallOptions& install_options() { return install_options_; }
 
  private:
-  void OnWebAppInstalled(ResultCallback result_callback,
+  void OnWebAppInstalled(bool is_placeholder,
+                         ResultCallback result_callback,
                          const web_app::AppId& app_id,
                          web_app::InstallResultCode code);
 
@@ -71,6 +79,8 @@ class BookmarkAppInstallationTask {
 
   web_app::ExtensionIdsMap extension_ids_map_;
 
+  std::unique_ptr<web_app::InstallFinalizer> install_finalizer_;
+
   const web_app::InstallOptions install_options_;
 
   base::WeakPtrFactory<BookmarkAppInstallationTask> weak_ptr_factory_{this};

chrome/browser/web_applications/extensions/bookmark_app_installation_task_unittest.cc

@@ -6,6 +6,7 @@
 
 #include <map>
 #include <memory>
+#include <string>
 #include <utility>
 #include <vector>
 
@@ -28,6 +29,8 @@
 #include "chrome/browser/web_applications/components/web_app_provider_base.h"
 #include "chrome/browser/web_applications/extensions/web_app_extension_ids_map.h"
 #include "chrome/browser/web_applications/test/test_data_retriever.h"
+#include "chrome/browser/web_applications/test/test_install_finalizer.h"
+#include "chrome/common/pref_names.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"
@@ -49,6 +52,17 @@ namespace {
 const char kWebAppTitle[] = "Foo Title";
 const char kWebAppUrl[] = "https://foo.example";
 
+// TODO(ortuno): Move this to ExtensionIdsMap or replace with a method
+// in ExtensionIdsMap once there is one.
+bool IsPlaceholderApp(Profile* profile, const GURL& url) {
+  const base::Value* map =
+      profile->GetPrefs()->GetDictionary(prefs::kWebAppsExtensionIDs);
+
+  const base::Value* entry = map->FindKey(url.spec());
+
+  return entry->FindBoolKey("is_placeholder").value();
+}
+
 }  // namespace
 
 class TestBookmarkAppHelper : public BookmarkAppHelper {
@@ -165,7 +179,7 @@ class BookmarkAppInstallationTaskTest : public ChromeRenderViewHostTestHarness {
 TEST_F(BookmarkAppInstallationTaskTest,
        WebAppOrShortcutFromContents_InstallationSucceeds) {
   auto task = std::make_unique<BookmarkAppInstallationTask>(
-      profile(),
+      profile(), std::make_unique<web_app::TestInstallFinalizer>(),
       web_app::InstallOptions(app_url(), web_app::LaunchContainer::kDefault,
                               web_app::InstallSource::kInternal));
 
@@ -181,6 +195,8 @@ TEST_F(BookmarkAppInstallationTaskTest,
             EXPECT_EQ(web_app::InstallResultCode::kSuccess, result.code);
             EXPECT_TRUE(result.app_id.has_value());
 
+            EXPECT_FALSE(IsPlaceholderApp(profile(), app_url()));
+
             EXPECT_EQ(result.app_id.value(), id.value());
 
             EXPECT_TRUE(test_helper().add_to_quick_launch_bar());
@@ -206,7 +222,7 @@ TEST_F(BookmarkAppInstallationTaskTest,
 TEST_F(BookmarkAppInstallationTaskTest,
        WebAppOrShortcutFromContents_InstallationFails) {
   auto task = std::make_unique<BookmarkAppInstallationTask>(
-      profile(),
+      profile(), std::make_unique<web_app::TestInstallFinalizer>(),
       web_app::InstallOptions(app_url(), web_app::LaunchContainer::kWindow,
                               web_app::InstallSource::kInternal));
 
@@ -244,7 +260,8 @@ TEST_F(BookmarkAppInstallationTaskTest,
                                           web_app::InstallSource::kInternal);
   install_options.add_to_desktop = false;
   auto task = std::make_unique<BookmarkAppInstallationTask>(
-      profile(), std::move(install_options));
+      profile(), std::make_unique<web_app::TestInstallFinalizer>(),
+      std::move(install_options));
 
   bool callback_called = false;
   task->Install(web_contents(),
@@ -273,7 +290,8 @@ TEST_F(BookmarkAppInstallationTaskTest,
                                           web_app::InstallSource::kInternal);
   install_options.add_to_quick_launch_bar = false;
   auto task = std::make_unique<BookmarkAppInstallationTask>(
-      profile(), std::move(install_options));
+      profile(), std::make_unique<web_app::TestInstallFinalizer>(),
+      std::move(install_options));
 
   bool callback_called = false;
   task->Install(web_contents(),
@@ -305,7 +323,8 @@ TEST_F(
   install_options.add_to_desktop = false;
   install_options.add_to_quick_launch_bar = false;
   auto task = std::make_unique<BookmarkAppInstallationTask>(
-      profile(), std::move(install_options));
+      profile(), std::make_unique<web_app::TestInstallFinalizer>(),
+      std::move(install_options));
 
   bool callback_called = false;
   task->Install(web_contents(),
@@ -334,7 +353,8 @@ TEST_F(BookmarkAppInstallationTaskTest,
       web_app::InstallOptions(app_url(), web_app::LaunchContainer::kWindow,
                               web_app::InstallSource::kInternal);
   auto task = std::make_unique<BookmarkAppInstallationTask>(
-      profile(), std::move(install_options));
+      profile(), std::make_unique<web_app::TestInstallFinalizer>(),
+      std::move(install_options));
 
   bool callback_called = false;
   task->Install(web_contents(),
@@ -361,7 +381,8 @@ TEST_F(BookmarkAppInstallationTaskTest,
       web_app::InstallOptions(app_url(), web_app::LaunchContainer::kTab,
                               web_app::InstallSource::kInternal);
   auto task = std::make_unique<BookmarkAppInstallationTask>(
-      profile(), std::move(install_options));
+      profile(), std::make_unique<web_app::TestInstallFinalizer>(),
+      std::move(install_options));
 
   bool callback_called = false;
   task->Install(web_contents(),
@@ -388,7 +409,8 @@ TEST_F(BookmarkAppInstallationTaskTest,
       web_app::InstallOptions(app_url(), web_app::LaunchContainer::kDefault,
                               web_app::InstallSource::kInternal);
   auto task = std::make_unique<BookmarkAppInstallationTask>(
-      profile(), std::move(install_options));
+      profile(), std::make_unique<web_app::TestInstallFinalizer>(),
+      std::move(install_options));
 
   bool callback_called = false;
   task->Install(web_contents(),
@@ -414,7 +436,8 @@ TEST_F(BookmarkAppInstallationTaskTest,
       web_app::InstallOptions(app_url(), web_app::LaunchContainer::kDefault,
                               web_app::InstallSource::kExternalPolicy);
   auto task = std::make_unique<BookmarkAppInstallationTask>(
-      profile(), std::move(install_options));
+      profile(), std::make_unique<web_app::TestInstallFinalizer>(),
+      std::move(install_options));
 
   bool callback_called = false;
   task->Install(web_contents(),
@@ -434,4 +457,34 @@ TEST_F(BookmarkAppInstallationTaskTest,
   EXPECT_TRUE(callback_called);
 }
 
+TEST_F(BookmarkAppInstallationTaskTest, InstallPlaceholder) {
+  web_app::InstallOptions options(app_url(), web_app::LaunchContainer::kWindow,
+                                  web_app::InstallSource::kExternalPolicy);
+  auto finalizer = std::make_unique<web_app::TestInstallFinalizer>();
+  auto* finalizer_ptr = finalizer.get();
+
+  auto task = std::make_unique<BookmarkAppInstallationTask>(
+      profile(), std::move(finalizer), std::move(options));
+
+  base::RunLoop run_loop;
+  task->InstallPlaceholder(base::BindLambdaForTesting(
+      [&](BookmarkAppInstallationTask::Result result) {
+        EXPECT_EQ(web_app::InstallResultCode::kSuccess, result.code);
+        EXPECT_TRUE(result.app_id.has_value());
+
+        EXPECT_TRUE(IsPlaceholderApp(profile(), app_url()));
+
+        std::unique_ptr<WebApplicationInfo> web_app_info =
+            finalizer_ptr->web_app_info();
+
+        EXPECT_EQ(base::UTF8ToUTF16(app_url().spec()), web_app_info->title);
+        EXPECT_EQ(app_url(), web_app_info->app_url);
+        EXPECT_TRUE(web_app_info->open_as_window);
+        EXPECT_TRUE(web_app_info->icons.empty());
+
+        run_loop.Quit();
+      }));
+  run_loop.Run();
+}
+
 }  // namespace extensions

chrome/browser/web_applications/extensions/pending_bookmark_app_manager.cc

@@ -14,6 +14,7 @@
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/web_applications/components/app_registrar.h"
 #include "chrome/browser/web_applications/components/web_app_constants.h"
+#include "chrome/browser/web_applications/extensions/bookmark_app_install_finalizer.h"
 #include "content/public/browser/web_contents.h"
 
 namespace extensions {
@@ -24,7 +25,8 @@ std::unique_ptr<BookmarkAppInstallationTask> InstallationTaskCreateWrapper(
     Profile* profile,
     web_app::InstallOptions install_options) {
   return std::make_unique<BookmarkAppInstallationTask>(
-      profile, std::move(install_options));
+      profile, std::make_unique<BookmarkAppInstallFinalizer>(profile),
+      std::move(install_options));
 }
 
 }  // namespace
@@ -188,18 +190,27 @@ void PendingBookmarkAppManager::CreateWebContentsIfNecessary() {
 
 void PendingBookmarkAppManager::OnUrlLoaded(
     web_app::WebAppUrlLoader::Result result) {
-  if (result != web_app::WebAppUrlLoader::Result::kUrlLoaded) {
-    CurrentInstallationFinished(base::nullopt);
+  if (result == web_app::WebAppUrlLoader::Result::kUrlLoaded) {
+    current_task_and_callback_->task->Install(
+        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)));
     return;
   }
 
-  current_task_and_callback_->task->Install(
-      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)));
+  // TODO(ortuno): Move this into BookmarkAppInstallationTask::Install() once
+  // loading the URL is part of Install().
+  if (current_task_and_callback_->task->install_options().install_placeholder) {
+    current_task_and_callback_->task->InstallPlaceholder(
+        base::BindOnce(&PendingBookmarkAppManager::OnInstalled,
+                       weak_ptr_factory_.GetWeakPtr()));
+    return;
+  }
+
+  CurrentInstallationFinished(base::nullopt);
 }
 
 void PendingBookmarkAppManager::OnInstalled(

chrome/browser/web_applications/extensions/web_app_extension_ids_map.cc

@@ -5,6 +5,7 @@
 #include "chrome/browser/web_applications/extensions/web_app_extension_ids_map.h"
 
 #include <string>
+#include <utility>
 #include <vector>
 
 #include "base/values.h"
@@ -28,7 +29,8 @@ namespace {
 //   "extension_ids": {
 //     "https://events.google.com/io2016/?utm_source=web_app_manifest": {
 //       "extension_id": "mjgafbdfajpigcjmkgmeokfbodbcfijl",
-//       "install_source": 1
+//       "install_source": 1,
+//       "is_placeholder": true,
 //     },
 //     "https://www.chromestatus.com/features": {
 //       "extension_id": "fedbieoalmbobgfjapopkghdmhgncnaa",
@@ -44,6 +46,7 @@ namespace {
 // kExtensionId and kInstallSource.
 constexpr char kExtensionId[] = "extension_id";
 constexpr char kInstallSource[] = "install_source";
+constexpr char kIsPlaceholder[] = "is_placeholder";
 
 // Returns the base::Value in |pref_service| corresponding to our stored dict
 // for |extension_id|, or nullptr if it doesn't exist.
@@ -175,4 +178,18 @@ base::Optional<std::string> ExtensionIdsMap::LookupExtensionId(
   return base::nullopt;
 }
 
+void ExtensionIdsMap::SetIsPlaceholder(const GURL& url, bool is_placeholder) {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+
+  DCHECK(pref_service_->GetDictionary(prefs::kWebAppsExtensionIDs)
+             ->HasKey(url.spec()));
+  DictionaryPrefUpdate update(pref_service_, prefs::kWebAppsExtensionIDs);
+  base::Value* map = update.Get();
+
+  auto* app_entry = map->FindKey(url.spec());
+  DCHECK(app_entry);
+
+  app_entry->SetBoolKey(kIsPlaceholder, is_placeholder);
+}
+
 }  // namespace web_app

chrome/browser/web_applications/extensions/web_app_extension_ids_map.h

@@ -50,6 +50,7 @@ class ExtensionIdsMap {
               const std::string& extension_id,
               InstallSource install_source);
   base::Optional<std::string> LookupExtensionId(const GURL& url) const;
+  void SetIsPlaceholder(const GURL& url, bool is_placeholder);
 
  private:
   PrefService* pref_service_;

chrome/browser/web_applications/policy/web_app_policy_manager.cc

@@ -99,6 +99,8 @@ void WebAppPolicyManager::RefreshPolicyInstalledApps() {
     // Web Apps, but for now never pin them. See crbug.com/880125.
     install_options.add_to_quick_launch_bar = false;
 
+    install_options.install_placeholder = true;
+
     install_options_list.push_back(std::move(install_options));
   }