[webkit-reviews] review denied: [Bug 28828] Make SVGStyleElement title/media/type setters do something : [Attachment 38774] Patch v1

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Mon Aug 31 03:17:14 PDT 2009


Eric Seidel <eric at webkit.org> has denied Cameron McCormack <cam at mcc.id.au>'s
request for review:
Bug 28828: Make SVGStyleElement title/media/type setters do something
https://bugs.webkit.org/show_bug.cgi?id=28828

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

------- Additional Comments from Eric Seidel <eric at webkit.org>
shouldBeEqualToString will make your life easier here:
 5 shouldBe("style.type", "'text/css'");

shouldBeEqualToString("style.type", "text/css");

Normally we would use longer argument names here:
 53 void SVGStyleElement::setXmlspace(const AtomicString& s, ExceptionCode&)
like "space"

and "type":
 65 void SVGStyleElement::setType(const AtomicString& s)

Why not just remove "setter raises()"?
 33			 /*setter raises(DOMException)*/;

Are these spec'd to raise, but they never should?  If so, please explain in
your ChangeLog.

In general this looks great!  If you were a committer, I would r+ and you could
land with changes.


More information about the webkit-reviews mailing list