[System PWAs] Hide the hosted app button container for System PWAs.

System PWAs should not show the Hosted App Button Container UI. This CL
hides it on all platforms for System Web Apps.

Bug: 836128
Change-Id: I2dc87dea17aa77699f6d6639759645a1c184a2bd
Reviewed-on: https://chromium-review.googlesource.com/c/1293294
Commit-Queue: calamity <calamity@chromium.org>
Reviewed-by: Trent Apted <tapted@chromium.org>
Reviewed-by: Alan Cutter <alancutter@chromium.org>
Cr-Commit-Position: refs/heads/master@{#606758}

chrome/browser/ui/extensions/hosted_app_browser_controller.cc

@@ -122,7 +122,7 @@ const char kPwaWindowEngagementTypeHistogram[] =
 bool HostedAppBrowserController::IsForExperimentalHostedAppBrowser(
     const Browser* browser) {
   return browser && browser->hosted_app_controller() &&
-         base::FeatureList::IsEnabled(::features::kDesktopPWAWindowing);
+         browser->hosted_app_controller()->IsForExperimentalHostedAppBrowser();
 }
 
 // static
@@ -172,6 +172,17 @@ HostedAppBrowserController::~HostedAppBrowserController() {
   browser_->tab_strip_model()->RemoveObserver(this);
 }
 
+bool HostedAppBrowserController::IsForSystemWebApp() const {
+  const Extension* extension = GetExtension();
+
+  return extension && extension->from_bookmark() &&
+         extension->location() == Manifest::EXTERNAL_COMPONENT;
+}
+
+bool HostedAppBrowserController::IsForExperimentalHostedAppBrowser() const {
+  return base::FeatureList::IsEnabled(::features::kDesktopPWAWindowing);
+}
+
 bool HostedAppBrowserController::ShouldShowLocationBar() const {
   // The extension can be null if this is invoked after uninstall.
   const Extension* extension = GetExtension();
@@ -236,6 +247,11 @@ void HostedAppBrowserController::UpdateLocationBarVisibility(
       ShouldShowLocationBar(), animate);
 }
 
+bool HostedAppBrowserController::ShouldShowHostedAppButtonContainer() const {
+  // System Web Apps don't get the Hosted App buttons.
+  return IsForExperimentalHostedAppBrowser() && !IsForSystemWebApp();
+}
+
 gfx::ImageSkia HostedAppBrowserController::GetWindowAppIcon() const {
   // TODO(calamity): Use the app name to retrieve the app icon without using the
   // extensions tab helper to make icon load more immediate.

chrome/browser/ui/extensions/hosted_app_browser_controller.h

@@ -33,6 +33,8 @@ class HostedAppBrowserController : public SiteEngagementObserver,
                                    public ExtensionUninstallDialog::Delegate {
  public:
   // Returns whether |browser| uses the experimental hosted app experience.
+  // Convenience wrapper for checking IsForExperimentalHostedAppBrowser() on
+  // |browser|'s HostedAppBrowserController if it exists.
   static bool IsForExperimentalHostedAppBrowser(const Browser* browser);
 
   // Functions to set preferences that are unique to app windows.
@@ -48,10 +50,20 @@ class HostedAppBrowserController : public SiteEngagementObserver,
   // Returns true if the associated Hosted App is for a PWA.
   bool created_for_installed_pwa() const { return created_for_installed_pwa_; }
 
+  // Returns true if this controller is for a System Web App.
+  bool IsForSystemWebApp() const;
+
+  // Returns true if this controller is for an experimental hosted app browser.
+  bool IsForExperimentalHostedAppBrowser() const;
+
   // Whether the browser being controlled should be currently showing the
   // location bar.
   bool ShouldShowLocationBar() const;
 
+  // Returns true if the hosted app buttons should be shown in the frame for
+  // this BrowserView.
+  bool ShouldShowHostedAppButtonContainer() const;
+
   // Updates the location bar visibility based on whether it should be
   // currently visible or not. If |animate| is set, the change will be
   // animated.

chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc

@@ -804,6 +804,9 @@ void BrowserNonClientFrameViewAsh::SetUpForHostedApp(
         ash::FrameCaptionButton::ColorMode::kThemed, *theme_color);
   }
 
+  if (!browser->hosted_app_controller()->ShouldShowHostedAppButtonContainer())
+    return;
+
   // Add the container for extra hosted app buttons (e.g app menu button).
   const float inactive_alpha_ratio =
       ash::FrameCaptionButton::GetInactiveButtonColorAlphaRatio();

chrome/browser/ui/views/frame/browser_non_client_frame_view_browsertest.cc

@@ -11,6 +11,7 @@
 #include "chrome/browser/themes/theme_service.h"
 #include "chrome/browser/themes/theme_service_factory.h"
 #include "chrome/browser/ui/views/frame/browser_view.h"
+#include "chrome/browser/web_applications/bookmark_apps/system_web_app_manager_browsertest.h"
 #include "chrome/common/chrome_features.h"
 #include "chrome/common/web_application_info.h"
 #include "chrome/test/base/in_process_browser_test.h"
@@ -144,3 +145,16 @@ IN_PROC_BROWSER_TEST_F(BrowserNonClientFrameViewBrowserTest,
             app_frame_view_->GetFrameColor(BrowserNonClientFrameView::kActive));
 #endif
 }
+
+using SystemWebAppNonClientFrameViewBrowserTest =
+    web_app::SystemWebAppManagerBrowserTest;
+
+// System Web Apps don't get the hosted app buttons.
+IN_PROC_BROWSER_TEST_F(SystemWebAppNonClientFrameViewBrowserTest,
+                       HideHostedAppButtonContainer) {
+  Browser* app_browser = InstallAndLaunchSystemApp();
+  EXPECT_EQ(nullptr, BrowserView::GetBrowserViewForBrowser(app_browser)
+                         ->frame()
+                         ->GetFrameView()
+                         ->hosted_app_button_container_for_testing());
+}

chrome/browser/ui/views/frame/browser_non_client_frame_view_mac.mm

@@ -11,6 +11,7 @@
 #include "chrome/browser/ui/cocoa/fullscreen/fullscreen_menubar_tracker.h"
 #include "chrome/browser/ui/cocoa/fullscreen/fullscreen_toolbar_controller_views.h"
 #include "chrome/browser/ui/exclusive_access/fullscreen_controller.h"
+#include "chrome/browser/ui/extensions/hosted_app_browser_controller.h"
 #include "chrome/browser/ui/layout_constants.h"
 #include "chrome/browser/ui/views/bookmarks/bookmark_bar_view.h"
 #include "chrome/browser/ui/views/frame/browser_frame.h"
@@ -58,10 +59,14 @@ BrowserNonClientFrameViewMac::BrowserNonClientFrameViewMac(
                           base::Unretained(this), true));
 
   if (browser_view->IsBrowserTypeHostedApp()) {
-    set_hosted_app_button_container(new HostedAppButtonContainer(
-        frame, browser_view, GetReadableFrameForegroundColor(kActive),
-        GetReadableFrameForegroundColor(kInactive), kHostedAppMenuMargin));
-    AddChildView(hosted_app_button_container());
+    if (browser_view->browser()
+            ->hosted_app_controller()
+            ->ShouldShowHostedAppButtonContainer()) {
+      set_hosted_app_button_container(new HostedAppButtonContainer(
+          frame, browser_view, GetReadableFrameForegroundColor(kActive),
+          GetReadableFrameForegroundColor(kInactive), kHostedAppMenuMargin));
+      AddChildView(hosted_app_button_container());
+    }
 
     DCHECK(browser_view->ShouldShowWindowTitle());
     window_title_ = new views::Label(browser_view->GetWindowTitle());
@@ -114,9 +119,11 @@ gfx::Rect BrowserNonClientFrameViewMac::GetBoundsForTabStrip(
 }
 
 int BrowserNonClientFrameViewMac::GetTopInset(bool restored) const {
-  if (browser_view()->IsBrowserTypeHostedApp())
+  if (hosted_app_button_container()) {
+    DCHECK(browser_view()->IsBrowserTypeHostedApp());
     return hosted_app_button_container()->GetPreferredSize().height() +
            kHostedAppMenuMargin * 2;
+  }
 
   if (!browser_view()->IsTabStripVisible())
     return 0;

chrome/browser/ui/views/frame/glass_browser_frame_view.cc

@@ -12,6 +12,7 @@
 #include "chrome/app/chrome_command_ids.h"
 #include "chrome/app/chrome_dll_resource.h"
 #include "chrome/browser/themes/theme_properties.h"
+#include "chrome/browser/ui/extensions/hosted_app_browser_controller.h"
 #include "chrome/browser/ui/view_ids.h"
 #include "chrome/browser/ui/views/frame/browser_view.h"
 #include "chrome/browser/ui/views/frame/hosted_app_button_container.h"
@@ -112,12 +113,15 @@ GlassBrowserFrameView::GlassBrowserFrameView(BrowserFrame* frame,
     AddChildView(window_title_);
   }
 
-  if (browser_view->IsBrowserTypeHostedApp()) {
+  extensions::HostedAppBrowserController* controller =
+      browser_view->browser()->hosted_app_controller();
+  if (controller && controller->ShouldShowHostedAppButtonContainer()) {
     // TODO(alancutter): Avoid snapshotting GetTitlebarFeatureColor() values
     // here and call it on demand in
     // HostedAppButtonContainer::UpdateIconsColor() via a delegate interface.
     SkColor active_color = GetTitlebarFeatureColor(kActive);
     SkColor inactive_color = GetTitlebarFeatureColor(kInactive);
+
     set_hosted_app_button_container(new HostedAppButtonContainer(
         frame, browser_view, active_color, inactive_color));
     AddChildView(hosted_app_button_container());

chrome/browser/ui/views/frame/hosted_app_button_container.cc

@@ -11,6 +11,7 @@
 #include "chrome/browser/ui/content_settings/content_setting_image_model.h"
 #include "chrome/browser/ui/extensions/hosted_app_browser_controller.h"
 #include "chrome/browser/ui/layout_constants.h"
+#include "chrome/browser/ui/view_ids.h"
 #include "chrome/browser/ui/views/frame/browser_view.h"
 #include "chrome/browser/ui/views/frame/hosted_app_menu_button.h"
 #include "chrome/browser/ui/views/frame/hosted_app_origin_text.h"
@@ -196,7 +197,12 @@ HostedAppButtonContainer::HostedAppButtonContainer(
                                       false /* interactive */)),
       app_menu_button_(new HostedAppMenuButton(browser_view)) {
   DCHECK(browser_view_);
-  DCHECK(browser_view_->IsBrowserTypeHostedApp());
+  DCHECK(browser_view_->browser()
+             ->hosted_app_controller()
+             ->IsForExperimentalHostedAppBrowser());
+
+  set_id(VIEW_ID_HOSTED_APP_BUTTON_CONTAINER);
+
   views::BoxLayout& layout =
       *SetLayoutManager(std::make_unique<views::BoxLayout>(
           views::BoxLayout::kHorizontal,

chrome/browser/ui/views/frame/opaque_browser_frame_view.cc

@@ -174,11 +174,12 @@ OpaqueBrowserFrameView::OpaqueBrowserFrameView(
   window_title_->set_id(VIEW_ID_WINDOW_TITLE);
   AddChildView(window_title_);
 
-  if (browser_view->IsBrowserTypeHostedApp()) {
+  extensions::HostedAppBrowserController* controller =
+      browser_view->browser()->hosted_app_controller();
+  if (controller && controller->ShouldShowHostedAppButtonContainer()) {
     set_hosted_app_button_container(new HostedAppButtonContainer(
         frame, browser_view, GetReadableFrameForegroundColor(kActive),
         GetReadableFrameForegroundColor(kInactive)));
-    hosted_app_button_container()->set_id(VIEW_ID_HOSTED_APP_BUTTON_CONTAINER);
     AddChildView(hosted_app_button_container());
   }
 }

chrome/browser/web_applications/bookmark_apps/BUILD.gn

@@ -86,6 +86,7 @@ source_set("browser_tests") {
 
   sources = [
     "system_web_app_manager_browsertest.cc",
+    "system_web_app_manager_browsertest.h",
   ]
 
   deps = [
@@ -99,6 +100,7 @@ source_set("browser_tests") {
     "//chrome/browser/web_applications/extensions",
     "//chrome/common:constants",
     "//chrome/test:test_support",
+    "//chrome/test:test_support_ui",
     "//extensions/browser",
     "//url",
   ]

chrome/browser/web_applications/bookmark_apps/system_web_app_manager_browsertest.cc

@@ -2,15 +2,16 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#include "chrome/browser/web_applications/bookmark_apps/system_web_app_manager.h"
+#include "chrome/browser/web_applications/bookmark_apps/system_web_app_manager_browsertest.h"
 
 #include <vector>
 
 #include "base/memory/ref_counted_memory.h"
 #include "base/test/scoped_feature_list.h"
-#include "build/build_config.h"
+#include "chrome/browser/extensions/browsertest_util.h"
 #include "chrome/browser/extensions/extension_browsertest.h"
 #include "chrome/browser/extensions/extension_util.h"
+#include "chrome/browser/ui/extensions/hosted_app_browser_controller.h"
 #include "chrome/browser/web_applications/bookmark_apps/test_system_web_app_manager.h"
 #include "chrome/browser/web_applications/components/web_app_constants.h"
 #include "chrome/browser/web_applications/extensions/pending_bookmark_app_manager.h"
@@ -43,7 +44,7 @@ constexpr char kSystemAppManifestText[] =
           "type": "image/png"
         }
       ],
-      "start_url": "/",
+      "start_url": "/pwa.html",
       "theme_color": "#00FF00"
     })";
 
@@ -106,44 +107,44 @@ class TestWebUIControllerFactory : public content::WebUIControllerFactory {
   DISALLOW_COPY_AND_ASSIGN(TestWebUIControllerFactory);
 };
 
-class SystemWebAppManagerIntegrationTest
-    : public extensions::ExtensionBrowserTest {
- public:
-  SystemWebAppManagerIntegrationTest() {
-    scoped_feature_list_.InitWithFeatures(
-        {features::kDesktopPWAWindowing, features::kSystemWebApps}, {});
-    content::WebUIControllerFactory::RegisterFactory(&factory_);
-  }
-  ~SystemWebAppManagerIntegrationTest() override {
-    content::WebUIControllerFactory::UnregisterFactoryForTesting(&factory_);
-  }
-
-  void SetUpOnMainThread() override {
-    extensions::ExtensionBrowserTest::SetUpOnMainThread();
-
-    // Reset WebAppProvider so that its SystemWebAppManager doesn't interfere
-    // with tests.
-    WebAppProvider::Get(profile())->Reset();
-  }
+SystemWebAppManagerBrowserTest::SystemWebAppManagerBrowserTest()
+    : factory_(std::make_unique<TestWebUIControllerFactory>()) {
+  scoped_feature_list_.InitWithFeatures(
+      {features::kDesktopPWAWindowing, features::kSystemWebApps}, {});
+  content::WebUIControllerFactory::RegisterFactory(factory_.get());
+}
+SystemWebAppManagerBrowserTest::~SystemWebAppManagerBrowserTest() {
+  content::WebUIControllerFactory::UnregisterFactoryForTesting(factory_.get());
+}
 
- private:
-  base::test::ScopedFeatureList scoped_feature_list_;
-  TestWebUIControllerFactory factory_;
+void SystemWebAppManagerBrowserTest::SetUpOnMainThread() {
+  InProcessBrowserTest::SetUpOnMainThread();
 
-  DISALLOW_COPY_AND_ASSIGN(SystemWebAppManagerIntegrationTest);
-};
+  // Reset WebAppProvider so that its SystemWebAppManager doesn't interfere
+  // with tests.
+  WebAppProvider::Get(browser()->profile())->Reset();
+}
 
-// Test that System Apps install correctly with a manifest.
-IN_PROC_BROWSER_TEST_F(SystemWebAppManagerIntegrationTest, WithManifest) {
+Browser* SystemWebAppManagerBrowserTest::InstallAndLaunchSystemApp() {
+  Profile* profile = browser()->profile();
   std::vector<GURL> system_apps;
   system_apps.emplace_back(GURL("chrome://test-system-app/pwa.html"));
-  extensions::PendingBookmarkAppManager pending_app_manager(profile());
-  TestSystemWebAppManager system_web_app_manager(
-      profile(), &pending_app_manager, std::move(system_apps));
+  extensions::PendingBookmarkAppManager pending_app_manager(profile);
+  TestSystemWebAppManager system_web_app_manager(profile, &pending_app_manager,
+                                                 std::move(system_apps));
   const extensions::Extension* app =
       extensions::TestExtensionRegistryObserver(
-          extensions::ExtensionRegistry::Get(profile()))
+          extensions::ExtensionRegistry::Get(profile))
           .WaitForExtensionInstalled();
+
+  return extensions::browsertest_util::LaunchAppBrowser(profile, app);
+}
+
+// Test that System Apps install correctly with a manifest.
+IN_PROC_BROWSER_TEST_F(SystemWebAppManagerBrowserTest, Install) {
+  const extensions::Extension* app = InstallAndLaunchSystemApp()
+                                         ->hosted_app_controller()
+                                         ->GetExtensionForTesting();
   EXPECT_EQ("Test System App", app->name());
   EXPECT_EQ(SkColorSetRGB(0, 0xFF, 0),
             extensions::AppThemeColorInfo::GetThemeColor(app));
@@ -152,7 +153,7 @@ IN_PROC_BROWSER_TEST_F(SystemWebAppManagerIntegrationTest, WithManifest) {
 
   // The app should be a PWA.
   EXPECT_EQ(extensions::util::GetInstalledPwaForUrl(
-                profile(), GURL("chrome://test-system-app/")),
+                browser()->profile(), GURL("chrome://test-system-app/")),
             app);
 }
 

chrome/browser/web_applications/bookmark_apps/system_web_app_manager_browsertest.h

+// Copyright 2018 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.
+
+#ifndef CHROME_BROWSER_WEB_APPLICATIONS_BOOKMARK_APPS_SYSTEM_WEB_APP_MANAGER_BROWSERTEST_H_
+#define CHROME_BROWSER_WEB_APPLICATIONS_BOOKMARK_APPS_SYSTEM_WEB_APP_MANAGER_BROWSERTEST_H_
+
+#include <memory>
+
+#include "base/test/scoped_feature_list.h"
+#include "chrome/test/base/in_process_browser_test.h"
+
+namespace web_app {
+
+class TestWebUIControllerFactory;
+
+class SystemWebAppManagerBrowserTest : public InProcessBrowserTest {
+ public:
+  SystemWebAppManagerBrowserTest();
+  ~SystemWebAppManagerBrowserTest() override;
+
+  // Overridden from InProcessBrowserTest:
+  void SetUpOnMainThread() override;
+
+ protected:
+  Browser* InstallAndLaunchSystemApp();
+
+ private:
+  base::test::ScopedFeatureList scoped_feature_list_;
+  std::unique_ptr<TestWebUIControllerFactory> factory_;
+
+  DISALLOW_COPY_AND_ASSIGN(SystemWebAppManagerBrowserTest);
+};
+
+}  // namespace web_app
+
+#endif  // CHROME_BROWSER_WEB_APPLICATIONS_BOOKMARK_APPS_SYSTEM_WEB_APP_MANAGER_BROWSERTEST_H_

chrome/test/BUILD.gn

@@ -298,6 +298,8 @@ static_library("test_support") {
 
   if (enable_extensions) {
     sources += [
+      "../browser/extensions/browsertest_util.cc",
+      "../browser/extensions/browsertest_util.h",
       "../browser/extensions/extension_browsertest.cc",
       "../browser/extensions/extension_browsertest.h",
     ]
@@ -1320,8 +1322,6 @@ test("browser_tests") {
         "../browser/extensions/bookmark_app_navigation_browsertest.cc",
         "../browser/extensions/bookmark_app_navigation_browsertest.h",
         "../browser/extensions/bookmark_app_navigation_throttle_browsertest.cc",
-        "../browser/extensions/browsertest_util.cc",
-        "../browser/extensions/browsertest_util.h",
         "../browser/extensions/chrome_app_api_browsertest.cc",
         "../browser/extensions/chrome_theme_url_browsertest.cc",
         "../browser/extensions/chrome_ui_overrides_browsertest.cc",
@@ -4624,7 +4624,6 @@ if (!is_android) {
       "../browser/extensions/api/notifications/notifications_apitest.cc",
       "../browser/extensions/api/omnibox/omnibox_api_interactive_test.cc",
       "../browser/extensions/api/tabs/tabs_interactive_test.cc",
-      "../browser/extensions/browsertest_util.cc",
       "../browser/extensions/chrome_extension_test_notification_observer.cc",
       "../browser/extensions/chrome_extension_test_notification_observer.h",
       "../browser/extensions/clipboard_extension_apitest_chromeos.cc",
@@ -5108,7 +5107,6 @@ if (!is_android && !is_chromecast) {
       "../app/chrome_version.rc.version",
       "../browser/extensions/api/cast_streaming/performance_test.cc",
       "../browser/extensions/api/tab_capture/tab_capture_performancetest.cc",
-      "../browser/extensions/browsertest_util.cc",
       "../browser/extensions/chrome_extension_test_notification_observer.cc",
       "../browser/extensions/chrome_extension_test_notification_observer.h",
       "../browser/extensions/extension_apitest.cc",