Skip to content

Commit

Permalink
Revert "Fix Linux hosted app / PWA window frame color with GTK theme."
Browse files Browse the repository at this point in the history 
This reverts commit cac6956.

Reason for revert: Suspected of causing consistent failure for mac-cocoa-rel tests

BrowserNonClientFrameViewBrowserTest.BrowserFrameColorThemed
BrowserNonClientFrameViewBrowserTest.BookmarkAppFrameColorSystemTheme

starting from:

https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/mac-cocoa-rel/1090

Original change's description:
> 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}

TBR=mgiuca@chromium.org,bsep@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug: 878636, 880363
Change-Id: I999ae00f1bb5f1963e77c8394c47b8516a036343
Reviewed-on: https://chromium-review.googlesource.com/1204450
Commit-Queue: Samuel Huang <huangs@chromium.org>
Reviewed-by: Samuel Huang <huangs@chromium.org>
Cr-Commit-Position: refs/heads/master@{#588581}
  • Loading branch information
@samuelhuang
samuelhuang authored and Commit Bot committed Sep 4, 2018
1 parent c0a7a8e commit 9e6f06a
Show file tree
Hide file tree
Showing 3 changed files with 27 additions and 161 deletions.
29 changes: 9 additions & 20 deletions chrome/browser/ui/views/frame/browser_non_client_frame_view.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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 {
Expand Down
157 changes: 16 additions & 141 deletions chrome/browser/ui/views/frame/browser_non_client_frame_view_browsertest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<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);
};
using BrowserNonClientFrameViewBrowserTest = extensions::ExtensionBrowserTest;

// Test is Flaky on Windows see crbug.com/600201.
#if defined(OS_WIN)
Expand All @@ -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.
Expand All @@ -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);
}
2 changes: 2 additions & 0 deletions chrome/browser/ui/views/frame/opaque_browser_frame_view_linux.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}

Expand Down

0 comments on commit 9e6f06a

Please sign in to comment.