[Webkit-unassigned] [Bug 92868] [css3-text] Add platform support for "wavy" text decoration style

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=92868


Benjamin Poulain <benjamin at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #192958|review?, commit-queue?      |review-, commit-queue-
               Flag|                            |




--- Comment #68 from Benjamin Poulain <benjamin at webkit.org>  2013-03-22 16:18:08 PST ---
(From update of attachment 192958)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.