Bug 28828 – Make SVGStyleElement title/media/type setters do something

• Status: RESOLVED
• Resolution: FIXED
• Component: SVG

[Cameron McCormack (:heycam)]

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.

[Cameron McCormack (:heycam)]

Created attachment 38774 [details]
Patch v1

[Eric Seidel (no email)]

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.

[Cameron McCormack (:heycam)]

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

[Eric Seidel (no email)]

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

[Eric Seidel (no email)]

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.

[Cameron McCormack (:heycam)]

Created attachment 38957 [details]
Patch v3

OK, that's reasonable about passing along the ExceptionCode.

[Cameron McCormack (:heycam)]

Comment on attachment 38957 [details]
Patch v3

How about I test it first before requesting review...

[Cameron McCormack (:heycam)]

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.

[Cameron McCormack (:heycam)]

Created attachment 38966 [details]
Patch v4a

Sigh, don't edit patches by hand without checking them. :/

[Eric Seidel (no email)]

Comment on attachment 38966 [details]
Patch v4a

Wonderful!

[Eric Seidel (no email)]

Comment on attachment 38966 [details]
Patch v4a

Clearing flags on attachment: 38966

Committed r48010: <http://trac.webkit.org/changeset/48010>

[Eric Seidel (no email)]

All reviewed patches have been landed.  Closing bug.