[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





--- Comment #33 from Lamarque V. Souza <Lamarque.Souza at basyskom.com>  2013-02-11 17:55:38 PST ---
(In reply to comment #32)
> (From update of attachment 186394 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=186394&action=review
> > 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?

Ok, I will do it.

> As an alternative, why not 'strokeWavyTextDecoration(GraphicsContext* context, const FloatPoint& point1, const FloatPoint& point2)'?
> 
> +don't const the input float;

Why not const them?

> > Source/WebCore/rendering/InlineTextBox.cpp:971
> > +    short signal = -1;
> 
> Why a short?
> What is this anyway? "Signal" is a strange name in this context.

"sign" is the correct name here. I will fix it.

> > 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?

Well, I am not used to use bezier curves. Also, we deciced to make this curse to look similar to mozilla's implementation [1]. There is also some performance concerns about using strokPath to implement wavy stroke [2], would bezier curve improve performance here?

[1] https://bugs.webkit.org/show_bug.cgi?id=94110#c16
[2] https://bugs.webkit.org/show_bug.cgi?id=93507#c43

> > 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?

I can use change it to a switch-case, but it would look like this

switch (decorationStyle) {
case TextDecorationStyleWavy:
    ...
    break;

default:
    ...
    break;
}

That more verbose than

if (decorationStyle == TextDecorationStyleWavy) {
    ...
} else {
    ...
}

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