Bug 119078

Summary: [GTK] [EFL] Enable tiled shadow blur for the inset shadows.
Product: WebKit Reporter: Alejandro G. Castro <alex>
Component: WebKitGTKAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, d-r, mrobinson
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Proposed patch
none
Proposed patch mrobinson: review+

Description Alejandro G. Castro 2013-07-25 02:22:53 PDT
We are not currently the tiled rendering in the inset shadow blur for the ports using cairo backend.
Comment 1 Alejandro G. Castro 2013-07-25 02:29:23 PDT
Created attachment 207444 [details]
Proposed patch
Comment 2 Martin Robinson 2013-07-26 14:26:03 PDT
Comment on attachment 207444 [details]
Proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=207444&action=review

Nice. Just a couple suggestions before landing.

> Source/WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:1054
> +    if (!roundedHoleRect.radii().isZero())
> +        path.addRoundedRect(roundedHoleRect);
> +    else
> +        path.addRect(roundedHoleRect.rect());

I wonder if this optimization should go into Path.cpp eventually?

> Source/WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:1061
> +    setFillRule(RULE_EVENODD);
> +    setFillColor(color, colorSpace);

I think that instead of setting the fill rule and color for the GraphicsContext you should just set them for the Cairo context. That way you can use cairo_save/cairo_restore, instead of oldFillRule and oldFillColor.

> Source/WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:1065
> +        ShadowBlur& shadow = platformContext()->shadowBlur();
> +        shadow.drawInsetShadow(this, rect, roundedHoleRect.rect(), roundedHoleRect.radii());

I think you can avoid the temporary and move this code to the very start of the method after the early return.
Comment 3 Alejandro G. Castro 2013-08-01 05:17:31 PDT
(In reply to comment #2)
> (From update of attachment 207444 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=207444&action=review
> 
> Nice. Just a couple suggestions before landing.
> 
> > Source/WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:1054
> > +    if (!roundedHoleRect.radii().isZero())
> > +        path.addRoundedRect(roundedHoleRect);
> > +    else
> > +        path.addRect(roundedHoleRect.rect());
> 
> I wonder if this optimization should go into Path.cpp eventually?
>

Good point, not sure, I can check it in a follow-up patch.

> > Source/WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:1061
> > +    setFillRule(RULE_EVENODD);
> > +    setFillColor(color, colorSpace);
> 
> I think that instead of setting the fill rule and color for the GraphicsContext you should just set them for the Cairo context. That way you can use cairo_save/cairo_restore, instead of oldFillRule and oldFillColor.
> 

Yep, it sounds better option.

> > Source/WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:1065
> > +        ShadowBlur& shadow = platformContext()->shadowBlur();
> > +        shadow.drawInsetShadow(this, rect, roundedHoleRect.rect(), roundedHoleRect.radii());
> 
> I think you can avoid the temporary and move this code to the very start of the method after the early return.

Ok,

Thanks for the review.
Comment 4 Alejandro G. Castro 2013-08-01 08:10:49 PDT
Created attachment 207927 [details]
Proposed patch
Comment 5 Martin Robinson 2013-08-07 04:14:42 PDT
Comment on attachment 207927 [details]
Proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=207927&action=review

> Source/WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:1056
> +    if (this->mustUseShadowBlur())
> +        platformContext()->shadowBlur().drawInsetShadow(this, rect, roundedHoleRect.rect(), roundedHoleRect.radii());

You could even move this above the path manipulation, I think.
Comment 6 Alejandro G. Castro 2013-08-09 10:02:55 PDT
(In reply to comment #5)
> (From update of attachment 207927 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=207927&action=review
> 
> > Source/WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:1056
> > +    if (this->mustUseShadowBlur())
> > +        platformContext()->shadowBlur().drawInsetShadow(this, rect, roundedHoleRect.rect(), roundedHoleRect.radii());
> 
> You could even move this above the path manipulation, I think.

Yeah, I've modified the patch before landing, thanks.

http://trac.webkit.org/changeset/142608