[webkit-reviews] review granted: [Bug 12047] SVGPathSegList needs better getTotalLength, getSegmentAtLength path traversal code : [Attachment 94238] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Dirk Schulze <krit at webkit.org>'s
request for review:
Bug 12047: SVGPathSegList needs better getTotalLength, getSegmentAtLength path
traversal code
https://bugs.webkit.org/show_bug.cgi?id=12047

Attachment 94238: Patch
https://bugs.webkit.org/attachment.cgi?id=94238&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=94238&action=review

> Source/WebCore/svg/SVGPathElement.cpp:67
> +    SVGPathParserFactory* factory = SVGPathParserFactory::self();

I don’t think this local variable helps clarity or performance.

> Source/WebCore/svg/SVGPathElement.cpp:75
> +    SVGPathParserFactory* factory = SVGPathParserFactory::self();

I don’t think this local variable helps clarity or performance.

> Source/WebCore/svg/SVGPathParserFactory.cpp:282
> +    totalLength = builder->totalLength();

Should this be done only if ok is true?

> Source/WebCore/svg/SVGPathParserFactory.cpp:299
> +    point = builder->currentPoint();

Should this be done only if ok is true?

> Source/WebCore/svg/SVGPathTraversalStateBuilder.cpp:88
>  bool SVGPathTraversalStateBuilder::continueConsuming()
>  {
> -    ASSERT(m_traversalState);
> -    ASSERT(m_traversalState->m_action ==
PathTraversalState::TraversalSegmentAtLength);
> -    return m_traversalState->m_totalLength <
m_traversalState->m_desiredLength;
> +    ASSERT(m_traversalState);    
> +    if (m_traversalState->m_action ==
PathTraversalState::TraversalSegmentAtLength
> +	   && m_traversalState->m_totalLength >=
m_traversalState->m_desiredLength)
> +	   m_traversalState->m_success = true;
> +    
> +    if ((m_traversalState->m_action ==
PathTraversalState::TraversalPointAtLength
> +	    || m_traversalState->m_action ==
PathTraversalState::TraversalNormalAngleAtLength)
> +	   && m_traversalState->m_totalLength >=
m_traversalState->m_desiredLength) {
> +	   FloatSize change = m_traversalState->m_current -
m_traversalState->m_previous;
> +	   float slope = atan2f(change.height(), change.width());
> +	   if (m_traversalState->m_action ==
PathTraversalState::TraversalPointAtLength) {
> +	       float offset = m_traversalState->m_desiredLength -
m_traversalState->m_totalLength;
> +	       m_traversalState->m_current.move(offset * cosf(slope), offset *
sinf(slope));
> +	   } else
> +	       m_traversalState->m_normalAngle = rad2deg(slope);
> +	   m_traversalState->m_success = true;
> +    }
> +    m_traversalState->m_previous = m_traversalState->m_current;
> +
> +    return !m_traversalState->m_success;
>  }

Seeing m_traversalState-> so many times over and over again leads to the
question of whether m_traversalState could have some function members to make
this code simpler. A function like this is a bit of an anti-pattern.