Skip to content

Commit

Permalink
[color] Simplify getting the ColorProviderManager::Key
Browse files Browse the repository at this point in the history 
Currently the construction of a ColorProviderKey by sources is a
set of virtual methods overridden by the ColorProviderSource
subclasses, or optional params set directly on the source (widget).

This CL simplifies the current state and makes it such that
ColorProviderSources are only required to override the
GetColorProviderKey() method to configure the theme properties for
their hosted UI.

The specific changes required here are for Widget and BrowserFrame.
Widget will always attempt to fetch the ColorProviderKey of its
parent if it exists, otherwise falling back to the key constructed
from the global NativeTheme instance.

Color mode and elevation overrides can be set on the Widget itself
so after it has fetched the key as described above these specific
overrides are applied (if necessary).

The BrowserFrame Widget subclass works similarly, first getting key
as described above and applying the BrowserFrame-specific overrides.

Follow up work will eliminate eliminate virtual from
ColorProviderSource::GetColorProvider and have the method always take
the key and get the ColorProvider from the manager.

Bug: 1431202, 1455535
Change-Id: I703a954423d760195374447ae47d92a9e700e5c3
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4614129
Code-Coverage: Findit <findit-for-me@appspot.gserviceaccount.com>
Reviewed-by: Dana Fried <dfried@chromium.org>
Commit-Queue: Thomas Lukaszewicz <tluk@chromium.org>
Reviewed-by: Sean Kau <skau@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1159135}
  • Loading branch information
Thomas Lukaszewicz authored and Chromium LUCI CQ committed Jun 16, 2023
1 parent 83a5b61 commit db70d87
Show file tree
Hide file tree
Showing 5 changed files with 144 additions and 98 deletions.
83 changes: 45 additions & 38 deletions chrome/browser/ui/views/frame/browser_frame.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -411,52 +411,59 @@ void BrowserFrame::OnNativeThemeUpdated(ui::NativeTheme* observed_theme) {

ui::ColorProviderManager::Key BrowserFrame::GetColorProviderKey() const {
auto key = Widget::GetColorProviderKey();
key.frame_type = UseCustomFrame()
? ui::ColorProviderManager::FrameType::kChromium
: ui::ColorProviderManager::FrameType::kNative;
auto* app_controller = browser_view_->browser()->app_controller();
key.app_controller = app_controller;
return key;
}

absl::optional<SkColor> BrowserFrame::GetUserColor() const {
// color_mode.
[this, &key]() {
// Currently the incognito browser is implemented as unthemed dark mode.
if (IsIncognitoBrowser()) {
key.color_mode = ui::ColorProviderManager::ColorMode::kDark;
return;
}

const auto* theme_service =
ThemeServiceFactory::GetForProfile(browser_view_->browser()->profile());
const auto browser_color_scheme = theme_service->GetBrowserColorScheme();

if (browser_color_scheme != ThemeService::BrowserColorScheme::kSystem) {
key.color_mode =
browser_color_scheme == ThemeService::BrowserColorScheme::kLight
? ui::ColorProviderManager::ColorMode::kLight
: ui::ColorProviderManager::ColorMode::kDark;
}
}();

// user_color.
[this, &key]() {
#if BUILDFLAG(IS_CHROMEOS_ASH)
// ChromeOS SystemWebApps use the OS theme all the time.
if (ash::IsSystemWebApp(browser_view_->browser())) {
return views::Widget::GetUserColor();
}
// ChromeOS SystemWebApps use the OS theme all the time.
if (ash::IsSystemWebApp(browser_view_->browser())) {
return;
}
#endif // BUILDFLAG(IS_CHROMEOS_ASH)

// Incognito profiles should always fall back to the material baseline.
if (IsIncognitoBrowser()) {
return absl::nullopt;
}

const auto* theme_service =
ThemeServiceFactory::GetForProfile(browser_view_->browser()->profile());
return theme_service->UsingAutogeneratedTheme()
? absl::optional<SkColor>(
theme_service->GetAutogeneratedThemeColor())
: views::Widget::GetUserColor();
}
// Incognito profiles should always fall back to the material baseline.
if (IsIncognitoBrowser()) {
key.user_color = absl::nullopt;
return;
}

ui::ColorProviderManager::ColorMode BrowserFrame::GetColorMode() const {
// Currently the incognito browser is implemented as unthemed dark mode.
if (IsIncognitoBrowser()) {
return ui::ColorProviderManager::ColorMode::kDark;
}
const auto* theme_service =
ThemeServiceFactory::GetForProfile(browser_view_->browser()->profile());
if (theme_service && theme_service->UsingAutogeneratedTheme()) {
key.user_color = theme_service->GetAutogeneratedThemeColor();
}
}();

const auto* theme_service =
ThemeServiceFactory::GetForProfile(browser_view_->browser()->profile());
const auto browser_color_scheme = theme_service->GetBrowserColorScheme();
// frame_type.
key.frame_type = UseCustomFrame()
? ui::ColorProviderManager::FrameType::kChromium
: ui::ColorProviderManager::FrameType::kNative;

if (browser_color_scheme == ThemeService::BrowserColorScheme::kSystem) {
return Widget::GetColorMode();
}
// app_controller.
auto* app_controller = browser_view_->browser()->app_controller();
key.app_controller = app_controller;

return browser_color_scheme == ThemeService::BrowserColorScheme::kLight
? ui::ColorProviderManager::ColorMode::kLight
: ui::ColorProviderManager::ColorMode::kDark;
return key;
}

void BrowserFrame::OnMenuClosed() {
Expand Down
2 changes: 0 additions & 2 deletions chrome/browser/ui/views/frame/browser_frame.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -182,8 +182,6 @@ class BrowserFrame : public views::Widget, public views::ContextMenuController {
// views::Widget:
void OnNativeThemeUpdated(ui::NativeTheme* observed_theme) override;
ui::ColorProviderManager::Key GetColorProviderKey() const override;
absl::optional<SkColor> GetUserColor() const override;
ui::ColorProviderManager::ColorMode GetColorMode() const override;

private:
void OnTouchUiChanged();
Expand Down
108 changes: 85 additions & 23 deletions chrome/browser/ui/views/frame/browser_frame_browsertest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
#include "chrome/browser/devtools/devtools_window_testing.h"
#include "chrome/browser/profiles/profile.h"
#include "chrome/browser/themes/theme_service.h"
#include "chrome/browser/themes/theme_service_factory.h"
#include "chrome/browser/ui/browser.h"
#include "chrome/browser/ui/browser_window.h"
#include "chrome/browser/ui/views/chrome_views_delegate.h"
Expand All @@ -29,6 +30,7 @@
#include "ui/color/color_provider.h"
#include "ui/color/color_provider_manager.h"
#include "ui/color/color_recipe.h"
#include "ui/native_theme/test_native_theme.h"
#include "ui/views/views_delegate.h"

class BrowserFrameBoundsChecker : public ChromeViewsDelegate {
Expand Down Expand Up @@ -71,14 +73,16 @@ IN_PROC_BROWSER_TEST_F(BrowserFrameTest, WebAppsHasBoundsOnOpen) {
app_browser->window()->Close();
}

// Runs browser color scheme tests with ChromeRefresh2023 enabled and disabled.
class BrowserFrameColorModeTest : public BrowserFrameTest,
public testing::WithParamInterface<bool> {
// Runs browser color provider tests with ChromeRefresh2023 enabled and
// disabled.
class BrowserFrameColorProviderTest : public BrowserFrameTest,
public testing::WithParamInterface<bool> {
public:
static constexpr SkColor kLightColor = SK_ColorWHITE;
static constexpr SkColor kDarkColor = SK_ColorBLACK;
static constexpr SkColor kTransparentColor = SK_ColorTRANSPARENT;

BrowserFrameColorModeTest() {
BrowserFrameColorProviderTest() {
feature_list_.InitWithFeatureState(features::kChromeRefresh2023,
GetParam());
}
Expand All @@ -87,6 +91,9 @@ class BrowserFrameColorModeTest : public BrowserFrameTest,
void SetUpOnMainThread() override {
BrowserFrameTest::SetUpOnMainThread();

test_native_theme_.SetDarkMode(false);
GetBrowserFrame(browser())->SetNativeThemeForTest(&test_native_theme_);

// Force a light / dark color to be returned for `kColorSysPrimary`
// depending on the ColorMode.
ui::ColorProviderManager::ResetForTesting();
Expand All @@ -105,10 +112,16 @@ class BrowserFrameColorModeTest : public BrowserFrameTest,
// Add a postprocessing mixer to ensure it is appended to the end of the
// pipeline.
ui::ColorMixer& mixer = provider->AddPostprocessingMixer();

// Used to track the light/dark color mode setting.
mixer[ui::kColorSysPrimary] = {
key.color_mode == ui::ColorProviderManager::ColorMode::kDark
? kDarkColor
: kLightColor};

// Used to track the user color.
mixer[ui::kColorSysSecondary] = {
key.user_color.value_or(kTransparentColor)};
}

// Sets the `kBrowserColorScheme` pref for the `profile`.
Expand All @@ -118,33 +131,37 @@ class BrowserFrameColorModeTest : public BrowserFrameTest,
static_cast<int>(color_scheme));
}

BrowserFrame* GetBrowserFrame(Browser* browser) {
return static_cast<BrowserFrame*>(
BrowserView::GetBrowserViewForBrowser(browser)->GetWidget());
}

Profile* profile() { return browser()->profile(); }

ui::TestNativeTheme test_native_theme_;

private:
base::test::ScopedFeatureList feature_list_;
};

// Verifies the BrowserFrame honors the BrowserColorScheme pref.
IN_PROC_BROWSER_TEST_P(BrowserFrameColorModeTest, TracksBrowserColorScheme) {
// Assert the browser follows the system color mode. Simulate the system color
// mode by setting the widget level color mode override.
views::Widget* browser_frame =
BrowserView::GetBrowserViewForBrowser(browser())->GetWidget();
browser_frame->SetColorModeOverride(
ui::ColorProviderManager::ColorMode::kLight);
IN_PROC_BROWSER_TEST_P(BrowserFrameColorProviderTest,
TracksBrowserColorScheme) {
// Assert the browser follows the system color scheme (i.e. the color scheme
// set on the associated native theme)
views::Widget* browser_frame = GetBrowserFrame(browser());
test_native_theme_.SetDarkMode(false);
EXPECT_EQ(kLightColor,
browser_frame->GetColorProvider()->GetColor(ui::kColorSysPrimary));

browser_frame->SetColorModeOverride(
ui::ColorProviderManager::ColorMode::kDark);
test_native_theme_.SetDarkMode(true);
EXPECT_EQ(kDarkColor,
browser_frame->GetColorProvider()->GetColor(ui::kColorSysPrimary));

// Set the BrowserColorScheme pref. The BrowserFrame should ignore the system
// color mode if running ChromeRefresh2023. Otherwise BrowserFrame should
// track the system color mode.
browser_frame->SetColorModeOverride(
ui::ColorProviderManager::ColorMode::kLight);
// color scheme if running ChromeRefresh2023. Otherwise BrowserFrame should
// track the system color scheme.
test_native_theme_.SetDarkMode(false);
SetBrowserColorScheme(profile(), ThemeService::BrowserColorScheme::kDark);
if (features::IsChromeRefresh2023()) {
EXPECT_EQ(kDarkColor, browser_frame->GetColorProvider()->GetColor(
Expand All @@ -154,8 +171,7 @@ IN_PROC_BROWSER_TEST_P(BrowserFrameColorModeTest, TracksBrowserColorScheme) {
ui::kColorSysPrimary));
}

browser_frame->SetColorModeOverride(
ui::ColorProviderManager::ColorMode::kDark);
test_native_theme_.SetDarkMode(true);
SetBrowserColorScheme(profile(), ThemeService::BrowserColorScheme::kLight);
if (features::IsChromeRefresh2023()) {
EXPECT_EQ(kLightColor, browser_frame->GetColorProvider()->GetColor(
Expand All @@ -167,11 +183,11 @@ IN_PROC_BROWSER_TEST_P(BrowserFrameColorModeTest, TracksBrowserColorScheme) {
}

// Verifies incognito browsers will always use the dark ColorMode.
IN_PROC_BROWSER_TEST_P(BrowserFrameColorModeTest, IncognitoAlwaysDarkMode) {
IN_PROC_BROWSER_TEST_P(BrowserFrameColorProviderTest, IncognitoAlwaysDarkMode) {
// Create an incognito browser.
Browser* incognito_browser = CreateIncognitoBrowser(profile());
views::Widget* incognito_browser_frame =
BrowserView::GetBrowserViewForBrowser(incognito_browser)->GetWidget();
views::Widget* incognito_browser_frame = GetBrowserFrame(incognito_browser);
incognito_browser_frame->SetNativeThemeForTest(&test_native_theme_);

// The incognito browser should reflect the dark color mode irrespective of
// the current BrowserColorScheme.
Expand All @@ -186,4 +202,50 @@ IN_PROC_BROWSER_TEST_P(BrowserFrameColorModeTest, IncognitoAlwaysDarkMode) {
ui::kColorSysPrimary));
}

INSTANTIATE_TEST_SUITE_P(All, BrowserFrameColorModeTest, testing::Bool());
// Verifies the BrowserFrame's user_color tracks the autogenerated theme color.
IN_PROC_BROWSER_TEST_P(BrowserFrameColorProviderTest,
UserColorTracksAutogeneratedThemeColor) {
// The Browser should initially have its user_color unset, tracking the user
// color of its NativeTheme.
views::Widget* browser_frame = GetBrowserFrame(browser());
EXPECT_EQ(kTransparentColor, browser_frame->GetColorProvider()->GetColor(
ui::kColorSysSecondary));

// Install an autogenerated them and verify that the browser's user_color has
// been updated to reflect.
ThemeService* theme_service = ThemeServiceFactory::GetForProfile(profile());
constexpr SkColor kAutogeneratedColor1 = SkColorSetRGB(100, 100, 100);
theme_service->BuildAutogeneratedThemeFromColor(kAutogeneratedColor1);
EXPECT_EQ(kAutogeneratedColor1, theme_service->GetAutogeneratedThemeColor());
EXPECT_EQ(kAutogeneratedColor1, browser_frame->GetColorProvider()->GetColor(
ui::kColorSysSecondary));

// Install a new autogenerated theme and verify that the user_color has been
// updated to reflect.
constexpr SkColor kAutogeneratedColor2 = SkColorSetRGB(200, 200, 200);
theme_service->BuildAutogeneratedThemeFromColor(kAutogeneratedColor2);
EXPECT_EQ(kAutogeneratedColor2, theme_service->GetAutogeneratedThemeColor());
EXPECT_EQ(kAutogeneratedColor2, browser_frame->GetColorProvider()->GetColor(
ui::kColorSysSecondary));
}

// Verifies incognito browsers will ignore the user_color set on their
// NativeTheme.
IN_PROC_BROWSER_TEST_P(BrowserFrameColorProviderTest,
IncognitoAlwaysIgnoresUserColor) {
// Create an incognito browser.
Browser* incognito_browser = CreateIncognitoBrowser(profile());
views::Widget* incognito_browser_frame = GetBrowserFrame(incognito_browser);
incognito_browser_frame->SetNativeThemeForTest(&test_native_theme_);

// Set the user color override.
test_native_theme_.set_user_color(SK_ColorBLUE);
incognito_browser_frame->ThemeChanged();

// The ingognito browser should unset the user color.
EXPECT_EQ(kTransparentColor,
incognito_browser_frame->GetColorProvider()->GetColor(
ui::kColorSysSecondary));
}

INSTANTIATE_TEST_SUITE_P(All, BrowserFrameColorProviderTest, testing::Bool());
40 changes: 14 additions & 26 deletions ui/views/widget/widget.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1982,44 +1982,32 @@ void Widget::SetColorModeOverride(
// Widget, ui::ColorProviderSource:

ui::ColorProviderManager::Key Widget::GetColorProviderKey() const {
// Generally all Widgets should inherit the key of their parent, falling back
// to the key set by the NativeTheme otherwise.
// TODO(crbug.com/1455535): `parent_` does not always resolve to the logical
// parent as expected here (e.g. bubbles). This should be addressed and the
// use of parent_ below replaced with something like GetLogicalParent().
ui::ColorProviderManager::Key key =
GetNativeTheme()->GetColorProviderKey(GetCustomTheme());
parent_ ? parent_->GetColorProviderKey()
: GetNativeTheme()->GetColorProviderKey(GetCustomTheme());

// Widgets may have specific overrides set on the Widget itself that should
// apply specifically to themselves and their children, apply these here.
#if BUILDFLAG(IS_CHROMEOS_ASH)
key.elevation_mode = background_elevation_;
#endif
key.user_color = GetUserColor();
key.color_mode = GetColorMode();
return key;
}
if (color_mode_override_.has_value()) {
key.color_mode = color_mode_override_.value();
}

absl::optional<SkColor> Widget::GetUserColor() const {
// Fall back to the user color defined in the NativeTheme if a user color is
// not provided by any widgets in this UI hierarchy.
return parent_ ? parent_->GetUserColor() : GetNativeTheme()->user_color();
return key;
}

const ui::ColorProvider* Widget::GetColorProvider() const {
return ui::ColorProviderManager::Get().GetColorProviderFor(
GetColorProviderKey());
}

ui::ColorProviderManager::ColorMode Widget::GetColorMode() const {
if (color_mode_override_.has_value()) {
return color_mode_override_.value();
}

// All children should share the color mode of their parent unless explicitly
// overridden.
if (parent_) {
return parent_->GetColorMode();
}

// In the default case fall back to the system's default color mode.
return GetNativeTheme()->ShouldUseDarkColors()
? ui::ColorProviderManager::ColorMode::kDark
: ui::ColorProviderManager::ColorMode::kLight;
}

////////////////////////////////////////////////////////////////////////////////
// Widget, protected:

Expand Down
9 changes: 0 additions & 9 deletions ui/views/widget/widget.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1189,15 +1189,6 @@ class VIEWS_EXPORT Widget : public internal::NativeWidgetDelegate,
// ui::ColorProviderSource:
ui::ColorProviderManager::Key GetColorProviderKey() const override;

// Gets the user color for this widget. This is used as an input to generate
// color palettes for the material design system. Tracks the user color of the
// parent widget by default.
virtual absl::optional<SkColor> GetUserColor() const;

// Gets the color mode for this widget. Tracks the color mode of the parent
// widget by default.
virtual ui::ColorProviderManager::ColorMode GetColorMode() const;

private:
// Type of ways to ignore activation changes.
enum class DisableActivationChangeHandlingType {
Expand Down

0 comments on commit db70d87

Please sign in to comment.