Commit ff9839f5 authored by Aran Gilman's avatar Aran Gilman Committed by Commit Bot

Do not cache dark mode settings.

It is unclear whether caching the settings improved performance. The
performance regression resolved before the CL adding the caches
landed, and there was no significant improvement to performance once
the CL did land.

However, it did make the code less clear and introduced greater
potential for bugs (e.g. https://crbug.com/1002664).

Bug: 991838
Change-Id: Ia67a693596ead2a3b392b6e1835e0a2963f47d84
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1822477Reviewed-by: default avatarRune Lillesveen <futhark@chromium.org>
Commit-Queue: Aran Gilman <gilmanmh@google.com>
Cr-Commit-Position: refs/heads/master@{#700335}
parent ad84c2e5
...@@ -87,31 +87,19 @@ int GetBackgroundBrightnessThreshold(const Settings& frame_settings) { ...@@ -87,31 +87,19 @@ int GetBackgroundBrightnessThreshold(const Settings& frame_settings) {
: frame_settings.GetDarkModeBackgroundBrightnessThreshold(); : frame_settings.GetDarkModeBackgroundBrightnessThreshold();
} }
// A static wrapper to hold DarkModeSettings after initial creation. DarkModeSettings GetEnabledSettings(const Settings& frame_settings) {
// DarkModeSettings settings;
// These settings are fixed after the browser launches, so they only need to be settings.mode = GetMode(frame_settings);
// constructed once for the "enabled" state and once for the "disabled" state. settings.image_policy = GetImagePolicy(frame_settings);
// Caching these values instead of re-calculating them should improve page load settings.text_brightness_threshold =
// performance. GetTextBrightnessThreshold(frame_settings);
const DarkModeSettings& GetCachedEnabledSettings( settings.background_brightness_threshold =
const Settings& frame_settings) { GetBackgroundBrightnessThreshold(frame_settings);
static const DarkModeSettings* settings =
new DarkModeSettings([](const Settings& frame_settings) { settings.grayscale = frame_settings.GetDarkModeGrayscale();
DarkModeSettings settings; settings.contrast = frame_settings.GetDarkModeContrast();
settings.mode = GetMode(frame_settings); settings.image_grayscale_percent = frame_settings.GetDarkModeImageGrayscale();
settings.image_policy = GetImagePolicy(frame_settings); return settings;
settings.text_brightness_threshold =
GetTextBrightnessThreshold(frame_settings);
settings.background_brightness_threshold =
GetBackgroundBrightnessThreshold(frame_settings);
settings.grayscale = frame_settings.GetDarkModeGrayscale();
settings.contrast = frame_settings.GetDarkModeContrast();
settings.image_grayscale_percent =
frame_settings.GetDarkModeImageGrayscale();
return settings;
}(frame_settings));
return *settings;
} }
// In theory it should be sufficient for the disabled settings to set mode to // In theory it should be sufficient for the disabled settings to set mode to
...@@ -122,14 +110,11 @@ const DarkModeSettings& GetCachedEnabledSettings( ...@@ -122,14 +110,11 @@ const DarkModeSettings& GetCachedEnabledSettings(
// //
// TODO(gilmanmh): Investigate unexpected image inversion behavior when // TODO(gilmanmh): Investigate unexpected image inversion behavior when
// image_policy not set to kFilterNone. // image_policy not set to kFilterNone.
const DarkModeSettings& GetCachedDisabledSettings() { DarkModeSettings GetDisabledSettings() {
static const DarkModeSettings* settings = new DarkModeSettings([]() { DarkModeSettings settings;
DarkModeSettings settings; settings.mode = DarkModeInversionAlgorithm::kOff;
settings.mode = DarkModeInversionAlgorithm::kOff; settings.image_policy = DarkModeImagePolicy::kFilterNone;
settings.image_policy = DarkModeImagePolicy::kFilterNone; return settings;
return settings;
}());
return *settings;
} }
} // namespace } // namespace
...@@ -139,9 +124,9 @@ DarkModeSettings BuildDarkModeSettings(const Settings& frame_settings, ...@@ -139,9 +124,9 @@ DarkModeSettings BuildDarkModeSettings(const Settings& frame_settings,
if (IsDarkModeEnabled(frame_settings) && if (IsDarkModeEnabled(frame_settings) &&
ShouldApplyDarkModeFilterToPage(frame_settings.GetDarkModePagePolicy(), ShouldApplyDarkModeFilterToPage(frame_settings.GetDarkModePagePolicy(),
root)) { root)) {
return GetCachedEnabledSettings(frame_settings); return GetEnabledSettings(frame_settings);
} }
return GetCachedDisabledSettings(); return GetDisabledSettings();
} }
bool ShouldApplyDarkModeFilterToPage(DarkModePagePolicy policy, bool ShouldApplyDarkModeFilterToPage(DarkModePagePolicy policy,
......
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