[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