Bug 92868 - [css3-text] Add platform support for "wavy" text decoration style
Summary: [css3-text] Add platform support for "wavy" text decoration style
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL: http://dev.w3.org/csswg/css-text-deco...
Keywords: WebExposed
Depends on: 90958 93507 93863 108571 109926
Blocks: 58491 100546 93509 94110 94111 94112 94114
  Show dependency treegraph
 
Reported: 2012-08-01 06:08 PDT by Bruno Abinader (history only)
Modified: 2013-03-31 13:57 PDT (History)
26 users (show)

See Also:


Attachments
Patch (8.86 KB, patch)
2013-02-04 09:11 PST, Lamarque V. Souza
no flags Details | Formatted Diff | Diff
Patch (EWS only version) (23.42 KB, patch)
2013-02-04 09:20 PST, Lamarque V. Souza
no flags Details | Formatted Diff | Diff
Patch (EWS only version) (25.26 KB, patch)
2013-02-05 13:39 PST, Lamarque V. Souza
webkit-ews: commit-queue-
Details | Formatted Diff | Diff
Patch (EWS only version) (25.78 KB, patch)
2013-02-05 14:30 PST, Lamarque V. Souza
no flags Details | Formatted Diff | Diff
Patch (15.26 KB, patch)
2013-02-12 04:47 PST, Lamarque V. Souza
no flags Details | Formatted Diff | Diff
Patch (EWS only version) (29.81 KB, patch)
2013-02-12 04:52 PST, Lamarque V. Souza
webkit-ews: commit-queue-
Details | Formatted Diff | Diff
Patch (EWS only version) (29.65 KB, patch)
2013-02-12 07:40 PST, Lamarque V. Souza
buildbot: commit-queue-
Details | Formatted Diff | Diff
Patch (EWS only version) (29.70 KB, patch)
2013-02-14 04:00 PST, Lamarque V. Souza
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Patch (EWS only version) (29.95 KB, patch)
2013-02-25 09:57 PST, Lamarque V. Souza
no flags Details | Formatted Diff | Diff
Patch (15.39 KB, patch)
2013-03-11 10:12 PDT, Lamarque V. Souza
no flags Details | Formatted Diff | Diff
Bezier with 1 pixel stroke thickness (6.87 KB, image/png)
2013-03-11 10:16 PDT, Lamarque V. Souza
no flags Details
Bezier with 2 pixels stroke thickness (7.46 KB, image/png)
2013-03-11 10:18 PDT, Lamarque V. Souza
no flags Details
Non-bezier with 1 pixel stroke thickness (6.46 KB, image/png)
2013-03-11 10:19 PDT, Lamarque V. Souza
no flags Details
Non-bezier with 2 pixels stroke thickness (6.98 KB, image/png)
2013-03-11 10:20 PDT, Lamarque V. Souza
no flags Details
Bezier implementation with instrumentation code. (3.90 KB, patch)
2013-03-11 10:28 PDT, Lamarque V. Souza
no flags Details | Formatted Diff | Diff
Patch (15.39 KB, patch)
2013-03-12 11:58 PDT, Lamarque V. Souza
no flags Details | Formatted Diff | Diff
Patch (15.18 KB, patch)
2013-03-13 12:16 PDT, Lamarque V. Souza
benjamin: review-
benjamin: commit-queue-
Details | Formatted Diff | Diff
Bezier with 1 pixel stroke thickness (6.78 KB, image/png)
2013-03-13 12:20 PDT, Lamarque V. Souza
no flags Details
Bezier with 2 pixels stroke thickness (7.14 KB, image/png)
2013-03-13 12:22 PDT, Lamarque V. Souza
no flags Details
Patch (EWS only version) (29.66 KB, patch)
2013-03-13 12:25 PDT, Lamarque V. Souza
buildbot: commit-queue-
Details | Formatted Diff | Diff
Patch (17.03 KB, patch)
2013-03-25 11:59 PDT, Lamarque V. Souza
no flags Details | Formatted Diff | Diff
Patch (21.90 KB, patch)
2013-03-25 16:25 PDT, Lamarque V. Souza
no flags Details | Formatted Diff | Diff
Patch (24.33 KB, patch)
2013-03-27 15:50 PDT, Lamarque V. Souza
benjamin: review+
benjamin: commit-queue-
Details | Formatted Diff | Diff
Patch (24.21 KB, patch)
2013-03-28 13:26 PDT, Lamarque V. Souza
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Bruno Abinader (history only) 2012-08-01 06:08:02 PDT
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).
Comment 1 Bruno Abinader (history only) 2012-08-08 12:54:58 PDT
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
Comment 2 Bruno Abinader (history only) 2012-08-15 07:55:49 PDT
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
Comment 3 Lamarque V. Souza 2013-02-04 09:11:33 PST
Created attachment 186394 [details]
Patch

Proposed patch. It works for all platforms.
Comment 4 Lamarque V. Souza 2013-02-04 09:20:35 PST
Created attachment 186397 [details]
Patch (EWS only version)

Same patch with css3-text and css3-conditional-rules enabled.
Comment 5 Early Warning System Bot 2013-02-04 09:32:10 PST
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
Comment 6 Early Warning System Bot 2013-02-04 09:35:31 PST
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
Comment 7 EFL EWS Bot 2013-02-04 09:41:14 PST
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
Comment 8 Build Bot 2013-02-04 09:54:01 PST
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
Comment 9 Build Bot 2013-02-04 10:20:43 PST
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
Comment 10 WebKit Review Bot 2013-02-04 10:41:15 PST
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
Comment 11 WebKit Review Bot 2013-02-04 10:52:49 PST
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
Comment 12 Peter Beverloo (cr-android ews) 2013-02-04 11:44:28 PST
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
Comment 13 Build Bot 2013-02-04 12:41:22 PST
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
Comment 14 Lamarque V. Souza 2013-02-05 13:39:43 PST
Created attachment 186694 [details]
Patch (EWS only version)

Attempt to fix build with css3-conditional-rules enabled.
Comment 15 Early Warning System Bot 2013-02-05 13:54:18 PST
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
Comment 16 Early Warning System Bot 2013-02-05 13:56:16 PST
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
Comment 17 Build Bot 2013-02-05 14:12:01 PST
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
Comment 18 WebKit Review Bot 2013-02-05 14:27:56 PST
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
Comment 19 Lamarque V. Souza 2013-02-05 14:30:23 PST
Created attachment 186707 [details]
Patch (EWS only version)

Attempt to fix build with css3-conditional-rules enabled.
Comment 20 WebKit Review Bot 2013-02-05 14:53:38 PST
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
Comment 21 Early Warning System Bot 2013-02-05 15:06:03 PST
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
Comment 22 Early Warning System Bot 2013-02-05 15:08:18 PST
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
Comment 23 Build Bot 2013-02-05 15:25:30 PST
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
Comment 24 WebKit Review Bot 2013-02-05 15:47:30 PST
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
Comment 25 EFL EWS Bot 2013-02-05 15:58:32 PST
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
Comment 26 WebKit Review Bot 2013-02-05 16:38:07 PST
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
Comment 27 Peter Beverloo (cr-android ews) 2013-02-05 16:40:24 PST
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
Comment 28 Build Bot 2013-02-05 17:18:08 PST
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
Comment 29 Peter Beverloo (cr-android ews) 2013-02-05 17:35:48 PST
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
Comment 30 Build Bot 2013-02-05 18:06:29 PST
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
Comment 31 Build Bot 2013-02-05 19:31:56 PST
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
Comment 32 Benjamin Poulain 2013-02-10 17:50:25 PST
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.
Comment 33 Lamarque V. Souza 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 {
    ...
}
Comment 34 Lamarque V. Souza 2013-02-12 04:15:21 PST
(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().
Comment 35 Lamarque V. Souza 2013-02-12 04:47:24 PST
Created attachment 187833 [details]
Patch

Fixes some issues in code style and update changelogs.
Comment 36 Lamarque V. Souza 2013-02-12 04:52:22 PST
Created attachment 187834 [details]
Patch (EWS only version)

Same patch with css3-text and css3-conditional-rules flags enabled.
Comment 37 Early Warning System Bot 2013-02-12 06:07:53 PST
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
Comment 38 Early Warning System Bot 2013-02-12 06:10:50 PST
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
Comment 39 WebKit Review Bot 2013-02-12 06:25:14 PST
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
Comment 40 WebKit Review Bot 2013-02-12 06:31:15 PST
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
Comment 41 EFL EWS Bot 2013-02-12 06:42:26 PST
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
Comment 42 Peter Beverloo (cr-android ews) 2013-02-12 06:54:28 PST
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
Comment 43 kov's GTK+ EWS bot 2013-02-12 07:28:13 PST
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
Comment 44 Lamarque V. Souza 2013-02-12 07:40:19 PST
Created attachment 187866 [details]
Patch (EWS only version)

Drop ENABLE_CSS_HIERARCHIES flag.
Comment 45 Build Bot 2013-02-13 20:22:34 PST
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
Comment 46 Lamarque V. Souza 2013-02-14 04:00:16 PST
Created attachment 188309 [details]
Patch (EWS only version)

Save/restore graphics context.
Comment 47 WebKit Review Bot 2013-02-14 05:17:47 PST
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
Comment 48 WebKit Review Bot 2013-02-14 05:22:04 PST
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
Comment 49 WebKit Review Bot 2013-02-14 06:11:06 PST
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
Comment 50 WebKit Review Bot 2013-02-14 06:14:43 PST
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
Comment 51 Build Bot 2013-02-16 01:21:45 PST
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
Comment 52 Lamarque V. Souza 2013-02-25 09:57:00 PST
Created attachment 190078 [details]
Patch (EWS only version)

Fix issue with cr-linux bot.
Comment 53 Build Bot 2013-02-26 07:33:26 PST
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
Comment 54 Benjamin Poulain 2013-03-07 16:27:21 PST
> > 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.
Comment 55 Lamarque V. Souza 2013-03-11 10:12:15 PDT
Created attachment 192492 [details]
Patch

Fix issues reported.
Comment 56 Lamarque V. Souza 2013-03-11 10:16:55 PDT
Created attachment 192493 [details]
Bezier with 1 pixel stroke thickness

Expectations using wavy bezier curve and 1 pixel stroke thickness
Comment 57 Lamarque V. Souza 2013-03-11 10:18:12 PDT
Created attachment 192494 [details]
Bezier with 2 pixels stroke thickness

Expectations using wavy bezier curve and 2 pixels stroke thickness
Comment 58 WebKit Review Bot 2013-03-11 10:18:57 PDT
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.
Comment 59 Lamarque V. Souza 2013-03-11 10:19:13 PDT
Created attachment 192495 [details]
Non-bezier with 1 pixel stroke thickness

Expectations using wavy curve and 1 pixel stroke thickness
Comment 60 Lamarque V. Souza 2013-03-11 10:20:28 PDT
Created attachment 192497 [details]
Non-bezier with 2 pixels stroke thickness

Expectations using wavy curve and 2 pixels stroke thickness
Comment 61 Lamarque V. Souza 2013-03-11 10:28:39 PDT
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.
Comment 62 Lamarque V. Souza 2013-03-12 11:58:05 PDT
Created attachment 192780 [details]
Patch

Use bezier curve and fix small issues.
Comment 63 Lamarque V. Souza 2013-03-13 12:16:22 PDT
Created attachment 192958 [details]
Patch

Make bezier curve symmetric and update changelogs.
Comment 64 Lamarque V. Souza 2013-03-13 12:20:29 PDT
Created attachment 192959 [details]
Bezier with 1 pixel stroke thickness

Expectations using wavy bezier curve and 1 pixel stroke thickness.
Comment 65 Lamarque V. Souza 2013-03-13 12:22:10 PDT
Created attachment 192960 [details]
Bezier with 2 pixels stroke thickness

Expectations using wavy bezier curve and 2 pixels stroke thickness.
Comment 66 Lamarque V. Souza 2013-03-13 12:25:29 PDT
Created attachment 192961 [details]
Patch (EWS only version)

Same patch with css3-text enabled.
Comment 67 Build Bot 2013-03-13 13:11:05 PDT
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
Comment 68 Benjamin Poulain 2013-03-22 16:18:08 PDT
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.
Comment 69 Lamarque V. Souza 2013-03-25 11:52:11 PDT
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.
Comment 70 Lamarque V. Souza 2013-03-25 11:59:55 PDT
Created attachment 194898 [details]
Patch

Fix issues reported, except the curve is too short one.
Comment 71 Lamarque V. Souza 2013-03-25 16:25:55 PDT
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.
Comment 72 Benjamin Poulain 2013-03-27 13:53:50 PDT
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;
Comment 73 Lamarque V. Souza 2013-03-27 15:50:10 PDT
Created attachment 195421 [details]
Patch

Fix remaining issues reported.
Comment 74 Lamarque V. Souza 2013-03-27 15:59:58 PDT
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.
Comment 75 Benjamin Poulain 2013-03-28 12:14:23 PDT
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.
Comment 76 Lamarque V. Souza 2013-03-28 12:40:44 PDT
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.
Comment 77 Lamarque V. Souza 2013-03-28 13:26:47 PDT
Created attachment 195633 [details]
Patch

Patch for landing.
Comment 78 WebKit Review Bot 2013-03-28 15:23:39 PDT
Comment on attachment 195633 [details]
Patch

Clearing flags on attachment: 195633

Committed r147170: <http://trac.webkit.org/changeset/147170>
Comment 79 Bruno Abinader (history only) 2013-03-31 13:57:58 PDT
All reviewed patches have landed. Closing bug.