setTitle(), setMedia() and setType() on SVGStyleElement should do something other than just throwing a NOT_MODIFIED_ERR. Also remove the "setter raises" clauses from the IDL, since an exception should never be thrown.
Created attachment 38774 [details] Patch v1
Comment on attachment 38774 [details] Patch v1 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.
Created attachment 38811 [details] Patch v2 (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. :-)
Comment on attachment 38811 [details] Patch v2 Shouldn't we just pass the ec along to setAttribute? Then we future-proof ourselves against the read-only node case, no? void setAttribute(const QualifiedName&, const AtomicString& value, ExceptionCode&);
Comment on attachment 38811 [details] Patch v2 r- for the unanswered setter issue. Seems no reason to remove these exception codes if setAttribute can/should still throw.
Created attachment 38957 [details] Patch v3 OK, that's reasonable about passing along the ExceptionCode.
Comment on attachment 38957 [details] Patch v3 How about I test it first before requesting review...
Created attachment 38965 [details] Patch v4 Turns out I couldn't use [Reflect] for title, since there needs to be a title() method on SVGStyleElement to override the one on StyleElement. Oh well. I had some changes to the ObjC bindings generators to make them work with [Reflect]ed attributes with setter exceptions, but I might file them separately.
Created attachment 38966 [details] Patch v4a Sigh, don't edit patches by hand without checking them. :/
Comment on attachment 38966 [details] Patch v4a Wonderful!
Comment on attachment 38966 [details] Patch v4a Clearing flags on attachment: 38966 Committed r48010: <http://trac.webkit.org/changeset/48010>
All reviewed patches have been landed. Closing bug.