This patch implements the "text-decoration-style" property parsing as specified in CSS3 working draft, with "-webkit-" prefix. The specification can be found below: http://dev.w3.org/csswg/css3-text/#text-decoration-style Additionally, Mozilla implementation details can be found here: https://developer-dev.allizom.org/en-US/docs/CSS/text-decoration-style This is an individual task for bug 90958. Rendering support will be handled on a different bug.
Created attachment 158567 [details] Patch Proposed patch.
Please note EWS will fail as this patch depends on bug 93863 to be landed first, reason why I didn't requested review / commit-queue flags for now.
Comment on attachment 158567 [details] Patch Requesting review and commit-queue flags now that bug 93863 has landed - r125716.
Created attachment 159116 [details] Patch (EWS run only) This patch adds changes to make CSS3_TEXT_DECORATION feature flag enabled and remove skip of fast/css3-text-decoration layout test directory from chromium build. This is intentional to get EWS results, but not intended for landing (as suggested by Peter Beverloo in http://lists.webkit.org/pipermail/webkit-dev/2012-August/021925.html ). It is not intended for landing, but acts as a workaround to get proper EWS results for the previous patch.
Comment on attachment 158567 [details] Patch 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-decoration-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/getComputedStyle-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/getComputedStyle-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.
Created attachment 159218 [details] Patch Fixed issues pointed out by Julien.
Created attachment 159221 [details] Patch (EWS run only) v2 "EWS run only" for the updated patch.
Comment on attachment 159218 [details] Patch 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/getComputedStyle-text-decoration-style.js:43 > +// Value 'double'. > +e.style.webkitTextDecorationStyle = "double"; > +shouldBe("e.style.getPropertyCSSValue('-webkit-text-decoration-style').toString()", "'[object CSSPrimitiveValue]'"); > +shouldBe("e.style.webkitTextDecorationStyle", "'double'"); > + > +computedStyle = window.getComputedStyle(e, null); > +shouldBe("computedStyle.getPropertyCSSValue('-webkit-text-decoration-style').toString()", "'[object CSSPrimitiveValue]'"); > +shouldBe("computedStyle.webkitTextDecorationStyle", "e.style.webkitTextDecorationStyle"); > +shouldBe("computedStyle.getPropertyCSSValue('-webkit-text-decoration-style').cssText", "'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/getComputedStyle-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.
Created attachment 159254 [details] Patch Added missing info to ChangeLog and made layout test results more readible.
Created attachment 159471 [details] Patch (updated "Reviewed by" section) Thanks to Igor, manually updated "Reviewed by" section from ChangeLogs.
Comment on attachment 159471 [details] Patch (updated "Reviewed by" section) Clearing flags on attachment: 159471 Committed r126054: <http://trac.webkit.org/changeset/126054>