[webkit-reviews] review denied: [Bug 94110] [cairo] Add support for text decoration "wavy" style : [Attachment 168106] Patch

[bugzilla-daemon at webkit.org]

Martin Robinson <mrobinson at webkit.org> has denied KyungTae Kim
<ktf.kim at samsung.com>'s request for review:
Bug 94110: [cairo] Add support for text decoration "wavy" style
https://bugs.webkit.org/show_bug.cgi?id=94110

Attachment 168106: Patch
https://bugs.webkit.org/attachment.cgi?id=168106&action=review

------- Additional Comments from Martin Robinson <mrobinson at webkit.org>
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?