Support bezier paths in clip-path property
Created attachment 262823 [details] Work in progress.
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.
Created attachment 264020 [details] Patch
<rdar://problem/23250919>
Created attachment 264021 [details] Patch
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;
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