Bug 138684 - Make clip-path work on <video>, <canvas> etc.
Summary: Make clip-path work on <video>, <canvas> etc.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Simon Fraser (smfr)
URL:
Keywords:
Depends on: 97194
Blocks:
  Show dependency treegraph
 
Reported: 2014-11-12 21:13 PST by Simon Fraser (smfr)
Modified: 2015-03-02 09:04 PST (History)
11 users (show)

See Also:


Attachments
Patch (81.55 KB, patch)
2015-03-01 14:42 PST, Simon Fraser (smfr)
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews100 for mac-mavericks (718.96 KB, application/zip)
2015-03-01 15:40 PST, Build Bot
no flags Details
Archive of layout-test-results from ews106 for mac-mavericks-wk2 (945.57 KB, application/zip)
2015-03-01 15:46 PST, Build Bot
no flags Details
Patch (84.45 KB, patch)
2015-03-01 17:35 PST, Simon Fraser (smfr)
no flags Details | Formatted Diff | Diff
Patch (84.45 KB, patch)
2015-03-01 18:23 PST, Simon Fraser (smfr)
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Simon Fraser (smfr) 2014-11-12 21:13:17 PST
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.
Comment 1 Simon Fraser (smfr) 2015-03-01 14:42:23 PST
Created attachment 247639 [details]
Patch
Comment 2 Dirk Schulze 2015-03-01 15:11:58 PST
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 3 Sam Weinig 2015-03-01 15:26:02 PST
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 4 Build Bot 2015-03-01 15:40:44 PST
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
Comment 5 Build Bot 2015-03-01 15:40:48 PST
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 6 Build Bot 2015-03-01 15:46:40 PST
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
Comment 7 Build Bot 2015-03-01 15:46:44 PST
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
Comment 8 Simon Fraser (smfr) 2015-03-01 17:35:40 PST
Created attachment 247645 [details]
Patch
Comment 9 Simon Fraser (smfr) 2015-03-01 18:23:19 PST
Created attachment 247648 [details]
Patch
Comment 10 Darin Adler 2015-03-01 20:55:34 PST
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.
Comment 11 Simon Fraser (smfr) 2015-03-01 21:13:12 PST
(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.
Comment 12 Simon Fraser (smfr) 2015-03-01 22:36:16 PST
https://trac.webkit.org/r180882
Comment 14 David Kilzer (:ddkilzer) 2015-03-02 07:31:43 PST
(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.
Comment 15 David Kilzer (:ddkilzer) 2015-03-02 07:32:29 PST
(In reply to comment #12)
> https://trac.webkit.org/r180882

Build fix for newer clang:  <http://trac.webkit.org/r180883>
Comment 16 David Kilzer (:ddkilzer) 2015-03-02 09:04:01 PST
(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?