[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