[webkit-reviews] review granted: [Bug 123358] -webkit-text-decoration-skip: ink; isn't parsed in CSS : [Attachment 215228] Patch

[bugzilla-daemon at webkit.org]

Dean Jackson <dino at apple.com> has granted Myles C. Maxfield
<mmaxfield at apple.com>'s request for review:
Bug 123358: -webkit-text-decoration-skip: ink; isn't parsed in CSS
https://bugs.webkit.org/show_bug.cgi?id=123358

Attachment 215228: Patch
https://bugs.webkit.org/attachment.cgi?id=215228&action=review

------- Additional Comments from Dean Jackson <dino at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=215228&action=review


> Source/JavaScriptCore/ChangeLog:4
> +	   -webkit-text-decoration-skip: ink; isn't parsed in CSS
> +	   https://bugs.webkit.org/show_bug.cgi?id=123358

I think this title should be what you're doing, not what is broken before this
patch.

> Source/WebCore/ChangeLog:9
> +	   -webkit-text-decoration-skip: ink; isn't parsed in CSS
> +	   https://bugs.webkit.org/show_bug.cgi?id=123358
> +
> +	   Reviewed by NOBODY (OOPS!).
> +
> +	   Adds initial parsing support for parsing
-webkit-text-decoration-skip with
> +	   values of "none" and "skip". This work is behind the new

I'm confused. The title says "ink" and here you're saying "none" and "skip".

> Source/WebCore/css/DeprecatedStyleBuilder.cpp:1272
> +    static TextDecorationSkip valueToDecorationSkip(CSSPrimitiveValue&
primitiveValue)

Please move this below public: into a new private: section, or just make it a
standalone file static function.

> Source/WebCore/css/DeprecatedStyleBuilder.cpp:1298
> +	   for (CSSValueListIterator i(value); i.hasMore(); i.advance()) {
> +	       CSSValue* item = i.value();

I believe both of these cases could use "auto". Not worth changing though.

> Source/WebCore/rendering/style/RenderStyleConstants.h:386
> +    TextDecorationSkipNone = 0x0, TextDecorationSkipInk = 1 << 0

Separate lines for these. And I think we tend to just use 0 rather than 0x0.

>
LayoutTests/fast/css3-text/css3-text-decoration/text-decoration-skip/text-decor
ation-skip-roundtrip.html:60
> +    stylesheet.insertRule(".p { -webkit-text-decoration-skip: none; }", 0);
> +    testNoneIsValid(stylesheet);

One more test: stylesheet.insertRule(".p { -webkit-text-decoration-skip: ; }",
0);