| Summary: | [Cairo] Text underline is not shadowed when text-shadow is enabled | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Martin Robinson <mrobinson> | ||||||||
| Component: | Layout and Rendering | Assignee: | Martin Robinson <mrobinson> | ||||||||
| Status: | RESOLVED FIXED | ||||||||||
| Severity: | Normal | CC: | alex, jonathon, krit | ||||||||
| Priority: | P3 | Keywords: | Gtk | ||||||||
| Version: | 528+ (Nightly build) | ||||||||||
| Hardware: | PC | ||||||||||
| OS: | Linux | ||||||||||
| Attachments: |
|
||||||||||
|
Description
Martin Robinson
2010-10-21 09:57:29 PDT
Created attachment 71448 [details]
Patch for this issue
Comment on attachment 71448 [details] Patch for this issue 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? (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. *** Bug 19676 has been marked as a duplicate of this bug. *** Created attachment 84137 [details]
Patch with Krit's suggestions and a good deal of cleanup
Comment on attachment 84137 [details] Patch with Krit's suggestions and a good deal of cleanup View in context: https://bugs.webkit.org/attachment.cgi?id=84137&action=review I'd like to see it a second time. > Source/WebCore/platform/graphics/GraphicsContext.h:509 > + Remove this line > Source/WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:326 > + bool isVerticalLine = (point1.x() == point2.x()); avoid the braces > Source/WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:431 > + patternWidth = floor(strokeThickness() / 2.f); floorf, but depends what strokeThickness returns. please check this. > Source/WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:434 > + patternWidth = 3 * floor(strokeThickness() / 2.f); ditto > Source/WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:445 > + distance = floor((piFloat * hRadius) / 2.0); floorf > Source/WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:447 > + distance = floor((piFloat * sqrtf((hRadius * hRadius + vRadius * vRadius) / 2.0)) / 2.0); ditto > Source/WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:448 > + double patternOffset = calculateStrokePatternOffset(distance, patternWidth); if patternOffset is double, you should may use double in general and ignore the last two comments. (In reply to comment #6) > (From update of attachment 84137 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=84137&action=review > I'd like to see it a second time. Thank you kindly for the review. > > Source/WebCore/platform/graphics/GraphicsContext.h:509 > > + > Remove this line Done. > > Source/WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:326 > > + bool isVerticalLine = (point1.x() == point2.x()); > avoid the braces Done. > > Source/WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:431 > > + patternWidth = floor(strokeThickness() / 2.f); > floorf, but depends what strokeThickness returns. please check this. > > Source/WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:434 > > + patternWidth = 3 * floor(strokeThickness() / 2.f); > ditto > > Source/WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:445 > > + distance = floor((piFloat * hRadius) / 2.0); > floorf > > Source/WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:447 > > + distance = floor((piFloat * sqrtf((hRadius * hRadius + vRadius * vRadius) / 2.0)) / 2.0); > ditto Done! > > Source/WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:448 > > + double patternOffset = calculateStrokePatternOffset(distance, patternWidth); > if patternOffset is double, you should may use double in general and ignore the last two comments. I've changed distance to a florat moved the floorf here. Created attachment 91327 [details]
Patch
Comment on attachment 91327 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=91327&action=review > LayoutTests/ChangeLog:12 > + * platform/gtk/fast/text/stroking-decorations-expected.checksum: > + * platform/gtk/fast/text/stroking-decorations-expected.png: At least the checksum is missing. And I can't see the image with the review tool :-(. But that is not your problem. > Source/WebCore/ChangeLog:8 > + Use ShadowContext to enable shadows for text underlines. Also remove quite a bit You mean ContextShadow > Source/WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:443 > + distance = (piFloat * hRadius) / 2.0; 2 or 2.f if you use float. But 2 shouldn't be a problem here. > Source/WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:445 > + distance = (piFloat * sqrtf((hRadius * hRadius + vRadius * vRadius) / 2.0)) / 2.0; Ditto. Committed r85279: <http://trac.webkit.org/changeset/85279> Comment on attachment 91327 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=91327&action=review Thanks for the review! >> LayoutTests/ChangeLog:12 >> + * platform/gtk/fast/text/stroking-decorations-expected.png: > > At least the checksum is missing. And I can't see the image with the review tool :-(. But that is not your problem. There are no longer checksum files, but this line was left by accident in the ChangeLog. I've removed it. >> Source/WebCore/ChangeLog:8 >> + Use ShadowContext to enable shadows for text underlines. Also remove quite a bit > > You mean ContextShadow Fixed. >> Source/WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:443 >> + distance = (piFloat * hRadius) / 2.0; > > 2 or 2.f if you use float. But 2 shouldn't be a problem here. Changed everything to x.f. |