[Webkit-unassigned] [Bug 34328] Manipulating SVG element attributes in Javascript does not work as expected

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=34328


Nikolas Zimmermann <zimmermann at kde.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #56900|review?                     |review-
               Flag|                            |




--- Comment #2 from Nikolas Zimmermann <zimmermann at kde.org>  2010-05-26 05:18:25 PST ---
(From update of attachment 56900)
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

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.