[webkit-reviews] review denied: [Bug 44009] Use SVGPathParser logic to traverse states of a Path : [Attachment 64420] Patch

[bugzilla-daemon at webkit.org]

Nikolas Zimmermann <zimmermann at kde.org> has denied Dirk Schulze
<krit at webkit.org>'s request for review:
Bug 44009: Use SVGPathParser logic to traverse states of a Path
https://bugs.webkit.org/show_bug.cgi?id=44009

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

------- Additional Comments from Nikolas Zimmermann <zimmermann at kde.org>
Hi Dirk, good job, some comments:

WebCore/ChangeLog:8
 +	    Move the getPathSegAtLength logic from SVGPathSegList to a new
SVGPathParser Consumer.
Move the getPathsegAtLength logic from SVGPathSegList into a new
SVGPathConsumer class: SVGPathTraversalStateBuilder.

WebCore/ChangeLog:9-10
 +	    This allows us to get a SVGPathSeg at a given length of
SVGPathByteStreams, as
 +	    well as of SVGPathSegLists.
This allows us to get a SVGPathSeg at a given length for SVGPathByteStreams and
SVGPathSegLists.

WebCore/svg/SVGPathBuilder.h:39
 +	virtual void nextPathSegment() { }
nextPathSegment sounds like it would give you the next path segment type or
something.
suggestion: "pathSegmentStarted". (wrong, see below :-)
Just saw that you're calling it after the segment has been processed, so
"incrementPathSegmentCount" sounds even better.


WebCore/svg/SVGPathBuilder.h:40
 +	virtual bool quitEarlier() { return false; }
I'd name it "continueConsuming" return true by default.

WebCore/svg/SVGPathElement.idl:40
 +	    unsigned long getPathSegAtLength(in float distance);
Ok, just checked SVG 1.1, this is indeed correct, getPathSegAtLength doesn't
raise exceptions, so fine with me.

WebCore/svg/SVGPathParserFactory.cpp:79
 +  static SVGPathTraversalStateBuilder*
globalSVGPathTraversalStateBuilder(PathTraversalState* traversalState, float
length)
Take a PathTraversalState& reference here.

WebCore/svg/SVGPathParserFactory.cpp:85
 +	s_builder->setCurrentTraversalState(traversalState);
Use &traversalState.

WebCore/svg/SVGPathParserFactory.cpp:275
 +	OwnPtr<PathTraversalState> traversalState = adoptPtr(new
PathTraversalState(PathTraversalState::TraversalSegmentAtLength));
Don't create this on the heap - there's no gain in doing so, just create it on
the stack.

WebCore/svg/SVGPathTraversalStateBuilder.cpp:70
 +	return (m_traversalState->m_totalLength >=
m_traversalState->m_desiredLength);
Useless braces.

WebCore/svg/SVGPathTraversalStateBuilder.cpp:76
 +	m_traversalState->m_segmentIndex++;
++m_traversalState->m_segmentIndex.