249048 - Implement SVG exceptions

Status: RESOLVED FIXED

(Core :: SVG, enhancement)

Description

[Jonathan Watt [:jwatt]]

Comment 1

[Jonathan Watt [:jwatt]]

Attachment: nsIDOMSVGException.idl

Comment 2

[Jonathan Watt [:jwatt]]

Attachment: nsSVGException.h

Comment 3

[Jonathan Watt [:jwatt]]

Attachment: nsSVGException.cpp

Comment 4

[Jonathan Watt [:jwatt]]

Attachment: nsSVGError.h

Comment 5

[Jonathan Watt [:jwatt]]

Attachment: broken patch

Comment 6

[Jonathan Watt [:jwatt]]

Okay, I think I need some help from someone who understands the existing DOM
code a lot better than I do. Peter can you help since you wrote the macros in
nsIBaseDOMException.h that I'm using? I think using them is the "right" thing to
do. The files and patch I just attached produce the following linker error: 

Creating library gklayout.lib and object gklayout.exp
gkcontentsvg_s.lib(nsSVGException.obj) : error LNK2019: unresolved external
symbol "class nsISupports * SVGException_classInfoGlobal"
(?SVGException_classInfoGlobal@@3PAVnsISupports@@A) referenced in function
"public: virtual unsigned int __stdcall nsSVGException::QueryInterface(struct
nsID const &,void * *)" (?QueryInterface@nsSVGException@@UAGIABUnsID@@PAPAX@Z)
gklayout.dll : fatal error LNK1120: 1 unresolved externals

I guess this is because IMPL_DOM_EXCEPTION_TAIL uses
NS_INTERFACE_MAP_ENTRY_EXTERNAL_DOM_CLASSINFO instead of
NS_INTERFACE_MAP_ENTRY_CONTENT_CLASSINFO, but I'm not sure what the correct
course of action is to fix this. I tried throwing in a
NS_DECL_DOM_CLASSINFO(SVGException) just after the IMPL_DOM_EXCEPTION_TAIL which
got it to build, but then null gets thrown as the exception instead of an
SVGException. 

Comment 7

[Peter Van der Beken [:peterv]]

Oh, hmm, SVG is inside Gecko and what I showed you was for implementing DOM
exceptions outside of Gecko. Sorry about that. I'm sure we can make it work, but
since I doubt we'll ever be able to move SVG outside of Gecko we might as well
use the internal stuff.
The internal DOM exceptions are in
http://lxr.mozilla.org/seamonkey/source/dom/src/base/nsDOMException.cpp and
http://lxr.mozilla.org/seamonkey/source/dom/src/base/nsDOMException.h Either we
define the SVG exception in nsDOMException.cpp, or we move the macros into the
header and you include that in nsSVGException. Then declare your class with

DECL_INTERNAL_DOM_EXCEPTION(SVGException);

and define it with

IMPL_INTERNAL_DOM_EXCEPTION_HEAD(nsSVGException, nsIDOMSVGException)
  NS_DECL_NSIDOMRANGEEXCEPTION
IMPL_INTERNAL_DOM_EXCEPTION_TAIL(nsSVGException, nsIDOMSVGException,
                                 SVGException, NS_ERROR_MODULE_DOM_SVG,
                                 NSResultToNameAndMessage)

I suppose it doesn't make sense to have nsSVGException for five lines :-/. Then
add the error code and their corresponding message in domerr.msg.

Sorry again about pointing you in the wrong direction.

Comment 8

[Peter Van der Beken [:peterv]]

Replace NS_DECL_NSIDOMRANGEEXCEPTION in comment 7 with
NS_DECL_NSIDOMSVGEXCEPTION and you'll also have to implement
nsSVGException::GetCode(...).

Comment 9

[Jonathan Watt [:jwatt]]

Peter, my preference would be to have SVGException in its own file. Is that okay
by you? BTW, why does the existing code declare functions just above where they
are defined and create global variables to point to the char arrays? Is what
I've done in the attachment above for nsSVGException.cpp okay? 

Comment 10

[Peter Van der Beken [:peterv]]

(In reply to comment #9)
> Peter, my preference would be to have SVGException in its own file. Is that okay
> by you?

Yes.

> BTW, why does the existing code declare functions just above where they
> are defined

I have no idea, want to clean that up?

> and create global variables to point to the char arrays?

Are you talking about gDOMErrorMsgMap? How else would you do it if the error
codes are defined in domerr.msg? The way you do it in nsSVGException is ok if
there's not too many codes. BTW, please prefix the codes with NS_ERROR_DOM_ (eg
NS_ERROR_DOM_SVG_WRONG_TYPE_ERR).

Comment 11

[Jonathan Watt [:jwatt]]

(In reply to comment #10)
> > BTW, why does the existing code declare functions just above where they
> > are defined
> 
> I have no idea, want to clean that up?

Sure, I was thinking of code like
http://lxr.mozilla.org/mozilla/source/dom/public/nsIBaseDOMException.h#102
http://lxr.mozilla.org/mozilla/source/extensions/transformiix/source/xpath/nsXPathException.cpp#50

I'll change the former since that is under dom/

> > and create global variables to point to the char arrays?
> 
> Are you talking about gDOMErrorMsgMap? How else would you do it if the error
> codes are defined in domerr.msg? 

No, I meant the code at
http://lxr.mozilla.org/mozilla/source/extensions/transformiix/source/xpath/nsXPathException.cpp#45

> BTW, please prefix the codes with NS_ERROR_DOM_ (eg
> NS_ERROR_DOM_SVG_WRONG_TYPE_ERR).

Should I also change the error module from NS_ERROR_MODULE_SVG to
NS_ERROR_MODULE_DOM_SVG? I wasn't going to do that since I thought that _DOM_XXX
was really for W3C DOM modules like Range etc. 

Comment 12

[Jonathan Watt [:jwatt]]

Attachment: patch

I've changed my mind. Although I'd like to have separate files for
SVGException, there doesn't seem to be a good way to work the macros without
creating a separate common header to include them into nsDOMException.h and
nsSVGException.h. This new patch just makes the necessary changes to the
current internal DOM exception stuff.

Comment 13

[Jonathan Watt [:jwatt]]

Attachment: patch

Without accidental change to nsDOMClassInfo.cpp

Comment 14

[Jonathan Watt [:jwatt]]

Comment on attachment 152812 [details]
nsIDOMSVGException.idl

...and we'll still need this...

Comment 15

[Jonathan Watt [:jwatt]]

Attachment: TESTCASE

Everything seems to work as expected except for the exception's location
property. But I don't think that's related to this bug.

Comment 16

[Alex Fritze]

Comment on attachment 153114 [details] [diff] [review]
patch

r=afri

>Index: mozilla/content/shared/public/nsSVGAtomList.h
>===================================================================
>[...] 
>+SVG_ATOM(SVGException, "SVGException")

Not needed.

>Index: mozilla/dom/src/base/domerr.msg
>===================================================================
>[...]
>+DOM_MSG_DEF(NS_ERROR_DOM_SVG_WRONG_TYPE_ERR, "The parameter is the wrong type of object")
This error is also raised for invalid unitTypes, so I think the message should
be more generic. Maybe:
"Unknown or invalid type"

>+DOM_MSG_DEF(NS_ERROR_DOM_SVG_MATRIX_NOT_INVERTABLE, "The currently defined transformation matrices make it impossible to compute the given matrix")
That message is only appropriate to 'getTransformToElement', not
matrix.invert(), so again I think we need something more generic. Maybe:
"The matrix is not invertable" or
"The matrix is not computable".

Comment 17

[Jonathan Watt [:jwatt]]

Attachment: updated patch

updated to address alex's comments

Comment 18

[Peter Van der Beken [:peterv]]

Comment on attachment 153180 [details] [diff] [review]
updated patch

>? mozilla/dom/public/idl/svg/nsIDOMSVGException.idl

I'm assuming this part didn't change from the previous patch.

>Index: mozilla/dom/src/base/nsDOMScriptObjectFactory.cpp
>===================================================================

> NS_IMETHODIMP
> nsDOMScriptObjectFactory::GetException(nsresult result,
> 				       nsIException *aDefaultException,
> 				       nsIException **_retval)
> {
>   if (NS_ERROR_GET_MODULE(result) == NS_ERROR_MODULE_DOM_RANGE) {

>+  if (NS_ERROR_GET_MODULE(result) == NS_ERROR_MODULE_SVG) {

I'd store the result of NS_ERROR_GET_MODULE in a local var

Comment 19

[Jonathan Watt [:jwatt]]

(In reply to comment #18)
> (From update of attachment 153180 [details] [diff] [review])
> >? mozilla/dom/public/idl/svg/nsIDOMSVGException.idl
> 
> I'm assuming this part didn't change from the previous patch.

no

> >Index: mozilla/dom/src/base/nsDOMScriptObjectFactory.cpp
> >===================================================================
> 
> > NS_IMETHODIMP
> > nsDOMScriptObjectFactory::GetException(nsresult result,
> > 				       nsIException *aDefaultException,
> > 				       nsIException **_retval)
> > {
> >   if (NS_ERROR_GET_MODULE(result) == NS_ERROR_MODULE_DOM_RANGE) {
> 
> >+  if (NS_ERROR_GET_MODULE(result) == NS_ERROR_MODULE_SVG) {
> 
> I'd store the result of NS_ERROR_GET_MODULE in a local var
> 

I'd prefer to use a switch statement, is that okay?

Comment 20

[Jonathan Watt [:jwatt]]

Attachment: final patch

Alex, can you add nsIDOMSVGException.idl and check in this patch?

Comment 21

[tor]

Checked in.