[Webkit-unassigned] [Bug 94110] [cairo] Add support for text decoration "wavy" style

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=94110


Martin Robinson <mrobinson at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #168106|review?                     |review-
               Flag|                            |




--- Comment #3 from Martin Robinson <mrobinson at webkit.org>  2012-10-11 20:13:32 PST ---
(From update of attachment 168106)
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?

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.