[webkit-reviews] review requested: [Bug 28828] Make SVGStyleElement title/media/type setters do something : [Attachment 38811] Patch v2

[bugzilla-daemon at webkit.org]

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

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

------- Additional Comments from Cameron McCormack <cam at mcc.id.au>
(In reply to comment #2)
> shouldBeEqualToString will make your life easier here:
>  5 shouldBe("style.type", "'text/css'");
> 
> shouldBeEqualToString("style.type", "text/css");

OK, changed to use that.  I just copied some other test that was using
shouldBe() in this manner.

> 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)

Changed.

> 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.

The spec says to raise these in case the DOM node is read only, but in reality
this is never the case.  Strictly, it can happen if the <style> is a descendant
of an Entity or EntityReference node.  Various methods would need to check
isReadOnly() and throw a NO_MODIFICATION_ALLOWED_ERR in this case.  Does
XMLTokenizerLibxml2 ever cause parsed XML documents to include EntityReference
nodes instead of expanding them?  I couldn't tell from a quick look.

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

Thanks!  WebKit code is pretty nice to work with. :-)