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




--- Comment #32 from Benjamin Poulain <benjamin at webkit.org>  2013-02-10 17:50:25 PST ---
(From update of attachment 186394)
View in context: https://bugs.webkit.org/attachment.cgi?id=186394&action=review

It looks promising but this needs to become much cleaner.

> Source/WebCore/ChangeLog:10
> +        platforms. This patch also obsoletes bugs 94110, 94111, 94112, 94114
> +        and 108571.

This "This patch also obsoletes bugs 94110, 94111, 94112, 94114 and 108571." is unnecessary.

What you should do instead is explain the change. Both the "Why?" and "What/How?" of the patch.

> Source/WebCore/ChangeLog:14
> +        Tests are in
> +        fast/css3-text/css3-text-decoration/text-decoration-style.html already,
> +        just need to rebaseline them (see bug 100546).

Then when not already rebaseline them for your platform in this patch?

> Source/WebCore/rendering/InlineTextBox.cpp:961
> +void InlineTextBox::createWavyPath(GraphicsContext* context, const FloatPoint& point1, const FloatPoint& point2, const float strokeThickness, Path& path)

This is either misnamed or the function does not do enough.
>From a function named createWavyPath(), the only return you would expect is a Path. Here you modify the path passed as a parameter, and modify the context.

As an alternative, why not 'strokeWavyTextDecoration(GraphicsContext* context, const FloatPoint& point1, const FloatPoint& point2)'?

+don't const the input float;

> Source/WebCore/rendering/InlineTextBox.cpp:965
> +    bool isVerticalLine = (p1.x() == p2.x());

You should compute this after adjustLineToPixelBoundaries(). This cold could be moved just before the if().

> Source/WebCore/rendering/InlineTextBox.cpp:971
> +    short signal = -1;

Why a short?
What is this anyway? "Signal" is a strange name in this context.

> Source/WebCore/rendering/InlineTextBox.cpp:994
> +        x1 = x2 = p1.x();
> +
> +        // Make sure (x1, y1) < (x2, y2)
> +        if (p1.y() < p2.y()) {
> +            y1 = p1.y();
> +            y2 = p2.y();
> +        } else {
> +            y1 = p2.y();
> +            y2 = p1.y();
> +        }
> +
> +        path.moveTo(FloatPoint(x1 + signal * step, y1));
> +        float y = y1 + 2 * step;
> +
> +        while (y <= y2) {
> +            signal = -signal;
> +            path.addLineTo(FloatPoint(x1 + signal * step, y));
> +            path.addLineTo(FloatPoint(x1 + signal * step, y + flat)); // Draw flat line between diagonal lines.
> +            y += 2 * step + flat;
> +        }

Instinctively, I would have used bezier curve. Why do you use straight lines?

> Source/WebCore/rendering/InlineTextBox.cpp:1115
> +#if ENABLE(CSS3_TEXT)
> +            if (decorationStyle == TextDecorationStyleWavy) {
> +                Path path;
> +                createWavyPath(context, FloatPoint(localOrigin.x(), localOrigin.y() + baseline + doubleOffset + 1), FloatPoint(localOrigin.x() + width, localOrigin.y() + baseline + doubleOffset + 1), textDecorationThickness, path);
> +                context->strokePath(path);
> +            } else {
> +#endif // CSS3_TEXT
>              // Leave one pixel of white between the baseline and the underline.
>              context->drawLineForText(FloatPoint(localOrigin.x(), localOrigin.y() + baseline + 1), width, isPrinting);
>  #if ENABLE(CSS3_TEXT)
> -            if (decorationStyle == TextDecorationStyleDouble)
> -                context->drawLineForText(FloatPoint(localOrigin.x(), localOrigin.y() + baseline + 1 + doubleOffset), width, isPrinting);
> +                if (decorationStyle == TextDecorationStyleDouble)
> +                    context->drawLineForText(FloatPoint(localOrigin.x(), localOrigin.y() + baseline + 1 + doubleOffset), width, isPrinting);
> +            }
>  #endif // CSS3_TEXT

Shouldn't this become a switch-case based on decorationStyle?

> Source/WebCore/rendering/InlineTextBox.cpp:1130
> +            if (decorationStyle == TextDecorationStyleWavy) {
> +                Path path;
> +                createWavyPath(context, FloatPoint(localOrigin.x(), localOrigin.y() - doubleOffset), FloatPoint(localOrigin.x() + width, localOrigin.y() - doubleOffset), textDecorationThickness, path);
> +                context->strokePath(path);
> +            } else {
> +#endif // CSS3_TEXT
>              context->drawLineForText(localOrigin, width, isPrinting);
>  #if ENABLE(CSS3_TEXT)
> -            if (decorationStyle == TextDecorationStyleDouble)
> -                context->drawLineForText(FloatPoint(localOrigin.x(), localOrigin.y() - doubleOffset), width, isPrinting);
> +                if (decorationStyle == TextDecorationStyleDouble)
> +                    context->drawLineForText(FloatPoint(localOrigin.x(), localOrigin.y() - doubleOffset), width, isPrinting);
> +            }

Ditto.

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