Bug 149996 – Support bezier paths in clip-path property

• Component: New Bugs
• Status: RESOLVED FIXED

[Simon Fraser (smfr)]

Support bezier paths in clip-path property

[Simon Fraser (smfr)]

Created attachment 262823 [details]
Work in progress.

[WebKit Commit Bot]

Attachment 262823 [details] did not pass style-queue:


ERROR: Source/WebCore/rendering/style/BasicShapes.cpp:41:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebCore/rendering/style/BasicShapes.cpp:315:  Use 'WTF::move()' instead of 'std::move()'.  [runtime/wtf_move] [4]
ERROR: Source/WebCore/page/animation/CSSPropertyAnimation.cpp:483:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/page/animation/CSSPropertyAnimation.cpp:484:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/page/animation/CSSPropertyAnimation.cpp:485:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/page/animation/CSSPropertyAnimation.cpp:486:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/page/animation/CSSPropertyAnimation.cpp:487:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/page/animation/CSSPropertyAnimation.cpp:488:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/WebCore/css/CSSBasicShapes.cpp:38:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 9 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Simon Fraser (smfr)]

Created attachment 264020 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/23250919>

[Simon Fraser (smfr)]

Created attachment 264021 [details]
Patch

[Darin Adler]

Comment on attachment 264021 [details]
Patch

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

> Source/WebCore/css/BasicShapeFunctions.cpp:108
> +        const auto& pathShape = downcast<BasicShapePath>(basicShape);

No need to say const here, since basicShape is already a const reference. We don’t need to explicitly specify const. Same applies in existing code.

> Source/WebCore/css/BasicShapeFunctions.cpp:111
> +        basicShapeValue = pathShapeValue.copyRef();

This should use WTF::move, not copyRef. Same mistake in existing code.

> Source/WebCore/css/BasicShapeFunctions.cpp:267
> +        const CSSBasicShapePath& pathValue = downcast<CSSBasicShapePath>(*basicShapeValue);

Should just use auto&. Same for existing code.

> Source/WebCore/css/BasicShapeFunctions.cpp:269
> +        RefPtr<BasicShapePath> path = BasicShapePath::create(pathValue.pathData()->copy());

Should use auto. Same for existing code.

> Source/WebCore/css/BasicShapeFunctions.cpp:271
> +        basicShape = path.release();

Should be WTF::move(path), not path.release().

> Source/WebCore/css/CSSBasicShapes.cpp:250
> +    auto& otherShapePath = downcast<CSSBasicShapePath>(otherShape);
> +    if (m_windRule != otherShapePath.windRule())
> +        return false;
> +
> +    return m_byteStream == otherShapePath.m_byteStream;

Should use m_windRule for both, not windRule() for one.

I suggest writing like this:

    auto& otherShapePath = downcast<CSSBasicShapePath>(otherShape);
    return m_windRule == otherShapePath.m_windRule && m_byteStream == otherShapePath.m_byteStream;

> Source/WebCore/css/CSSBasicShapes.h:219
> +    static Ref<CSSBasicShapePath> create(std::unique_ptr<SVGPathByteStream>&& byteStream)

Argument name should be pathData, not byteStream.

> Source/WebCore/css/CSSBasicShapes.h:224
> +    const SVGPathByteStream* pathData() const;

Do we need to support pathData of null? I suggest having this return a reference rather than a pointer.

> Source/WebCore/css/CSSBasicShapes.h:226
> +    void setWindRule(WindRule w) { m_windRule = w; }

I suggest the word rule instead of the letter "w" for the argument name.

> Source/WebCore/css/CSSBasicShapes.h:230
> +    virtual String cssText() const override;
> +    virtual bool equals(const CSSBasicShape&) const override;

I suggest marking these private unless they need to be called non-polymorphically.

> Source/WebCore/css/CSSBasicShapes.h:237
> +    std::unique_ptr<SVGPathByteStream> m_byteStream;

Why is this called m_byteStream rather than m_pathData?

> Source/WebCore/css/CSSParser.cpp:6670
> +    args.next(); // FIXME: why?

It’s strange to have brand new code with a FIXME, especially such a cryptic one. Can you deal with this before landing?

> Source/WebCore/rendering/style/BasicShapes.cpp:112
> +    auto& otherCircle = downcast<BasicShapeCircle>(other);
> +    return radius().canBlend(otherCircle.radius());

Not sure the local variable is helpful.

> Source/WebCore/rendering/style/BasicShapes.cpp:268
> +void BasicShapePath::path(Path& path, const FloatRect& boundingBox)
> +{
> +    ASSERT(path.isEmpty());
> +    buildPathFromByteStream(*m_byteStream, path);
> +    path.translate(toFloatSize(boundingBox.location()));
> +}

This is a peculiar function name given what it does. Also, why does it take a Path by value rather than returning a Path?

> Source/WebCore/rendering/style/BasicShapes.cpp:279
> +    const auto& otherPath = downcast<BasicShapePath>(other);
> +    if (m_windRule != otherPath.m_windRule)
> +        return false;
> +
> +    return *m_byteStream == *otherPath.m_byteStream;

I would write:

    auto& otherPath = downcast<BasicShapePath>(other);
    return m_windRule == otherPath.m_windRule && *m_byteStream == *otherPath.m_byteStream;

> Source/WebCore/rendering/style/BasicShapes.cpp:298
> +    const auto& fromPath = downcast<BasicShapePath>(from);

auto& would be fine, no need for const auto&

> Source/WebCore/rendering/style/BasicShapes.cpp:300
> +    std::unique_ptr<SVGPathByteStream> resultingPathBytes = std::make_unique<SVGPathByteStream>();

auto is better than repeating the type twice here.

> Source/WebCore/rendering/style/BasicShapes.cpp:304
> +    RefPtr<BasicShapePath> result =  BasicShapePath::create(WTF::move(resultingPathBytes));
> +    return result.releaseNonNull();

This should be a single line without a RefPtr and without a releaseNonNull.

> Source/WebCore/rendering/style/BasicShapes.h:243
> +    BasicShapePolygon()
> +    {
> +    }

Better way to write this is:

    BasicShapePolygon() = default;

[Simon Fraser (smfr)]

Addressed all the comments except this:

> Source/WebCore/rendering/style/BasicShapes.cpp:268
> +void BasicShapePath::path(Path& path, const FloatRect& boundingBox)
> +{
> +    ASSERT(path.isEmpty());
> +    buildPathFromByteStream(*m_byteStream, path);
> +    path.translate(toFloatSize(boundingBox.location()));
> +}

This is a peculiar function name given what it does. Also, why does it take a Path by value rather than returning a Path?

which can be done as a follow-up, fixing all the classes.

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