[webkit-reviews] review denied: [Bug 34328] Manipulating SVG element attributes in Javascript does not work as expected : [Attachment 56900] Patch

[bugzilla-daemon at webkit.org]

Nikolas Zimmermann <zimmermann at kde.org> has denied Rob Buis
<rwlbuis at gmail.com>'s request for review:
Bug 34328: Manipulating SVG element attributes in Javascript does not work as
expected
https://bugs.webkit.org/show_bug.cgi?id=34328

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

------- Additional Comments from Nikolas Zimmermann <zimmermann at kde.org>
Hi Rob,

in general the patch looks fine, though some comments, regarding code & test:
We're not adding new tests using the "manual style", but instead make use of
make-js-test-wrappers.
You should add a test under svg/custom/script-tests/foo.js, and use
make-js-test-wrappers to let it create svg/custom/foo.html for you.
This way you dynamically create the testcase, and use the existing nice
framework for logging / verifying (shouldBe, etc..).
Can you have a look and change that?

Some notes on the code:
* Can you make parseViewBox take a "const String&" parameter, that would save
the need to pass in two pointers. Sounds safer to encapsulate that code in
parseViewBox, instead of letting the callers do that.
* I'd suggest to change parseViewBox to take a "FloatRect&" parameter instead
of four individual floats. That would make the code more readable:
FloatRect viewBox;
if (!attr->value().isNull()) {
    if (!parseViewBox(document, attr->value(), viewBox))
	return true;
}
setViewBoxBaseValue(viewBox);

Thanks in advance,
Niko