[webkit-reviews] review denied: [Bug 25645] SVG - numeric overflow for very large elements : [Attachment 57913] Revised patch to fix SVG number parsing of large numbers

[bugzilla-daemon at webkit.org]

Nikolas Zimmermann <zimmermann at kde.org> has denied W. James MacLean
<wjmaclean at chromium.org>'s request for review:
Bug 25645: SVG - numeric overflow for very large elements
https://bugs.webkit.org/show_bug.cgi?id=25645

Attachment 57913: Revised patch to fix SVG number parsing of large numbers
https://bugs.webkit.org/attachment.cgi?id=57913&action=review

------- Additional Comments from Nikolas Zimmermann <zimmermann at kde.org>
Hi W. James, almost r+. Your patch is missing a proper ChangeLog (use
prepare-Changelog --bug=25645, to generate a template to be filled out).
The style bot also reported issues regarding trailing spaces that need to be
fixed.Some more comments:

WebCore/platform/graphics/FloatRect.cpp:124
 +	float lf = floorf(rect.x());
Please use descriptive variable names like "left" "right" "top" "bottom" here.

WebCore/platform/graphics/FloatRect.cpp:126
 +	float wf = ceilf(rect.right()) - lf ;
Why do we need to use right - left here to figure out the width? Why is using
width() not sufficient?

WebCore/svg/SVGParserUtilities.cpp:83
 +	const UChar *ptr2 = ptr ;
Wrong style: you need to use "const UChar*" (star next to type). Also "ptr2" is
not a good name, please use longer descriptive names.

Waiting your new patch :-)
I guess you tested running all webkit tests - enclosingIntRect() is used in
lots of places - and I am afraid of side-effects in tests. Please rule that
out.