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

Dana Jansens
Reported 2012-02-07 20:58:52 PST
[Chromium] Fix opaque tracking for box shadows/filters and non-composited child elements
Attachments
Patch (32.58 KB, patch)
2012-02-07 22:10 PST, Dana Jansens 		no flags
DetailsFormatted DiffDiff
Patch (32.22 KB, patch)
2012-02-07 22:21 PST, Dana Jansens 		no flags
DetailsFormatted DiffDiff
Patch (31.61 KB, patch)
2012-02-07 22:33 PST, Dana Jansens 		no flags
DetailsFormatted DiffDiff
Patch (31.97 KB, patch)
2012-02-08 08:56 PST, Dana Jansens 		no flags
DetailsFormatted DiffDiff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.
Dana Jansens
Comment 1 2012-02-07 22:10:16 PST
Created attachment 126000 [details] Patch
James Robinson
Comment 2 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
Dana Jansens
Comment 3 2012-02-07 22:21:57 PST
Created attachment 126003 [details] Patch
Dana Jansens
Comment 4 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
Dana Jansens
Comment 5 2012-02-07 22:33:56 PST
Created attachment 126006 [details] Patch - added DOCTYPE tag and dumpAsText(true) to layout tests
Stephen White
Comment 6 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.
Dana Jansens
Comment 7 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
Dana Jansens
Comment 8 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?
Dana Jansens
Comment 9 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.
James Robinson
Comment 10 2012-02-08 11:43:30 PST
All you, Stephen. I don't have any more substantive comments.
Stephen White
Comment 11 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.
Dana Jansens
Comment 12 2012-02-08 14:08:19 PST
Comment on attachment 126092 [details] Patch Thanks, that sounds like a perfect match.
WebKit Review Bot
Comment 13 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>
WebKit Review Bot
Comment 14 2012-02-08 15:48:39 PST
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.