Bug 138684 – Make clip-path work on <video>, <canvas> etc.

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Simon Fraser (smfr)]

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.

[Simon Fraser (smfr)]

Created attachment 247639 [details]
Patch

[Dirk Schulze]

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

[Sam Weinig]

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.

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Simon Fraser (smfr)]

Created attachment 247645 [details]
Patch

[Simon Fraser (smfr)]

Created attachment 247648 [details]
Patch

[Darin Adler]

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.

[Simon Fraser (smfr)]

(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.

[Simon Fraser (smfr)]

https://trac.webkit.org/r180882

[Gyuyoung Kim]

(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

[David Kilzer (:ddkilzer)]

(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.

[David Kilzer (:ddkilzer)]

(In reply to comment #12)
> https://trac.webkit.org/r180882

Build fix for newer clang:  <http://trac.webkit.org/r180883>

[David Kilzer (:ddkilzer)]

(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?