Bug 138537 – Implement round-rect clipping on video elements

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Simon Fraser (smfr)]

Implement round-rect clipping on video elements

[Simon Fraser (smfr)]

Created attachment 241237 [details]
Patch

[Simon Fraser (smfr)]

rdar://problem/9534399

[Tim Horton]

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?

[Simon Fraser (smfr)]

Created attachment 241238 [details]
Patch

[Darin Adler]

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.

[Simon Fraser (smfr)]

https://trac.webkit.org/changeset/175794

[Daniel Bates]

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