RESOLVED FIXED 46475
[Cairo] Activate ContextShadow in all places where shadows are drawn 
https://bugs.webkit.org/show_bug.cgi?id=46475
Summary [Cairo] Activate ContextShadow in all places where shadows are drawn
Martin Robinson
Reported 2010-09-24 09:52:27 PDT
Now that the ContextShadow implementation also works for Cairo, we should turn it on wherever we draw shadows and remove the old shadow code. This includes removing the shadow tiling optimization. I'm coordinating with Alex to re-implement it for ContextShadow. He has informed me that the patch for that will be ready soon.
Attachments
Patch for this issue (990.51 KB, patch)
2010-09-24 12:12 PDT, Martin Robinson 		no flags
DetailsFormatted DiffDiff
Patch fixing SVG text regression (1.04 MB, patch)
2010-09-24 19:51 PDT, Martin Robinson 		no flags
DetailsFormatted DiffDiff
Patch for this issue using drawRectShadow (1.29 MB, patch)
2010-10-06 17:16 PDT, Martin Robinson 		no flags
DetailsFormatted DiffDiff
Patch with Dirk's suggestions (1.26 MB, patch)
2010-10-13 10:35 PDT, Martin Robinson 		krit: review+
DetailsFormatted DiffDiff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.
Martin Robinson
Comment 1 2010-09-24 12:12:02 PDT
Created attachment 68726 [details] Patch for this issue
Martin Robinson
Comment 2 2010-09-24 13:33:23 PDT
Comment on attachment 68726 [details] Patch for this issue Clearing the flag on this one, because I noticed a regression with SVG text.
Martin Robinson
Comment 3 2010-09-24 19:51:55 PDT
Created attachment 68802 [details] Patch fixing SVG text regression
Dirk Schulze
Comment 4 2010-09-24 23:00:03 PDT
Isn't it better to move the tile code to ContextShadow first?
Martin Robinson
Comment 5 2010-09-25 10:30:47 PDT
(In reply to comment #4) > Isn't it better to move the tile code to ContextShadow first? This will definitely reduce code churn. I'll clear the flags and wait for Alex to finish his patch.
Dirk Schulze
Comment 6 2010-10-06 10:31:02 PDT
Comment on attachment 68802 [details] Patch fixing SVG text regression View in context: https://bugs.webkit.org/attachment.cgi?id=68802&action=review LayoutTests/platform/gtk/fast/text/shadow-no-blur looks a bit strange. Is this really a progression? Of course you have to update the patch. :-) > WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:160 > +enum PathDrawingStyle { Fill, Stroke }; > +static inline void drawPathShadow(GraphicsContext* context, GraphicsContextPrivate* gcp, PathDrawingStyle drawingStyle) I would add a newline. Doesn't an enum for fill and stroke exists some how?
Martin Robinson
Comment 7 2010-10-06 10:38:13 PDT
(In reply to comment #6) Thanks for the comments! > LayoutTests/platform/gtk/fast/text/shadow-no-blur looks a bit strange. Is this really a progression? Yeah, that one is really odd. :) I think when I rebase this patch the text will show up properly there (since I've made some font fixes on GTK+). It's hard to tell right now, but previously the text shadow was clipped incorrectly and this patch fixes it. > > WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:160 > > +enum PathDrawingStyle { Fill, Stroke }; > > +static inline void drawPathShadow(GraphicsContext* context, GraphicsContextPrivate* gcp, PathDrawingStyle drawingStyle) > > I would add a newline. Doesn't an enum for fill and stroke exists some how? I'll see if I can find something.
Martin Robinson
Comment 8 2010-10-06 17:16:30 PDT
Created attachment 70017 [details] Patch for this issue using drawRectShadow
Dirk Schulze
Comment 9 2010-10-13 02:21:43 PDT
Comment on attachment 70017 [details] Patch for this issue using drawRectShadow View in context: https://bugs.webkit.org/attachment.cgi?id=70017&action=review Patch looks great. Please update the patch to trunk and fix the style issues. > WebCore/platform/graphics/cairo/FontCairo.cpp:51 > + cairo_matrix_t mat = {1, 0, -tanf(SYNTHETIC_OBLIQUE_ANGLE * acosf(0) / 90), 1, point.x(), point.y()}; Can you remove SYNTHETIC_OBLIQUE_ANGLE and replace it with a static const for SYNTHETIC_OBLIQUE_ANGLE * acosf(0) / 90 in WebKit style? Something like: static const float gSyntheticObliqueSkew = 14 * acosf(0) / 90; and move it after the header includes? > WebCore/platform/graphics/cairo/FontCairo.cpp:55 > + } else { > + cairo_translate(context, point.x(), point.y()); > + } No braces for 'else' here. > WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:157 > double x0, x1, y0, y1; Please use multiple lines and a initialization for the values. > WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:863 > + } else { > + m_data->shadow = ContextShadow(color, blur, FloatSize(size.width(), size.height())); > } no braces for 'else' here. > WebCore/platform/graphics/cairo/GraphicsContextPlatformPrivateCairo.h:106 > + bool hasShadow() const > + { > + return shadow.m_type != ContextShadow::NoShadow; > + } You can use one line, if a function just includes a one liner.
Martin Robinson
Comment 10 2010-10-13 10:35:12 PDT
Created attachment 70624 [details] Patch with Dirk's suggestions
Martin Robinson
Comment 11 2010-10-13 10:35:43 PDT
(In reply to comment #9) > (From update of attachment 70017 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=70017&action=review > > Patch looks great. Please update the patch to trunk and fix the style issues. Thanks for the review. I've uploaded a new patch with your suggestions.
Dirk Schulze
Comment 12 2010-10-13 11:58:59 PDT
Comment on attachment 70624 [details] Patch with Dirk's suggestions Great patch! r=me
Martin Robinson
Comment 13 2010-10-13 12:18:33 PDT
Committed r69681: <http://trac.webkit.org/changeset/69681>
WebKit Review Bot
Comment 14 2010-10-13 13:14:57 PDT
http://trac.webkit.org/changeset/69681 might have broken GTK Linux 64-bit Debug
Note You need to log in before you can comment on or make changes to this bug.