[Webkit-unassigned] [Bug 48074] [Cairo] Text underline is not shadowed when text-shadow is enabled

[bugzilla-daemon at webkit.org]

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





--- Comment #3 from Martin Robinson <mrobinson at webkit.org>  2010-11-24 12:50:20 PST ---
(In reply to comment #2)

Thanks for the review!

> (From update of attachment 71448 [details])
> 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?

This is from the original code, as far as I can tell (patWidth) is an int. I hoped to not change the behavior of this portion at all, but I can address that in a followup patch, if necesarry.

-        int coverage = distance-remainder;
-        int numSegments = coverage/patWidth;
-

> 2.f
> 2.f

The style guide is actually inconclusive on this and in the one example that is shows, it shows the style I used in the patch: "Floating point literals" at http://webkit.org/coding/coding-style.html. Maybe this should be decided on the list, if there is a preference one way or another?

> > WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:341
> > +        double patternWidthAsDouble = patternWidth;
> You already defined patternWidth as double above :-P

Good point! I've changed it to an int.

> > 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?

Unfortunately, drawLineOnCairoContext must call cairo_set_dash and there doesn't appear to be a way to fetch from the context and restore manually. I'm in agreement though that, in general, we shouldn't rely on the cairo context to store the state. We can avoid most needless save/restore pairs that way. So if GraphicsContext::setPlatformStrokeStyle didn't call cairo_set_dash, but just marked it as dirty, we could update the context lazily.

> > WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:421
> > +        patternWidth = strokeThickness() / 2.0;
> patternWidth is an integer, please use floorf or ceilf

The original code used patWidth = static_cast<int>(width / 2);, so I guess floorf is the right call here?

> > 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>

I will change all references to M_PI to piFloat.

> > 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?

The original code used an int here and I was worrying about causing a regresson by making too many changes. I can modify this to use double everywhere though. Doing this in general, might remove the need to call floorf/ceilf so much. I'm just nervous about adjusting the code too much, as it appears sensitive.

> > 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.

Fixed! Thank you.

> > WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:681
> > +    cairo_restore(cairoContext);
> Again, is save and restore necessary here?

Answered above.

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