Add experimental tabstrip support to System Web Apps

Added AppBrowserController::HasTabstrip to allow
System Web Apps to turn on browser FEATURE_TABSTRIP.

The tabstrip is required for CrOS chrome://terminal.

Bug: 846546
Change-Id: I3f1a6ebafd7b32be51bde17d7ad31526b8cd1ef3
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1714121Reviewed-by: Dana Fried <dfried@chromium.org>
Reviewed-by: Elly Fong-Jones <ellyjones@chromium.org>
Reviewed-by: Alan Cutter <alancutter@chromium.org>
Commit-Queue: Joel Hockey <joelhockey@chromium.org>
Cr-Commit-Position: refs/heads/master@{#683417}

chrome/browser/ui/BUILD.gn

@@ -3708,8 +3708,8 @@ jumbo_split_static_library("ui") {
       "views/extensions/media_gallery_checkbox_view.h",
       "web_applications/app_browser_controller.cc",
       "web_applications/app_browser_controller.h",
-      "web_applications/system_web_app_ui_utils_chromeos.cc",
-      "web_applications/system_web_app_ui_utils_chromeos.h",
+      "web_applications/system_web_app_ui_utils.cc",
+      "web_applications/system_web_app_ui_utils.h",
       "web_applications/web_app_browser_controller.cc",
       "web_applications/web_app_browser_controller.h",
       "web_applications/web_app_dialog_manager.cc",

chrome/browser/ui/browser.cc

@@ -2630,6 +2630,10 @@ bool Browser::SupportsWindowFeatureImpl(WindowFeature feature,
   if (web_app::AppBrowserController::IsForWebAppBrowser(this))
     features |= FEATURE_TOOLBAR;
 
+  // Some types of web apps will have a tabstrip.
+  if (app_controller_ && app_controller_->HasTabStrip())
+    features |= FEATURE_TABSTRIP;
+
   return !!(features & feature);
 }
 

chrome/browser/ui/browser.h

@@ -123,6 +123,10 @@ class Browser : public TabStripModelObserver,
   enum Type {
     // If you add a new type, consider updating the test
     // BrowserTest.StartMaximized.
+    // TODO(crbug.com/990158): It is now possible that even TYPE_POPUP can have
+    // tabs.  Rename TYPE_TABBED to TYPE_DEFAULT, and replace calls to
+    // |is_type_tabbed()| with SupportsWindowFeature(Browser::FEATURE_TABSTRIP)
+    // where the client needs to know if tabs are supported in the browser.
     TYPE_TABBED = 1,
     TYPE_POPUP = 2
   };

chrome/browser/ui/browser_commands.cc

@@ -630,7 +630,7 @@ void NewTab(Browser* browser) {
       ReopenTabInProductHelpFactory::GetForProfile(browser->profile());
   reopen_tab_iph->NewTabOpened();
 
-  if (browser->is_type_tabbed()) {
+  if (browser->SupportsWindowFeature(Browser::FEATURE_TABSTRIP)) {
     TabStripModel* const model = browser->tab_strip_model();
     const auto group_id = model->GetTabGroupForTab(model->active_index());
     AddTabAt(browser, GURL(), -1, true, group_id);

chrome/browser/ui/settings_window_manager_chromeos.cc

@@ -16,7 +16,7 @@
 #include "chrome/browser/ui/settings_window_manager_observer_chromeos.h"
 #include "chrome/browser/ui/tabs/tab_strip_model.h"
 #include "chrome/browser/ui/web_applications/app_browser_controller.h"
-#include "chrome/browser/ui/web_applications/system_web_app_ui_utils_chromeos.h"
+#include "chrome/browser/ui/web_applications/system_web_app_ui_utils.h"
 #include "chrome/browser/web_applications/system_web_app_manager.h"
 #include "chrome/common/webui_url_constants.h"
 #include "chromeos/constants/chromeos_features.h"

chrome/browser/ui/views/toolbar/toolbar_view.cc

@@ -108,12 +108,14 @@ namespace {
 
 // Gets the display mode for a given browser.
 ToolbarView::DisplayMode GetDisplayMode(Browser* browser) {
-  if (browser->SupportsWindowFeature(Browser::FEATURE_TABSTRIP))
-    return ToolbarView::DisplayMode::NORMAL;
-
+  // Checked in this order because even tabbed PWAs use the CUSTOM_TAB
+  // display mode.
   if (web_app::AppBrowserController::IsForWebAppBrowser(browser))
     return ToolbarView::DisplayMode::CUSTOM_TAB;
 
+  if (browser->SupportsWindowFeature(Browser::FEATURE_TABSTRIP))
+    return ToolbarView::DisplayMode::NORMAL;
+
   return ToolbarView::DisplayMode::LOCATION;
 }
 

chrome/browser/ui/web_applications/app_browser_controller.cc

@@ -12,6 +12,7 @@
 #include "chrome/browser/ui/extensions/hosted_app_browser_controller.h"
 #include "chrome/browser/ui/manifest_web_app_browser_controller.h"
 #include "chrome/browser/ui/tabs/tab_strip_model.h"
+#include "chrome/browser/ui/web_applications/system_web_app_ui_utils.h"
 #include "chrome/browser/ui/web_applications/web_app_browser_controller.h"
 #include "chrome/browser/web_applications/components/app_registrar.h"
 #include "chrome/browser/web_applications/components/web_app_helpers.h"
@@ -88,6 +89,13 @@ bool AppBrowserController::CreatedForInstalledPwa() const {
   return false;
 }
 
+bool AppBrowserController::HasTabStrip() const {
+  // Show tabs for Terminal only.
+  // TODO(crbug.com/846546): Generalise this as a SystemWebApp capability.
+  return GetAppIdForSystemWebApp(browser()->profile(),
+                                 SystemAppType::TERMINAL) == GetAppId();
+}
+
 bool AppBrowserController::IsInstalled() const {
   return false;
 }
@@ -176,21 +184,21 @@ void AppBrowserController::OnTabStripModelChanged(
     for (const auto& contents : change.GetRemove()->contents)
       OnTabRemoved(contents.contents);
   }
+  if (selection.active_tab_changed()) {
+    content::WebContentsObserver::Observe(selection.new_contents);
+    DidChangeThemeColor(GetThemeColor());
+  }
+
+  // WebContents should be null when the last tab is closed.
+  DCHECK_EQ(web_contents() == nullptr, tab_strip_model->empty());
 }
 
 void AppBrowserController::OnTabInserted(content::WebContents* contents) {
-  DCHECK(!web_contents()) << " App windows are single tabbed only";
-  content::WebContentsObserver::Observe(contents);
-  DidChangeThemeColor(GetThemeColor());
-
-  if (!contents->GetVisibleURL().is_empty())
+  if (!contents->GetVisibleURL().is_empty() && initial_url_.is_empty())
     SetInitialURL(contents->GetVisibleURL());
 }
 
-void AppBrowserController::OnTabRemoved(content::WebContents* contents) {
-  DCHECK_EQ(contents, web_contents());
-  content::WebContentsObserver::Observe(nullptr);
-}
+void AppBrowserController::OnTabRemoved(content::WebContents* contents) {}
 
 void AppBrowserController::SetInitialURL(const GURL& initial_url) {
   DCHECK(initial_url_.is_empty());

chrome/browser/ui/web_applications/app_browser_controller.h

@@ -57,6 +57,9 @@ class AppBrowserController : public TabStripModelObserver,
   // Whether the custom tab bar should be visible.
   virtual bool ShouldShowCustomTabBar() const = 0;
 
+  // Whether the browser should include the tab strip.
+  virtual bool HasTabStrip() const;
+
   // Returns true if the hosted app buttons should be shown in the frame for
   // this BrowserView.
   virtual bool ShouldShowHostedAppButtonContainer() const = 0;

chrome/browser/ui/web_applications/app_browser_controller_browsertest.cc

+// Copyright 2019 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "chrome/browser/ui/web_applications/app_browser_controller.h"
+
+#include "base/test/bind_test_util.h"
+#include "base/test/scoped_feature_list.h"
+#include "chrome/browser/extensions/extension_browsertest.h"
+#include "chrome/browser/installable/installable_metrics.h"
+#include "chrome/browser/ui/browser_commands.h"
+#include "chrome/browser/ui/extensions/application_launch.h"
+#include "chrome/browser/ui/web_applications/system_web_app_ui_utils.h"
+#include "chrome/browser/web_applications/components/externally_installed_web_app_prefs.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/system_web_app_manager.h"
+#include "chrome/browser/web_applications/web_app_provider.h"
+#include "chrome/common/chrome_features.h"
+#include "chrome/common/web_application_info.h"
+#include "extensions/browser/extension_registry.h"
+
+namespace web_app {
+
+class AppBrowserControllerBrowserTest
+    : public extensions::ExtensionBrowserTest {
+ public:
+  AppBrowserControllerBrowserTest() {
+    scoped_feature_list_.InitWithFeatures({}, {features::kTerminalSystemApp});
+  }
+
+  ~AppBrowserControllerBrowserTest() override {}
+
+  void SetUp() override { extensions::ExtensionBrowserTest::SetUp(); }
+
+ protected:
+  void InstallAndLaunchTerminalApp() {
+    GURL app_url = GetAppURL();
+    auto web_app_info = std::make_unique<WebApplicationInfo>();
+    web_app_info->app_url = app_url;
+    web_app_info->scope = app_url.GetWithoutFilename();
+    web_app_info->open_as_window = true;
+    AppId app_id = InstallWebApp(std::move(web_app_info));
+
+    auto* provider = WebAppProvider::Get(profile());
+    provider->system_web_app_manager().SetSystemAppsForTesting(
+        {{SystemAppType::TERMINAL, {app_url, app_id}}});
+    web_app::ExternallyInstalledWebAppPrefs(profile()->GetPrefs())
+        .Insert(app_url, app_id,
+                web_app::ExternalInstallSource::kInternalDefault);
+    ASSERT_EQ(web_app::GetAppIdForSystemWebApp(browser()->profile(),
+                                               SystemAppType::TERMINAL),
+              app_id);
+
+    const extensions::Extension* extension =
+        extensions::ExtensionRegistry::Get(profile())->GetInstalledExtension(
+            app_id);
+    ASSERT_TRUE(extension);
+
+    app_browser_ = LaunchAppBrowser(extension);
+
+    ASSERT_TRUE(app_browser_);
+    ASSERT_NE(app_browser_, browser());
+  }
+
+  std::string InstallWebApp(
+      std::unique_ptr<WebApplicationInfo>&& web_app_info) {
+    std::string app_id;
+    base::RunLoop run_loop;
+    auto* provider = WebAppProvider::Get(profile());
+    DCHECK(provider);
+    provider->install_manager().InstallWebAppFromInfo(
+        std::move(web_app_info),
+        /*no_network_install=*/true, WebappInstallSource::OMNIBOX_INSTALL_ICON,
+        base::BindLambdaForTesting(
+            [&](const std::string& installed_app_id, InstallResultCode code) {
+              EXPECT_EQ(InstallResultCode::kSuccess, code);
+              app_id = installed_app_id;
+              run_loop.Quit();
+            }));
+
+    run_loop.Run();
+    return app_id;
+  }
+
+  GURL GetAppURL() {
+    return embedded_test_server()->GetURL("app.com", "/simple.html");
+  }
+
+  GURL GetActiveTabURL() {
+    return app_browser_->tab_strip_model()
+        ->GetActiveWebContents()
+        ->GetVisibleURL();
+  }
+
+  Browser* app_browser_ = nullptr;
+
+ private:
+  base::test::ScopedFeatureList scoped_feature_list_;
+
+  DISALLOW_COPY_AND_ASSIGN(AppBrowserControllerBrowserTest);
+};
+
+IN_PROC_BROWSER_TEST_F(AppBrowserControllerBrowserTest, SomeTest) {
+  ASSERT_TRUE(embedded_test_server()->Start());
+  InstallAndLaunchTerminalApp();
+
+  EXPECT_TRUE(app_browser_->SupportsWindowFeature(Browser::FEATURE_TABSTRIP));
+
+  // Check URL of tab1.
+  EXPECT_EQ(GetActiveTabURL(), GetAppURL());
+  // Create tab2, check URL, number of tabs.
+  chrome::NewTab(app_browser_);
+  EXPECT_EQ(app_browser_->tab_strip_model()->count(), 2);
+  EXPECT_EQ(GetActiveTabURL(), GURL("chrome://newtab/"));
+  // Switch to tab1, check URL.
+  chrome::SelectNextTab(app_browser_);
+  EXPECT_EQ(app_browser_->tab_strip_model()->count(), 2);
+  EXPECT_EQ(GetActiveTabURL(), GetAppURL());
+  // Switch to tab2, check URL.
+  chrome::SelectNextTab(app_browser_);
+  EXPECT_EQ(app_browser_->tab_strip_model()->count(), 2);
+  EXPECT_EQ(GetActiveTabURL(), GURL("chrome://newtab/"));
+  // Close tab2, check number of tabs.
+  chrome::CloseTab(app_browser_);
+  EXPECT_EQ(app_browser_->tab_strip_model()->count(), 1);
+  EXPECT_EQ(GetActiveTabURL(), GetAppURL());
+}
+
+}  // namespace web_app

chrome/browser/ui/web_applications/system_web_app_ui_utils_chromeos.cc → chrome/browser/ui/web_applications/system_web_app_ui_utils.cc

@@ -2,7 +2,7 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#include "chrome/browser/ui/web_applications/system_web_app_ui_utils_chromeos.h"
+#include "chrome/browser/ui/web_applications/system_web_app_ui_utils.h"
 
 #include <string>
 #include <utility>

chrome/browser/ui/web_applications/system_web_app_ui_utils_chromeos.h → chrome/browser/ui/web_applications/system_web_app_ui_utils.h

@@ -2,8 +2,8 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#ifndef CHROME_BROWSER_UI_WEB_APPLICATIONS_SYSTEM_WEB_APP_UI_UTILS_CHROMEOS_H_
-#define CHROME_BROWSER_UI_WEB_APPLICATIONS_SYSTEM_WEB_APP_UI_UTILS_CHROMEOS_H_
+#ifndef CHROME_BROWSER_UI_WEB_APPLICATIONS_SYSTEM_WEB_APP_UI_UTILS_H_
+#define CHROME_BROWSER_UI_WEB_APPLICATIONS_SYSTEM_WEB_APP_UI_UTILS_H_
 
 #include <utility>
 
@@ -35,4 +35,4 @@ Browser* FindSystemWebAppBrowser(Profile* profile, SystemAppType app_type);
 
 }  // namespace web_app
 
-#endif  // CHROME_BROWSER_UI_WEB_APPLICATIONS_SYSTEM_WEB_APP_UI_UTILS_CHROMEOS_H_
+#endif  // CHROME_BROWSER_UI_WEB_APPLICATIONS_SYSTEM_WEB_APP_UI_UTILS_H_

chrome/browser/web_applications/system_web_app_manager.h

@@ -93,8 +93,8 @@ class SystemWebAppManager {
 
   static void RegisterProfilePrefs(user_prefs::PrefRegistrySyncable* registry);
 
-  // Returns the app id for the given System App |id|.
-  base::Optional<AppId> GetAppIdForSystemApp(SystemAppType id) const;
+  // Returns the app id for the given System App |type|.
+  base::Optional<AppId> GetAppIdForSystemApp(SystemAppType type) const;
 
   // Returns whether |app_id| points to an installed System App.
   bool IsSystemWebApp(const AppId& app_id) const;
@@ -103,11 +103,12 @@ class SystemWebAppManager {
     return *on_apps_synchronized_;
   }
 
- protected:
   void SetSystemAppsForTesting(
       base::flat_map<SystemAppType, SystemAppInfo> system_apps);
+
   void SetUpdatePolicyForTesting(UpdatePolicy policy);
 
+ protected:
   virtual const base::Version& CurrentVersion() const;
 
  private:

chrome/common/chrome_features.cc

@@ -221,11 +221,11 @@ const base::Feature kUploadZippedSystemLogs{"UploadZippedSystemLogs",
                                             base::FEATURE_DISABLED_BY_DEFAULT};
 #endif
 
-#if defined(OS_CHROMEOS) || defined(OS_LINUX)
-// Enable chrome://terminal in Chrome OS or Linux.
+// Enable chrome://terminal.  Terminal System App will only run on
+// OS_CHROMEOS, but this flag must be defined for all platforms since
+// it is required for SystemWebApp tests.
 const base::Feature kTerminalSystemApp{"TerminalSystemApp",
                                        base::FEATURE_DISABLED_BY_DEFAULT};
-#endif
 
 // Enable using tab sharing infobars for desktop capture.
 const base::Feature kDesktopCaptureTabSharingInfobar{

chrome/common/chrome_features.h

@@ -129,9 +129,7 @@ COMPONENT_EXPORT(CHROME_FEATURES)
 extern const base::Feature kUploadZippedSystemLogs;
 #endif
 
-#if defined(OS_CHROMEOS) || defined(OS_LINUX)
 COMPONENT_EXPORT(CHROME_FEATURES) extern const base::Feature kTerminalSystemApp;
-#endif
 
 COMPONENT_EXPORT(CHROME_FEATURES)
 extern const base::Feature kDesktopCaptureTabSharingInfobar;

chrome/test/BUILD.gn

@@ -1233,6 +1233,7 @@ if (!is_android) {
       "../browser/ui/views/webauthn/authenticator_dialog_view_browsertest.cc",
       "../browser/ui/views/webauthn/authenticator_qr_code_test.cc",
       "../browser/ui/views/webview_accessibility_browsertest.cc",
+      "../browser/ui/web_applications/app_browser_controller_browsertest.cc",
       "../browser/ui/web_applications/bookmark_app_browsertest.cc",
       "../browser/ui/web_applications/web_app_badging_browsertest.cc",
       "../browser/ui/web_applications/web_app_file_handling_browsertest.cc",