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




--- Comment #75 from Benjamin Poulain <benjamin at webkit.org>  2013-03-28 12:14:23 PST ---
(From update of attachment 195421)
View in context: https://bugs.webkit.org/attachment.cgi?id=195421&action=review

> Source/WebCore/rendering/InlineTextBox.cpp:988
> +// Adjust step to make wavy decoration to cover all the distance between start point (p1) and end point (p2).

You can remove this, the name of your function says it all.

> Source/WebCore/rendering/InlineTextBox.cpp:991
> +    if (step <= 0 || length <= 0)

Could step really be <= 0?
Sounds like this should be an assertion.

> Source/WebCore/rendering/InlineTextBox.cpp:999
> +    float uncoveredLength = length - (stepCount * step - (stepCount -1));

Missing space between minus and 1.

> Source/WebCore/rendering/InlineTextBox.cpp:1029
> + *                              step
> + *                         |-----------|
> + *                   controlPoint1
> + *                         +
> + *
> + *
> + *                  . .
> + *                .     .
> + *              .         .
> + * (x1, y1) p1 +           .            + p2 (x2, y2) - <--- Decoration's axis
> + *                          .         .               |
> + *                            .     .                 |
> + *                              . .                   | controlPointDistance
> + *                                                    |
> + *                                                    |
> + *                         +                          -
> + *                   controlPoint2
> + *
> + *             |-----------|
> + *                 step

Usually I am strongly against comment but I have to say this is great to help understand your code. Good job on this.

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

Maybe name the x1 differently to express it is the stable axis?

Maybe just xAxis?

> Source/WebCore/rendering/InlineTextBox.cpp:1081
> +        float y1 = p1.y();

Ditto for the name + move thins one line up.

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