Bug 138537 - Implement round-rect clipping on video elements
Summary: Implement round-rect clipping on video elements
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Simon Fraser (smfr)
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2014-11-08 15:20 PST by Simon Fraser (smfr)
Modified: 2014-11-10 10:13 PST (History)
13 users (show)

See Also:


Attachments
Patch (70.46 KB, patch)
2014-11-08 15:31 PST, Simon Fraser (smfr)
no flags Details | Formatted Diff | Diff
Patch (69.48 KB, patch)
2014-11-08 15:53 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-08 15:20:10 PST
Implement round-rect clipping on video elements
Comment 1 Simon Fraser (smfr) 2014-11-08 15:31:15 PST
Created attachment 241237 [details]
Patch
Comment 2 Simon Fraser (smfr) 2014-11-08 15:33:37 PST
rdar://problem/9534399
Comment 3 Tim Horton 2014-11-08 15:47:32 PST
Comment on attachment 241237 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=241237&action=review

> Source/WebCore/platform/graphics/FloatRoundedRect.h:70
> +        bool isUniformCornerRadius() const; // Including no radius.

hasUniformCornerRadii?
Comment 4 Simon Fraser (smfr) 2014-11-08 15:53:13 PST
Created attachment 241238 [details]
Patch
Comment 5 Darin Adler 2014-11-08 16:21:52 PST
Comment on attachment 241238 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=241238&action=review

> Source/WebCore/platform/graphics/FloatRoundedRect.cpp:70
> +    return m_topLeft.width() == m_topLeft.height()
> +        && m_topLeft == m_topRight
> +        && m_topLeft == m_bottomLeft
> +        && m_topLeft == m_bottomRight;

Given these are floats, I am not sure that == will give us the results we want. We might need smarter floating point number comparison.

> Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:2091
> +        auto end = m_contentsLayerClones->end();
> +        for (auto it = m_contentsLayerClones->begin(); it != end; ++it) {
>              it->value->setPosition(contentOrigin);
>              it->value->setBounds(contentBounds);
>          }

I think you can write this as a modern for loop:

    for (auto& layer : m_contentsLayerClones->values()) {
        layer->setPosition(contentOrigin);
        layer->setBounds(contentBounds);
    }

> Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:2096
> +            m_shapeMaskLayerClones = adoptPtr(new LayerMap);

This should be std::make_unique, not adoptPtr(new).

> Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:2099
> +        auto end = m_contentsClippingLayerClones->end();
> +        for (auto it = m_contentsClippingLayerClones->begin(); it != end; ++it) {

I think you can write this as a modern for loop:

    for (auto& clone : *m_contentsClippingLayerClones) {
        CloneID cloneID = clone.key;
        ...

> Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:2983
>  
> +
> +static void dumpInnerLayer(TextStream& textStream, String label, PlatformCALayer* layer, int indent, LayerTreeAsTextBehavior behavior)

Extra blank line here.

> Source/WebCore/platform/graphics/ca/GraphicsLayerCA.h:465
> +    OwnPtr<LayerMap> m_shapeMaskLayerClones;

This should be std::unique_ptr, not OwnPtr.

> Source/WebCore/platform/graphics/ca/PlatformCALayer.h:29
> +#include "FloatRoundedRect.h"

Shouldn’t need to include here. A forward declaration should suffice for arguments and return values both.

> Source/WebCore/platform/graphics/ca/mac/PlatformCALayerMac.mm:515
> +    [m_layer.get() setMask:layer ? layer->platformLayer() : nil];

Should not need the .get() here.

> Source/WebCore/platform/graphics/ca/mac/PlatformCALayerMac.mm:856
> +    [static_cast<CAShapeLayer *>(m_layer.get()) setPath:shapePath.platformPath()];

It’s a little strange to use a C++ cast with the Objective-C type like this. I would have probably used a C-style cast.

> Source/WebCore/rendering/RenderLayerBacking.cpp:1618
> +    m_graphicsLayer->setContentsClippingRect(FloatRoundedRect(destRect));

Do we need the explicit conversions on the various lines like these? Why?

> Source/WebKit2/Shared/mac/RemoteLayerTreePropertyApplier.mm:201
> +        [static_cast<CAShapeLayer *>(layer) setPath:path.platformPath()];

It’s a little strange to use a C++ cast with the Objective-C type like this. I would have probably used a C-style cast.
Comment 6 Simon Fraser (smfr) 2014-11-09 11:42:42 PST
https://trac.webkit.org/changeset/175794
Comment 7 Daniel Bates 2014-11-10 10:13:42 PST
(In reply to comment #6)
> https://trac.webkit.org/changeset/175794

This broke the iOS build. Dan Bernstein committed a build fix in <http://trac.webkit.org/changeset/175811>.