[webkit-reviews] review granted: [Bug 94093] [css3-text] Add parsing support for -webkit-text-decoration-style : [Attachment 159218] Patch

[bugzilla-daemon at webkit.org]

Julien Chaffraix <jchaffraix at webkit.org> has granted Bruno Abinader
<bruno.abinader at basyskom.com>'s request for review:
Bug 94093: [css3-text] Add parsing support for -webkit-text-decoration-style
https://bugs.webkit.org/show_bug.cgi?id=94093

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

------- Additional Comments from Julien Chaffraix <jchaffraix at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=159218&action=review


r=me, with comments

> Source/WebCore/ChangeLog:43
> +	   (StyleRareNonInheritedData):

Still missing those information from the ChangeLog, please don't forget it
before landing.

>
LayoutTests/fast/css3-text-decoration/getComputedStyle/script-tests/getComputed
Style-text-decoration-style.js:43
> +// Value 'double'.
> +e.style.webkitTextDecorationStyle = "double";
>
+shouldBe("e.style.getPropertyCSSValue('-webkit-text-decoration-style').toStrin
g()", "'[object CSSPrimitiveValue]'");
> +shouldBe("e.style.webkitTextDecorationStyle", "'double'");
> +
> +computedStyle = window.getComputedStyle(e, null);
>
+shouldBe("computedStyle.getPropertyCSSValue('-webkit-text-decoration-style').t
oString()", "'[object CSSPrimitiveValue]'");
> +shouldBe("computedStyle.webkitTextDecorationStyle",
"e.style.webkitTextDecorationStyle");
>
+shouldBe("computedStyle.getPropertyCSSValue('-webkit-text-decoration-style').c
ssText", "'double'");

This would probably be written as a function for all the valid values. That
would remove the need for the leading "// Value XXX" comments as they would be
obvious (and hopefully dumped into the output, see below as to how).

>
LayoutTests/fast/css3-text-decoration/getComputedStyle/script-tests/getComputed
Style-text-decoration-style.js:86
> +// Values 'double' and 'dotted' (invalid, previous valid value 'initial' is
assumed).

All in all, I think those should be dumped in the output using debug("..."),
including some spaces for readability using debug(''); as needed.

That would make the output above more readable by adding some information about
what you trying to test. Think of the port maintainers or developers who
haven't followed this bug and will have to understand the output of this test
if it ever starts failing.