Closed Bug 743888 Opened 9 years ago Closed 9 years ago
Replace SVGException and XPath
Exception with DOMException
We need a spec update first unless I'm overlooking.
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?
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.
Posted to the WG: http://lists.w3.org/Archives/Public/www-svg/2012Apr/0001.html
http://wiki.whatwg.org/wiki/DOM_XPath is errata for DOM XPath btw.
Assignee: nobody → VYV03354
Status: NEW → ASSIGNED
Attachment #615097 - Flags: review?(jonas)
Update exception types based on comment #2. It didn't mention SVG_WRONG_TYPE_ERR, so I changed it to JS TypeError.
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.
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
Will SVG2 define a new Web IDL exception? Or just reuse DOMException?
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.)
(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 ? > (By the way, is SVG_WRONG_TYPE_ERR actually being thrown for anything > currently? Our implementation does . > 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?  http://dvcs.w3.org/hg/domcore/raw-file/tip/Overview.html#error-types-0  https://mxr.mozilla.org/mozilla-central/search?string=SVG_WRONG_TYPE_ERR
(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 ? 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 . > > > 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.)
I sent this: http://www.w3.org/mid/4F99DF00.firstname.lastname@example.org
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.
> 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.
Or just make nsIDOMSVGNumber builtinclass already.
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)
Updated to tip
Updated to tip
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?
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
I don't think consistency is important in this case, personally.
SVGStringList is very similar to DOMTokenList form DOM Core . 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.  http://dvcs.w3.org/hg/domcore/raw-file/tip/Overview.html#domtokenlist
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
Attachment #625634 - Flags: feedback?(jwatt) → feedback+
Attachment #625469 - Flags: review?(jwatt) → review+
patch for checkin
Target Milestone: --- → mozilla16
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.