[webkit-reviews] review granted: [Bug 231801] Implement parsing and animation support for offset-path : [Attachment 442646] Patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has granted Kiet Ho
<tho22 at apple.com>'s request for review:
Bug 231801: Implement parsing and animation support for offset-path
https://bugs.webkit.org/show_bug.cgi?id=231801

Attachment 442646: Patch

https://bugs.webkit.org/attachment.cgi?id=442646&action=review




--- Comment #18 from Simon Fraser (smfr) <simon.fraser at apple.com> ---
Comment on attachment 442646
  --> https://bugs.webkit.org/attachment.cgi?id=442646
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.