[webkit-reviews] review granted: [Bug 91638] [css3-text] Add support for text-decoration-color : [Attachment 179057] 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 91638: [css3-text] Add support for text-decoration-color
https://bugs.webkit.org/show_bug.cgi?id=91638

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

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


r=me, the change will very likely not apply on ToT and should be updated.

>
LayoutTests/fast/css3-text/css3-text-decoration/getComputedStyle/getComputedSty
le-text-decoration-color-expected.txt:12
> +Parent should cointain 'green':

Typo: cointain

>
LayoutTests/fast/css3-text/css3-text-decoration/repaint/repaint-text-decoration
-color.html:7
> +	   <!-- Bugzilla link: http://webkit.org/b/91638 -->
> +	   <title>CSS Test: CSS3 text-decoration-color repaint</title>
> +	   <link rel="help"
href="http://http://dev.w3.org/csswg/css3-text/#text-decoration-color"/>
> +	   <meta name="flags" content="ahem"/>

My take on those meta information is that if they are important enough, they
should be displayed. If not, they should just be removed as they will probably
not be read.

Here you would need:
* A passing condition (probably dumped, though I would be fine as a comment
(it's an exception from the previous rule as it is important for debugging the
test to have the intent somewhere)).
* What you are testing (a 'description').

This applies to the ref-test too.

> LayoutTests/fast/css3-text/css3-text-decoration/text-decoration-color.html:14

> +	       #blue-underline { text-decoration: underline;
-webkit-text-decoration-color: blue; }
> +	       #gray-overline { text-decoration: overline;
-webkit-text-decoration-color: gray; }
> +	       #green-line-through { text-decoration: line-through;
-webkit-text-decoration-color: green; }
> +	       #transparent-fill { -webkit-text-fill-color: transparent;
-webkit-text-stroke-width: 1px; -webkit-text-stroke-color: black; }

I prefer tests to follow WebKit's style and have one statement per line.