fix(browser): persist theme setting by checking system theme before reset#647
Conversation
…eset Checks ::prefs::kUsesSystemTheme in SyncDefaultTheme to avoid overwriting custom/system themes (like GTK on Linux) on browser startup. This fixes an issue where the theme would reset to BrowserOS default after relaunch.
|
All contributors have signed the CLA. Thank you! |
Greptile SummaryThis PR fixes a theme persistence bug where selecting "Use GTK" (system theme) on Linux would be reset to the BrowserOS default theme on every browser restart. The fix adds a guard clause to
Confidence Score: 3/5Safe to merge on Linux-only builds; potential CHECK failure on non-Linux platforms where kUsesSystemTheme is not registered. The fix correctly solves the reported GTK theme persistence bug, but the unchecked call to GetBoolean on a potentially unregistered pref (kUsesSystemTheme is Linux-specific) poses a real risk of debug-build crashes or undefined behavior on macOS/Windows if SyncDefaultTheme is called cross-platform. packages/browseros/chromium_patches/chrome/browser/browseros/core/browseros_prefs.cc — specifically the new guard clause at line 68. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Browser Startup] --> B[SyncDefaultTheme called]
B --> C{kUsesSystemTheme == true?}
C -- Yes NEW GUARD --> D[Return early
System theme preserved]
C -- No --> E{kUserColor at
default value?}
E -- No --> F[Return
User custom color preserved]
E -- Yes --> G[Set BrowserOS default theme
SkColor gray + kNeutral variant]
Prompt To Fix All With AIThis is a comment left during a code review.
Path: packages/browseros/chromium_patches/chrome/browser/browseros/core/browseros_prefs.cc
Line: 68-70
Comment:
**Potential crash on non-Linux platforms**
`::prefs::kUsesSystemTheme` is a Linux-specific preference in Chromium. On macOS and Windows, this preference is typically not registered. Calling `pref_service->GetBoolean()` on an unregistered preference is a `CHECK` failure in debug builds and undefined behavior in release builds.
Since `SyncDefaultTheme` appears to be called cross-platform, this guard should be wrapped in a Linux build flag, or a safer `FindPreference` pattern (matching the existing pattern used for `kUserColor` below) should be used:
```suggestion
// BrowserOS: If the user is using a system theme (GTK on Linux), do not
// overwrite it with the default BrowserOS value.
#if BUILDFLAG(IS_LINUX)
if (pref_service->GetBoolean(::prefs::kUsesSystemTheme)) {
return;
}
#endif
```
Alternatively, mirror the safer pattern already used in this function:
```cpp
const PrefService::Preference* system_theme_pref =
pref_service->FindPreference(::prefs::kUsesSystemTheme);
if (system_theme_pref && system_theme_pref->GetValue()->GetBool()) {
return;
}
```
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "fix(browser): persist theme setting by c..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| + if (pref_service->GetBoolean(::prefs::kUsesSystemTheme)) { | ||
| + return; | ||
| + } |
There was a problem hiding this comment.
::prefs::kUsesSystemTheme is a Linux-specific preference in Chromium. On macOS and Windows, this preference is typically not registered. Calling pref_service->GetBoolean() on an unregistered preference is a CHECK failure in debug builds and undefined behavior in release builds.
Since SyncDefaultTheme appears to be called cross-platform, this guard should be wrapped in a Linux build flag, or a safer FindPreference pattern (matching the existing pattern used for kUserColor below) should be used:
| + if (pref_service->GetBoolean(::prefs::kUsesSystemTheme)) { | |
| + return; | |
| + } | |
| // BrowserOS: If the user is using a system theme (GTK on Linux), do not | |
| // overwrite it with the default BrowserOS value. | |
| #if BUILDFLAG(IS_LINUX) | |
| if (pref_service->GetBoolean(::prefs::kUsesSystemTheme)) { | |
| return; | |
| } | |
| #endif | |
Alternatively, mirror the safer pattern already used in this function:
const PrefService::Preference* system_theme_pref =
pref_service->FindPreference(::prefs::kUsesSystemTheme);
if (system_theme_pref && system_theme_pref->GetValue()->GetBool()) {
return;
}Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/browseros/chromium_patches/chrome/browser/browseros/core/browseros_prefs.cc
Line: 68-70
Comment:
**Potential crash on non-Linux platforms**
`::prefs::kUsesSystemTheme` is a Linux-specific preference in Chromium. On macOS and Windows, this preference is typically not registered. Calling `pref_service->GetBoolean()` on an unregistered preference is a `CHECK` failure in debug builds and undefined behavior in release builds.
Since `SyncDefaultTheme` appears to be called cross-platform, this guard should be wrapped in a Linux build flag, or a safer `FindPreference` pattern (matching the existing pattern used for `kUserColor` below) should be used:
```suggestion
// BrowserOS: If the user is using a system theme (GTK on Linux), do not
// overwrite it with the default BrowserOS value.
#if BUILDFLAG(IS_LINUX)
if (pref_service->GetBoolean(::prefs::kUsesSystemTheme)) {
return;
}
#endif
```
Alternatively, mirror the safer pattern already used in this function:
```cpp
const PrefService::Preference* system_theme_pref =
pref_service->FindPreference(::prefs::kUsesSystemTheme);
if (system_theme_pref && system_theme_pref->GetValue()->GetBool()) {
return;
}
```
How can I resolve this? If you propose a fix, please make it concise.
|
I have read the CLA Document and I hereby sign the CLA |
Vaidik Bajpai (vaidik-bajpai)
changed the title
|
Vaidik Bajpai (@vaidik-bajpai) Thank you for opening this PR about the issue I opened. Could you please have a look on the comment and fix proposal about the potential crash on non-Linux platforms? |
2 participants
This PR fixes this issue where the user's theme selection (specifically "Use GTK" on Linux) would not persist after restarting the browser.
The Problem: The SyncDefaultTheme function was executing on every startup and overwriting the theme with the "BrowserOS Blue" tonal spot if it found the kUserColor preference at its default value. When using GTK, kUserColor is indeed default, causing the reset.
The Fix: Added a guard clause to SyncDefaultTheme in browseros_prefs.cc that check if ::prefs::kUsesSystemTheme is enabled. If true, the function returns early, preserving the user's system theme selection.