[webkit-reviews] review granted: [Bug 41159] Come up with a more efficient way to represent Path segments : [Attachment 63816] Patch v5

[bugzilla-daemon at webkit.org]

Nikolas Zimmermann <zimmermann at kde.org> has granted  review:
Bug 41159: Come up with a more efficient way to represent Path segments
https://bugs.webkit.org/show_bug.cgi?id=41159

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

------- Additional Comments from Nikolas Zimmermann <zimmermann at kde.org>
WebCore/ChangeLog:8
 +	    Introduce SVGPathByteStream to have a fast and effiecient way to
organize the synchronization
typo: efficient.
suggestion: s/to have a/as/

WebCore/ChangeLog:9
 +	    of Path, SVG path data string and the different SVGPathSegLists.
s/the different/SVGPathSegList in normalized and unaltered modes/ (some readers
of the ChangeLog may not know what you refer to).

WebCore/ChangeLog:10
 +	    SVGPathParserFactory is now able to parse a SVGPathByteStream as a
SVGPathSource or
Extended SVGPathParserFactory to accept SVGPathByteStreams as input source and
to create a SVGPathByteStream from a SVG path data string.

WebCore/WebCore.xcodeproj/project.pbxproj: 
 +		81BE209811F4AB8D00915DFA /* IDBCursor.cpp in Sources */ = {isa
= PBXBuildFile; fileRef = 81BE209311F4AB8D00915DFA /* IDBCursor.cpp */; };
Eek, again the IDBCursor problems - you need to revert them :(

WebCore/svg/SVGPathByteStream.h:61
 +	bool isEmpty() { return !m_data.size(); }
isEmpty should be const.
Do you need the "const Data& data" accessor somewhere? If not please remove.

WebCore/svg/SVGPathByteStreamSource.h:46
 +  private:
Add a newline before private please.

WebCore/svg/SVGPathParserFactory.cpp:110
 +	if (stream->isEmpty())
ASSERT(stream); before accessing stream.

WebCore/svg/SVGPathParserFactory.cpp:153
 +	SVGPathSegListBuilder* builder = globalSVGPathSegListBuilder();
ASSERT(stream); should be added.
and ASSERT(result);

WebCore/svg/SVGPathParserFactory.h:43
 +	PassOwnPtr<SVGPathByteStream> buildSVGPathByteStreamFromString(const
String&, PathParsingMode, bool& ok);
Maybe name this function "createSVGPathByteStreamFromString" to differentiate
from the other methods, which build upon an existing object, where this method
creates an object...

r=me, with these suggestions.