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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Wed Mar 27 13:55:39 PDT 2013


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 194941: Patch
https://bugs.webkit.org/attachment.cgi?id=194941&action=review

------- Additional Comments from Benjamin Poulain <benjamin at webkit.org>
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;


More information about the webkit-reviews mailing list