[webkit-reviews] review granted: [Bug 190499] Add support for prefers-color-scheme media query : [Attachment 352111] Patch

[bugzilla-daemon at webkit.org]

Dean Jackson <dino at apple.com> has granted Timothy Hatcher <timothy at apple.com>'s
request for review:
Bug 190499: Add support for prefers-color-scheme media query
https://bugs.webkit.org/show_bug.cgi?id=190499

Attachment 352111: Patch

https://bugs.webkit.org/attachment.cgi?id=352111&action=review




--- Comment #4 from Dean Jackson <dino at apple.com> ---
Comment on attachment 352111
  --> https://bugs.webkit.org/attachment.cgi?id=352111
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=352111&action=review

> Source/JavaScriptCore/Configurations/FeatureDefines.xcconfig:100
> +ENABLE_DARK_MODE_CSS = $(ENABLE_DARK_MODE_CSS_$(WK_PLATFORM_NAME));
> +ENABLE_DARK_MODE_CSS_macosx = ENABLE_DARK_MODE_CSS;

Why not do the bit from FeatureDefines.h here too?

I think it would be something like this:

ENABLE_DARK_MODE_CSS_macosx = $(ENABLE_DARK_MODE_CSS$(WK_MACOS_1014));
ENABLE_DARK_MODE_CSS_MACOS_SINCE_1014 = ENABLE_DARK_MODE_CSS;

> Source/WTF/wtf/FeatureDefines.h:207
> +#if !defined(ENABLE_DARK_MODE_CSS)
> +#define ENABLE_DARK_MODE_CSS __MAC_OS_X_VERSION_MIN_REQUIRED >= 101400
> +#endif

I think you can avoid any changes in FeatureDefines.h by using the macros
above.

> Source/WebCore/css/MediaQueryEvaluator.cpp:48
> +#include "RuntimeEnabledFeatures.h"

Amazing that this is the first media query to be runtime enabled!

> Source/WebCore/css/MediaQueryEvaluator.cpp:733
> +    ASSERT(RuntimeEnabledFeatures::sharedFeatures().darkModeCSSEnabled());

I wonder if we'd be better returning false if it isn't enabled? I guess that
means it looks like the media query "worked"

> Source/WebCore/css/MediaQueryExpression.cpp:58
> +    || (mediaFeature == MediaFeatureNames::prefersColorScheme &&
RuntimeEnabledFeatures::sharedFeatures().darkModeCSSEnabled())

OK. Now I understand the ASSERT. Does this allow the WI to show that the query
wasn't understood, as opposed to returning false?

> Source/WebKit/Shared/WebPreferences.yaml:1288
> +  humanReadableName: "Dark Mode CSS Support"
> +  humanReadableDescription: "Enable Dark Mode CSS Support"

Did you pick this name over prefers-color-scheme because you'll use it for the
<meta> as well?

> LayoutTests/ChangeLog:10
> +	   * css-dark-mode/perfers-color-scheme-expected.txt: Added.
> +	   * css-dark-mode/perfers-color-scheme.html: Added.

perfers :)