705640 - Implement DOMError as defined in DOM 4

Status: RESOLVED FIXED

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

Description

[Mounir Lamouri (:mounir)]

DOM 4 (previously DOM Core) introduces a DOMError object that contains the error name.
See: http://dvcs.w3.org/hg/domcore/raw-file/tip/Overview.html#interface-domerror

Comment 1

[Mounir Lamouri (:mounir)]

Actually, after speaking about that with Olli... why using DOMError instead of DOMString? Both would be better than the usual current long attribute but DOMError seems to add useless complexity.

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

In DOM 3 DOMError has more properties, and I can see some of those perhaps useful, but the
current DOMError in DOM4 is useless, IMHO.

Perhaps Anne or Ms2ger remember why DOM4 has such DOMError.

Comment 3

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

It serves as a replacement for FileError: <http://dev.w3.org/2006/webapi/FileAPI/#dfn-error-codes>.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Well, why does FileAPI use such useless thing?
Why FileReader.error couldn't be string?


/me files a spec bug.

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

http://www.w3.org/Bugs/Public/show_bug.cgi?id=14991

Comment 6

[Mounir Lamouri (:mounir)]

CCing Ben given that he asked me if that was in m-c.

Not yet ;)

Comment 7

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Stealing.

Comment 8

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Patch, v1

Should suffice!

Comment 9

[Mounir Lamouri (:mounir)]

Comment on attachment 590317 [details] [diff] [review]
Patch, v1

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

The way you declare your classes is very specific...

r=me with the comments taken into accounts.

::: dom/base/DOMError.cpp
@@ +7,5 @@
> +#include "DOMError.h"
> +
> +#include "nsDOMClassInfo.h"
> +
> +using mozilla::dom::DOMError;

AFAIK, what is recommended is:
namespace mozilla {
namespace dom {
}
}

That would require to move DOMCI_DATA before the namespaces though.

@@ +35,5 @@
> +    { 21, "URLMismatchError" },
> +    { 22, "QuotaExceededError" },
> +    { 23, "TimeoutError" },
> +    { 24, "InvalidNodeTypeError" },
> +    { 25, "DataCloneError" }, 

Wouldn't that be better to use the macros here instead of plain numbers?

::: dom/base/Makefile.in
@@ +65,5 @@
>  		$(NULL)
>  endif
>  
>  XPIDLSRCS = \
> +  nsIDOMDOMError.idl \

Why not in dom/interfaces/base?

Comment 10

[Ben Turner (not reading bugmail, use the needinfo flag!)]

(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #9)
>
> That would require to move DOMCI_DATA before the namespaces though.

This is precisely why I don't like doing the |namespace { }| thing... In the middle of the file it's hard to tell what namespace you're in and you end up with puzzling linker errors. A simple |using| makes everything better. 

> Wouldn't that be better to use the macros here instead of plain numbers?

What macros? There are no macros for this stuff that I know of.

> Why not in dom/interfaces/base?

I thought we were no longer going to split interfaces and implementation. Some day I expect someone to move dom/interfaces/* to the dom/* folders.

Comment 11

[Masatoshi Kimura [:emk]]

nsDOMDOMError.idl already exists in <https://mxr.mozilla.org/mozilla-central/source/dom/interfaces/core/> implementing old DOM3 Working Draft. It should be removed.

Comment 12

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

(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #9)
> Comment on attachment 590317 [details] [diff] [review]
> Patch, v1
> 
> Review of attachment 590317 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The way you declare your classes is very specific...
> 
> r=me with the comments taken into accounts.
> 
> ::: dom/base/DOMError.cpp
> @@ +7,5 @@
> > +#include "DOMError.h"
> > +
> > +#include "nsDOMClassInfo.h"
> > +
> > +using mozilla::dom::DOMError;
> 
> AFAIK, what is recommended is:
> namespace mozilla {
> namespace dom {
> }
> }

Indeed, do change this.

> That would require to move DOMCI_DATA before the namespaces though.

Wrapping DOMCI_CLASS in namespace too would work, IIRC.

Comment 13

[Ben Turner (not reading bugmail, use the needinfo flag!)]

(In reply to Ms2ger from comment #12)
> Indeed, do change this.

Is there some technical reason why this is superior? If this is just a personal preference then I'm inclined to leave it the way it is for the reason I mentioned above.

Comment 14

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

There is no reason for you to use another style than the rest of the code.

Comment 15

[Ben Turner (not reading bugmail, use the needinfo flag!)]

(In reply to Ms2ger from comment #14)
> There is no reason for you to use another style than the rest of the code.

Your assertion that the rest of the code does it differently is demonstrably false. We have both styles in lots of places. Unless there is a technical reason why one style is superior I don't see any reason to change this, especially since I laid out a simple reason for choosing this style.

I also can't find anything about this in our style guide (or google's, for that matter). If you feel strongly that we should only use one style then I suggest you bring the issue up on m.d.platform and let everyone weigh in.

Comment 16

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

Look better: https://developer.mozilla.org/En/Developer_Guide/Coding_Style#C.2B.2B_namespaces

Comment 17

[Ben Turner (not reading bugmail, use the needinfo flag!)]

(In reply to Ms2ger from comment #16)
> Look better:
> https://developer.mozilla.org/En/Developer_Guide/Coding_Style#C.2B.
> 2B_namespaces

I assume you're referring to this line (though hard to be sure, you seem to like terse comments!):

  using namespace ...; is only allowed in .cpp files after all #includes. Prefer to wrap code in namespace ... { ... }; instead if possible.

That's specifically about a "using namespace" directive, not about a using declaration for a particular class. The "using namespace" opens up the whole namespace for the entire file which of course might be risky (hence "Prefer to wrap code" to limit that exposure).

I'm really not interested in arguing about this further (you have yet to offer any technical reason why the other method is superior). If you feel strongly that we should only use one style for this kind of thing please take it to m.d.platform. I don't think such a discussion should block landing this patch, however, as we would have to change lots of code anyway.

Comment 18

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Mounir, waiting on you about this macros business. What was that about?

Comment 19

[Mounir Lamouri (:mounir)]

(In reply to ben turner [:bent] from comment #18)
> Mounir, waiting on you about this macros business. What was that about?

I was confused with something else. Forget that comment.

Comment 20

[Ed Morley [:emorley]]

Landed on inbound by bent as:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fcb3a7884fa9

But backed out for build failures:
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=fcb3a7884fa9

https://hg.mozilla.org/integration/mozilla-inbound/rev/f0eaab2dfe71

Comment 21

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Relanded with an include fix.

https://hg.mozilla.org/integration/mozilla-inbound/rev/143f01714f2b

Comment 22

[Ed Morley [:emorley]]

Backed out of inbound as it fails to build still (sorry):
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=143f01714f2b

https://hg.mozilla.org/integration/mozilla-inbound/rev/551030e6e380

Comment 23

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Ugh, mac's gcc can't figure out function-scoped structs apparently.

https://hg.mozilla.org/integration/mozilla-inbound/rev/117f6d70c584

Comment 24

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/mozilla-central/rev/117f6d70c584

Comment 25

[Florian Scholz (Open Web Docs)]

Updated https://developer.mozilla.org/en/DOM/DOMError
Mentioned on https://developer.mozilla.org/en/Firefox_12_for_developers#DOM