[webkit-reviews] review denied: [Bug 92868] [css3-text] Add platform support for "wavy" text decoration style : [Attachment 186394] Patch

[bugzilla-daemon at webkit.org]

Benjamin Poulain <benjamin at webkit.org> has denied Lamarque V. Souza
<Lamarque.Souza at basyskom.com>'s request for review:
Bug 92868: [css3-text] Add platform support for "wavy" text decoration style
https://bugs.webkit.org/show_bug.cgi?id=92868

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

------- Additional Comments from Benjamin Poulain <benjamin at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=186394&action=review


It looks promising but this needs to become much cleaner.

> Source/WebCore/ChangeLog:10
> +	   platforms. This patch also obsoletes bugs 94110, 94111, 94112, 94114

> +	   and 108571.

This "This patch also obsoletes bugs 94110, 94111, 94112, 94114 and 108571." is
unnecessary.

What you should do instead is explain the change. Both the "Why?" and
"What/How?" of the patch.

> Source/WebCore/ChangeLog:14
> +	   Tests are in
> +	   fast/css3-text/css3-text-decoration/text-decoration-style.html
already,
> +	   just need to rebaseline them (see bug 100546).

Then when not already rebaseline them for your platform in this patch?

> Source/WebCore/rendering/InlineTextBox.cpp:961
> +void InlineTextBox::createWavyPath(GraphicsContext* context, const
FloatPoint& point1, const FloatPoint& point2, const float strokeThickness,
Path& path)

This is either misnamed or the function does not do enough.