Bug 93507 – [Qt] Add support for text decoration "wavy" style

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Qt

[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 on Qt due to previous usage on "border-style" property:
http://www.w3.org/TR/css3-background/#the-border-style

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

[Lamarque V. Souza]

Created attachment 167591 [details]
Patch

Proposed patch

[Bruno Abinader (history only)]

Created attachment 167642 [details]
Patch (EWS run only)

[Build Bot]

Comment on attachment 167642 [details]
Patch (EWS run only)

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

[Build Bot]

Comment on attachment 167642 [details]
Patch (EWS run only)

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

[Lamarque V. Souza]

Created attachment 168229 [details]
Patch

Proposed patch updated

[Lamarque V. Souza]

Created attachment 168245 [details]
Patch (EWS run only)

[Build Bot]

Comment on attachment 168245 [details]
Patch (EWS run only)

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

[Lamarque V. Souza]

Created attachment 168263 [details]
Patch (EWS run only)

Attempt to fix EWS build errors.

[Build Bot]

Comment on attachment 168263 [details]
Patch (EWS run only)

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

[Build Bot]

Comment on attachment 168263 [details]
Patch (EWS run only)

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

[Simon Hausmann]

Comment on attachment 168245 [details]
Patch (EWS run only)

Clearning review flag on patch marked obsolete

[Bruno Abinader (history only)]

Thank you Simon for taking a look at the attachment flags. These are correct now (EWS-related patches are not supposed to be reviewed neither commited, only regular patches).

[Bruno Abinader (history only)]

CC'in Julien on this one as well.

[Lamarque V. Souza]

Created attachment 170361 [details]
Patch

Update patch for CSSGrammar.y change

[Rafael Brandao]

Comment on attachment 170361 [details]
Patch

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

> Source/WebCore/platform/graphics/qt/GraphicsContextQt.cpp:491
> +                y += step + 1;

Is it possible that step <= -1? If so, this would lead to an infinite loop, most probably.

[Michael Brüning]

Comment on attachment 170361 [details]
Patch

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

> Source/WebCore/platform/graphics/qt/GraphicsContextQt.cpp:489
> +                signal = ~signal + 1; // alternates between +1 and -1

Is there any reason (e.g. performance) to use the two complement by hand here instead of setting using "signal = -signal;" ?

> Source/WebCore/platform/graphics/qt/GraphicsContextQt.cpp:502
> +                signal = ~signal + 1; // alternates between +1 and -1

Same question as above.

[Lamarque V. Souza]

(In reply to comment #15)
> (From update of attachment 170361 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=170361&action=review
> 
> > Source/WebCore/platform/graphics/qt/GraphicsContextQt.cpp:491
> > +                y += step + 1;
> 
> Is it possible that step <= -1? If so, this would lead to an infinite loop, most probably.

step == strokeThickness(). I am not entirely sure but I think strokeThickness() is not supposed to return negative values.

[Lamarque V. Souza]

(In reply to comment #16)
> (From update of attachment 170361 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=170361&action=review
> 
> > Source/WebCore/platform/graphics/qt/GraphicsContextQt.cpp:489
> > +                signal = ~signal + 1; // alternates between +1 and -1
> 
> Is there any reason (e.g. performance) to use the two complement by hand here instead of setting using "signal = -signal;" ?

No particular reason. I can use use your suggestion.

[Lamarque V. Souza]

Created attachment 171291 [details]
Patch

Update patch to prevent step from being negative and use 'signal = -signal' instead of 'signal = ~signal + 1' (the assembly codes generated by g++ for both are equal anyway)

[Rafael Brandao]

Comment on attachment 171291 [details]
Patch

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

This is just an informal review. I've left a few comments.

> Source/WebCore/ChangeLog:9
> +        is three pixels high and four pixels long.

Shouldn't we get some new passing tests with this patch? You should unskip the wavy related patches in this patch, so you can detect whether the approach is ok or not.

> Source/WebCore/platform/graphics/qt/GraphicsContextQt.cpp:479
> +        const unsigned short step = width; // up and down as strokeThickness() (or left/right if line is vertical)

Wait, is width a float? Is it ok to cast it to an unsigned short? Right now, instead of detecting the wrong case where we get a negative value for width, we're going to use a wrong value, which I believe to be worse. I was reading the spec and it seems negative values are not valid, so maybe you shouldn't worry about this at all. Just double check if you should be using a short rather than a float. And check again the following comment. On WebKit, comments should be full english and meaningful sentence, with a dot in the end of the sentences.

[Lamarque V. Souza]

(In reply to comment #20)
> (From update of attachment 171291 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=171291&action=review
> 
> This is just an informal review. I've left a few comments.
> 
> > Source/WebCore/ChangeLog:9
> > +        is three pixels high and four pixels long.
> 
> Shouldn't we get some new passing tests with this patch? You should unskip the wavy related patches in this patch, so you can detect whether the approach is ok or not.

Hmmm that comment is outdated, the wave has variable height now. This patch depends on https://bugs.webkit.org/show_bug.cgi?id=93863, which used to include a pixel test that among other things tested for wavy style (even though it does not implement it). When #93863 was accepted the test was removed, I can readd it if that is what you want. I do not understand what you mean by "wavy related patches in this patch".
 
> > Source/WebCore/platform/graphics/qt/GraphicsContextQt.cpp:479
> > +        const unsigned short step = width; // up and down as strokeThickness() (or left/right if line is vertical)
> 
> Wait, is width a float? Is it ok to cast it to an unsigned short? Right now, instead of detecting the wrong case where we get a negative value for width, we're going to use a wrong value, which I believe to be worse. I was reading the spec and it seems negative values are not valid, so maybe you shouldn't worry about this at all. Just double check if you should be using a short rather than a float. And check again the following comment. On WebKit, comments should be full english and meaningful sentence, with a dot in the end of the sentences.

Yes, width is a float. step used to be hardcoded to 1, so I used a short in my first implementation. Now that it is not hardcoded maybe it is better changing it to be a float as well.

[Rafael Brandao]

(In reply to comment #21)
> (In reply to comment #20)
> > (From update of attachment 171291 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=171291&action=review
> > 
> > This is just an informal review. I've left a few comments.
> > 
> > > Source/WebCore/ChangeLog:9
> > > +        is three pixels high and four pixels long.
> > 
> > Shouldn't we get some new passing tests with this patch? You should unskip the wavy related patches in this patch, so you can detect whether the approach is ok or not.
> 
> Hmmm that comment is outdated, the wave has variable height now. This patch depends on https://bugs.webkit.org/show_bug.cgi?id=93863, which used to include a pixel test that among other things tested for wavy style (even though it does not implement it). When #93863 was accepted the test was removed, I can readd it if that is what you want. I do not understand what you mean by "wavy related patches in this patch".

Sorry it was a typo. I was expecting that this new feature (wavy style) was somehow tested via layout tests, so it would be nice to have those wavy-related tests unskipped for Qt when your fixes are done. :-)

[Lamarque V. Souza]

Created attachment 171305 [details]
Patch

Store step as float instead of unsighed short, also update comments.

[Lamarque V. Souza]

Created attachment 171925 [details]
Patch

Fix some issues

[Antonio Gomes]

Comment on attachment 171925 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        Add support for text decoration "wavy" style for Qt platform.

should a test start to pass after your fix?

[Lamarque V. Souza]

(In reply to comment #25)
> (From update of attachment 171925 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=171925&action=review
> 
> > Source/WebCore/ChangeLog:8
> > +        Add support for text decoration "wavy" style for Qt platform.
> 
> should a test start to pass after your fix?

Bug https://bugs.webkit.org/show_bug.cgi?id=100546 is about that.

As I wrote in comment #21 https://bugs.webkit.org/show_bug.cgi?id=93863 used to include a pixel test that among other things tested wavy decoration, but the test was removed before the patch in https://bugs.webkit.org/show_bug.cgi?id=93863 was commited. So no, currently there is no test for this feature.

[Lamarque V. Souza]

Created attachment 172637 [details]
Replace drawErrorUnderline() with drawLine() using wavy stroke

This patch depends on patch 171925 and replaces drawErrorUnderline() with drawLine() using wavy stroke. The objective is to reduce duplicated code.

[Simon Hausmann]

Comment on attachment 171925 [details]
Patch

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

> Source/WebCore/platform/graphics/qt/GraphicsContextQt.cpp:534
> +        p->strokePath(path, pen);

I'm sceptical about this approach from a performance point of view. It's rather expensive to stroke such a path and it's unnecessary if you consider that it contains a lot of repeated pixels that could be drawn more efficiently. We used to do the same in Qt and it become a performance bottleneck in text-heavy use-cases (such as Qt Creator), to the extend that we optimized it. Instead of drawing the same segment of a path again and again we used a pixmap:

https://qt.gitorious.org/qt/qt/commit/a78e7a6e7a7d5725467d0538bf8e0ea50c2506cc

Would you consider a similar approach here?

[Lamarque V. Souza]

(In reply to comment #28)
> (From update of attachment 171925 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=171925&action=review
> 
> > Source/WebCore/platform/graphics/qt/GraphicsContextQt.cpp:534
> > +        p->strokePath(path, pen);
> 
> I'm sceptical about this approach from a performance point of view. It's rather expensive to stroke such a path and it's unnecessary if you consider that it contains a lot of repeated pixels that could be drawn more efficiently. We used to do the same in Qt and it become a performance bottleneck in text-heavy use-cases (such as Qt Creator), to the extend that we optimized it. Instead of drawing the same segment of a path again and again we used a pixmap:
> 
> https://qt.gitorious.org/qt/qt/commit/a78e7a6e7a7d5725467d0538bf8e0ea50c2506cc
> 
> Would you consider a similar approach here?

Ok, I will try to implement something using the patch above.

[Lamarque V. Souza]

Created attachment 173042 [details]
Implement wavy strok using QPixmap

I have reimplemented wavy stroke using QPixmap as suggested, but it does not work properly sometimes. For testing I changed the style used in matches of the search pattern to use wavy stroke. In images [1] and [2] the word "bugs" appears with wavy stroke style. The wavy pixmap is not correct in the first word "bugs" in [1] while it is for the second word. If I resize the QtTestBrowser window then the second one becomes wrong too (image [2]). Although it looks like the first word "bugs" in [2] is now correct it is not, it just improved a little but in the real window I can still see some dots that should not be there.

I have not been able to fix this, does anyone has any suggestion?

OBS: this patch is not for review because it contains changes that should not go to webkit, it is just for testing.

[1] http://imageshack.us/photo/my-images/90/wavypixmap1.png/
[2] http://imageshack.us/photo/my-images/145/wavypixmap1resized.png/

[Lamarque V. Souza]

Created attachment 177519 [details]
Patch

Make wave format similar to firefox's

[Lamarque V. Souza]

Created attachment 180686 [details]
Wavy stroke using QPixmapCache

Implementation using QPixmapCache for optimization

[Lamarque V. Souza]

(In reply to comment #28)
> (From update of attachment 171925 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=171925&action=review
> 
> > Source/WebCore/platform/graphics/qt/GraphicsContextQt.cpp:534
> > +        p->strokePath(path, pen);
> 
> I'm sceptical about this approach from a performance point of view. It's rather expensive to stroke such a path and it's unnecessary if you consider that it contains a lot of repeated pixels that could be drawn more efficiently. We used to do the same in Qt and it become a performance bottleneck in text-heavy use-cases (such as Qt Creator), to the extend that we optimized it. Instead of drawing the same segment of a path again and again we used a pixmap:
> 
> https://qt.gitorious.org/qt/qt/commit/a78e7a6e7a7d5725467d0538bf8e0ea50c2506cc
> 
> Would you consider a similar approach here?

Sure. Can you test the patch in comment #32? It is called "Wavy stroke using QPixmapCache".

[Lamarque V. Souza]

Created attachment 181600 [details]
Wavy stroke using QPixmapCache

Fix some issues in the implementation

[Lamarque V. Souza]

Created attachment 182590 [details]
Wavy stroke using QPixmapCache (EWS version)

Path with css3_text enabled by default

[Build Bot]

Comment on attachment 182590 [details]
Wavy stroke using QPixmapCache (EWS version)

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

[Lamarque V. Souza]

The read in the mac buildbot is because of bug https://bugs.webkit.org/show_bug.cgi?id=106819 "Mac buildbot failing to compile if css3-text feature flag is enabled", which is not related to this patch, it can be ignore until #106819 is fixed. The patch passes all other buildbots.

[Build Bot]

Comment on attachment 182590 [details]
Wavy stroke using QPixmapCache (EWS version)

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

[Lamarque V. Souza]

Created attachment 183191 [details]
How it looks like

Adding screenshot of what it looks like (look at the last line).

[Lamarque V. Souza]

Created attachment 184577 [details]
Wavy stroke using QPixmapCache

Some adjustments in wave format.

[Lamarque V. Souza]

Created attachment 184578 [details]
Screenshot

[Lamarque V. Souza]

Created attachment 185914 [details]
Patch

Proposed patch

[Simon Hausmann]

Comment on attachment 185914 [details]
Patch

I just wish there was a way to get a similar look without always stroking the path. It's going to be very slow :(

[Simon Hausmann]

Comment on attachment 172637 [details]
Replace drawErrorUnderline() with drawLine() using wavy stroke

This change is missing a ChangeLog entry and it also should explain as to why this is needed. At least from a performance point of view this is a regression, isn't it?

[WebKit Review Bot]

Comment on attachment 185914 [details]
Patch

Clearing flags on attachment: 185914

Committed r141542: <http://trac.webkit.org/changeset/141542>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Lamarque V. Souza]

(In reply to comment #44)
> (From update of attachment 172637 [details])
> This change is missing a ChangeLog entry and it also should explain as to why this is needed. At least from a performance point of view this is a regression, isn't it?

The reason for this change is removing duplicated code, but since the performance would not be that good then I think it should not land until the performance problem is fixed.

[Lamarque V. Souza]

(In reply to comment #43)
> (From update of attachment 185914 [details])
> I just wish there was a way to get a similar look without always stroking the path. It's going to be very slow :(

Is there any performance gain in using QPainter::drawPath() instead of QPainter::strokePath()? Both work.

[Benjamin Poulain]

Instead of defining everything in terms of StrokeStyle, wouldn't it be possible to use existing rendering primitives and share the code?