[webkit-reviews] review granted: [Bug 72401] [SVG2]: Implement support for the 'pathLength' attribute : [Attachment 383900] Patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has granted Said Abou-Hallawa
<sabouhallawa at apple.com>'s request for review:
Bug 72401: [SVG2]: Implement support for the 'pathLength' attribute
https://bugs.webkit.org/show_bug.cgi?id=72401

Attachment 383900: Patch

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




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

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

> Source/WebCore/rendering/svg/RenderSVGTextPath.h:38
> +    const SVGLengthValue& startOffset() const;

We seem to usually return SVGLengthValue by value.

> Source/WebCore/rendering/svg/SVGRenderSupport.cpp:421
> +    Element* element = renderer.element();
> +    ASSERT(element);

Maybe we should be defensive and return if element is null.

> Source/WebCore/rendering/svg/SVGRenderSupport.cpp:446
> +	       if (float pathLength =
downcast<SVGGeometryElement>(element)->pathLength())
> +		   scaleFactor =
downcast<RenderSVGShape>(renderer).getTotalLength() / pathLength;

Are we sure that pathLength is always non-zero here? Otherwise you have a
divide by zero.

> Source/WebCore/rendering/svg/SVGTextLayoutEngine.cpp:178
> +	   m_textPathStartOffset = startOffset.valueInSpecifiedUnits() *
m_textPathLength / 100;

Why / 100 ?

> Source/WebCore/rendering/svg/SVGTextLayoutEngine.cpp:184
> +	       if (float pathLength = tragetElement->pathLength())
> +		   m_textPathStartOffset *= m_textPathLength / pathLength;

Same comment about zero pathLength.