diff --git a/chrome/browser/ui/views/frame/browser_non_client_frame_view.cc b/chrome/browser/ui/views/frame/browser_non_client_frame_view.cc index 609e079e408d2e..7ed89c0ac484d3 100644 --- a/chrome/browser/ui/views/frame/browser_non_client_frame_view.cc +++ b/chrome/browser/ui/views/frame/browser_non_client_frame_view.cc @@ -162,6 +162,11 @@ 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; @@ -170,26 +175,10 @@ SkColor BrowserNonClientFrameView::GetFrameColor( ? ThemeProperties::COLOR_FRAME : ThemeProperties::COLOR_FRAME_INACTIVE; } - - // 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()); + return ShouldPaintAsThemed() + ? GetThemeProviderForProfile()->GetColor(color_id) + : ThemeProperties::GetDefaultColor(color_id, + browser_view_->IsIncognito()); } SkColor BrowserNonClientFrameView::GetToolbarTopSeparatorColor() const { diff --git a/chrome/browser/ui/views/frame/browser_non_client_frame_view_browsertest.cc b/chrome/browser/ui/views/frame/browser_non_client_frame_view_browsertest.cc index c7b0df97b5bb7a..40baf83271a42c 100644 --- a/chrome/browser/ui/views/frame/browser_non_client_frame_view_browsertest.cc +++ b/chrome/browser/ui/views/frame/browser_non_client_frame_view_browsertest.cc @@ -5,65 +5,13 @@ #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" -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 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); -}; +using BrowserNonClientFrameViewBrowserTest = extensions::ExtensionBrowserTest; // Test is Flaky on Windows see crbug.com/600201. #if defined(OS_WIN) @@ -77,7 +25,7 @@ class BrowserNonClientFrameViewBrowserTest // Tests that the color returned by // BrowserNonClientFrameView::GetToolbarTopSeparatorColor() tracks the window -// activation state. +// actiavtion state. IN_PROC_BROWSER_TEST_F(BrowserNonClientFrameViewBrowserTest, MAYBE_InactiveSeparatorColor) { // Refresh does not draw the toolbar top separator. @@ -91,98 +39,25 @@ IN_PROC_BROWSER_TEST_F(BrowserNonClientFrameViewBrowserTest, const BrowserNonClientFrameView* frame_view = browser_view->frame()->GetFrameView(); const ui::ThemeProvider* theme_provider = frame_view->GetThemeProvider(); - const SkColor expected_active_color = + const SkColor theme_active_color = theme_provider->GetColor(ThemeProperties::COLOR_TOOLBAR_TOP_SEPARATOR); - const SkColor expected_inactive_color = theme_provider->GetColor( - ThemeProperties::COLOR_TOOLBAR_TOP_SEPARATOR_INACTIVE); - EXPECT_NE(expected_active_color, expected_inactive_color); + const SkColor theme_inactive_color = + theme_provider->GetColor( + ThemeProperties::COLOR_TOOLBAR_TOP_SEPARATOR_INACTIVE); + EXPECT_NE(theme_active_color, theme_inactive_color); + // Check that the separator color is the active color when the window is + // active. browser_view->Activate(); EXPECT_TRUE(browser_view->IsActive()); - EXPECT_EQ(expected_active_color, frame_view->GetToolbarTopSeparatorColor()); + const SkColor frame_active_color = frame_view->GetToolbarTopSeparatorColor(); + EXPECT_EQ(theme_active_color, frame_active_color); + // Check that the separator color is the inactive color when the window is + // inactive. browser_view->Deactivate(); EXPECT_FALSE(browser_view->IsActive()); - 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 + const SkColor frame_inactive_color = + frame_view->GetToolbarTopSeparatorColor(); + EXPECT_EQ(theme_inactive_color, frame_inactive_color); } diff --git a/chrome/browser/ui/views/frame/opaque_browser_frame_view_linux.cc b/chrome/browser/ui/views/frame/opaque_browser_frame_view_linux.cc index 05b10c0e1dffc4..3d7b8d4e12e65b 100644 --- a/chrome/browser/ui/views/frame/opaque_browser_frame_view_linux.cc +++ b/chrome/browser/ui/views/frame/opaque_browser_frame_view_linux.cc @@ -31,6 +31,8 @@ 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(); }