Fix Linux hosted app / PWA window frame color with GTK theme.

The title text color and side/bottom frame color now match the GTK
theme, rather than the app's theme color (which clashed with the title
bar background color, that would use the native color scheme regardless
of the app theme color).

Adds a browser test for GetFrameColor.

Bug: 878636
Change-Id: If5eaca1bc800f6c52f3f49918f455ffa2d881016
Reviewed-on: https://chromium-review.googlesource.com/1195226
Commit-Queue: Matt Giuca <mgiuca@chromium.org>
Reviewed-by: Bret Sepulveda <bsep@chromium.org>
Cr-Commit-Position: refs/heads/master@{#587490}

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

@@ -162,11 +162,6 @@ gfx::ImageSkia BrowserNonClientFrameView::GetIncognitoAvatarIcon() const {
 
 SkColor BrowserNonClientFrameView::GetFrameColor(
     ActiveState active_state) const {
-  extensions::HostedAppBrowserController* hosted_app_controller =
-      browser_view_->browser()->hosted_app_controller();
-  if (hosted_app_controller && hosted_app_controller->GetThemeColor())
-    return *hosted_app_controller->GetThemeColor();
-
   ThemeProperties::OverwritableByUserThemeProperty color_id;
   if (ShouldPaintAsSingleTabMode()) {
     color_id = ThemeProperties::COLOR_TOOLBAR;
@@ -175,10 +170,26 @@ SkColor BrowserNonClientFrameView::GetFrameColor(
                    ? ThemeProperties::COLOR_FRAME
                    : ThemeProperties::COLOR_FRAME_INACTIVE;
   }
-  return ShouldPaintAsThemed()
-             ? GetThemeProviderForProfile()->GetColor(color_id)
-             : ThemeProperties::GetDefaultColor(color_id,
-                                                browser_view_->IsIncognito());
+
+  // For hosted app windows, if "painting as themed" (which is only true when on
+  // Linux and using the system theme), prefer the system theme color over the
+  // hosted app theme color. The title bar will be painted in the system theme
+  // color (regardless of what we do here), so by returning the system title bar
+  // background color here, we ensure that:
+  // a) The side and bottom borders are painted in the same color as the title
+  // bar background, and
+  // b) The title text is painted in a color that contrasts with the title bar
+  // background.
+  if (ShouldPaintAsThemed())
+    return GetThemeProviderForProfile()->GetColor(color_id);
+
+  extensions::HostedAppBrowserController* hosted_app_controller =
+      browser_view_->browser()->hosted_app_controller();
+  if (hosted_app_controller && hosted_app_controller->GetThemeColor())
+    return *hosted_app_controller->GetThemeColor();
+
+  return ThemeProperties::GetDefaultColor(color_id,
+                                          browser_view_->IsIncognito());
 }
 
 SkColor BrowserNonClientFrameView::GetToolbarTopSeparatorColor() const {

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

@@ -5,13 +5,65 @@
 #include "chrome/browser/ui/views/frame/browser_non_client_frame_view.h"
 
 #include "build/build_config.h"
+#include "chrome/browser/extensions/browsertest_util.h"
 #include "chrome/browser/extensions/extension_browsertest.h"
 #include "chrome/browser/themes/theme_properties.h"
+#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/common/chrome_features.h"
+#include "chrome/common/web_application_info.h"
+#include "chrome/test/base/in_process_browser_test.h"
+#include "content/public/test/test_navigation_observer.h"
 #include "ui/base/material_design/material_design_controller.h"
 #include "ui/base/theme_provider.h"
 
-using BrowserNonClientFrameViewBrowserTest = extensions::ExtensionBrowserTest;
+class BrowserNonClientFrameViewBrowserTest
+    : public extensions::ExtensionBrowserTest {
+ public:
+  BrowserNonClientFrameViewBrowserTest() = default;
+  ~BrowserNonClientFrameViewBrowserTest() override = default;
+
+  void SetUpOnMainThread() override {
+    ExtensionBrowserTest::SetUpOnMainThread();
+    scoped_feature_list_.InitAndEnableFeature(features::kDesktopPWAWindowing);
+  }
+
+  // Note: A "bookmark app" is a type of hosted app. All of these tests apply
+  // equally to hosted and bookmark apps, but it's easier to install a bookmark
+  // app in a test.
+  void InstallAndLaunchBookmarkApp() {
+    WebApplicationInfo web_app_info;
+    web_app_info.app_url = GetAppURL();
+    web_app_info.scope = GetAppURL().GetWithoutFilename();
+    if (app_theme_color_)
+      web_app_info.theme_color = *app_theme_color_;
+
+    const extensions::Extension* app =
+        extensions::browsertest_util::InstallBookmarkApp(browser()->profile(),
+                                                         web_app_info);
+    content::TestNavigationObserver navigation_observer(GetAppURL());
+    navigation_observer.StartWatchingNewWebContents();
+    Browser* app_browser = extensions::browsertest_util::LaunchAppBrowser(
+        browser()->profile(), app);
+    navigation_observer.WaitForNavigationFinished();
+
+    BrowserView* browser_view =
+        BrowserView::GetBrowserViewForBrowser(app_browser);
+    app_frame_view_ = browser_view->frame()->GetFrameView();
+  }
+
+ protected:
+  base::Optional<SkColor> app_theme_color_ = SK_ColorBLUE;
+  BrowserNonClientFrameView* app_frame_view_ = nullptr;
+
+ private:
+  GURL GetAppURL() { return GURL("https://test.org"); }
+
+  base::test::ScopedFeatureList scoped_feature_list_;
+
+  DISALLOW_COPY_AND_ASSIGN(BrowserNonClientFrameViewBrowserTest);
+};
 
 // Test is Flaky on Windows see crbug.com/600201.
 #if defined(OS_WIN)
@@ -25,7 +77,7 @@ using BrowserNonClientFrameViewBrowserTest = extensions::ExtensionBrowserTest;
 
 // Tests that the color returned by
 // BrowserNonClientFrameView::GetToolbarTopSeparatorColor() tracks the window
-// actiavtion state.
+// activation state.
 IN_PROC_BROWSER_TEST_F(BrowserNonClientFrameViewBrowserTest,
                        MAYBE_InactiveSeparatorColor) {
   // Refresh does not draw the toolbar top separator.
@@ -39,25 +91,98 @@ IN_PROC_BROWSER_TEST_F(BrowserNonClientFrameViewBrowserTest,
   const BrowserNonClientFrameView* frame_view =
       browser_view->frame()->GetFrameView();
   const ui::ThemeProvider* theme_provider = frame_view->GetThemeProvider();
-  const SkColor theme_active_color =
+  const SkColor expected_active_color =
       theme_provider->GetColor(ThemeProperties::COLOR_TOOLBAR_TOP_SEPARATOR);
-  const SkColor theme_inactive_color =
-      theme_provider->GetColor(
-          ThemeProperties::COLOR_TOOLBAR_TOP_SEPARATOR_INACTIVE);
-  EXPECT_NE(theme_active_color, theme_inactive_color);
+  const SkColor expected_inactive_color = theme_provider->GetColor(
+      ThemeProperties::COLOR_TOOLBAR_TOP_SEPARATOR_INACTIVE);
+  EXPECT_NE(expected_active_color, expected_inactive_color);
 
-  // Check that the separator color is the active color when the window is
-  // active.
   browser_view->Activate();
   EXPECT_TRUE(browser_view->IsActive());
-  const SkColor frame_active_color = frame_view->GetToolbarTopSeparatorColor();
-  EXPECT_EQ(theme_active_color, frame_active_color);
+  EXPECT_EQ(expected_active_color, frame_view->GetToolbarTopSeparatorColor());
 
-  // Check that the separator color is the inactive color when the window is
-  // inactive.
   browser_view->Deactivate();
   EXPECT_FALSE(browser_view->IsActive());
-  const SkColor frame_inactive_color =
-      frame_view->GetToolbarTopSeparatorColor();
-  EXPECT_EQ(theme_inactive_color, frame_inactive_color);
+  EXPECT_EQ(expected_inactive_color, frame_view->GetToolbarTopSeparatorColor());
+}
+
+// Tests the frame color for a normal browser window.
+IN_PROC_BROWSER_TEST_F(BrowserNonClientFrameViewBrowserTest,
+                       BrowserFrameColorThemed) {
+  InstallExtension(test_data_dir_.AppendASCII("theme"), 1);
+
+  BrowserView* browser_view = BrowserView::GetBrowserViewForBrowser(browser());
+  const BrowserNonClientFrameView* frame_view =
+      browser_view->frame()->GetFrameView();
+  const ui::ThemeProvider* theme_provider = frame_view->GetThemeProvider();
+  const SkColor expected_active_color =
+      theme_provider->GetColor(ThemeProperties::COLOR_FRAME);
+  const SkColor expected_inactive_color =
+      theme_provider->GetColor(ThemeProperties::COLOR_FRAME_INACTIVE);
+
+  EXPECT_EQ(expected_active_color,
+            frame_view->GetFrameColor(BrowserNonClientFrameView::kActive));
+  EXPECT_EQ(expected_inactive_color,
+            frame_view->GetFrameColor(BrowserNonClientFrameView::kInactive));
+}
+
+// Tests the frame color for a bookmark app when a theme is applied.
+//
+// Disabled because it hits a DCHECK in BrowserView.
+// TODO(mgiuca): Remove this DCHECK, since it seems legitimate.
+// https://crbug.com/879030.
+IN_PROC_BROWSER_TEST_F(BrowserNonClientFrameViewBrowserTest,
+                       DISABLED_BookmarkAppFrameColorCustomTheme) {
+  // The theme color should not affect the window, but the theme must not be the
+  // default GTK theme for Linux so we install one anyway.
+  InstallExtension(test_data_dir_.AppendASCII("theme"), 1);
+  InstallAndLaunchBookmarkApp();
+  // Note: This is checking for the bookmark app's theme color, not the user's
+  // theme color.
+  EXPECT_EQ(*app_theme_color_,
+            app_frame_view_->GetFrameColor(BrowserNonClientFrameView::kActive));
+}
+
+// Tests the frame color for a bookmark app when a theme is applied, with the
+// app itself having no theme color.
+//
+// Disabled because it hits a DCHECK in BrowserView.
+// TODO(mgiuca): Remove this DCHECK, since it seems legitimate.
+// https://crbug.com/879030.
+IN_PROC_BROWSER_TEST_F(BrowserNonClientFrameViewBrowserTest,
+                       DISABLED_BookmarkAppFrameColorCustomThemeNoThemeColor) {
+  InstallExtension(test_data_dir_.AppendASCII("theme"), 1);
+  app_theme_color_.reset();
+  InstallAndLaunchBookmarkApp();
+  // Bookmark apps are not affected by browser themes.
+  EXPECT_EQ(
+      ThemeProperties::GetDefaultColor(ThemeProperties::COLOR_FRAME, false),
+      app_frame_view_->GetFrameColor(BrowserNonClientFrameView::kActive));
+}
+
+// Tests the frame color for a bookmark app when the system theme is applied.
+IN_PROC_BROWSER_TEST_F(BrowserNonClientFrameViewBrowserTest,
+                       BookmarkAppFrameColorSystemTheme) {
+  ThemeService* theme_service =
+      ThemeServiceFactory::GetForProfile(browser()->profile());
+  // Should be using the system theme by default, but this assert was not true
+  // on the bots. Explicitly set.
+  theme_service->UseSystemTheme();
+  ASSERT_TRUE(theme_service->UsingSystemTheme());
+
+  InstallAndLaunchBookmarkApp();
+#if defined(OS_LINUX) && !defined(OS_CHROMEOS)
+  // On Linux, the system theme is the GTK theme and should change the frame
+  // color to the system color (not the app theme color); otherwise the title
+  // and border would clash horribly with the GTK title bar.
+  // (https://crbug.com/878636)
+  const ui::ThemeProvider* theme_provider = app_frame_view_->GetThemeProvider();
+  const SkColor frame_color =
+      theme_provider->GetColor(ThemeProperties::COLOR_FRAME);
+  EXPECT_EQ(frame_color,
+            app_frame_view_->GetFrameColor(BrowserNonClientFrameView::kActive));
+#else
+  EXPECT_EQ(*app_theme_color_,
+            app_frame_view_->GetFrameColor(BrowserNonClientFrameView::kActive));
+#endif
 }

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

@@ -31,8 +31,6 @@ OpaqueBrowserFrameViewLinux::~OpaqueBrowserFrameViewLinux() {
 }
 
 bool OpaqueBrowserFrameViewLinux::IsUsingSystemTheme() {
-  // On X11, this does the correct thing. On Windows, UsingSystemTheme() will
-  // return true when using the default blue theme too.
   return theme_service_->UsingSystemTheme();
 }