Closed Bug 705640 Opened 13 years ago Closed 12 years ago

Implement DOMError as defined in DOM 4

Categories

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

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla12

People

(Reporter: mounir, Assigned: bent.mozilla)

References

Details

(Keywords: dev-doc-complete)

Attachments

(1 file)

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
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.
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>.
Well, why does FileAPI use such useless thing?
Why FileReader.error couldn't be string?


/me files a spec bug.
Blocks: 708546
CCing Ben given that he asked me if that was in m-c.

Not yet ;)
Stealing.
Assignee: nobody → bent.mozilla
Attached patch Patch, v1Splinter Review
Should suffice!
Attachment #590317 - Flags: review?(khuey)
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.
(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?
(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.
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
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla12
You need to log in before you can comment on or make changes to this bug.