Bug 94110

Created attachment 168106 [details] Patch

Comment on attachment 168106 [details] Patch Attachment 168106 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/14255370

Comment on attachment 168106 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=168106&action=review > Source/WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:305 > +#if ENABLE(CSS3_TEXT_DECORATION) Perhaps we can just always handle the WavyStroke style instead of conditionally on a feature that may or may not be related. > Source/WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:306 > + if (style == WavyStroke) { It seems that WavyStroke doesn't exist yet, so I guess this patch still depends on some future patch. > Source/WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:308 > + cairo_set_line_join(context, CAIRO_LINE_JOIN_BEVEL); // set line join to BEVEL (more suitable for 'wavy' than MITTER or ROUND) You comment doesn't need to repeat in words what the code does, it just needs to explain why. This can just say: // A bevelled line join is more suitable for wavy than miter or round. Note that comments in WebKit should begin with a capital letter and end with a period. > Source/WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:309 > + const short step = strokeThickness; // up and down as strokeThickness (or left/right if line is vertical) Why const short? strokeThickness is an int, so you might be truncating here. > Source/WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:318 > + if (point1OnPixelBoundaries.x() == point2OnPixelBoundaries.x()) { > + float y = point1OnPixelBoundaries.y() + step + 1; > + while (y <= point2OnPixelBoundaries.y() - 1) { > + signal = ~signal + 1; // alternates between +1 and -1 > + cairo_line_to(context, point1OnPixelBoundaries.x() + signal * step, y); > + y += step + 1; > + }; > + } else { There are a few things that could be improved here: 1. The code is a bit hard to follow and repeats the meat of the loop twice. 2. Instead of reusing isVerticalLine from above it does the check again. 3. It starts and stops one pixel from the starting and ending point and there's no indication why. 4. The code assumes that p2.(x,y) > p1.(x,y). I'm not sure if you can make this assumption here. > Source/WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:320 > + while (x <= point2OnPixelBoundaries.x() -1) { The spacing is weird here at -1 > Source/WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:326 > + cairo_set_line_join(context, savedLineJoin); // restore line join The comment is unnecessary here. > Source/WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:331 > if (patternWidth) { > // Do a rect fill of our endpoints. This ensures we always have the > // appearance of being a border. We then draw the actual dotted/dashed line. Is there some reason this code does not apply for wavy strokes?

Martin Robinson: Thank you for the review! That's very helpful! Actually, this bug depends on the following bug for QT: https://bugs.webkit.org/show_bug.cgi?id=93507 Why that is not listed on is, I couldn't find how to make a dependency. :) I'll update the patch by your review soon. Thank you.

Comment on attachment 168106 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=168106&action=review Hi all, Just sharing with you my informal review. I'm also looking forward to get patch from bug 93507 (Qt port) and the one I'm about to upload for Skia port, so we can finally have pixel results for text-decoration-style layout tests. >> Source/WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:305 >> +#if ENABLE(CSS3_TEXT_DECORATION) > > Perhaps we can just always handle the WavyStroke style instead of conditionally on a feature that may or may not be related. This namespace has recently been renamed to CSS3_TEXT. >> Source/WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:306 >> + if (style == WavyStroke) { > > It seems that WavyStroke doesn't exist yet, so I guess this patch still depends on some future patch. The WavyStroke exists (see http://trac.webkit.org/browser/trunk/Source/WebCore/platform/graphics/GraphicsContext.h#L143 for details), as it makes part of the CSS3 "text-decoration-style" property, however it's safeguarded by the CSS3_TEXT namespace, which is only enabled by default on EFL build for now. > Source/WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:314 > + signal = ~signal + 1; // alternates between +1 and -1 As suggested on bug 93507 as well, for readibility issues this could be changed to "signal = -signal;". > Source/WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:321 > + signal = ~signal + 1; // alternates between +1 and -1 Ditto. > Source/WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:328 > +#endif // CSS3_TEXT_DECORATION s/CSS3_TEXT_DECORATION/CSS3_TEXT/ > Source/WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:350 > +#if ENABLE(CSS3_TEXT_DECORATION) Ditto. > Source/WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:352 > +#endif // CSS3_TEXT_DECORATION Ditto.

Created attachment 171623 [details] Patch

(In reply to comment #6) > Created an attachment (id=171623) [details] > Patch The patch looks great and works fine on my EFL layout test run. As we discussed through IRC, we could later merge this function together with drawErrorUnderline on a separate patch. There is also a possibility to use pixmaps (as Chromium does and Qt does internally on QPainter) instead of generating the wavy lines, these were commented by Simon on bug 93507.

Created attachment 174059 [details] Patch

Looking at the results of this output they seem similar to what DrawErrorUnderline.h produces. Bruno mentioned on the mailing list the possibility of re-using that code for all platforms. What was the result of that investigation?

(In reply to comment #9) > Looking at the results of this output they seem similar to what DrawErrorUnderline.h produces. Bruno mentioned on the mailing list the possibility of re-using that code for all platforms. What was the result of that investigation? drawErrorUnderline does not draw vertical lines and is more complex than this implementation, so Bruno and me thought in doing the opposite: remove drawErrorUnderline and use drawLine with wavyStroke instead. That is not platform agnostic but helps removing duplicated code. There is an implementation for the Qt platform in https://bugs.webkit.org/show_bug.cgi?id=93507#c27

Okay. That seems like a reasonable approach.

I think committing this patch first, and then removing duplicate code on another bug seems better. Martin Robinson, could you please review?

Created attachment 175068 [details] mozilla's implementation (zoomlevel 1.0)

Created attachment 175069 [details] mozilla's implementation (larger zoomlevel)

Created attachment 175084 [details] this implementation (zoom 1.0) The Mozilla implementation has a wave with a larger amplitude and a small frequency. The amplitude or frequency of wavy line is not defined on a spec. If this implementation should be modified like Mozilla, it's same issue with other ports. Bruno Abinader and Lamarque V. Souza, how do you think about that?

(In reply to comment #15) > Created an attachment (id=175084) [details] > this implementation (zoom 1.0) > > The Mozilla implementation has a wave with a larger amplitude and a small frequency. > The amplitude or frequency of wavy line is not defined on a spec. > If this implementation should be modified like Mozilla, it's same issue with other ports. > Bruno Abinader and Lamarque V. Souza, how do you think about that? I think we should follow Mozilla's implementation. As an browser user I would like to see pages looking the same in any browser.

Created attachment 177092 [details] Patch

Created attachment 177155 [details] screenshot (zoom 1.0, 1.5, 2.0, 3.0) screenshot from above patch with zoom level 1.0, 1.5, 2.0, 3.0.

Hi KyungTae, sorry for not replying earlier (got back to work today), yes, the spec gives us space for interpretation on this aspect, and following the Mozilla implementation sounds a good idea for me as well. From the screenshots, it looks very stylish :) Good work!

(In reply to comment #17) > Created an attachment (id=177092) [details] > Patch I am porting your changes to the Qt patch [1] and have some small doubts. Why the vertical implementation contains "float y = y1 + step + 1;" and the horizontal contains "float x = x1 + step;". I guess the vertical one does not need the "+ 1", right? Another question regards the condition in the while loops, for the horizontal one it is "(x <= x2 - 1)", why the "-1" here? In Qt we subtract one pixel from the coordinates because Qt and WebKit threat coordinates slightly different. Does the same happen in Cairo? [1] https://bugs.webkit.org/show_bug.cgi?id=93507

Fixed in bug 92868. Closing bug.

Comment on attachment 177092 [details] Patch Clearing the review flag on the last patch.