Bug 46475 - [Cairo] Activate ContextShadow in all places where shadows are drawn
Summary: [Cairo] Activate ContextShadow in all places where shadows are drawn
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P3 Normal
Assignee: Nobody
URL:
Keywords:
Depends on: 45902 46918
Blocks:
  Show dependency treegraph
 
Reported: 2010-09-24 09:52 PDT by Martin Robinson
Modified: 2010-10-13 13:14 PDT (History)
5 users (show)

See Also:


Attachments
Patch for this issue (990.51 KB, patch)
2010-09-24 12:12 PDT, Martin Robinson
no flags Details | Formatted Diff | Diff
Patch fixing SVG text regression (1.04 MB, patch)
2010-09-24 19:51 PDT, Martin Robinson
no flags Details | Formatted Diff | Diff
Patch for this issue using drawRectShadow (1.29 MB, patch)
2010-10-06 17:16 PDT, Martin Robinson
no flags Details | Formatted Diff | Diff
Patch with Dirk's suggestions (1.26 MB, patch)
2010-10-13 10:35 PDT, Martin Robinson
krit: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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