[webkit-reviews] review denied: [Bug 201905] CRASH shape-outside should support path() syntax : [Attachment 382673] Patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has denied Dirk Schulze
<krit at webkit.org>'s request for review:
Bug 201905: CRASH shape-outside should support path() syntax
https://bugs.webkit.org/show_bug.cgi?id=201905

Attachment 382673: Patch

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




--- Comment #6 from Simon Fraser (smfr) <simon.fraser at apple.com> ---
Comment on attachment 382673
  --> https://bugs.webkit.org/attachment.cgi?id=382673
Patch

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

> Source/WebCore/ChangeLog:3
> +	   CRASH shape-outside should support path() syntax

Let's call this "Fix crash/assertion by implementing the path() syntax on
shape-outside

> Source/WebCore/ChangeLog:17
> +	   Tests: css3/shapes/spec-examples/shape-outside-020.html
> +		  css3/shapes/spec-examples/shape-outside-021.html

I imported WPT into imported/w3c/web-platform-tests/css/css-shapes recently. Do
these not test the path() syntax?

> Source/WebCore/platform/graphics/PathTraversalState.h:91
> +    Vector<FloatPoint> m_points;

Can we reserveCapacity() on this vector somewhere to improve performance?

> Source/WebCore/rendering/shapes/Shape.cpp:147
> +	   // TODO: Currently, WebCoree just supports single shapes. Path
syntax may have multiple sub-paths.

WebCoree!

> Source/WebCore/rendering/shapes/Shape.cpp:153
> +	   std::unique_ptr<Vector<FloatPoint>> vertices =
makeUnique<Vector<FloatPoint>>(valuesSize);
> +	   for (unsigned i = 0; i < valuesSize; ++i)
> +	       (*vertices)[i] = physicalPointToLogical(values[i],
logicalBoxSize.height(), writingMode);

This could be far more efficient in the common case (isHorizontalWritingMode)
because it could just copy the vector.

> Source/WebCore/rendering/style/BasicShapes.cpp:353
> +    buildLengthVectorFromByteStream(*m_byteStream, m_values, 3.f);

What is 3.f? Put into a named constant.

> Source/WebCore/svg/SVGPathUtilities.cpp:251
> +    points = WTFMove(builder.pointsList());

Make Vector<FloatPoint> the return value of this function?