[webkit-reviews] review denied: [Bug 41410] SVG CleanUp of SVGPathData parsing : [Attachment 61881] 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 41410: SVG CleanUp of SVGPathData parsing
https://bugs.webkit.org/show_bug.cgi?id=41410

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

------- Additional Comments from Nikolas Zimmermann <zimmermann at kde.org>
Ok, I tried hard, that's all I could find: :-)

JavaScriptCore/wtf/MathExtras.h:58
 +  const double piOverTwoDouble = 1.57079632679489661923;
Needs a ChangeLog.

WebCore/ChangeLog:5
 +	    SVG CleanUp of SVGPathData parsing
Please provide a _detailed_ ChangeLog what happens, so others know it too :-)

WebCore/WebCore.vcproj/WebCore.vcproj:2197
 +  
Still newlines here, please remove.

WebCore/svg/SVGPathConsumer.h:60
 +  
A newline too much.

WebCore/svg/SVGPathParser.cpp:65
 +	    if (m_mode == RelativeCoordinates)
This still looks weird. How about:

if (m_mode == RelativeCoordinates)
    m_currentPoint += toPoint;
else
    m_currentPoint = toPoint;

m_subPathPoint = m_currentPoint;

WebCore/svg/SVGPathParser.cpp:86
 +		m_currentPoint = m_currentPoint + toPoint;
m_currentPoint += toPoint;

WebCore/svg/SVGPathParser.cpp:175
 +	if (!(m_lastCommand == 'c'
How about reversing this check:
if (m_lastCommand != 'c' && m_...)
Boolean algebra simplifications :-)

WebCore/svg/SVGPathParser.cpp:243
 +	if (!(m_lastCommand == 'q'
Ditto.

WebCore/svg/SVGPathParser.cpp:274
 +	bool largeArc, sweep;
Two lines needed unfortuantely.

WebCore/svg/SVGPathParser.cpp:290
 +	// Spec: radii are nonnegative numbers
Make this a whole sentence please with a trailing period.

WebCore/svg/SVGPathParser.cpp:402
 +		if (command == 'M')
I wish this was commented :(

WebCore/svg/SVGPathParser.cpp:404
 +		else if (command == 'm')
Ditto. But maybe you don't even know why it's needed? Given that it's old
legacy code...

WebCore/svg/SVGPathParser.cpp:454
 +	if (scaleFactorSquared < 0)
Can you use float scaleFactorSquared = std::min(1 / d - 0.25f, 0)?

WebCore/svg/SVGPathParser.cpp:484
 +	    if (!isfinite(t))
This seems wrong, isfinite returns a non-zero value if it's finite! So you
return here, if the value is finite.