After r175794 and some other work for border-radius clipping of composited elements, it shouldn't be too hard to get -webkit-clip path to correctly clip video etc.
Created attachment 247639 [details] Patch
Comment on attachment 247639 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=247639&action=review The changes seem to look ok. The platform and WebKit2 code needs review of someone familiar with it though. > Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:816 > + // FIXME: need to check for path equality. No bool Path::operator==(const Path&)!. > + GraphicsLayer::setShapeLayerPath(path); Oh, because you create a new Path object which doesn't habe an corresponding == operator? Because we seem to have the path stored in GraphicsLayer with your change. IIRC we lazily create Path objects in other places and might be able to see if the path changed there. Not sure. > Source/WebCore/platform/graphics/ca/mac/PlatformCALayerMac.mm:-667 > - [m_layer.get() setMagnificationFilter:toCAFilterType(value)]; This clean up causes a lot of noise in the review :P
Comment on attachment 247639 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=247639&action=review > Source/WebCore/platform/graphics/GraphicsLayer.h:508 > + { > + switch (type) { > + case Type::Normal: return true; > + case Type::PageTiledBacking: return true; > + case Type::Scrolling: return true; This would more commonly be written as: switch (type) { case Type::Normal: case Type::PageTiledBacking: case Type::Scrolling: return true; case Type::Shape: ... > Source/WebCore/platform/graphics/GraphicsLayer.h:635 > +#if USE(CA) Given that these are using existing abstractions, it seems like this should either have a better conditional (maybe, HAVE(SHAPE_LAYER)) or no conditional. Otherwise, it feels like too much of CA is leaking into the abstraction interface.
Comment on attachment 247639 [details] Patch Attachment 247639 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/6065868538642432 New failing tests: compositing/backgrounds/background-image-with-negative-zindex.html compositing/repaint/foreground-layer-change.html
Created attachment 247640 [details] Archive of layout-test-results from ews100 for mac-mavericks The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews100 Port: mac-mavericks Platform: Mac OS X 10.9.5
Comment on attachment 247639 [details] Patch Attachment 247639 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/6529175250796544 New failing tests: compositing/backgrounds/background-image-with-negative-zindex.html fast/inline/hidpi-selection-gap-on-subpixel-position.html compositing/repaint/foreground-layer-change.html
Created attachment 247641 [details] Archive of layout-test-results from ews106 for mac-mavericks-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews106 Port: mac-mavericks-wk2 Platform: Mac OS X 10.9.5
Created attachment 247645 [details] Patch
Created attachment 247648 [details] Patch
Comment on attachment 247648 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=247648&action=review > Source/WebCore/platform/graphics/GraphicsLayer.cpp:85 > + return false; ASSERT_NOT_REACHED here? > Source/WebCore/platform/graphics/GraphicsLayer.cpp:315 > +#if USE(CA) > + return m_shapeLayerPath; > +#endif > + return Path(); Should be #else here instead of double returns. > Source/WebCore/platform/graphics/GraphicsLayer.cpp:322 > +#if USE(CA) > + m_shapeLayerPath = path; > +#endif Probably need UNUSED_PARAM in the !USE(CA) case. That’s one reason I normally put the entire function inside the #if, not just the body. > Source/WebCore/platform/graphics/GraphicsLayer.cpp:330 > +#if USE(CA) > + return m_shapeLayerWindRule; > +#endif > + return RULE_NONZERO; Same here. > Source/WebCore/platform/graphics/GraphicsLayer.cpp:337 > +#if USE(CA) > + m_shapeLayerWindRule = windRule; > +#endif Same UNUSED_PARAM thing. > Source/WebCore/platform/graphics/GraphicsLayer.h:205 > enum class Type { > Normal, > PageTiledBacking, > - Scrolling > + Scrolling, > + Shape > }; I think this would read better horizontally. > Source/WebCore/platform/graphics/GraphicsLayerClient.h:49 > + GraphicsLayerPaintBackground = 1 << 0, > + GraphicsLayerPaintForeground = 1 << 1, > + GraphicsLayerPaintMask = 1 << 2, > + GraphicsLayerPaintClipPath = 1 << 3, > + GraphicsLayerPaintOverflowContents = 1 << 4, > + GraphicsLayerPaintCompositedScroll = 1 << 5, > + GraphicsLayerPaintChildClippingMask = 1 << 6, > + GraphicsLayerPaintAllWithOverflowClip = GraphicsLayerPaintBackground | GraphicsLayerPaintForeground | GraphicsLayerPaintMask Remove parentheses seems worthwhile. Lining up vertically, not so much. Makes later renaming a pain. > Source/WebCore/platform/graphics/GraphicsLayerClient.h:69 > + LayerTreeAsTextBehaviorNormal = 0, > + LayerTreeAsTextDebug = 1 << 0, // Dump extra debugging info like layer addresses. > + LayerTreeAsTextIncludeVisibleRects = 1 << 1, > + LayerTreeAsTextIncludeTileCaches = 1 << 2, > + LayerTreeAsTextIncludeRepaintRects = 1 << 3, > + LayerTreeAsTextIncludePaintingPhases = 1 << 4, > + LayerTreeAsTextIncludeContentLayers = 1 << 5, > + LayerTreeAsTextIncludePageOverlayLayers = 1 << 6, Same comment. > Source/WebCore/platform/graphics/Path.h:94 > + WEBCORE_EXPORT Path(const Path&); > + WEBCORE_EXPORT Path& operator=(const Path&); This class needs a move constructor and move assignment operator for better performance. > Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:294 > + return false; ASSERT_NOT_REACHED(). > Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:838 > + // FIXME: need to check for path equality. No bool Path::operator==(const Path&)!. Should be pretty easy to add for Cocoa platforms since we do have CGPathEqualToPath. > Source/WebCore/platform/graphics/ca/mac/PlatformCALayerMac.mm:858 > + if ([fillRule isEqualToString:@"even-odd"]) Is it correct to have this be case sensitive? > Source/WebCore/platform/graphics/cg/PathCG.cpp:77 > - : m_path(0) > + : m_path(nullptr) Would be better to initialize this in the class definition: "m_path { nullptr };" > Source/WebCore/rendering/RenderLayer.cpp:4073 > + context->clipPath(WTF::move(path), windRule); Not sure this WTF::move does any good. > Source/WebCore/rendering/RenderLayerBacking.cpp:1005 > +// FIXME: avoid repaints when clip path changes. Please capitalize these sentence-like comments. > Source/WebKit2/Shared/mac/RemoteLayerTreeTransaction.mm:99 > + , windRule(RULE_NONZERO) Better to initialize this in the class definition. > Source/WebKit2/WebProcess/WebPage/mac/PlatformCALayerRemote.h:147 > + // FIXME: use paths? Sentence style comment would be better. Also a slightly longer sentence would make it clearer what you mean. I think you are saying that we should see if all callers can use the path functions instead and then we could delete the rounded rect functions.
(In reply to comment #10) > > > Source/WebCore/platform/graphics/GraphicsLayerClient.h:49 > > + GraphicsLayerPaintBackground = 1 << 0, > > + GraphicsLayerPaintForeground = 1 << 1, > > + GraphicsLayerPaintMask = 1 << 2, > > + GraphicsLayerPaintClipPath = 1 << 3, > > + GraphicsLayerPaintOverflowContents = 1 << 4, > > + GraphicsLayerPaintCompositedScroll = 1 << 5, > > + GraphicsLayerPaintChildClippingMask = 1 << 6, > > + GraphicsLayerPaintAllWithOverflowClip = GraphicsLayerPaintBackground | GraphicsLayerPaintForeground | GraphicsLayerPaintMask > > Remove parentheses seems worthwhile. Lining up vertically, not so much. > Makes later renaming a pain. > > > Source/WebCore/platform/graphics/GraphicsLayerClient.h:69 > > + LayerTreeAsTextBehaviorNormal = 0, > > + LayerTreeAsTextDebug = 1 << 0, // Dump extra debugging info like layer addresses. > > + LayerTreeAsTextIncludeVisibleRects = 1 << 1, > > + LayerTreeAsTextIncludeTileCaches = 1 << 2, > > + LayerTreeAsTextIncludeRepaintRects = 1 << 3, > > + LayerTreeAsTextIncludePaintingPhases = 1 << 4, > > + LayerTreeAsTextIncludeContentLayers = 1 << 5, > > + LayerTreeAsTextIncludePageOverlayLayers = 1 << 6, > > Same comment. After finding the issue fixed in http://trac.webkit.org/changeset/176348 I am a convert to lining these up. It also makes adding new bits easier to type.
https://trac.webkit.org/r180882
(In reply to comment #12) > https://trac.webkit.org/r180882 It looks this commit generated jsc regression on all ports. - https://build.webkit.org/builders/EFL%20Linux%2064-bit%20Release%20WK2/builds/20120 - https://build.webkit.org/builders/GTK%20Linux%2064-bit%20Debug%20%28Tests%29/builds/3718 - https://build.webkit.org/builders/Apple%20Mavericks%20Release%20WK2%20%28Tests%29/builds/11978
(In reply to comment #13) > (In reply to comment #12) > > https://trac.webkit.org/r180882 > > It looks this commit generated jsc regression on all ports. > > - > https://build.webkit.org/builders/EFL%20Linux%2064-bit%20Release%20WK2/ > builds/20120 > - > https://build.webkit.org/builders/GTK%20Linux%2064-bit%20Debug%20%28Tests%29/ > builds/3718 > - > https://build.webkit.org/builders/ > Apple%20Mavericks%20Release%20WK2%20%28Tests%29/builds/11978 I think those failures are related to running the tests too close to midnight PST. If they run again, they should be fine.
(In reply to comment #12) > https://trac.webkit.org/r180882 Build fix for newer clang: <http://trac.webkit.org/r180883>
(In reply to comment #14) > (In reply to comment #13) > > (In reply to comment #12) > > > https://trac.webkit.org/r180882 > > > > It looks this commit generated jsc regression on all ports. > > > > - > > https://build.webkit.org/builders/EFL%20Linux%2064-bit%20Release%20WK2/ > > builds/20120 > > - > > https://build.webkit.org/builders/GTK%20Linux%2064-bit%20Debug%20%28Tests%29/ > > builds/3718 > > - > > https://build.webkit.org/builders/ > > Apple%20Mavericks%20Release%20WK2%20%28Tests%29/builds/11978 > > I think those failures are related to running the tests too close to > midnight PST. If they run again, they should be fine. Or maybe because it's just past the end of February (which has only 28 days), and the tests are assuming a 30-day month?