DPWA: override user_display_mode when install is user initiated

On WebAppInstallFinalizer::FinalizeInstall, if there is an existing app
but the install is user initiated, overwrite user controllable fields
like user_display_mode.

Bug: 1124300
Change-Id: I011fb0f9306b89d6de76256d3d6c7b197c310171
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2583202
Commit-Queue: Phillis Tang <phillis@chromium.org>
Reviewed-by: Peter Conn <peconn@chromium.org>
Reviewed-by: Alan Cutter <alancutter@chromium.org>
Reviewed-by: Dominick Ng <dominickn@chromium.org>
Reviewed-by: Daniel Murphy <dmurph@chromium.org>
Cr-Commit-Position: refs/heads/master@{#836795}

chrome/browser/ui/web_applications/create_shortcut_browsertest.cc

@@ -29,8 +29,8 @@ namespace web_app {
 
 class CreateShortcutBrowserTest : public WebAppControllerBrowserTest {
  public:
-  AppId InstallShortcutAppForCurrentUrl() {
-    chrome::SetAutoAcceptWebAppDialogForTesting(true, false);
+  AppId InstallShortcutAppForCurrentUrl(bool open_as_window = false) {
+    chrome::SetAutoAcceptWebAppDialogForTesting(true, open_as_window);
     WebAppInstallObserver observer(profile());
     CHECK(chrome::ExecuteCommand(browser(), IDC_CREATE_SHORTCUT));
     AppId app_id = observer.AwaitNextInstall();
@@ -195,4 +195,20 @@ IN_PROC_BROWSER_TEST_F(CreateShortcutBrowserTest, IgnoreInvalidManifestData) {
   EXPECT_EQ(registrar().GetAppStartUrl(app_id), url);
 }
 
+IN_PROC_BROWSER_TEST_F(CreateShortcutBrowserTest,
+                       CreateShortcutAgainOverwriteUserDisplayMode) {
+  base::UserActionTester user_action_tester;
+  NavigateToURLAndWait(browser(), GetInstallableAppURL());
+
+  AppId app_id = InstallShortcutAppForCurrentUrl();
+  EXPECT_EQ(registrar().GetAppShortName(app_id), GetInstallableAppName());
+  // Shortcut apps to PWAs should launch in a tab.
+  EXPECT_EQ(registrar().GetAppUserDisplayMode(app_id), DisplayMode::kBrowser);
+
+  InstallShortcutAppForCurrentUrl(/*open_as_window=*/true);
+  // Re-install with enabling open_as_window should update user display mode.
+  EXPECT_EQ(registrar().GetAppUserDisplayMode(app_id),
+            DisplayMode::kStandalone);
+}
+
 }  // namespace web_app

chrome/browser/web_applications/web_app_install_finalizer.cc

@@ -130,11 +130,8 @@ void WebAppInstallFinalizer::FinalizeInstall(
   const WebApp* existing_web_app = GetWebAppRegistrar().GetAppById(app_id);
 
   std::unique_ptr<WebApp> web_app;
-
   if (existing_web_app) {
-    // There is an existing app from other source(s). Preserve
-    // |user_display_mode| and any user-controllable fields here, do not modify
-    // them. Prepare copy-on-write:
+    // Prepare copy-on-write:
     DCHECK_EQ(web_app_info.start_url, existing_web_app->start_url());
     web_app = std::make_unique<WebApp>(*existing_web_app);
 
@@ -148,11 +145,18 @@ void WebAppInstallFinalizer::FinalizeInstall(
     web_app = std::make_unique<WebApp>(app_id);
     web_app->SetStartUrl(web_app_info.start_url);
     web_app->SetIsLocallyInstalled(options.locally_installed);
+    if (options.locally_installed)
+      web_app->SetInstallTime(base::Time::Now());
+  }
+
+  // Set |user_display_mode| and any user-controllable fields here if this
+  // install is user initiated or it's a new app.
+  if (webapps::InstallableMetrics::IsUserInitiatedInstallSource(
+          options.install_source) ||
+      !existing_web_app) {
     web_app->SetUserDisplayMode(web_app_info.open_as_window
                                     ? DisplayMode::kStandalone
                                     : DisplayMode::kBrowser);
-    if (options.locally_installed)
-      web_app->SetInstallTime(base::Time::Now());
   }
 
   // `WebApp::chromeos_data` has a default value already. Only override if the

chrome/browser/web_applications/web_app_install_manager_unittest.cc

@@ -1633,7 +1633,9 @@ TEST_F(WebAppInstallManagerTest,
 
   EXPECT_TRUE(registrar().IsInstalled(app_id));
   EXPECT_TRUE(registrar().IsLocallyInstalled(app_id));
-  EXPECT_EQ(DisplayMode::kStandalone,
+  // InstallWebAppFromManifestWithFallback sets user_display_mode to kBrowser
+  // because TestAcceptDialogCallback doesn't set open_as_window to true.
+  EXPECT_EQ(DisplayMode::kBrowser,
             registrar().GetAppEffectiveDisplayMode(app_id));
 }
 

components/webapps/installable/installable_metrics.cc

@@ -5,6 +5,7 @@
 #include "components/webapps/installable/installable_metrics.h"
 
 #include "base/metrics/histogram_macros.h"
+#include "base/notreached.h"
 #include "build/build_config.h"
 #include "components/webapps/webapps_client.h"
 #include "content/public/browser/web_contents.h"
@@ -38,6 +39,36 @@ bool InstallableMetrics::IsReportableInstallSource(WebappInstallSource source) {
          source == WebappInstallSource::MENU_CREATE_SHORTCUT;
 }
 
+// static
+bool InstallableMetrics::IsUserInitiatedInstallSource(
+    WebappInstallSource source) {
+  switch (source) {
+    case WebappInstallSource::MENU_BROWSER_TAB:
+    case WebappInstallSource::MENU_CUSTOM_TAB:
+    case WebappInstallSource::AUTOMATIC_PROMPT_BROWSER_TAB:
+    case WebappInstallSource::AUTOMATIC_PROMPT_CUSTOM_TAB:
+    case WebappInstallSource::API_BROWSER_TAB:
+    case WebappInstallSource::API_CUSTOM_TAB:
+    case WebappInstallSource::AMBIENT_BADGE_BROWSER_TAB:
+    case WebappInstallSource::AMBIENT_BADGE_CUSTOM_TAB:
+    case WebappInstallSource::ARC:
+    case WebappInstallSource::OMNIBOX_INSTALL_ICON:
+    case WebappInstallSource::MENU_CREATE_SHORTCUT:
+      return true;
+    case WebappInstallSource::DEVTOOLS:
+    case WebappInstallSource::MANAGEMENT_API:
+    case WebappInstallSource::INTERNAL_DEFAULT:
+    case WebappInstallSource::EXTERNAL_DEFAULT:
+    case WebappInstallSource::EXTERNAL_POLICY:
+    case WebappInstallSource::SYSTEM_DEFAULT:
+    case WebappInstallSource::SYNC:
+      return false;
+    case WebappInstallSource::COUNT:
+      NOTREACHED();
+      return false;
+  }
+}
+
 // static
 WebappInstallSource InstallableMetrics::GetInstallSource(
     content::WebContents* web_contents,

components/webapps/installable/installable_metrics.h

@@ -112,6 +112,9 @@ class InstallableMetrics {
   // TrackInstallEvent.
   static bool IsReportableInstallSource(WebappInstallSource source);
 
+  // Returns whether the install initiated by the user based on install source.
+  static bool IsUserInitiatedInstallSource(WebappInstallSource source);
+
   // Returns the appropriate WebappInstallSource for |web_contents| when the
   // install originates from |trigger|.
   static WebappInstallSource GetInstallSource(