Commit deaf4dd0 authored by v.paturi's avatar v.paturi Committed by Commit Bot

Update comment for GetDisabledSettings() in apply_dark_mode.cc.

The issue mentioned in the comment is regarding the unexpected
inversion of images when |mode| in DarkModeSettings is set to
kOff.

This issue is no longer reproducible in the latest code.
Currently the code is structured in such a way that
|image_filter_| is always nullptr when the mode is set to kOff.

Unit tests are added to check this behaviour in
dark_mode_filter_test.cc.

The default value for |image_policy| in DarkModeSettings is
currently kFilterAll. Even though this does not break any
functionality, its better to have the default value to be
kFilterNone which is more consistent with the other default
values in the structure.

Change-Id: Ia37dbf24fff99cf3ab3cc6930a441a8b2800e44f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1874993Reviewed-by: default avatarStephen Chenney <schenney@chromium.org>
Reviewed-by: default avatarAran Gilman <gilmanmh@google.com>
Reviewed-by: default avatarPrashant Nevase <prashant.n@samsung.com>
Commit-Queue: Varun Chowdhary Paturi <v.paturi@samsung.com>
Cr-Commit-Position: refs/heads/master@{#708902}
parent 9eb0f0ed
......@@ -102,18 +102,9 @@ DarkModeSettings GetEnabledSettings(const Settings& frame_settings) {
return settings;
}
// In theory it should be sufficient for the disabled settings to set mode to
// kOff (or to just return the default struct) without also setting
// image_policy. However, this causes images to be inverted unexpectedly in
// some cases (such as when toggling between the site's light and dark theme
// on arstechnica.com).
//
// TODO(gilmanmh): Investigate unexpected image inversion behavior when
// image_policy not set to kFilterNone.
DarkModeSettings GetDisabledSettings() {
DarkModeSettings settings;
settings.mode = DarkModeInversionAlgorithm::kOff;
settings.image_policy = DarkModeImagePolicy::kFilterNone;
return settings;
}
......
......@@ -49,6 +49,8 @@ class PLATFORM_EXPORT DarkModeFilter {
Image* image,
cc::PaintFlags* flags);
SkColorFilter* GetImageFilterForTesting() { return image_filter_.get(); }
private:
DarkModeSettings settings_;
......
......@@ -14,7 +14,6 @@
namespace blink {
namespace {
// TODO(gilmanmh): Add expectations for image inversion to each test.
TEST(DarkModeFilterTest, DoNotApplyFilterWhenDarkModeIsOff) {
DarkModeFilter filter;
......@@ -32,6 +31,8 @@ TEST(DarkModeFilterTest, DoNotApplyFilterWhenDarkModeIsOff) {
EXPECT_EQ(base::nullopt,
filter.ApplyToFlagsIfNeeded(
cc::PaintFlags(), DarkModeFilter::ElementRole::kBackground));
EXPECT_EQ(nullptr, filter.GetImageFilterForTesting());
}
TEST(DarkModeFilterTest, ApplyDarkModeToColorsAndFlags) {
......@@ -54,6 +55,8 @@ TEST(DarkModeFilterTest, ApplyDarkModeToColorsAndFlags) {
flags, DarkModeFilter::ElementRole::kBackground);
ASSERT_NE(flags_or_nullopt, base::nullopt);
EXPECT_EQ(SK_ColorBLACK, flags_or_nullopt.value().getColor());
EXPECT_NE(nullptr, filter.GetImageFilterForTesting());
}
} // namespace
......
......@@ -48,7 +48,7 @@ struct DarkModeSettings {
bool grayscale = false;
float image_grayscale_percent = 0.0; // Valid range from 0.0 to 1.0
float contrast = 0.0; // Valid range from -1.0 to 1.0
DarkModeImagePolicy image_policy = DarkModeImagePolicy::kFilterAll;
DarkModeImagePolicy image_policy = DarkModeImagePolicy::kFilterNone;
DarkModeClassifierType classifier_type = DarkModeClassifierType::kGeneric;
// Text colors with brightness below this threshold will be inverted, and
......
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