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

[bugzilla-daemon at webkit.org]

Julien Chaffraix <jchaffraix at webkit.org> has canceled 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 158567: Patch
https://bugs.webkit.org/attachment.cgi?id=158567&action=review

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


> Source/WebCore/ChangeLog:13
> +	  
https://developer-dev.allizom.org/en-US/docs/CSS/text-decoration-style

Interesting link, I though MDN was the reference for those but I may be wrong.

> Source/WebCore/ChangeLog:42
> +	   * rendering/style/StyleRareNonInheritedData.h:

Don't forget the function level entries.

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:-1953
> -	   case CSSPropertyTextDecoration:

Is this necessary to move this property?

> Source/WebCore/rendering/style/RenderStyleConstants.h:350
> +    TextDecorationStyleSolid, TextDecorationStyleDouble,
TextDecorationStyleDotted, TextDecorationStyleDashed, TextDecorationStyleWavy

Nit: I prefer one per line as it's easier to read but there is no rule in our
guide and this file is not very consistent so it's up to you.

>
LayoutTests/fast/css3-text-decoration/getComputedStyle/getComputedStyle-text-de
coration-style-expected.txt:12
> +PASS [dashed] computed style of -webkit-text-decoration-style should contain
dashed value

The output is kinda confusing and you don't strictly test the computed value as
taken back from JS.

Here is what is missing from your testing:
* setting the value from JS (style.webkitTextDecorationStyle)
* erroneous values
* testing that *all the possible values* are properly parsed, set and converted
back when queried.
* inheritance

>
LayoutTests/fast/css3-text-decoration/getComputedStyle/script-tests/getComputed
Style-text-decoration-style.js:12
> +	   testPassed(desc+" EXPECTED:"+expected+" ACTUAL:"+actual);

We prefer tests to follow WebKit style (ie space before and after binary
operators)

>
LayoutTests/fast/css3-text-decoration/getComputedStyle/script-tests/getComputed
Style-text-decoration-style.js:18
> +expect('[initial] -webkit-text-decoration-style should be null',
e.style.getPropertyCSSValue('-webkit-text-decoration-style'), null);

I think you should use shouldBe here instead of rolling out your own version of
that. shouldBe does a lot more than just the comparison.