Implement DOMError as defined in DOM 4

RESOLVED FIXED in mozilla12

Status

()

Core
DOM: Core & HTML
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: mounir, Assigned: Ben Turner (not reading bugmail, use the needinfo flag!))

Tracking

({dev-doc-complete})

Trunk
mozilla12
dev-doc-complete
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

6 years ago
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
(Reporter)

Comment 1

6 years ago
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

6 years ago
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.
It serves as a replacement for FileError: <http://dev.w3.org/2006/webapi/FileAPI/#dfn-error-codes>.

Comment 4

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


/me files a spec bug.

Comment 5

6 years ago
http://www.w3.org/Bugs/Public/show_bug.cgi?id=14991
(Reporter)

Updated

6 years ago
Blocks: 708546
(Reporter)

Comment 6

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

Not yet ;)
Stealing.
Assignee: nobody → bent.mozilla
Created attachment 590317 [details] [diff] [review]
Patch, v1

Should suffice!
Attachment #590317 - Flags: review?(khuey)
(Reporter)

Comment 9

5 years ago
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?
Attachment #590317 - Flags: review?(khuey) → review+
(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.
nsDOMDOMError.idl already exists in <https://mxr.mozilla.org/mozilla-central/source/dom/interfaces/core/> implementing old DOM3 Working Draft. It should be removed.
(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.
(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.
There is no reason for you to use another style than the rest of the code.
(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.
Look better: https://developer.mozilla.org/En/Developer_Guide/Coding_Style#C.2B.2B_namespaces
(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.
Mounir, waiting on you about this macros business. What was that about?
(Reporter)

Comment 19

5 years ago
(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.
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
Status: NEW → ASSIGNED
Relanded with an include fix.

https://hg.mozilla.org/integration/mozilla-inbound/rev/143f01714f2b
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
Ugh, mac's gcc can't figure out function-scoped structs apparently.

https://hg.mozilla.org/integration/mozilla-inbound/rev/117f6d70c584
https://hg.mozilla.org/mozilla-central/rev/117f6d70c584
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla12
Keywords: dev-doc-needed

Updated

5 years ago
Blocks: 732769
Updated https://developer.mozilla.org/en/DOM/DOMError
Mentioned on https://developer.mozilla.org/en/Firefox_12_for_developers#DOM
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.