Repository navigation
Conversation
 The PR is not yet safe to merge because some HTTP(S) navigations can retain stale site chrome indefinitely, and page-controlled colors can grow the color-provider cache without a bound. Reviews (1) · Last reviewed commit: "helium/ui: add adaptive tab color from p..." |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| +void AdaptiveTabColor::PrimaryPageChanged(content::Page& page) { | ||
| + if (!CanAdapt()) { | ||
| + Update(); | ||
| + } |
There was a problem hiding this comment.
When one HTTP(S) page navigates to another, PrimaryPageChanged() skips Update() because CanAdapt() remains true. The later theme-color, background-color, and first-paint callbacks are conditional, so an empty or non-painting destination can retain the previous site’s frame and toolbar color indefinitely. Update the adaptive state on every primary-page change instead of relying only on optional follow-up callbacks.
Knowledge Base Used: Helium browser interface
There was a problem hiding this comment.
Updating on every primary page change wld flash the default color between any two page b/c the new page's theme-color and background aren't known until it renders
Instead I added a DidStopLoading() hook that rereads the color when a load finishes, whether or not anything painted. A destination that never paints now falls back to the default instead of keeping the previous site's color
The theme color, bg color and first-paint hooks still handle the normal case
There was a problem hiding this comment.
That addresses the normal navigation case without flashing: DidStopLoading() rereads the color even when no paint callback occurs, and pages with no theme/background color fall back to the default.
One stale-state path remains: colors_seen_ is only cleared when switching tabs, not in PrimaryPageChanged(). Once a tab has observed 64 distinct quantized colors, Update() returns before calling SetAdaptiveColor() for every subsequent new color (adaptive_tab_color.cc:99-104). Navigating that same tab to a page with a new color can therefore retain the previous page’s toolbar color, including after DidStopLoading(). Clear the per-page color budget in PrimaryPageChanged() so the load-completion fallback can always apply.
Tip: You can customize Greptile's behavior for this repo with .greptile/rules.md and .greptile/config.json.
There was a problem hiding this comment.
In my most recent commit, I added colors_seen_.clear() to PrimaryPageChanged(). Primary page navigations now reset the budget before evaluating page adaptation, allowing subsequent color updates and the load-completion fallback to apply
| --- a/ui/color/color_provider_key.h | ||
| +++ b/ui/color/color_provider_key.h | ||
| @@ -140,6 +140,7 @@ struct COMPONENT_EXPORT(COLOR_PROVIDER_KEY) ColorProvi | ||
| FrameStyle frame_style = FrameStyle::kDefault; |
There was a problem hiding this comment.
adaptive_color is controlled by the page and is now part of ColorProviderKey, while providers are cached by the complete key without per-entry eviction. A page can publish a new theme color slightly more than every 100 ms, repeatedly initializing providers and growing the process-wide cache for as long as it remains active. Avoid using arbitrary page colors directly as unbounded cache-key values, or add a bound or eviction strategy.
There was a problem hiding this comment.
Good suggestion, made two changes in AdaptiveTabColor::Update() ^^
Color is quantized to 32 levels per channel b4 the contrast blend, so there are at most ~32.8k possible key values in total. Quantizing b4 blend keeps contrast guarantee since blend is a deterministic fn of its input
A tab can contribute at most 64 distinct colors while it is the active tab (kMaxColorsPerTab). Past that, the last color stays. The set resets on tab switch, which a page can't trigger
Separately, the 100ms timer restarts on every change, so a page that changes its theme-color faster than that never triggers an update at all
7d5f076 to
dbe998e
Compare
Possibly related issuesThese relationships were inferred from the effective change and may need maintainer confirmation:
|
![helium-gardener[bot]](https://avatars.githubusercontent.com/in/4914865?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review completed. 1 finding(s) were already covered by other reviewers; 2 new finding(s) remain.
Review walkthrough
Changes
| Files | Summary |
|---|---|
chrome/browser/ui/views/helium/adaptive_tab_color.hchrome/browser/ui/views/helium/adaptive_tab_color.ccchrome/browser/ui/views/frame/browser_widget.hchrome/browser/ui/views/frame/browser_widget.ccchrome/browser/ui/BUILD.gn |
Adds an active-tab observer, debounced page-color updates, contrast adjustment, widget-owned adaptive state, and build wiring. The observer currently reaches incognito windows and its color budget is scoped to the window activation rather than the tab. |
ui/color/color_provider_key.hui/color/color_provider_manager.ccchrome/browser/ui/color/material_chrome_color_mixer.ccui/views/widget/widget.cc |
Adds the adaptive color to provider cache keys, initializes chrome mixers in a light or dark mode derived from that color, overrides frame and toolbar colors, and removes the adaptive value from renderer color maps. |
chrome/browser/ui/ui_features.hchrome/browser/ui/ui_features.ccchrome/browser/chrome_content_browser_client.cc |
Defines the default-enabled feature and contrast parameter, and makes root scrollbar preference follow the page-facing color scheme while the feature is enabled. |
patches/helium/ui/adaptive-tab-color.patchpatches/series |
Introduces one coherent feature patch with no application warnings, but appends it after unrelated UI patches instead of placing it with the existing color-mixer stack. |
Review checks
| Check | Result | Notes |
|---|---|---|
| Scope | ✅ Passed | The PR adds one patch and one series entry; effective reviewable additions map to 12 Chromium source paths, with no rebase detected. |
| Project Policy | ✅ Passed | No upstream test file, test data, expectation, or test target is changed. |
| Correctness And Safety | Incognito windows receive page-derived shell colors, and the 64-color guard can both leave stale cross-navigation color and be reset by tab activation. | |
| Chromium Idioms | The existing 'IsIncognitoBrowser()' guard is not used, and nominally per-tab state is instead stored on the window despite Chromium’s documented 'TabFeatures'/'TabInterface' ownership pattern. | |
| Simplicity And Decomposition | 'colors_seen_' has overlapping window, activation, and claimed per-tab lifetimes, producing non-obvious cache and update behavior. | |
| Patch Organization | The new patch is appended at the series tail despite sharing the material color-mixer seam with 'helium/ui/helium-color-mixers.patch'. |
Analysis record
- Changed files considered: 2
- Findings: 2 new, 1 retained, 0 resolved
- Already covered by other reviewers: 1
adaptive-tab-color-budget-lifetime— greptile-apps
- Source snapshot:
d9de58346842 - Codex:
gpt-5.6-sol, reasoninghigh, fast mode on, web search cached - Patch application: clean
- Chromium context tool calls: none
- Maintainer conventions applied:
ignore-disabled-telemetry,script-resource-lifecycle,ui-command-contexts,verify-runtime-selection-paths
Limitations
- None reported.
| non_client_view()->set_context_menu_controller(this); | ||
| + } | ||
| + | ||
| + if (browser->GetType() == BrowserWindowInterface::Type::TYPE_NORMAL && |
There was a problem hiding this comment.
medium: Preserve the incognito window color mode
Incognito tabbed windows also have TYPE_NORMAL, so they construct AdaptiveTabColor. Because incognito has no custom_theme, GetColorProviderKey() accepts the adaptive color, and ColorProviderManager can replace Chromium’s forced dark incognito color_mode with a light mode derived from the page. A light-themed site can therefore make an incognito window resemble a normal window, weakening an intentional privacy-mode distinction. Exclude IsIncognitoBrowser() here; that existing helper also preserves Helium’s explicit enable-incognito-themes opt-in behavior.
Evidence: ThemeService::GetColorProviderKey() forces incognito profiles to ColorMode::kDark, but this new condition enables adaptation for every TYPE_NORMAL window without checking IsIncognitoBrowser().
There was a problem hiding this comment.
My recent commit added !IsIncognitoBrowser() to the setup check in BrowserWidget::InitBrowserWidget(). Incognito windows now skip AdaptiveTabColor initialization and preserve default dark framing
| helium/ui/compact-debugger-warning.patch | ||
| helium/ui/progress-bar.patch | ||
| helium/ui/exclusive-access-bubble.patch | ||
| helium/ui/adaptive-tab-color.patch |
There was a problem hiding this comment.
medium: Place the adaptive-color patch with the theming stack
The patch is appended after unrelated debugger, progress-bar, and exclusive-access patches even though it directly edits the same AddMaterialChromeColorMixer() area established by helium/ui/helium-color-mixers.patch and feeds the frame-color consumers that follow it. Move helium/ui/adaptive-tab-color.patch immediately after helium/ui/helium-color-mixers.patch; this keeps the shared color-mixer rebase surface together and places the provider prerequisite before the neighboring frame consumers.
Evidence: The new entry is at the end of patches/series, while helium/ui/helium-color-mixers.patch is at line 317 and owns the toolbar-color mixer hunk adapted by this patch.
There was a problem hiding this comment.
Moved helium/ui/adaptive-tab-color.patch directly after helium/ui/helium-color-mixers.patch in series to group the mixer modifications together in my most recent commit
|
Bump, all comments resolved (?) |
elricfd
left a comment
elricfd
left a comment
There was a problem hiding this comment.
Responded to all comments. The changes are in line with what my goal was with this feature.
44143de to
a7bbbad
Compare
|
Bump, handled Chromium update merge.
|
3cc561b to
f71fc49
Compare
|
could you put it behind a flag and attach screenshots of how it looks like in other layouts, such as vertical? the first screenshot shows that the location bar and selected tab are the same color as the background, but that shouldn't be the case. |
f71fc49 to
c0c22a3
Compare
put it behind chrome://flags/#helium-adaptive-tab-color, off by default. location bar and selected tab are now derived from the page color with a minimum contrast, so they no longer blend into the tinted frame. will attach screenshots momentarily |
c0c22a3 to
c246cf0
Compare
Screenshots attached: 
|
|
Forgot to attach the vertical and dynamic screenshots: Dynamic |
af7c6ba to
b0bc40c
Compare
b0bc40c to
50e693b
Compare
ede954a to
7f4c727
Compare
- Tint frame and toolbar with active page's color - Add chrome://flags#helium-adaptive-tab-color, off by default - Prefer theme-color, use rendered top edge when page has none - Use rendered color if theme-color has opposite lightness (Dark Reader) - Cover NTP background image, re-sample 1s after load for fade-ins - Pick light or dark mixers from color, blend it for text contrast - Keep location bar and selected tab distinct from tinted background - Skip incognito, app windows, custom themes and high contrast modes - Cap distinct colors per page at 64 to bound color provider cache - Keep adaptive color out of renderer color maps - Derive root scrollbar scheme from page scheme, not toolbar color
With the grayscale theme on macOS, the native postprocessing mixer converts every ui color to grayscale when the system accent is graphite. That turned the adaptive frame color, and the compact tab strip painted with it, grey while the toolbar kept the page color.
7f4c727 to
c309eb6
Compare
Closes #725.
kHeliumAdaptiveTabColorfeature flag (on by default) andmin-contrastparamtheme-colorinAdaptiveTabColor, falling back to the page background; updates are debounced by 100 msmin-contrastratioadaptive_colorthroughColorProviderKey;ColorProviderManagerpicks light or dark UI from its brightness without touchingcolor_mode, which pages read for their own light/dark themeadaptive_colorin renderer color mapsTested on: MacOS 27.0
