Bug 46475

Created attachment 68726 [details] Patch for this issue

Comment on attachment 68726 [details] Patch for this issue Clearing the flag on this one, because I noticed a regression with SVG text.

Created attachment 68802 [details] Patch fixing SVG text regression

Isn't it better to move the tile code to ContextShadow first?

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

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?

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

Created attachment 70017 [details] Patch for this issue using drawRectShadow

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.

Created attachment 70624 [details] Patch with Dirk's suggestions

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

Comment on attachment 70624 [details] Patch with Dirk's suggestions Great patch! r=me

Committed r69681: <http://trac.webkit.org/changeset/69681>

http://trac.webkit.org/changeset/69681 might have broken GTK Linux 64-bit Debug