Bug 92868 – [css3-text] Add platform support for "wavy" text decoration style

• Component: CSS
• Status: RESOLVED FIXED

[Bruno Abinader (history only)]

The CSS3 property "text-decoration-style" accepts the following values: "solid | double | dotted | dashed | wavy" as seen on the link below:
http://dev.w3.org/csswg/css3-text/#text-decoration-style

All values except "wavy" were already supported due to previous usage on "border-style" property:
http://www.w3.org/TR/css3-background/#the-border-style

This bug intends to provide platform support for "wavy" value (already parsed for "text-decoration-style" in bug 90958).

[Bruno Abinader (history only)]

Two new platform specific bugs have been created so far:

[Qt] Add support for text decoration "wavy" style
https://bugs.webkit.org/show_bug.cgi?id=93507

[Skia] Add support for text decoration "wavy" and "double" styles
https://bugs.webkit.org/show_bug.cgi?id=93509

[Bruno Abinader (history only)]

Four new platform specific bugs have been added:

[cairo] Add support for text decoration "wavy" style
https://bugs.webkit.org/show_bug.cgi?id=94110

[wx] Add support for text decoration "wavy" style
https://bugs.webkit.org/show_bug.cgi?id=94111

[cg] Add support for text decoration "wavy" style
https://bugs.webkit.org/show_bug.cgi?id=94112

[wince] Add support for text decoration "wavy" style
https://bugs.webkit.org/show_bug.cgi?id=94114

[Lamarque V. Souza]

Created attachment 186394 [details]
Patch

Proposed patch. It works for all platforms.

[Lamarque V. Souza]

Created attachment 186397 [details]
Patch (EWS only version)

Same patch with css3-text and css3-conditional-rules enabled.

[Early Warning System Bot]

Comment on attachment 186397 [details]
Patch (EWS only version)

Attachment 186397 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/16377169

[Early Warning System Bot]

Comment on attachment 186397 [details]
Patch (EWS only version)

Attachment 186397 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/16373256

[EFL EWS Bot]

Comment on attachment 186397 [details]
Patch (EWS only version)

Attachment 186397 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/16377173

[Build Bot]

Comment on attachment 186397 [details]
Patch (EWS only version)

Attachment 186397 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/16376214

[Build Bot]

Comment on attachment 186397 [details]
Patch (EWS only version)

Attachment 186397 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/16365378

[WebKit Review Bot]

Comment on attachment 186397 [details]
Patch (EWS only version)

Attachment 186397 [details] did not pass cr-linux-debug-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16370291

[WebKit Review Bot]

Comment on attachment 186397 [details]
Patch (EWS only version)

Attachment 186397 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16376238

[Peter Beverloo (cr-android ews)]

Comment on attachment 186397 [details]
Patch (EWS only version)

Attachment 186397 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/16378204

[Build Bot]

Comment on attachment 186397 [details]
Patch (EWS only version)

Attachment 186397 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/16368329

[Lamarque V. Souza]

Created attachment 186694 [details]
Patch (EWS only version)

Attempt to fix build with css3-conditional-rules enabled.

[Early Warning System Bot]

Comment on attachment 186694 [details]
Patch (EWS only version)

Attachment 186694 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/16379762

[Early Warning System Bot]

Comment on attachment 186694 [details]
Patch (EWS only version)

Attachment 186694 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/16370892

[Build Bot]

Comment on attachment 186694 [details]
Patch (EWS only version)

Attachment 186694 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/16373822

[WebKit Review Bot]

Comment on attachment 186694 [details]
Patch (EWS only version)

Attachment 186694 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16372934

[Lamarque V. Souza]

Created attachment 186707 [details]
Patch (EWS only version)

Attempt to fix build with css3-conditional-rules enabled.

[WebKit Review Bot]

Comment on attachment 186694 [details]
Patch (EWS only version)

Attachment 186694 [details] did not pass cr-linux-debug-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16368866

[Early Warning System Bot]

Comment on attachment 186707 [details]
Patch (EWS only version)

Attachment 186707 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/16374892

[Early Warning System Bot]

Comment on attachment 186707 [details]
Patch (EWS only version)

Attachment 186707 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/16386009

[Build Bot]

Comment on attachment 186707 [details]
Patch (EWS only version)

Attachment 186707 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/16367968

[WebKit Review Bot]

Comment on attachment 186707 [details]
Patch (EWS only version)

Attachment 186707 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16369967

[EFL EWS Bot]

Comment on attachment 186707 [details]
Patch (EWS only version)

Attachment 186707 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/16386020

[WebKit Review Bot]

Comment on attachment 186707 [details]
Patch (EWS only version)

Attachment 186707 [details] did not pass cr-linux-debug-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16373873

[Peter Beverloo (cr-android ews)]

Comment on attachment 186707 [details]
Patch (EWS only version)

Attachment 186707 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/16368895

[Build Bot]

Comment on attachment 186707 [details]
Patch (EWS only version)

Attachment 186707 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/16387019

[Peter Beverloo (cr-android ews)]

Comment on attachment 186707 [details]
Patch (EWS only version)

Attachment 186707 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/16388019

[Build Bot]

Comment on attachment 186707 [details]
Patch (EWS only version)

Attachment 186707 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/16376952

[Build Bot]

Comment on attachment 186707 [details]
Patch (EWS only version)

Attachment 186707 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/16387078

[Benjamin Poulain]

Comment on attachment 186394 [details]
Patch

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.

[Lamarque V. Souza]

(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 {
    ...
}

[Lamarque V. Souza]

(In reply to comment #33)
> (In reply to comment #32)
> > (From update of attachment 186394 [details] [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.

I have noticed that my other patch for https://bugs.webkit.org/show_bug.cgi?id=93507 already includes the baseline for my platform. We will have to revert that other patch to include the baseline in this patch.
 
> > As an alternative, why not 'strokeWavyTextDecoration(GraphicsContext* context, const FloatPoint& point1, const FloatPoint& point2)'?
> > 
> > +don't const the input float;
> 
> Why not const them?

By the way, if I do not const them then I cannot write

strokeWavyTextDecoration(context, FloatPoint(localOrigin.x(), localOrigin.y() + baseline + doubleOffset + 1), FloatPoint(localOrigin.x() + width, localOrigin.y() + baseline + doubleOffset + 1), textDecorationThickness);

in InlineTextBox::paintDecoration(). I would have to create a temporary variable for each FloatPoint parameter before passing them to strokeWavyTextDecoration().

[Lamarque V. Souza]

Created attachment 187833 [details]
Patch

Fixes some issues in code style and update changelogs.

[Lamarque V. Souza]

Created attachment 187834 [details]
Patch (EWS only version)

Same patch with css3-text and css3-conditional-rules flags enabled.

[Early Warning System Bot]

Comment on attachment 187834 [details]
Patch (EWS only version)

Attachment 187834 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/16488942

[Early Warning System Bot]

Comment on attachment 187834 [details]
Patch (EWS only version)

Attachment 187834 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/16522180

[WebKit Review Bot]

Comment on attachment 187834 [details]
Patch (EWS only version)

Attachment 187834 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16520166

[WebKit Review Bot]

Comment on attachment 187834 [details]
Patch (EWS only version)

Attachment 187834 [details] did not pass cr-linux-debug-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16488945

[EFL EWS Bot]

Comment on attachment 187834 [details]
Patch (EWS only version)

Attachment 187834 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/16519233

[Peter Beverloo (cr-android ews)]

Comment on attachment 187834 [details]
Patch (EWS only version)

Attachment 187834 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/16439252

[kov's GTK+ EWS bot]

Comment on attachment 187834 [details]
Patch (EWS only version)

Attachment 187834 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/16440233

[Lamarque V. Souza]

Created attachment 187866 [details]
Patch (EWS only version)

Drop ENABLE_CSS_HIERARCHIES flag.

[Build Bot]

Comment on attachment 187866 [details]
Patch (EWS only version)

Attachment 187866 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/16514475

[Lamarque V. Souza]

Created attachment 188309 [details]
Patch (EWS only version)

Save/restore graphics context.

[WebKit Review Bot]

Comment on attachment 188309 [details]
Patch (EWS only version)

Attachment 188309 [details] did not pass cr-linux-debug-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16542545

[WebKit Review Bot]

Comment on attachment 188309 [details]
Patch (EWS only version)

Attachment 188309 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16537584

[WebKit Review Bot]

Comment on attachment 188309 [details]
Patch (EWS only version)

Attachment 188309 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16527657

[WebKit Review Bot]

Comment on attachment 188309 [details]
Patch (EWS only version)

Attachment 188309 [details] did not pass cr-linux-debug-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16530644

[Build Bot]

Comment on attachment 188309 [details]
Patch (EWS only version)

Attachment 188309 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/16586556

[Lamarque V. Souza]

Created attachment 190078 [details]
Patch (EWS only version)

Fix issue with cr-linux bot.

[Build Bot]

Comment on attachment 190078 [details]
Patch (EWS only version)

Attachment 190078 [details] did not pass win-ews (win):
Output: http://webkit-commit-queue.appspot.com/results/16769201

[Benjamin Poulain]

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

This: "const float strokeThickness)" is not the WebKit convention.

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

I don't really care about Mozilla's implementation if we can make ours looks better! :)

I doubt a bezier path would improve performance. On a good rasterizer, that will not change performance too much.
It is probably worth testing and comparing the two.

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

Verbose is good. Take the one with the best readability.

[Lamarque V. Souza]

Created attachment 192492 [details]
Patch

Fix issues reported.

[Lamarque V. Souza]

Created attachment 192493 [details]
Bezier with 1 pixel stroke thickness

Expectations using wavy bezier curve and 1 pixel stroke thickness

[Lamarque V. Souza]

Created attachment 192494 [details]
Bezier with 2 pixels stroke thickness

Expectations using wavy bezier curve and 2 pixels stroke thickness

[WebKit Review Bot]

Attachment 192492 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/platform/chromium-linux/fast/css3-text/css3-text-decoration/text-decoration-style-expected.png', u'Source/WebCore/ChangeLog', u'Source/WebCore/rendering/InlineTextBox.cpp', u'Source/WebCore/rendering/InlineTextBox.h']" exit_code: 1
LayoutTests/platform/chromium-linux/fast/css3-text/css3-text-decoration/text-decoration-style-expected.png:0:  Have to enable auto props in the subversion config file (/home/alancutter/.subversion/config "enable-auto-props = yes"). Have to set the svn:mime-type in the subversion config file (/home/alancutter/.subversion/config "*.png = svn:mime-type=image/png").  [image/png] [5]
Total errors found: 1 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Lamarque V. Souza]

Created attachment 192495 [details]
Non-bezier with 1 pixel stroke thickness

Expectations using wavy curve and 1 pixel stroke thickness

[Lamarque V. Souza]

Created attachment 192497 [details]
Non-bezier with 2 pixels stroke thickness

Expectations using wavy curve and 2 pixels stroke thickness

[Lamarque V. Souza]

Created attachment 192500 [details]
Bezier implementation with instrumentation code.

(In reply to comment #54)
> I don't really care about Mozilla's implementation if we can make ours looks better! :)

For comparison you can see some screenshots of the Mozilla implementation in https://bugs.webkit.org/show_bug.cgi?id=94110
 
> I doubt a bezier path would improve performance. On a good rasterizer, that will not change performance too much.
> It is probably worth testing and comparing the two.

I have implemented the bezier version of this patch (find the attached patch). I did some benchmarks (the patch also includes the code used to get the data used in my analysis) and the bezier version is one order of magnitude slower than the non-bezier version. According to my results, the non-bezier version takes about 20 microseconds to draw while the bezier version takes about 220 microseconds. That is still fast in my oppinion.

[Lamarque V. Souza]

Created attachment 192780 [details]
Patch

Use bezier curve and fix small issues.

[Lamarque V. Souza]

Created attachment 192958 [details]
Patch

Make bezier curve symmetric and update changelogs.

[Lamarque V. Souza]

Created attachment 192959 [details]
Bezier with 1 pixel stroke thickness

Expectations using wavy bezier curve and 1 pixel stroke thickness.

[Lamarque V. Souza]

Created attachment 192960 [details]
Bezier with 2 pixels stroke thickness

Expectations using wavy bezier curve and 2 pixels stroke thickness.

[Lamarque V. Souza]

Created attachment 192961 [details]
Patch (EWS only version)

Same patch with css3-text enabled.

[Build Bot]

Comment on attachment 192961 [details]
Patch (EWS only version)

Attachment 192961 [details] did not pass win-ews (win):
Output: http://webkit-commit-queue.appspot.com/results/17217028

[Benjamin Poulain]

Comment on attachment 192958 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=192958&action=review

This looks good overall (except maybe the tail of the line).
Mostly nitpicks:

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

Does this access any member of InlineTextBox?
If not, it should be a local static function and not a member function.

> Source/WebCore/rendering/InlineTextBox.cpp:971
> +    const float controlPointDistance = 3 * max<float>(2, strokeThickness);
> +    const float step = 2 * max<float>(2, strokeThickness);

Magic numbers.

> Source/WebCore/rendering/InlineTextBox.cpp:972
> +    float x1, y1, x2, y2;

This should be declared locally + WebKit style.

> Source/WebCore/rendering/InlineTextBox.cpp:973
> +    float cp1x, cp1y, cp2x, cp2y;

Ditto.
Obscure names.

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

WebKit style.

> Source/WebCore/rendering/InlineTextBox.cpp:979
> +        // Make sure (x1, y1) < (x2, y2)

Period.

> Source/WebCore/rendering/InlineTextBox.cpp:992
> +        while (y + 2 * step <= y2) {

This should be a for() loop.

Wouldn't this be too short for the tail?

> Source/WebCore/rendering/InlineTextBox.cpp:998
> +    } else {

You should assert p1.y() == p2.y().

> Source/WebCore/rendering/InlineTextBox.cpp:1106
> +                strokeWavyTextDecoration(context, FloatPoint(localOrigin.x(), localOrigin.y() + baseline + doubleOffset + 1), FloatPoint(localOrigin.x() + width, localOrigin.y() + baseline + doubleOffset + 1), textDecorationThickness);

Break the arguments on local variables.

> Source/WebCore/rendering/InlineTextBox.cpp:1123
> +                strokeWavyTextDecoration(context, FloatPoint(localOrigin.x(), localOrigin.y() - doubleOffset), FloatPoint(localOrigin.x() + width, localOrigin.y() - doubleOffset), textDecorationThickness);

Ditto.

> Source/WebCore/rendering/InlineTextBox.cpp:1127
>              context->drawLineForText(localOrigin, width, isPrinting);

I would indent this line for readability.

> Source/WebCore/rendering/InlineTextBox.cpp:1131
> +            }

missing break?

> Source/WebCore/rendering/InlineTextBox.cpp:1139
> +                strokeWavyTextDecoration(context, FloatPoint(localOrigin.x(), localOrigin.y() + 2 * baseline / 3), FloatPoint(localOrigin.x() + width, localOrigin.y() + 2 * baseline / 3), textDecorationThickness);

Move the arguments outside the line.

[Lamarque V. Souza]

Comment on attachment 192958 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=192958&action=review

>> Source/WebCore/rendering/InlineTextBox.cpp:992
>> +        while (y + 2 * step <= y2) {
> 
> This should be a for() loop.
> 
> Wouldn't this be too short for the tail?

Yes, that can happen. With strokeThickness equals 1 pixel then it can be up to 7 pixels too short. I am not sure how to solve this problem. Do you know to draw a half Bezier curve?

>> Source/WebCore/rendering/InlineTextBox.cpp:1131
>> +            }
> 
> missing break?

No. This part belongs to the "default" part of the switch statement. Break is not necessary here and as far as I know about WebKit's code style it should not be used in this case.

[Lamarque V. Souza]

Created attachment 194898 [details]
Patch

Fix issues reported, except the curve is too short one.

[Lamarque V. Souza]

Created attachment 194941 [details]
Patch

Fix the too short curve problem. I did a benchmark and this fix adds about 4% overhead, which translated to microseconds is less than the standard deviation I calculated. I think that is reasonable. One problem with this fix is that two wavy decorations in the same page but in different text strings can have slightly different shapes. I think most people cannot notice that without zooming in the page.

[Benjamin Poulain]

Comment on attachment 194941 [details]
Patch

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;

[Lamarque V. Souza]

Created attachment 195421 [details]
Patch

Fix remaining issues reported.

[Lamarque V. Souza]

Comment on attachment 194941 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=194941&action=review

>> Source/WebCore/rendering/InlineTextBox.cpp:1044
>> +        // Make sure (x1, y1) < (x2, y2).
> 
> This only make sure y1 <= y2.
> I would just remove the comment.

I meant "Make sure point (x1, y1) is at left of point (x2, y2)". I will remove the comment.

>> Source/WebCore/rendering/InlineTextBox.cpp:1062
>> +        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;

I used a module operator (%), I was not sure it if would work with float. Anyway, your suggestion does not need that operation so the casting is not necessary. I just fixed a small issue when calculating the covered lenght. Since one Bezier curve starts at the same pixels as the previous one ended we need to subtract (stepCount - 1) pixels from the covered lenght. The second line becomes "float uncoveredLength = length - (stepCount * step - (stepCount -1));". Without the -1 the last Bezier curve was not draw sometimes because the decoration crossed the x2 coordinate (for horizontal wavy decoration). Now everything works as it should and the -1 can be removed.

[Benjamin Poulain]

Comment on attachment 195421 [details]
Patch

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.

[Lamarque V. Souza]

Comment on attachment 195421 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=195421&action=review

>> Source/WebCore/rendering/InlineTextBox.cpp:991
>> +    if (step <= 0 || length <= 0)
> 
> Could step really be <= 0?
> Sounds like this should be an assertion.

I do not think so. I will change the check on step to an assert and keep the one for lenght since it can be 0 if the start and end points of the decoration are the same.

>> Source/WebCore/rendering/InlineTextBox.cpp:1029
>> + *                 step
> 
> Usually I am strongly against comment but I have to say this is great to help understand your code. Good job on this.

Thanks :-)

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

I will change it to xAxis and yAxis.

[Lamarque V. Souza]

Created attachment 195633 [details]
Patch

Patch for landing.

[WebKit Review Bot]

Comment on attachment 195633 [details]
Patch

Clearing flags on attachment: 195633

Committed r147170: <http://trac.webkit.org/changeset/147170>

[Bruno Abinader (history only)]

All reviewed patches have landed. Closing bug.