Bookmark apps: Synced apps load display mode from manifest

When a user-installed PWA was newly synced to a Chromebook, and the
PWA was opened, we were defaulting to showing minimal-ui controls
even if the manifest specified display: standalone.

We now fetch the display mode from the manifest during sync.

Bug: 1067504

Change-Id: I1189da9cdf05a1fc581ba19e90b67e020e83e1cb
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2134089
Commit-Queue: Eric Willigers <ericwilligers@chromium.org>
Auto-Submit: Eric Willigers <ericwilligers@chromium.org>
Reviewed-by: Alan Cutter <alancutter@chromium.org>
Cr-Commit-Position: refs/heads/master@{#756182}

chrome/browser/ui/extensions/hosted_app_browsertest.cc

@@ -17,6 +17,7 @@
 #include "base/strings/string_split.h"
 #include "base/strings/stringprintf.h"
 #include "base/strings/utf_string_conversions.h"
+#include "base/test/bind_test_util.h"
 #include "base/test/metrics/user_action_tester.h"
 #include "base/test/scoped_feature_list.h"
 #include "base/values.h"
@@ -38,8 +39,12 @@
 #include "chrome/browser/ui/web_applications/test/web_app_browsertest_util.h"
 #include "chrome/browser/ui/web_applications/web_app_launch_utils.h"
 #include "chrome/browser/ui/web_applications/web_app_menu_model.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/install_manager.h"
+#include "chrome/browser/web_applications/components/web_app_constants.h"
+#include "chrome/browser/web_applications/components/web_app_helpers.h"
 #include "chrome/browser/web_applications/components/web_app_id.h"
 #include "chrome/browser/web_applications/components/web_app_provider_base.h"
 #include "chrome/browser/web_applications/test/web_app_install_observer.h"
@@ -647,6 +652,36 @@ IN_PROC_BROWSER_TEST_P(HostedOrWebAppTest, SubframeRedirectsToHostedApp) {
       EvalJs(subframe, "document.body.innerText.trim();").ExtractString());
 }
 
+using BookmarkAppTest = HostedOrWebAppTest;
+
+IN_PROC_BROWSER_TEST_P(BookmarkAppTest, InstallFromSync) {
+  ASSERT_TRUE(https_server()->Start());
+
+  const GURL app_url =
+      https_server()->GetURL("/banners/manifest_test_page.html");
+  const web_app::AppId app_id = web_app::GenerateAppIdFromURL(app_url);
+
+  auto web_app_info = std::make_unique<WebApplicationInfo>();
+  web_app_info->app_url = app_url;
+  web_app_info->scope = app_url.GetWithoutFilename();
+
+  base::RunLoop run_loop;
+  web_app::WebAppProviderBase* const provider =
+      web_app::WebAppProviderBase::GetProviderBase(profile());
+  DCHECK(provider);
+  provider->install_manager().InstallBookmarkAppFromSync(
+      app_id, std::move(web_app_info),
+      base::BindLambdaForTesting([&](const web_app::AppId& installed_app_id,
+                                     web_app::InstallResultCode code) {
+        EXPECT_EQ(web_app::InstallResultCode::kSuccessNewInstall, code);
+        EXPECT_EQ(app_id, installed_app_id);
+        run_loop.Quit();
+      }));
+  run_loop.Run();
+  EXPECT_EQ(web_app::DisplayMode::kStandalone,
+            provider->registrar().GetAppDisplayMode(app_id));
+}
+
 // Tests that platform apps can still load mixed content.
 IN_PROC_BROWSER_TEST_P(HostedAppTestWithAutoupgradesDisabled,
                        MixedContentInPlatformApp) {
@@ -1796,6 +1831,10 @@ INSTANTIATE_TEST_SUITE_P(All,
                          HostedAppTest,
                          ::testing::Values(AppType::HOSTED_APP));
 
+INSTANTIATE_TEST_SUITE_P(All,
+                         BookmarkAppTest,
+                         ::testing::Values(AppType::BOOKMARK_APP));
+
 INSTANTIATE_TEST_SUITE_P(All,
                          HostedAppTestWithAutoupgradesDisabled,
                          ::testing::Values(AppType::HOSTED_APP));

chrome/browser/web_applications/extensions/install_manager_bookmark_app_unittest.cc

@@ -643,6 +643,8 @@ TEST_F(InstallManagerBookmarkAppTest, InstallBookmarkAppFromSync) {
 
   url_loader().SetNextLoadUrlResult(
       GURL("about:blank"), web_app::WebAppUrlLoader::Result::kUrlLoaded);
+  url_loader().SetNextLoadUrlResult(
+      AppUrl(), web_app::WebAppUrlLoader::Result::kRedirectedUrlLoaded);
 
   {
     base::RunLoop run_loop;

chrome/browser/web_applications/web_app_install_manager.cc

@@ -147,10 +147,10 @@ void WebAppInstallManager::InstallBookmarkAppFromSync(
       finalizer(), data_retriever_factory_.Run());
 
   base::OnceClosure start_task = base::BindOnce(
-      &WebAppInstallTask::InstallWebAppFromInfoRetrieveIcons,
+      &WebAppInstallTask::LoadAndInstallWebAppFromSync,
       base::Unretained(task.get()), EnsureWebContentsCreated(),
-      std::move(web_application_info), is_locally_installed,
-      WebappInstallSource::SYNC,
+      base::Unretained(url_loader_.get()), std::move(web_application_info),
+      is_locally_installed, WebappInstallSource::SYNC,
       base::BindOnce(&WebAppInstallManager::OnQueuedTaskCompleted,
                      base::Unretained(this), task.get(), std::move(callback)));
 

chrome/browser/web_applications/web_app_install_task.cc

@@ -228,8 +228,9 @@ void WebAppInstallTask::InstallWebAppWithParams(
                      base::Unretained(this), /*force_shortcut_app=*/false));
 }
 
-void WebAppInstallTask::InstallWebAppFromInfoRetrieveIcons(
+void WebAppInstallTask::LoadAndInstallWebAppFromSync(
     content::WebContents* web_contents,
+    WebAppUrlLoader* url_loader,
     std::unique_ptr<WebApplicationInfo> web_application_info,
     bool is_locally_installed,
     WebappInstallSource install_source,
@@ -241,17 +242,16 @@ void WebAppInstallTask::InstallWebAppFromInfoRetrieveIcons(
   install_source_ = install_source;
   background_installation_ = true;
 
-  std::vector<GURL> icon_urls =
-      GetValidIconUrlsToDownload(*web_application_info);
+  // Default display mode, used if we fail to load the manifest.
+  web_application_info->display_mode = DisplayMode::kBrowser;
 
-  // Skip downloading the page favicons as everything in is the URL list.
-  data_retriever_->GetIcons(
-      web_contents, icon_urls, /*skip_page_fav_icons*/ true,
-      install_source_ == WebappInstallSource::SYNC
-          ? WebAppIconDownloader::Histogram::kForSync
-          : WebAppIconDownloader::Histogram::kForCreate,
-      base::BindOnce(&WebAppInstallTask::OnIconsRetrieved,
-                     base::Unretained(this), std::move(web_application_info),
+  const GURL launch_url = web_application_info->app_url;
+  url_loader->LoadUrl(
+      launch_url, web_contents,
+      WebAppUrlLoader::UrlComparison::kIgnoreQueryParamsAndRef,
+      base::BindOnce(
+          &WebAppInstallTask::OnWebAppFromSyncUrlLoadedRetrieveManifest,
+          weak_ptr_factory_.GetWeakPtr(), std::move(web_application_info),
           is_locally_installed));
 }
 
@@ -616,6 +616,59 @@ void WebAppInstallTask::OnDidCheckForIntentToPlayStore(
                      for_installable_site));
 }
 
+void WebAppInstallTask::OnWebAppFromSyncUrlLoadedRetrieveManifest(
+    std::unique_ptr<WebApplicationInfo> web_application_info,
+    bool is_locally_installed,
+    WebAppUrlLoader::Result result) {
+  if (ShouldStopInstall())
+    return;
+
+  if (result != WebAppUrlLoader::Result::kUrlLoaded) {
+    WebAppFromSyncLoadIcons(std::move(web_application_info),
+                            is_locally_installed);
+    return;
+  }
+
+  web_contents()->GetManifest(
+      base::BindOnce(&WebAppInstallTask::OnWebAppFromSyncManifestRetrieved,
+                     weak_ptr_factory_.GetWeakPtr(),
+                     std::move(web_application_info), is_locally_installed));
+}
+
+void WebAppInstallTask::OnWebAppFromSyncManifestRetrieved(
+    std::unique_ptr<WebApplicationInfo> web_application_info,
+    bool is_locally_installed,
+    const GURL& manifest_url,
+    const blink::Manifest& manifest) {
+  if (ShouldStopInstall())
+    return;
+
+  if (!manifest.IsEmpty()) {
+    // TODO(crbug.com/1067519): Copy more fields from the manifest.
+    web_application_info->display_mode = manifest.display;
+  }
+
+  WebAppFromSyncLoadIcons(std::move(web_application_info),
+                          is_locally_installed);
+}
+
+void WebAppInstallTask::WebAppFromSyncLoadIcons(
+    std::unique_ptr<WebApplicationInfo> web_application_info,
+    bool is_locally_installed) {
+  std::vector<GURL> icon_urls =
+      GetValidIconUrlsToDownload(*web_application_info);
+
+  // Skip downloading the page favicons as everything in is the URL list.
+  data_retriever_->GetIcons(
+      web_contents(), icon_urls, /*skip_page_fav_icons*/ true,
+      install_source_ == WebappInstallSource::SYNC
+          ? WebAppIconDownloader::Histogram::kForSync
+          : WebAppIconDownloader::Histogram::kForCreate,
+      base::BindOnce(&WebAppInstallTask::OnIconsRetrieved,
+                     base::Unretained(this), std::move(web_application_info),
+                     is_locally_installed));
+}
+
 void WebAppInstallTask::OnIconsRetrieved(
     std::unique_ptr<WebApplicationInfo> web_app_info,
     bool is_locally_installed,

chrome/browser/web_applications/web_app_install_task.h

@@ -118,10 +118,13 @@ class WebAppInstallTask : content::WebContentsObserver {
       InstallManager::OnceInstallCallback callback);
 
   // Starts background installation of a web app: does not show UI dialog.
-  // |web_application_info| contains all the data needed for installation. Icons
-  // will be downloaded from the icon URLs provided in |web_application_info|.
-  void InstallWebAppFromInfoRetrieveIcons(
+  // |web_application_info| contains most of the data needed for installation.
+  // The launch URL and its manifest are loaded to determine the display mode.
+  // Icons will be downloaded from the icon URLs provided in
+  // |web_application_info|. Doesn't memorize |url_loader| pointer.
+  void LoadAndInstallWebAppFromSync(
       content::WebContents* web_contents,
+      WebAppUrlLoader* url_loader,
       std::unique_ptr<WebApplicationInfo> web_application_info,
       bool is_locally_installed,
       WebappInstallSource install_source,
@@ -204,6 +207,19 @@ class WebAppInstallTask : content::WebContentsObserver {
       const std::string& intent,
       bool should_intent_to_store);
 
+  void OnWebAppFromSyncUrlLoadedRetrieveManifest(
+      std::unique_ptr<WebApplicationInfo> web_application_info,
+      bool is_locally_installed,
+      WebAppUrlLoader::Result result);
+  void OnWebAppFromSyncManifestRetrieved(
+      std::unique_ptr<WebApplicationInfo> web_application_info,
+      bool is_locally_installed,
+      const GURL& manifest_url,
+      const blink::Manifest& manifest);
+  void WebAppFromSyncLoadIcons(
+      std::unique_ptr<WebApplicationInfo> web_application_info,
+      bool is_locally_installed);
+
   void OnIconsRetrieved(std::unique_ptr<WebApplicationInfo> web_app_info,
                         bool is_locally_installed,
                         IconsMap icons_map);

chrome/browser/web_applications/web_app_install_task_unittest.cc

@@ -333,16 +333,16 @@ class WebAppInstallTaskTest : public WebAppTest {
     return app_id;
   }
 
-  AppId InstallWebAppFromInfoRetrieveIcons(const GURL& url,
+  AppId LoadAndInstallWebAppFromSync(const GURL& url,
                                      bool is_locally_installed) {
     AppId app_id;
     auto web_app_info = std::make_unique<WebApplicationInfo>();
     web_app_info->app_url = url;
 
     base::RunLoop run_loop;
-    install_task_->InstallWebAppFromInfoRetrieveIcons(
-        web_contents(), std::move(web_app_info), is_locally_installed,
-        WebappInstallSource::SYNC,
+    install_task_->LoadAndInstallWebAppFromSync(
+        web_contents(), &url_loader(), std::move(web_app_info),
+        is_locally_installed, WebappInstallSource::SYNC,
         base::BindLambdaForTesting(
             [&](const AppId& installed_app_id, InstallResultCode code) {
               ASSERT_EQ(InstallResultCode::kSuccessNewInstall, code);
@@ -908,12 +908,14 @@ TEST_F(WebAppInstallTaskTest, InstallWebAppFromInfo_GenerateIcons) {
   run_loop.Run();
 }
 
-TEST_F(WebAppInstallTaskTest, InstallWebAppFromInfoRetrieveIcons_TwoIcons) {
+TEST_F(WebAppInstallTaskTest, LoadAndInstallWebAppFromSync_TwoIcons) {
   SetInstallFinalizerForTesting();
 
   const GURL url{"https://example.com/path"};
   const GURL icon1_url{"https://example.com/path/icon1.png"};
   const GURL icon2_url{"https://example.com/path/icon2.png"};
+  url_loader().SetNextLoadUrlResult(
+      url, WebAppUrlLoader::Result::kRedirectedUrlLoaded);
 
   CreateDefaultDataToRetrieve(url);
 
@@ -940,9 +942,9 @@ TEST_F(WebAppInstallTaskTest, InstallWebAppFromInfoRetrieveIcons_TwoIcons) {
 
   base::RunLoop run_loop;
 
-  install_task_->InstallWebAppFromInfoRetrieveIcons(
-      web_contents(), std::move(web_app_info), /*is_locally_installed=*/true,
-      WebappInstallSource::SYNC,
+  install_task_->LoadAndInstallWebAppFromSync(
+      web_contents(), &url_loader(), std::move(web_app_info),
+      /*is_locally_installed=*/true, WebappInstallSource::SYNC,
       base::BindLambdaForTesting(
           [&](const AppId& installed_app_id, InstallResultCode code) {
             EXPECT_EQ(InstallResultCode::kSuccessNewInstall, code);
@@ -978,10 +980,12 @@ TEST_F(WebAppInstallTaskTest, InstallWebAppFromInfoRetrieveIcons_TwoIcons) {
   run_loop.Run();
 }
 
-TEST_F(WebAppInstallTaskTest, InstallWebAppFromInfoRetrieveIcons_NoIcons) {
+TEST_F(WebAppInstallTaskTest, LoadAndInstallWebAppFromSync_NoIcons) {
   SetInstallFinalizerForTesting();
 
   const GURL url{"https://example.com/path"};
+  url_loader().SetNextLoadUrlResult(
+      url, WebAppUrlLoader::Result::kRedirectedUrlLoaded);
   CreateDefaultDataToRetrieve(url);
   const AppId app_id = GenerateAppIdFromURL(url);
 
@@ -992,9 +996,9 @@ TEST_F(WebAppInstallTaskTest, InstallWebAppFromInfoRetrieveIcons_NoIcons) {
 
   base::RunLoop run_loop;
 
-  install_task_->InstallWebAppFromInfoRetrieveIcons(
-      web_contents(), std::move(web_app_info), /*is_locally_installed=*/false,
-      WebappInstallSource::SYNC,
+  install_task_->LoadAndInstallWebAppFromSync(
+      web_contents(), &url_loader(), std::move(web_app_info),
+      /*is_locally_installed=*/false, WebappInstallSource::SYNC,
       base::BindLambdaForTesting(
           [&](const AppId& installed_app_id, InstallResultCode code) {
             EXPECT_EQ(InstallResultCode::kSuccessNewInstall, code);
@@ -1162,21 +1166,26 @@ TEST_F(WebAppInstallTaskTest, InstallWebAppWithParams_DisplayMode) {
   }
 }
 
-TEST_F(WebAppInstallTaskTest,
-       InstallWebAppFromInfoRetrieveIcons_LocallyInstallled) {
+TEST_F(WebAppInstallTaskTest, LoadAndInstallWebAppFromSync_LocallyInstallled) {
   {
     const auto url = GURL("https://example.com/");
+    url_loader().SetNextLoadUrlResult(
+        url, WebAppUrlLoader::Result::kRedirectedUrlLoaded);
     CreateDataToRetrieve(url, /*open_as_window*/ false);
     auto app_id =
-        InstallWebAppFromInfoRetrieveIcons(url, /*is_locally_installed*/ false);
+        LoadAndInstallWebAppFromSync(url, /*is_locally_installed*/ false);
     EXPECT_FALSE(registrar().GetAppById(app_id)->is_locally_installed());
+    EXPECT_EQ(registrar().GetAppDisplayMode(app_id), DisplayMode::kBrowser);
   }
   {
     const auto url = GURL("https://example.org/");
+    url_loader().SetNextLoadUrlResult(
+        url, WebAppUrlLoader::Result::kRedirectedUrlLoaded);
     CreateDataToRetrieve(url, /*open_as_window*/ false);
     auto app_id =
-        InstallWebAppFromInfoRetrieveIcons(url, /*is_locally_installed*/ true);
+        LoadAndInstallWebAppFromSync(url, /*is_locally_installed*/ true);
     EXPECT_TRUE(registrar().GetAppById(app_id)->is_locally_installed());
+    EXPECT_EQ(registrar().GetAppDisplayMode(app_id), DisplayMode::kBrowser);
   }
 }