Commit 830bf615 authored by Rune Lillesveen's avatar Rune Lillesveen Committed by Commit Bot

Revert static member LayoutThemeDefault cleanup.

Making the selection colors and caret interval members non-static
members of LayoutThemeDefault in [1] caused regressions for the devtools
inspector because GetTheme() returns different objects depending on
whether we emulate a mobile device or not, and these values are only
updated on the current object returned by LayoutTheme::GetTheme().

Also added a test to avoid this regression in the future.

[1] https://crrev.com/67575409937d6cc6d2226f7f5bcea667075e5d7d

Bug: 1062214
Change-Id: I06144fb4ff74e204432591498e7cd2e934018819
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2106070
Commit-Queue: Rune Lillesveen <futhark@chromium.org>
Reviewed-by: default avatarMorten Stenshorne <mstensho@chromium.org>
Cr-Commit-Position: refs/heads/master@{#751273}
parent db8fc612
...@@ -46,10 +46,16 @@ static const float kDefaultCancelButtonSize = 9; ...@@ -46,10 +46,16 @@ static const float kDefaultCancelButtonSize = 9;
static const float kMinCancelButtonSize = 5; static const float kMinCancelButtonSize = 5;
static const float kMaxCancelButtonSize = 21; static const float kMaxCancelButtonSize = 21;
LayoutThemeDefault::LayoutThemeDefault() base::TimeDelta LayoutThemeDefault::caret_blink_interval_;
: LayoutTheme(),
caret_blink_interval_(LayoutTheme::CaretBlinkInterval()), Color LayoutThemeDefault::active_selection_background_color_ = 0xff1e90ff;
painter_(*this) {} Color LayoutThemeDefault::active_selection_foreground_color_ = Color::kBlack;
Color LayoutThemeDefault::inactive_selection_background_color_ = 0xffc8c8c8;
Color LayoutThemeDefault::inactive_selection_foreground_color_ = 0xff323232;
LayoutThemeDefault::LayoutThemeDefault() : LayoutTheme(), painter_(*this) {
caret_blink_interval_ = LayoutTheme::CaretBlinkInterval();
}
LayoutThemeDefault::~LayoutThemeDefault() = default; LayoutThemeDefault::~LayoutThemeDefault() = default;
......
...@@ -150,12 +150,13 @@ class CORE_EXPORT LayoutThemeDefault : public LayoutTheme { ...@@ -150,12 +150,13 @@ class CORE_EXPORT LayoutThemeDefault : public LayoutTheme {
int MenuListInternalPadding(const ComputedStyle&, int padding) const; int MenuListInternalPadding(const ComputedStyle&, int padding) const;
static const RGBA32 kDefaultTapHighlightColor = 0x2e000000; // 18% black. static const RGBA32 kDefaultTapHighlightColor = 0x2e000000; // 18% black.
base::TimeDelta caret_blink_interval_;
Color active_selection_background_color_ = 0xff1e90ff; static base::TimeDelta caret_blink_interval_;
Color active_selection_foreground_color_ = Color::kBlack;
Color inactive_selection_background_color_ = 0xffc8c8c8; static Color active_selection_background_color_;
Color inactive_selection_foreground_color_ = 0xff323232; static Color active_selection_foreground_color_;
static Color inactive_selection_background_color_;
static Color inactive_selection_foreground_color_;
ThemePainterDefault painter_; ThemePainterDefault painter_;
// Cached values for crbug.com/673754. // Cached values for crbug.com/673754.
......
...@@ -80,8 +80,8 @@ TEST_F(LayoutThemeTest, ChangeFocusRingColor) { ...@@ -80,8 +80,8 @@ TEST_F(LayoutThemeTest, ChangeFocusRingColor) {
EXPECT_EQ(custom_color, OutlineColor(span)); EXPECT_EQ(custom_color, OutlineColor(span));
} }
// The expectations are based on LayoutThemeDefault::SystemColor. // The expectations in the tests below are relying on LayoutThemeDefault.
// LayoutThemeMac doesn't use that code path. // LayoutThemeMac doesn't inherit from that class.
#if !defined(OS_MACOSX) #if !defined(OS_MACOSX)
TEST_F(LayoutThemeTest, SystemColorWithColorScheme) { TEST_F(LayoutThemeTest, SystemColorWithColorScheme) {
SetHtmlInnerHTML(R"HTML( SetHtmlInnerHTML(R"HTML(
...@@ -112,6 +112,32 @@ TEST_F(LayoutThemeTest, SystemColorWithColorScheme) { ...@@ -112,6 +112,32 @@ TEST_F(LayoutThemeTest, SystemColorWithColorScheme) {
EXPECT_EQ(Color(0x44, 0x44, 0x44), EXPECT_EQ(Color(0x44, 0x44, 0x44),
style->VisitedDependentColor(GetCSSPropertyColor())); style->VisitedDependentColor(GetCSSPropertyColor()));
} }
TEST_F(LayoutThemeTest, SetSelectionColors) {
LayoutTheme::GetTheme().SetSelectionColors(Color::kBlack, Color::kBlack,
Color::kBlack, Color::kBlack);
EXPECT_EQ(Color::kBlack,
LayoutTheme::GetTheme().ActiveSelectionForegroundColor(
WebColorScheme::kLight));
{
// Enabling MobileLayoutTheme switches which instance is returned from
// LayoutTheme::GetTheme(). Devtools expect SetSelectionColors() to affect
// both LayoutTheme instances.
ScopedMobileLayoutThemeForTest scope(true);
EXPECT_EQ(Color::kBlack,
LayoutTheme::GetTheme().ActiveSelectionForegroundColor(
WebColorScheme::kLight));
LayoutTheme::GetTheme().SetSelectionColors(Color::kWhite, Color::kWhite,
Color::kWhite, Color::kWhite);
EXPECT_EQ(Color::kWhite,
LayoutTheme::GetTheme().ActiveSelectionForegroundColor(
WebColorScheme::kLight));
}
EXPECT_EQ(Color::kWhite,
LayoutTheme::GetTheme().ActiveSelectionForegroundColor(
WebColorScheme::kLight));
}
#endif // !defined(OS_MACOSX) #endif // !defined(OS_MACOSX)
} // namespace blink } // namespace blink
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment