[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 #194941|review?, commit-queue?      |review-, commit-queue-
               Flag|                            |




--- Comment #72 from Benjamin Poulain <benjamin at webkit.org>  2013-03-27 13:53:50 PST ---
(From update of attachment 194941)
View in context: https://bugs.webkit.org/attachment.cgi?id=194941&action=review

This looks a lot better.

Only minor comments.

By the way, if this is the only test covering this feature, I think it needs _A LOT_ more tests: WebKit Transform, right to left text, vertical text, BIDI text, ruby text, etc. This must be addressed but it can be done in another patch.

> Source/WebCore/rendering/InlineTextBox.cpp:991
> + * The start point (p1), controlPoint1), controlPoint2 and end point (p2) of the Bezier curve
> + * form a diamond shape:

Stale closing parenthesis after "controlPoint1".

> Source/WebCore/rendering/InlineTextBox.cpp:1039
> +        FloatPoint controlPoint1;
> +        FloatPoint controlPoint2;

Move those down, closer to where they are used.

The closer a declaration is to the variable use, the less you have to keep in mind when reading the code.

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

This only make sure y1 <= y2.
I would just remove the comment.

> Source/WebCore/rendering/InlineTextBox.cpp:1062
> +        int divisor = static_cast<int>(step);
> +        float quotient = static_cast<int>(y2 - y1 - 1) / divisor;
> +        float remainder = static_cast<int>(y2 - y1 - 1) % divisor;
> +        float adjustment = remainder / quotient;
> +        controlPointDistance += 2 * adjustment;
> +        step += adjustment;

This whole block should be a static function.

Looks like you should use unsigned instead of int.

Why do you switch to integer at all for the step?
Why do you use -1 for the length?

I would think something like that would be better:
float length = y2 - y1.
unsigned stepCount = static_cast<unsigned>(length / step);
float uncoveredLength = length - (stepCount * step);
float adjustment = uncoveredLength / stepCount;
step += adjustment;

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