[webkit-reviews] review denied: [Bug 92868] [css3-text] Add platform support for "wavy" text decoration style : [Attachment 192958] 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 192958: Patch
https://bugs.webkit.org/attachment.cgi?id=192958&action=review

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


This looks good overall (except maybe the tail of the line).
Mostly nitpicks:

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

Does this access any member of InlineTextBox?
If not, it should be a local static function and not a member function.

> Source/WebCore/rendering/InlineTextBox.cpp:971
> +    const float controlPointDistance = 3 * max<float>(2, strokeThickness);
> +    const float step = 2 * max<float>(2, strokeThickness);

Magic numbers.

> Source/WebCore/rendering/InlineTextBox.cpp:972
> +    float x1, y1, x2, y2;

This should be declared locally + WebKit style.

> Source/WebCore/rendering/InlineTextBox.cpp:973
> +    float cp1x, cp1y, cp2x, cp2y;

Ditto.
Obscure names.

> Source/WebCore/rendering/InlineTextBox.cpp:977
> +	   x1 = x2 = p1.x();

WebKit style.

> Source/WebCore/rendering/InlineTextBox.cpp:979
> +	   // Make sure (x1, y1) < (x2, y2)

Period.

> Source/WebCore/rendering/InlineTextBox.cpp:992
> +	   while (y + 2 * step <= y2) {

This should be a for() loop.

Wouldn't this be too short for the tail?

> Source/WebCore/rendering/InlineTextBox.cpp:998
> +    } else {

You should assert p1.y() == p2.y().

> Source/WebCore/rendering/InlineTextBox.cpp:1106
> +		   strokeWavyTextDecoration(context,
FloatPoint(localOrigin.x(), localOrigin.y() + baseline + doubleOffset + 1),
FloatPoint(localOrigin.x() + width, localOrigin.y() + baseline + doubleOffset +
1), textDecorationThickness);

Break the arguments on local variables.

> Source/WebCore/rendering/InlineTextBox.cpp:1123
> +		   strokeWavyTextDecoration(context,
FloatPoint(localOrigin.x(), localOrigin.y() - doubleOffset),
FloatPoint(localOrigin.x() + width, localOrigin.y() - doubleOffset),
textDecorationThickness);

Ditto.

> Source/WebCore/rendering/InlineTextBox.cpp:1127
>	       context->drawLineForText(localOrigin, width, isPrinting);

I would indent this line for readability.

> Source/WebCore/rendering/InlineTextBox.cpp:1131
> +	       }

missing break?

> Source/WebCore/rendering/InlineTextBox.cpp:1139
> +		   strokeWavyTextDecoration(context,
FloatPoint(localOrigin.x(), localOrigin.y() + 2 * baseline / 3),
FloatPoint(localOrigin.x() + width, localOrigin.y() + 2 * baseline / 3),
textDecorationThickness);

Move the arguments outside the line.