[webkit-reviews] review denied: [Bug 48074] [Cairo] Text underline is not shadowed when text-shadow is enabled : [Attachment 71448] Patch for this issue

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Tue Nov 23 10:31:42 PST 2010


Dirk Schulze <krit at webkit.org> has denied Martin Robinson
<mrobinson at webkit.org>'s request for review:
Bug 48074: [Cairo] Text underline is not shadowed when text-shadow is enabled
https://bugs.webkit.org/show_bug.cgi?id=48074

Attachment 71448: Patch for this issue
https://bugs.webkit.org/attachment.cgi?id=71448&action=review

------- Additional Comments from Dirk Schulze <krit at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=71448&action=review

> WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:281
> +    int numSegments = (distance - remainder) / patternWidth;

You really want to use a integer division?

> WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:294
> +	       return (patternWidth - remainder) + (remainder / 2.0);
> +	   return patternWidth / 2.0;

2.f

> WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:299
> +	   return (patternWidth - remainder) / 2.0;

2.f

> WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:341
> +	   double patternWidthAsDouble = patternWidth;

You already defined patternWidth as double above :-P

> WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:359
> +    cairo_save(cairoContext);
> +    drawLineOnCairoContext(this, cairoContext, point1, point2);
> +    cairo_restore(cairoContext);

Why do you use cairo_save? What gets restored after drawLineOnCairoContext()?
Maybe it's faster to do it manually?

> WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:421
> +	   patternWidth = strokeThickness() / 2.0;

patternWidth is an integer, please use floorf or ceilf

> WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:424
> +	   patternWidth = 3.0 * strokeThickness() / 2.0;

Ditto.

> WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:435
>	       distance = static_cast<int>((M_PI * hRadius) / 2.0);

Can you use floor(f) or ceil(f) here please? And 2.f should be ok. Don't use
M_PI, take piDouble or piFloat from <wtf/MathExtras.h>

> WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:437
>	       distance = static_cast<int>((M_PI * sqrtf((hRadius * hRadius +
vRadius * vRadius) / 2.0)) / 2.0);

Ditto.

> WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:440
> +	   float patternOffset = calculateStrokePatternOffset(distance,
patternWidth);
> +	   double patternWidthAsDouble = patternWidth;

why don't you use double directly and avoid copying the data?

> WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:441
> +	   cairo_set_dash(cr, &patternWidthAsDouble, 1, patternOffset);

hm, does cairo_set_dash really use a double and a float? I thought cairo always
uses double.

> WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:681
> +    cairo_restore(cairoContext);

Again, is save and restore necessary here?


More information about the webkit-reviews mailing list