[webkit-reviews] review granted: [Bug 167729] Support string values on list-style-type : [Attachment 410421] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Oriol Brufau <obrufau at igalia.com>'s
request for review:
Bug 167729: Support string values on list-style-type
https://bugs.webkit.org/show_bug.cgi?id=167729

Attachment 410421: Patch

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




--- Comment #33 from Darin Adler <darin at apple.com> ---
Comment on attachment 410421
  --> https://bugs.webkit.org/attachment.cgi?id=410421
Patch

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

> Source/WebCore/css/parser/CSSPropertyParser.cpp:3884
> +	   // Some properties need to fallback onto the regular parser.

The verb "fall back" is two words.

> Source/WebCore/css/parser/CSSPropertyParser.cpp:4343
> +	   // NOTE: All the keyword values for the list-style-type property are
handled by the CSSParserFastPaths.

Leave out "NOTE:" unless there is some reason to include it. What comment is
not a note?

> Source/WebCore/rendering/RenderListMarker.cpp:1803
> +	   return ""_str;

emptyString(), not ""_str.

> Source/WebCore/rendering/style/RenderStyle.h:1002
> +    void setListStyleStringValue(const AtomString& s) {
SET_VAR(m_rareInheritedData, listStyleStringValue, s); }

How about the word 'value" instead of the letter "s"?

> LayoutTests/platform/mac/TestExpectations:2234
> +# These tests fail in Mac due to subpixel differences for the marker
position
>
+imported/w3c/web-platform-tests/css/css-lists/list-style-type-string-002.html
[ ImageOnlyFailure ]
>
+imported/w3c/web-platform-tests/css/css-lists/list-style-type-string-004.html
[ ImageOnlyFailure ]

We should find a way to resolve this. It’s unfortunate to not have test
coverage.