Summary: | Standardize the <meta name="color-scheme"> separator. | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Rune Lillesveen <rune.lillesveen> | ||||||
Component: | CSS | Assignee: | 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
Rune Lillesveen
2019-01-28 15:31:20 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 Created attachment 367695 [details]
Patch
Comment on attachment 367695 [details] Patch Clearing flags on attachment: 367695 Committed r244413: <https://trac.webkit.org/changeset/244413> All reviewed patches have been landed. Closing bug. 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. Reopening to attach new patch. Created attachment 367817 [details]
Followup Patch
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 on attachment 367817 [details] Followup Patch Clearing flags on attachment: 367817 Committed r244467: <https://trac.webkit.org/changeset/244467> All reviewed patches have been landed. Closing bug. |