Bug 231801

Created attachment 441355 [details] WIP

Created attachment 441360 [details] WIP

Okay, the problem is that this patch depends on the previous offset-position/offset-anchor/offset-distance patch, it won't apply cleanly without the mentioned patch. I'll push new patches once the offset-position/offset-anchor/offset-distance patch got merged.

Created attachment 441588 [details] WIP

Created attachment 442124 [details] Patch for review

Note for reviewers: * Two tests in offset-path-parsing-valid.html are still failing. WebKit is able to parse them just fine, however the serialized value returned by getPropertyValue() doesn't match the expected value. Maybe someone can help me determine which is the correct serialization, since Chrome and Firefox also fails this test. * Some animation tests (in offset-path-interpolation-004.html) are failing. This is probably due to an existing bug in SVGPathBlender before this patch.

Created attachment 442128 [details] Patch for review (try 2)

(please hold off from reviewing this because I found some nasty bugs that surfaces just from changing build configuration (Debug to Release))

<rdar://problem/84542616>

Created attachment 442146 [details] Patch for review (try 3)

Comment on attachment 442146 [details] Patch for review (try 3) Please do the PathOperation rename in a first, initial patch.

(In reply to Simon Fraser (smfr) from comment #11) > Comment on attachment 442146 [details] > Patch for review (try 3) > > Please do the PathOperation rename in a first, initial patch. I typed the exact same thing when I saw your comment as I submitted mine.

Created attachment 442227 [details] Patch

Comment on attachment 442227 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=442227&action=review > Source/WebCore/animation/CSSPropertyAnimation.cpp:279 > + if (!from || !to) { > + // fall back to discrete animation. > + return context.progress < 0.5 ? from : to; > + } This is a behavior change that you should separate out into its own patch (and seems testable via animations). > Source/WebCore/animation/CSSPropertyAnimation.cpp:1039 > + auto fromPathOperation = value(from), toPathOperation = value(to); Declare each variable on its own line (no commas). > Source/WebCore/animation/CSSPropertyAnimation.cpp:1042 > + if (!fromPathOperation || !toPathOperation) You can combine the type and null checks here: if (!is<ShapePathOperation>(fromPathOperation) || !is<ShapePathOperation>(toPathOperation)) return false > Source/WebCore/animation/CSSPropertyAnimation.cpp:1047 > + const BasicShape& fromShape = downcast<ShapePathOperation>(*fromPathOperation).basicShape(); auto& (const is inferred). > Source/WebCore/css/BasicShapeFunctions.cpp:112 > + std::unique_ptr<SVGPathByteStream> path = pathShape.pathData()->copy(); auto. And it's not a path, it's a byte stream. > Source/WebCore/css/BasicShapeFunctions.cpp:114 > + auto absolutePath = makeUnique<SVGPathByteStream>(); Shame we have to make another byte stream here. Maybe give copy() an argument that says if the path should be absolutized (or add absoluteCopy()). > Source/WebCore/css/BasicShapeFunctions.h:43 > +Ref<CSSPrimitiveValue> valueForBasicShape(const RenderStyle&, const BasicShape&, bool forceSVGAbsolutePath = false); It's bad to use 'bool' where the call sites will pass "true" or "false" - it results in hard to read code. An enum would be better. > Source/WebCore/rendering/style/StyleRareNonInheritedData.h:238 > + RefPtr<PathOperation> offsetPath; Move this up next to the other pointer types (to below translate). > Source/WebCore/svg/SVGPathAbsoluteConverter.h:54 > + void incrementPathSegmentCount() final; > + bool continueConsuming() final; > + > + // Used in UnalteredParsing/NormalizedParsing modes. > + void moveTo(const FloatPoint& targetPoint, bool closed, PathCoordinateMode) final; > + void lineTo(const FloatPoint& targetPoint, PathCoordinateMode) final; > + void curveToCubic(const FloatPoint& point1, const FloatPoint& point2, const FloatPoint& targetPoint, PathCoordinateMode) final; > + void closePath() final; > + > + // Only used in UnalteredParsing mode. > + void lineToHorizontal(float targetX, PathCoordinateMode) final; > + void lineToVertical(float targetY, PathCoordinateMode) final; > + void curveToCubicSmooth(const FloatPoint& point2, const FloatPoint& targetPoint, PathCoordinateMode) final; > + void curveToQuadratic(const FloatPoint& point1, const FloatPoint& targetPoint, PathCoordinateMode) final; > + void curveToQuadraticSmooth(const FloatPoint& targetPoint, PathCoordinateMode) final; > + void arcTo(float r1, float r2, float angle, bool largeArcFlag, bool sweepFlag, const FloatPoint& targetPoint, PathCoordinateMode) final; Can these all be private? It's weird that PathCoordinateMode is a per-segment thing rather than a mode on the path consumer.

Comment on attachment 442227 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=442227&action=review >> Source/WebCore/animation/CSSPropertyAnimation.cpp:279 >> + } > > This is a behavior change that you should separate out into its own patch (and seems testable via animations). I agree, I'll factor this out to a separate patch. (which should be merged first before this one) >> Source/WebCore/css/BasicShapeFunctions.cpp:114 >> + auto absolutePath = makeUnique<SVGPathByteStream>(); > > Shame we have to make another byte stream here. Maybe give copy() an argument that says if the path should be absolutized (or add absoluteCopy()). I'll see if there's a way to avoid double copying here. >> Source/WebCore/css/BasicShapeFunctions.h:43 >> +Ref<CSSPrimitiveValue> valueForBasicShape(const RenderStyle&, const BasicShape&, bool forceSVGAbsolutePath = false); > > It's bad to use 'bool' where the call sites will pass "true" or "false" - it results in hard to read code. An enum would be better. This enum would be declared in BasicShapeFunctions.h right? >> Source/WebCore/rendering/style/StyleRareNonInheritedData.h:238 >> + RefPtr<PathOperation> offsetPath; > > Move this up next to the other pointer types (to below translate). I put offsetPath here to group the offset* elements together. Is there any benefits to move this next to other pointer members? Struct padding perhaps? >> Source/WebCore/svg/SVGPathAbsoluteConverter.h:54 >> + void arcTo(float r1, float r2, float angle, bool largeArcFlag, bool sweepFlag, const FloatPoint& targetPoint, PathCoordinateMode) final; > > Can these all be private? > > It's weird that PathCoordinateMode is a per-segment thing rather than a mode on the path consumer. You mean everything including the constructor? Is there any benefit to that? Also, PathCoordinateMode is per-segment because SVG allows mixing absolute and relative draw commands.

Comment on attachment 442227 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=442227&action=review >> Source/WebCore/animation/CSSPropertyAnimation.cpp:279 >> + } > > This is a behavior change that you should separate out into its own patch (and seems testable via animations). I agree, I'll factor this out to a separate patch. (which should be merged first before this one) >> Source/WebCore/css/BasicShapeFunctions.cpp:114 >> + auto absolutePath = makeUnique<SVGPathByteStream>(); > > Shame we have to make another byte stream here. Maybe give copy() an argument that says if the path should be absolutized (or add absoluteCopy()). I'll see if there's a way to avoid double copying here. >> Source/WebCore/css/BasicShapeFunctions.h:43 >> +Ref<CSSPrimitiveValue> valueForBasicShape(const RenderStyle&, const BasicShape&, bool forceSVGAbsolutePath = false); > > It's bad to use 'bool' where the call sites will pass "true" or "false" - it results in hard to read code. An enum would be better. This enum would be declared in BasicShapeFunctions.h right? >> Source/WebCore/rendering/style/StyleRareNonInheritedData.h:238 >> + RefPtr<PathOperation> offsetPath; > > Move this up next to the other pointer types (to below translate). I put offsetPath here to group the offset* elements together. Is there any benefits to move this next to other pointer members? Struct padding perhaps? >> Source/WebCore/svg/SVGPathAbsoluteConverter.h:54 >> + void arcTo(float r1, float r2, float angle, bool largeArcFlag, bool sweepFlag, const FloatPoint& targetPoint, PathCoordinateMode) final; > > Can these all be private? > > It's weird that PathCoordinateMode is a per-segment thing rather than a mode on the path consumer. You mean everything including the constructor? Is there any benefit to that? Also, PathCoordinateMode is per-segment because SVG allows mixing absolute and relative draw commands.

Created attachment 442646 [details] Patch

Comment on attachment 442646 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=442646&action=review > Source/WebCore/ChangeLog:7 > + Some words here please. > Source/WebCore/css/BasicShapeFunctions.cpp:72 > + return source.copy(); Shame to copy when no conversion is necessary. It looks like this copy is already there but maybe we can avoid it. > Source/WebCore/css/BasicShapeFunctions.h:48 > + // Keep the path as is. > + None, > + > + // Convert relative commands in a path to absolute. I don't think you need the comments. > Source/WebCore/css/BasicShapeFunctions.h:49 > + ForceAbsolute Maybe just "Absolute" > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:3209 > + // The computed value of offset-path must only contain absolute draw commands. > + // Reference: https://github.com/w3c/fxtf-drafts/issues/225#issuecomment-334322738: > + // > RESOLVED: Computed-value normalize case of path commands (to the absolute version). I don't think this comment is necessary,. > Source/WebCore/svg/SVGPathAbsoluteConverter.cpp:35 > + , m_currentPoint() > + , m_subpathPoint() No need to initialize these. They have default constructors. > Source/WebCore/svg/SVGPathAbsoluteConverter.cpp:44 > +bool SVGPathAbsoluteConverter::continueConsuming() continueConsuming() const (needs fixing in the base class too). > Source/WebCore/svg/SVGPathUtilities.cpp:214 > +bool convertSVGPathByteStreamToAbsoluteCoordinates(const SVGPathByteStream& stream, SVGPathByteStream& result) Would be nicer to write this as std::unique_ptr<SVGPathByteStream> convertSVGPathByteStreamToAbsoluteCoordinates(const SVGPathByteStream& stream) and return nullptr on failure.

Comment on attachment 442646 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=442646&action=review >> Source/WebCore/css/BasicShapeFunctions.h:49 >> + ForceAbsolute > > Maybe just "Absolute" SVGPathConversion::Absolute doesn't sound right to me. I think ForceAbsolute is more appropriate, as it sounds like an action compared to just Absolute. >> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:3209 >> + // > RESOLVED: Computed-value normalize case of path commands (to the absolute version). > > I don't think this comment is necessary,. I think it's necessary because someone might ask the question of why offset-path needs this but not clip-path. The Motion Path spec doesn't mention this either. >> Source/WebCore/svg/SVGPathAbsoluteConverter.cpp:35 >> + , m_subpathPoint() > > No need to initialize these. They have default constructors. Will remove. >> Source/WebCore/svg/SVGPathAbsoluteConverter.cpp:44 >> +bool SVGPathAbsoluteConverter::continueConsuming() > > continueConsuming() const (needs fixing in the base class too). It does seems like continueConsuming() can be made const, but what if something in the future needs it to be non-const? >> Source/WebCore/svg/SVGPathUtilities.cpp:214 >> +bool convertSVGPathByteStreamToAbsoluteCoordinates(const SVGPathByteStream& stream, SVGPathByteStream& result) > > Would be nicer to write this as std::unique_ptr<SVGPathByteStream> convertSVGPathByteStreamToAbsoluteCoordinates(const SVGPathByteStream& stream) and return nullptr on failure. Will change.

Created attachment 443387 [details] Patch

Created attachment 443388 [details] Patch

Comment on attachment 443388 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=443388&action=review > Source/WebCore/ChangeLog:20 > + Implements parsing and animation support for offset-path based on clip-path. > + offset-path additionally supports ray() shape, which will be implemented in a future patch. This goes above the Tests: list. > Source/WebCore/css/BasicShapeFunctions.h:44 > +enum class SVGPathConversion { enum class SVGPathConversion : uint8_t

Created attachment 443398 [details] Patch

Committed r285343 (243904@main): <https://commits.webkit.org/243904@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 443398 [details].