Bug 193931 – Standardize the <meta name="color-scheme"> separator.

• Component: CSS
• Status: RESOLVED FIXED

[Rune Lillesveen]

<meta name="supported-color-schemes"> accepts both spaces and commas as separators. Shouldn't we standardize on either space[1] or comma[2] separation?

[1] https://html.spec.whatwg.org/multipage/common-microsyntaxes.html#space-separated-tokens
[2] https://html.spec.whatwg.org/multipage/common-microsyntaxes.html#comma-separated-tokens

[Timothy Hatcher]

This was to match parsing of <meta name="viewport">, <meta name="format-detection">, and window.open() features string, which allows comma and whitespace [1]. But I'm open to changing it to just whitespace since this is a simple list and no key-value pairs.

[1] https://html.spec.whatwg.org/#concept-window-open-features-tokenize

[Radar WebKit Bug Importer]

<rdar://problem/49995929>

[Timothy Hatcher]

Created attachment 367695 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 367695 [details]
Patch

Clearing flags on attachment: 367695

Committed r244413: <https://trac.webkit.org/changeset/244413>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

Comment on attachment 367695 [details]
Patch

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

> Source/WebCore/dom/Document.cpp:3619
> +static inline bool isColorSchemeSeparator(UChar character)

A few super-small thoughts:

1) Pretty sure this should be isHTMLSpace instead of isASCIISpace. The difference is in the handling of U+000B, which returns true from isASCIISpace, but false from isHTMLSpace. I don’t think the tests cover this but they should.

2) We’ve learned that with today’s C++ compilers there is no need to put the inline keyword on a function like this one. The compiler will inline it without a keyword. Roughly speaking, we only need to use "inline" when we want to include the function in multiple translation units (functions in headers), but the function is not a template and is not using "constexpr".

3) This function could be constexpr.

4) We don’t need a named function; the extra documentation that "these are the valid color scheme separators" was valuable when the rule was at all unclear, but now that it’s literally isHTMLSpace, I don’t think it’s helpful to have a named function.

[Timothy Hatcher]

Reopening to attach new patch.

[Timothy Hatcher]

Created attachment 367817 [details]
Followup Patch

[Timothy Hatcher]

Comment on attachment 367695 [details]
Patch

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

>> Source/WebCore/dom/Document.cpp:3619
>> +static inline bool isColorSchemeSeparator(UChar character)
> 
> A few super-small thoughts:
> 
> 1) Pretty sure this should be isHTMLSpace instead of isASCIISpace. The difference is in the handling of U+000B, which returns true from isASCIISpace, but false from isHTMLSpace. I don’t think the tests cover this but they should.
> 
> 2) We’ve learned that with today’s C++ compilers there is no need to put the inline keyword on a function like this one. The compiler will inline it without a keyword. Roughly speaking, we only need to use "inline" when we want to include the function in multiple translation units (functions in headers), but the function is not a template and is not using "constexpr".
> 
> 3) This function could be constexpr.
> 
> 4) We don’t need a named function; the extra documentation that "these are the valid color scheme separators" was valuable when the rule was at all unclear, but now that it’s literally isHTMLSpace, I don’t think it’s helpful to have a named function.

Good points. I attached a followup patch to address these.

[WebKit Commit Bot]

Comment on attachment 367817 [details]
Followup Patch

Clearing flags on attachment: 367817

Committed r244467: <https://trac.webkit.org/changeset/244467>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.