743888 - Replace SVGException and XPathException with DOMException

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Masatoshi Kimura [:emk]]

We need a spec update first unless I'm overlooking.

Comment 1

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I think we can remove XPathException without any spec changes. While ideally the spec should be updated, DOM-XPath is no longer maintained by anyone so it's unlikely that we'll get such an update.

What I think we should do is to change NS_ERROR_DOM_INVALID_EXPRESSION_ERR into a SyntaxError, and NS_ERROR_DOM_TYPE_ERR into a TypeError. I'm happy to send this to the WebApps list where I think all other vendors that implement XPathException hang out. That way we can probably come to consensus among browsers even though the spec itself remains outdated.


As for SVGException: Heycam, Jwatt, do you have a sense for if we can get the SVG spec changed soon-ish to use new better exceptions?

Comment 2

[Cameron McCormack (:heycam)]

I think we can definitely replace the two (used) SVGExceptions SVG_INVALID_VALUE_ERR and SVG_MATRIX_NOT_INVERTABLE with predefined exceptions.  Not sure what timeframe we can get it done in.  If you need it in the spec (rather than just say a resolution of the WG) then I can get to it in a couple of weeks.

For the cases where SVG_INVALID_VALUE_ERR is being thrown because of a bad DOMString value, that should become a SyntaxError.

For the cases where it's being used for an out of range integer or float value, maybe a RangeError?  What do other specs throw?

For SVG_MATRIX_NOT_INVERTABLE a DOMException of type "InvalidStateError" looks like a good enough match.

Comment 3

[Cameron McCormack (:heycam)]

Posted to the WG: http://lists.w3.org/Archives/Public/www-svg/2012Apr/0001.html

Comment 4

[Anne (:annevk)]

http://wiki.whatwg.org/wiki/DOM_XPath is errata for DOM XPath btw.

Comment 5

[Masatoshi Kimura [:emk]]

Attachment: Part 1: Remove SVGException and XPathException

Comment 6

[Masatoshi Kimura [:emk]]

Attachment: Part 2: Update SVG exception types

Update exception types based on comment #2.
It didn't mention SVG_WRONG_TYPE_ERR, so I changed it to JS TypeError.

Comment 7

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 615097 [details] [diff] [review]
Part 1: Remove SVGException and XPathException

Review of attachment 615097 [details] [diff] [review]:
-----------------------------------------------------------------

Looks great. But jwatt or one of the SVG peers should sign off on the SVG changes.

Comment 8

[Cameron McCormack (:heycam)]

We discussed the SVGException change in the SVG WG telcon today and resolved on it.
http://www.w3.org/2012/04/26-svg-minutes.html#item03

Comment 9

[Masatoshi Kimura [:emk]]

Will SVG2 define a new Web IDL exception? Or just reuse DOMException?

Comment 10

[Cameron McCormack (:heycam)]

There weren't any preferences for specific exception types, so I think going along with the comment 2 suggestions are fine:

* use the built-in exception SyntaxError in place of SVG_INVALID_VALUE_ERR where
  it was being thrown due to a bad string value
* use the built-in exception RangeError in place of SVG_INVALID_VALUE_ERR where
  it was being thrown due to an out of range numeric value
* use the InvalidStateError DOMException in place of SVG_MATRIX_NOT_INVERTABLE

I have the action to edit the spec but won't be able to get to it soon.

(By the way, is SVG_WRONG_TYPE_ERR actually being thrown for anything currently?  There was something in the SVG 1.1 First Edition spec that threw this exception but this was removed from in the Second Edition spec.)

Comment 11

[Masatoshi Kimura [:emk]]

(In reply to Cameron McCormack (:heycam) from comment #10)
> * use the built-in exception SyntaxError in place of SVG_INVALID_VALUE_ERR
> where
>   it was being thrown due to a bad string value
Not DOM4 SyntaxError [1]?

> (By the way, is SVG_WRONG_TYPE_ERR actually being thrown for anything
> currently?
Our implementation does [2].

> There was something in the SVG 1.1 First Edition spec that threw
> this exception but this was removed from in the Second Edition spec.)
What should be thrown instead?

[1] http://dvcs.w3.org/hg/domcore/raw-file/tip/Overview.html#error-types-0
[2] https://mxr.mozilla.org/mozilla-central/search?string=SVG_WRONG_TYPE_ERR

Comment 12

[Cameron McCormack (:heycam)]

(In reply to Masatoshi Kimura [:emk] from comment #11)
> (In reply to Cameron McCormack (:heycam) from comment #10)
> > * use the built-in exception SyntaxError in place of SVG_INVALID_VALUE_ERR
> > where
> >   it was being thrown due to a bad string value
> Not DOM4 SyntaxError [1]?

Hmm, I forgot that DOM has a "SyntaxError" type for the old SYNTAX_ERR code.  Having that as well as the built-in SyntaxError seems confusing.  We should probably be promoting the use of one over the other, but that should be discussed on the www-dom list.  I'll send a mail.

> > (By the way, is SVG_WRONG_TYPE_ERR actually being thrown for anything
> > currently?
> Our implementation does [2].
> 
> > There was something in the SVG 1.1 First Edition spec that threw
> > this exception but this was removed from in the Second Edition spec.)
> What should be thrown instead?

The SVG_WRONG_TYPE_ERR exceptions thrown from the SVGXXXList methods were removed from the Second Edition.  That's because if the wrong kind of object is passed to for example SVGNumberList.Initialize(), then a TypeError should be thrown before we even get into the method.  The checks that we do in our list methods, QIing to interfaces like DOMSVGNumber -- I am not really sure in what cases this can happen, so they seem more like internal errors to me.

Anyway, I think it's fine to replace these all these with TypeErrors.  (The alternative would be to return something like NS_ERROR_FAILURE.)

Comment 13

[Cameron McCormack (:heycam)]

I sent this: http://www.w3.org/mid/[e-mail]

Comment 14

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I agree that having two SyntaxError types will be confusing. But I don't think we should block this bug on figuring that out. Can we simply use a DOM4 SyntaxError for now and file a followup if needed? (Such a followup might change things beyond the scope of this bug anyway).

The QIs can fail if you pass a non-DOM JS-object which can be QIed to nsIDOMSVGNumber to for example the initialize function. This can currently happen because XPConnect by default will let JS implement almost any type of interface. Eventually that will be prevented as we roll out the new DOM bindings which have proper WebIDL support.

For now I think simply throwing a DOM TypeError is as close as we can get with XPConnect, which is what the patch does.

Comment 15

[Cameron McCormack (:heycam)]

> This can currently happen because XPConnect by default will let JS implement almost
> any type of interface.

Gotcha.

> For now I think simply throwing a DOM TypeError is as close as we can get with
> XPConnect, which is what the patch does.

Sounds good to me.

Comment 16

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Or just make nsIDOMSVGNumber builtinclass already.

Comment 17

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

We should mark a whole slew of interfaces builtinonly, and simplify a bunch of code after. But that's separate from this bug (and might not be worth doing considering that we're moving to new bindings anyway)

Comment 18

[Masatoshi Kimura [:emk]]

Attachment: Part 2: Update SVG exception types

Updated to tip

Comment 19

[Masatoshi Kimura [:emk]]

Attachment: Part 1: Remove SVGException and XPathException

Updated to tip

Comment 20

[Jonathan Watt [:jwatt]]

Comment on attachment 625469 [details] [diff] [review]
Part 2: Update SVG exception types

I think using SyntaxError is just wrong. I think what we need in a very general InvalidArgError that we can use for all sorts of things when we want to draw an author's attention to a problem with an argument. What would need to happen to get that? Can we just add it, or does it need to be spec'ed somewhere?

Comment 21

[Masatoshi Kimura [:emk]]

I agree about JS SyntaxError, but it would be appropriate to use DOM4 SyntaxError for consistency with other specs' usage pattern, IMO.
For example:
http://www.w3.org/TR/html5/apis-in-html-documents.html#dom-insertadjacenthtml
http://dev.w3.org/html5/spec/media-elements.html#dom-media-addtexttrack
http://dev.w3.org/html5/spec/editing.html#contenteditable

Comment 22

[Jonathan Watt [:jwatt]]

I don't think consistency is important in this case, personally.

Comment 23

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

SVGStringList is very similar to DOMTokenList form DOM Core [1]. Basically they provide the same functionality but with different syntax.

DOMTokenList throws SyntaxError for empty strings.

I agree it's not a perfect match, but it seems close enough that the alternative is worse. I.e. introducing a new exception which authors have to learn seems worse.

If you still want to propose a new exception, could you raise that on the SVG in response to heycam's email in comment 3.

[1] http://dvcs.w3.org/hg/domcore/raw-file/tip/Overview.html#domtokenlist

Comment 24

[Jonathan Watt [:jwatt]]

Comment on attachment 625634 [details] [diff] [review]
Part 1: Remove SVGException and XPathException

I haven't got time to pursue this in the WG, and I guess I don't care enough to block it. r=me

Comment 25

[Masatoshi Kimura [:emk]]

Attachment: Part 1: Remove SVGException and XPathException. r=jonas, feedback=jwatt

patch for checkin

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/42056a433aee
https://hg.mozilla.org/integration/mozilla-inbound/rev/c051f2810865

Comment 27

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/42056a433aee
https://hg.mozilla.org/mozilla-central/rev/c051f2810865