Bug 78073

Summary: [Chromium] Fix opaque tracking for box shadows and non-composited child elements
Product: WebKit Reporter: Dana Jansens <danakj>
Component: New BugsAssignee: Dana Jansens <danakj>
Status: RESOLVED FIXED    
Severity: Normal CC: backer, cc-bugs, enne, jamesr, piman, reed, reveman, senorblanco, tony, vangelis, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch none

Description Dana Jansens 2012-02-07 20:58:52 PST
[Chromium] Fix opaque tracking for box shadows/filters and non-composited child elements
Comment 1 Dana Jansens 2012-02-07 22:10:16 PST
Created attachment 126000 [details]
Patch
Comment 2 James Robinson 2012-02-07 22:16:45 PST
Comment on attachment 126000 [details]
Patch

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

Are the box shadow tracking and the transform fixes really part of the same patch, or can this be broken into two patches? It's kinda big

> LayoutTests/compositing/culling/scrolled-within-boxshadow.html:1
> +<html>

<!DOCTYPE html> before this to trigger standards (instead of quirks) mode, please

> LayoutTests/compositing/culling/scrolled-within-boxshadow.html:36
> +    layoutTestController.waitUntilDone();

please also add layoutTestController.dumpAsText(true) since the render tree isn't useful and will probably make the test results incompatible across platforms

> LayoutTests/compositing/culling/translated-boxshadow.html:1
> +<html>

same comments as for earlier test
Comment 3 Dana Jansens 2012-02-07 22:21:57 PST
Created attachment 126003 [details]
Patch
Comment 4 Dana Jansens 2012-02-07 22:27:06 PST
(In reply to comment #2)
> (From update of attachment 126000 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=126000&action=review
> 
> Are the box shadow tracking and the transform fixes really part of the same patch, or can this be broken into two patches? It's kinda big

A box shadow gives both a filter and a transform.  The shadow part is drawn with a filter, and the solid background is drawn with a transform.

> > LayoutTests/compositing/culling/scrolled-within-boxshadow.html:1
> > +<html>
> 
> <!DOCTYPE html> before this to trigger standards (instead of quirks) mode, please

k.

> > LayoutTests/compositing/culling/scrolled-within-boxshadow.html:36
> > +    layoutTestController.waitUntilDone();
> 
> please also add layoutTestController.dumpAsText(true) since the render tree isn't useful and will probably make the test results incompatible across platforms

k. 

> > LayoutTests/compositing/culling/translated-boxshadow.html:1
> > +<html>
> 
> same comments as for earlier test
Comment 5 Dana Jansens 2012-02-07 22:33:56 PST
Created attachment 126006 [details]
Patch

- added DOCTYPE tag and dumpAsText(true) to layout tests
Comment 6 Stephen White 2012-02-08 08:39:56 PST
Comment on attachment 126006 [details]
Patch

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

Looks OK to me, but I'll leave for James.

> Source/WebCore/platform/graphics/skia/OpaqueRegionSkia.cpp:255
> +    canvasToTargetTransform.setAll(transform.a(), transform.c(), transform.e(), transform.b(), transform.d(), transform.f(), 0, 0, 1);

Nit:  I think you can use AffineTransform's SkMatrix cast operator here.
Comment 7 Dana Jansens 2012-02-08 08:56:57 PST
Created attachment 126092 [details]
Patch

- use AffineTransform::SKMatrix()
- little beefier unit tests to cover transforms in the GraphicsContext
Comment 8 Dana Jansens 2012-02-08 09:41:40 PST
It was pointed out there is some skia code in here now that's not guarded by USE(SKIA). I'm told on #chromium that we're using skia on all platforms as of m18 unless huge problems are found with mac, which is unlikely now. Is it okay to do this then or do I need to make sure things are guarded?
Comment 9 Dana Jansens 2012-02-08 11:40:01 PST
Support for non-skia platforms is not needed in trunk, so going to leave this as it is for review.
Comment 10 James Robinson 2012-02-08 11:43:30 PST
All you, Stephen. I don't have any more substantive comments.
Comment 11 Stephen White 2012-02-08 14:06:10 PST
Comment on attachment 126092 [details]
Patch

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

Looks good.  r=me

> Source/WebCore/platform/graphics/skia/OpaqueRegionSkia.cpp:146
> +    if (paint.getLooper())
> +        return false;
> +    if (paint.getImageFilter())
> +        return false;
> +    if (paint.getMaskFilter())
> +        return false;
> +    SkColorFilter* colorFilter = paint.getColorFilter();
> +    if (colorFilter && !(colorFilter->getFlags() & SkColorFilter::kAlphaUnchanged_Flag))
> +        return false;

You might want to sync with Justin Novosad.  He's been implementing a similar function for the deferred canvas case.  It's in isPaintOpaque() in SkDeferredCanvas.cpp.  His version errs on the side of transparent, that is, it only returns true if he's certain that the paint is opaque.  If those semantics work for you, it would be cool to unify these two functions as a public API in skia in the future.
Comment 12 Dana Jansens 2012-02-08 14:08:19 PST
Comment on attachment 126092 [details]
Patch

Thanks, that sounds like a perfect match.
Comment 13 WebKit Review Bot 2012-02-08 15:48:33 PST
Comment on attachment 126092 [details]
Patch

Clearing flags on attachment: 126092

Committed r107143: <http://trac.webkit.org/changeset/107143>
Comment 14 WebKit Review Bot 2012-02-08 15:48:39 PST
All reviewed patches have been landed.  Closing bug.