[Chromium] Fix opaque tracking for box shadows/filters and non-composited child elements
Created attachment 126000 [details] Patch
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
Created attachment 126003 [details] Patch
(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
Created attachment 126006 [details] Patch - added DOCTYPE tag and dumpAsText(true) to layout tests
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.
Created attachment 126092 [details] Patch - use AffineTransform::SKMatrix() - little beefier unit tests to cover transforms in the GraphicsContext
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?
Support for non-skia platforms is not needed in trunk, so going to leave this as it is for review.
All you, Stephen. I don't have any more substantive comments.
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 on attachment 126092 [details] Patch Thanks, that sounds like a perfect match.
Comment on attachment 126092 [details] Patch Clearing flags on attachment: 126092 Committed r107143: <http://trac.webkit.org/changeset/107143>
All reviewed patches have been landed. Closing bug.
Could this have caused the compositing/rubberbanding tests to need a new baseline? http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=platform%2Fchromium%2Fcompositing%2Frubberbanding%2Ftransform-overhang-nw.html%2Cplatform%2Fchromium%2Fcompositing%2Frubberbanding%2Ftransform-overhang-sw.html%2Cplatform%2Fchromium%2Fcompositing%2Frubberbanding%2Ftransform-overhang-n.html%2Cplatform%2Fchromium%2Fcompositing%2Frubberbanding%2Ftransform-overhang-ne.html%2Cplatform%2Fchromium%2Fcompositing%2Frubberbanding%2Ftransform-overhang-size-change.html%2Cplatform%2Fchromium%2Fcompositing%2Frubberbanding%2Ftransform-overhang-w.html