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

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

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


I am not supportive of such massive changes (especially since there are several
of them) and would prefer if those were split. You are a new contributor to
WebCore which means that there are rough edges and there is so much that can be
caught by inspection when the patch is so big.

I would rather see you introduce an umbrella feature flag (runtime probably)
for all the CSS 3 text changes and land your changes bits by bits so that each
patch is atomic and well tested. That would also bring the average time for
review down.

The current patch is lacking enough testing to be landed on top of that.

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

It's preferred to fill the function level entries.

> LayoutTests/ChangeLog:27
> +	   * svg/css/getComputedStyle-basic-expected.txt:

All in all, your testing is extremely poor. You implemented a way to get the
value back from JavaScript (using getComputedStyle), yet it's not tested. So is
the invalidation logic (which was indeed implemented in the previous patch, my
bad) which would require a very simple repaint test.