Bug 46475

Summary: [Cairo] Activate ContextShadow in all places where shadows are drawn
Product: WebKit Reporter: Martin Robinson <mrobinson>
Component: WebKitGTKAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, alex, eric, krit, webkit.review.bot
Priority: P3    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Linux   
Bug Depends on: 45902, 46918    
Bug Blocks:    
Attachments:
Description Flags
Patch for this issue
none
Patch fixing SVG text regression
none
Patch for this issue using drawRectShadow
none
Patch with Dirk's suggestions krit: review+

Description Martin Robinson 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.
Comment 1 Martin Robinson 2010-09-24 12:12:02 PDT
Created attachment 68726 [details]
Patch for this issue
Comment 2 Martin Robinson 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.
Comment 3 Martin Robinson 2010-09-24 19:51:55 PDT
Created attachment 68802 [details]
Patch fixing SVG text regression
Comment 4 Dirk Schulze 2010-09-24 23:00:03 PDT
Isn't it better to move the tile code to ContextShadow first?
Comment 5 Martin Robinson 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.
Comment 6 Dirk Schulze 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?
Comment 7 Martin Robinson 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.
Comment 8 Martin Robinson 2010-10-06 17:16:30 PDT
Created attachment 70017 [details]
Patch for this issue using drawRectShadow
Comment 9 Dirk Schulze 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.
Comment 10 Martin Robinson 2010-10-13 10:35:12 PDT
Created attachment 70624 [details]
Patch with Dirk's suggestions
Comment 11 Martin Robinson 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.
Comment 12 Dirk Schulze 2010-10-13 11:58:59 PDT
Comment on attachment 70624 [details]
Patch with Dirk's suggestions

Great patch! r=me
Comment 13 Martin Robinson 2010-10-13 12:18:33 PDT
Committed r69681: <http://trac.webkit.org/changeset/69681>
Comment 14 WebKit Review Bot 2010-10-13 13:14:57 PDT
http://trac.webkit.org/changeset/69681 might have broken GTK Linux 64-bit Debug