Bug 193931

Summary: Standardize the <meta name="color-scheme"> separator.
Product: WebKit Reporter: Rune Lillesveen <rune.lillesveen>
Component: CSSAssignee: Timothy Hatcher <timothy>
Status: RESOLVED FIXED    
Severity: Normal CC: cdumez, commit-queue, darin, dbates, esprehn+autocc, ews-watchlist, kangil.han, simon.fraser, timothy, webkit-bug-importer
Priority: P2 Keywords: BrowserCompat, InRadar
Version: Safari Technology Preview   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 197016    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Followup Patch none

Description Rune Lillesveen 2019-01-28 15:31:20 PST
<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
Comment 1 Timothy Hatcher 2019-01-29 10:13:00 PST
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
Comment 2 Radar WebKit Bug Importer 2019-04-17 15:50:43 PDT
<rdar://problem/49995929>
Comment 3 Timothy Hatcher 2019-04-17 16:19:42 PDT
Created attachment 367695 [details]
Patch
Comment 4 WebKit Commit Bot 2019-04-17 16:55:53 PDT
Comment on attachment 367695 [details]
Patch

Clearing flags on attachment: 367695

Committed r244413: <https://trac.webkit.org/changeset/244413>
Comment 5 WebKit Commit Bot 2019-04-17 16:55:54 PDT
All reviewed patches have been landed.  Closing bug.
Comment 6 Darin Adler 2019-04-18 09:17:24 PDT
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.
Comment 7 Timothy Hatcher 2019-04-19 13:15:53 PDT
Reopening to attach new patch.
Comment 8 Timothy Hatcher 2019-04-19 13:15:54 PDT
Created attachment 367817 [details]
Followup Patch
Comment 9 Timothy Hatcher 2019-04-19 13:16:37 PDT
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.
Comment 10 WebKit Commit Bot 2019-04-19 14:20:02 PDT
Comment on attachment 367817 [details]
Followup Patch

Clearing flags on attachment: 367817

Committed r244467: <https://trac.webkit.org/changeset/244467>
Comment 11 WebKit Commit Bot 2019-04-19 14:20:04 PDT
All reviewed patches have been landed.  Closing bug.