Replace experimental tabbed window pref with user_display_mode enum

This removes the use of the experimental_tabbed_window_mode web app pref
in favour of using WebApp::user_display_mode by allowing kTabbed as a
value in addition to kStandalone and kBrowser.

This kTabbed only takes effect when the kDesktopPWAsTabStripSettings
feature is enabled, otherwise it is interpreted as kStandalone.

This CL has no changes in behaviour other than the kTabbed state now
syncing across devices (because user_display_mode syncs across devices).
Old versions of Chromium will interpret it as UNSPECIFIED which is
treated as STANDALONE.

Bug: 1241283
Change-Id: I97540e7838de81242bed2f4f73926a04f403b2bc
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3115707
Commit-Queue: Alan Cutter <[email protected]>
Reviewed-by: Marc Treib <[email protected]>
Reviewed-by: Nancy Wang <[email protected]>
Reviewed-by: Robert Kaplow <[email protected]>
Reviewed-by: Alexey Baskakov <[email protected]>
Cr-Commit-Position: refs/heads/main@{#915942}

Author: alancutter

chrome/browser/sync/test/integration/two_client_web_apps_sync_test.cc

@@ -4,6 +4,7 @@
 
 #include "base/bind.h"
 #include "base/macros.h"
+#include "base/scoped_observation.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/test/bind.h"
 #include "base/test/scoped_feature_list.h"
@@ -22,6 +23,7 @@
 #include "chrome/browser/web_applications/web_app_icon_manager.h"
 #include "chrome/browser/web_applications/web_app_provider.h"
 #include "chrome/browser/web_applications/web_app_registrar.h"
+#include "chrome/browser/web_applications/web_app_sync_bridge.h"
 #include "chrome/common/chrome_features.h"
 #include "chrome/test/base/ui_test_utils.h"
 #include "content/public/test/browser_test.h"
@@ -30,6 +32,27 @@
 
 namespace web_app {
 
+namespace {
+
+class DisplayModeChangeWaiter : public AppRegistrarObserver {
+ public:
+  explicit DisplayModeChangeWaiter(WebAppRegistrar& registrar) {
+    observation_.Observe(&registrar);
+  }
+  void OnWebAppUserDisplayModeChanged(const AppId& app_id,
+                                      DisplayMode user_display_mode) override {
+    run_loop_.Quit();
+  }
+  void Wait() { run_loop_.Run(); }
+
+ private:
+  base::RunLoop run_loop_;
+  base::ScopedObservation<WebAppRegistrar, AppRegistrarObserver> observation_{
+      this};
+};
+
+}  // namespace
+
 class TwoClientWebAppsSyncTest : public SyncTest {
  public:
   TwoClientWebAppsSyncTest() : SyncTest(TWO_CLIENT) {
@@ -383,4 +406,31 @@ IN_PROC_BROWSER_TEST_F(TwoClientWebAppsSyncTest, SyncUsingIconUrlFallback) {
             SK_ColorBLUE);
 }
 
+IN_PROC_BROWSER_TEST_F(TwoClientWebAppsSyncTest, SyncUserDisplayModeChange) {
+  WebAppTestInstallObserver install_observer(GetProfile(1));
+  install_observer.BeginListening();
+
+  WebApplicationInfo info;
+  info.title = u"Test name";
+  info.description = u"Test description";
+  info.start_url = GURL("http://www.chromium.org/path");
+  info.scope = GURL("http://www.chromium.org/");
+  info.user_display_mode = DisplayMode::kStandalone;
+  AppId app_id = apps_helper::InstallWebApp(GetProfile(0), info);
+
+  EXPECT_EQ(install_observer.Wait(), app_id);
+  EXPECT_TRUE(AllProfilesHaveSameWebAppIds());
+
+  auto* provider1 = WebAppProvider::GetForTest(GetProfile(1));
+  WebAppRegistrar& registrar1 = provider1->registrar();
+  EXPECT_EQ(registrar1.GetAppUserDisplayMode(app_id), DisplayMode::kStandalone);
+
+  DisplayModeChangeWaiter display_mode_change_waiter(registrar1);
+  provider1->sync_bridge().SetAppUserDisplayMode(app_id, DisplayMode::kTabbed,
+                                                 /*is_user_action=*/true);
+  display_mode_change_waiter.Wait();
+
+  EXPECT_EQ(registrar1.GetAppUserDisplayMode(app_id), DisplayMode::kTabbed);
+}
+
 }  // namespace web_app

chrome/browser/ui/ash/shelf/app_service/app_service_shelf_context_menu.cc

@@ -467,7 +467,7 @@ void AppServiceShelfContextMenu::SetLaunchType(int command_id) {
   switch (app_type_) {
     case apps::mojom::AppType::kWeb:
     case apps::mojom::AppType::kSystemWeb: {
-      // Web apps can only toggle between kWindow and kBrowser.
+      // Web apps can only toggle between kWindow, kTabbed and kBrowser.
       apps::mojom::WindowMode user_window_mode =
           ConvertLaunchTypeCommandToWindowMode(command_id);
       if (user_window_mode != apps::mojom::WindowMode::kUnknown) {

chrome/browser/ui/ash/shelf/app_service/app_service_shelf_context_menu_browsertest.cc

@@ -132,7 +132,7 @@ IN_PROC_BROWSER_TEST_F(AppServiceShelfContextMenuWebAppBrowserTest,
   apps::AppServiceProxyFactory::GetForProfile(profile)
       ->FlushMojoCallsForTesting();
 
-  EXPECT_EQ(user_action_tester.GetActionCount("WebApp.SetWindowMode.Window"),
+  EXPECT_EQ(user_action_tester.GetActionCount("WebApp.SetWindowMode.Tabbed"),
             1);
 
   // App window should have tab strip.

chrome/browser/ui/views/web_apps/pwa_confirmation_bubble_view.cc

@@ -161,8 +161,8 @@ PWAConfirmationBubbleView::PWAConfirmationBubbleView(
     tabbed_window_checkbox_ = labels->AddChildView(
         std::make_unique<views::Checkbox>(l10n_util::GetStringUTF16(
             IDS_BOOKMARK_APP_BUBBLE_OPEN_AS_TABBED_WINDOW)));
-    tabbed_window_checkbox_->SetChecked(
-        web_app_info_->enable_experimental_tabbed_window);
+    tabbed_window_checkbox_->SetChecked(web_app_info_->user_display_mode ==
+                                        web_app::DisplayMode::kTabbed);
   }
 
   chrome::RecordDialogCreation(chrome::DialogIdentifier::PWA_CONFIRMATION);
@@ -203,10 +203,10 @@ void PWAConfirmationBubbleView::WindowClosing() {
 
 bool PWAConfirmationBubbleView::Accept() {
   DCHECK(web_app_info_);
-  if (tabbed_window_checkbox_) {
-    web_app_info_->enable_experimental_tabbed_window =
-        tabbed_window_checkbox_->GetChecked();
-  }
+  web_app_info_->user_display_mode =
+      tabbed_window_checkbox_ && tabbed_window_checkbox_->GetChecked()
+          ? web_app::DisplayMode::kTabbed
+          : web_app::DisplayMode::kStandalone;
 
   if (iph_state_ == chrome::PwaInProductHelpState::kShown) {
     web_app::AppId app_id = web_app::GenerateAppId(web_app_info_->manifest_id,

chrome/browser/ui/views/web_apps/web_app_confirmation_view.cc

@@ -129,10 +129,10 @@ WebAppConfirmationView::WebAppConfirmationView(
 
     if (web_app_info_->user_display_mode == web_app::DisplayMode::kBrowser)
       open_as_tab_radio_->SetChecked(true);
-    else if (!web_app_info_->enable_experimental_tabbed_window)
-      open_as_window_radio_->SetChecked(true);
-    else
+    else if (web_app_info_->user_display_mode == web_app::DisplayMode::kTabbed)
       open_as_tabbed_window_radio_->SetChecked(true);
+    else
+      open_as_window_radio_->SetChecked(true);
   } else {
     auto open_as_window_checkbox = std::make_unique<views::Checkbox>(
         l10n_util::GetStringUTF16(IDS_BOOKMARK_APP_BUBBLE_OPEN_AS_WINDOW));
@@ -175,13 +175,11 @@ bool WebAppConfirmationView::Accept() {
   web_app_info_->title = GetTrimmedTitle();
   if (ShowRadioButtons()) {
     if (open_as_tabbed_window_radio_->GetChecked()) {
-      web_app_info_->user_display_mode = web_app::DisplayMode::kStandalone;
-      web_app_info_->enable_experimental_tabbed_window = true;
+      web_app_info_->user_display_mode = web_app::DisplayMode::kTabbed;
     } else {
       web_app_info_->user_display_mode = open_as_window_radio_->GetChecked()
                                              ? web_app::DisplayMode::kStandalone
                                              : web_app::DisplayMode::kBrowser;
-      web_app_info_->enable_experimental_tabbed_window = false;
     }
   } else {
     web_app_info_->user_display_mode = open_as_window_checkbox_->GetChecked()

chrome/browser/ui/views/web_apps/web_app_tab_strip_browsertest.cc

@@ -67,8 +67,7 @@ class WebAppTabStripBrowserTest : public InProcessBrowserTest {
     web_app_info->scope = start_url.GetWithoutFilename();
     web_app_info->title = u"Test app";
     web_app_info->background_color = kAppBackgroundColor;
-    web_app_info->user_display_mode = DisplayMode::kStandalone;
-    web_app_info->enable_experimental_tabbed_window = true;
+    web_app_info->user_display_mode = DisplayMode::kTabbed;
     AppId app_id = test::InstallWebApp(profile, std::move(web_app_info));
 
     Browser* app_browser = LaunchWebAppBrowser(profile, app_id);

chrome/browser/ui/web_applications/web_app_link_capturing_browsertest.cc

@@ -203,8 +203,8 @@ class WebAppTabStripLinkCapturingBrowserTest
   void InstallTestApp() {
     WebAppLinkCapturingBrowserTest::InstallTestApp("/web_apps/basic.html",
                                                    /*await_metric=*/false);
-    provider().sync_bridge().SetExperimentalTabbedWindowMode(
-        app_id_, true, /*is_user_action=*/false);
+    provider().sync_bridge().SetAppUserDisplayMode(
+        app_id_, DisplayMode::kTabbed, /*is_user_action=*/false);
   }
 
  private:

chrome/browser/web_applications/app_service/web_app_publisher_helper.cc

@@ -264,9 +264,7 @@ apps::mojom::AppPtr WebAppPublisherHelper::ConvertWebApp(
   app->publisher_id = web_app->start_url().spec();
 
   auto display_mode = registrar().GetAppUserDisplayMode(web_app->app_id());
-  app->window_mode = ConvertDisplayModeToWindowMode(
-      display_mode,
-      registrar().IsInExperimentalTabbedWindowMode(web_app->app_id()));
+  app->window_mode = ConvertDisplayModeToWindowMode(display_mode);
 
   // app->version is left empty here.
   PopulateWebAppPermissions(web_app, &app->permissions);
@@ -645,40 +643,33 @@ void WebAppPublisherHelper::SetWindowMode(const std::string& app_id,
       display_mode = blink::mojom::DisplayMode::kStandalone;
       break;
     case apps::mojom::WindowMode::kTabbedWindow:
-      provider_->sync_bridge().SetExperimentalTabbedWindowMode(
-          app_id, /*enabled=*/true, /*is_user_action=*/true);
-      return;
+      display_mode = blink::mojom::DisplayMode::kTabbed;
+      break;
   }
-  provider_->sync_bridge().SetExperimentalTabbedWindowMode(
-      app_id, /*enabled=*/false, /*is_user_action=*/true);
   provider_->sync_bridge().SetAppUserDisplayMode(app_id, display_mode,
                                                  /*is_user_action=*/true);
 }
 
 apps::mojom::WindowMode WebAppPublisherHelper::ConvertDisplayModeToWindowMode(
-    blink::mojom::DisplayMode display_mode,
-    bool in_experimental_tabbed_window) {
-  if (in_experimental_tabbed_window) {
-    return apps::mojom::WindowMode::kTabbedWindow;
-  }
+    blink::mojom::DisplayMode display_mode) {
   switch (display_mode) {
     case blink::mojom::DisplayMode::kUndefined:
       return apps::mojom::WindowMode::kUnknown;
     case blink::mojom::DisplayMode::kBrowser:
       return apps::mojom::WindowMode::kBrowser;
+    case blink::mojom::DisplayMode::kTabbed:
+      return apps::mojom::WindowMode::kTabbedWindow;
     case blink::mojom::DisplayMode::kMinimalUi:
     case blink::mojom::DisplayMode::kStandalone:
     case blink::mojom::DisplayMode::kFullscreen:
     case blink::mojom::DisplayMode::kWindowControlsOverlay:
-    case blink::mojom::DisplayMode::kTabbed:
       return apps::mojom::WindowMode::kWindow;
   }
 }
 
 void WebAppPublisherHelper::PublishWindowModeUpdate(
     const std::string& app_id,
-    blink::mojom::DisplayMode display_mode,
-    bool in_experimental_tabbed_window) {
+    blink::mojom::DisplayMode display_mode) {
   const WebApp* web_app = GetWebApp(app_id);
   if (!web_app || !Accepts(app_id)) {
     return;
@@ -687,8 +678,7 @@ void WebAppPublisherHelper::PublishWindowModeUpdate(
   apps::mojom::AppPtr app = apps::mojom::App::New();
   app->app_type = app_type();
   app->app_id = app_id;
-  app->window_mode = ConvertDisplayModeToWindowMode(
-      display_mode, in_experimental_tabbed_window);
+  app->window_mode = ConvertDisplayModeToWindowMode(display_mode);
   delegate_->PublishWebApp(std::move(app));
 }
 
@@ -788,15 +778,7 @@ void WebAppPublisherHelper::OnWebAppLastLaunchTimeChanged(
 void WebAppPublisherHelper::OnWebAppUserDisplayModeChanged(
     const AppId& app_id,
     DisplayMode user_display_mode) {
-  PublishWindowModeUpdate(app_id, user_display_mode,
-                          registrar().IsInExperimentalTabbedWindowMode(app_id));
-}
-
-void WebAppPublisherHelper::OnWebAppExperimentalTabbedWindowModeChanged(
-    const AppId& app_id,
-    bool enabled) {
-  auto display_mode = registrar().GetAppUserDisplayMode(app_id);
-  PublishWindowModeUpdate(app_id, display_mode, enabled);
+  PublishWindowModeUpdate(app_id, user_display_mode);
 }
 
 #if BUILDFLAG(IS_CHROMEOS_ASH) || BUILDFLAG(IS_CHROMEOS_LACROS)

chrome/browser/web_applications/app_service/web_app_publisher_helper.h

@@ -175,12 +175,10 @@ class WebAppPublisherHelper : public AppRegistrarObserver,
 
   // Converts |display_mode| to a |window_mode|.
   apps::mojom::WindowMode ConvertDisplayModeToWindowMode(
-      blink::mojom::DisplayMode display_mode,
-      bool in_experimental_tabbed_window);
+      blink::mojom::DisplayMode display_mode);
 
   void PublishWindowModeUpdate(const std::string& app_id,
-                               blink::mojom::DisplayMode display_mode,
-                               bool in_experimental_tabbed_window);
+                               blink::mojom::DisplayMode display_mode);
 
   // Execute the user command from the context menu items. Currently
   // on the web app shortcut need to be execute in the publisher.
@@ -229,8 +227,6 @@ class WebAppPublisherHelper : public AppRegistrarObserver,
       const base::Time& last_launch_time) override;
   void OnWebAppUserDisplayModeChanged(const AppId& app_id,
                                       DisplayMode user_display_mode) override;
-  void OnWebAppExperimentalTabbedWindowModeChanged(const AppId& app_id,
-                                                   bool enabled) override;
 #if BUILDFLAG(IS_CHROMEOS_ASH) || BUILDFLAG(IS_CHROMEOS_LACROS)
   void OnWebAppDisabledStateChanged(const AppId& app_id,
                                     bool is_disabled) override;

chrome/browser/web_applications/components/app_registrar_observer.h

@@ -67,8 +67,6 @@ class AppRegistrarObserver : public base::CheckedObserver {
                                           const base::Time& time) {}
   virtual void OnWebAppUserDisplayModeChanged(const AppId& app_id,
                                               DisplayMode user_display_mode) {}
-  virtual void OnWebAppExperimentalTabbedWindowModeChanged(const AppId& app_id,
-                                                           bool enabled) {}
 };
 
 }  // namespace web_app