Last Comment Bug 705640 - Implement DOMError as defined in DOM 4
: Implement DOMError as defined in DOM 4
Status: RESOLVED FIXED
: dev-doc-complete
Product: Core
Classification: Components
Component: DOM: Core & HTML (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla12
Assigned To: Ben Turner (not reading bugmail, use the needinfo flag!)
:
Mentors:
Depends on:
Blocks: 708546 732769
  Show dependency treegraph
 
Reported: 2011-11-28 04:42 PST by Mounir Lamouri (:mounir)
Modified: 2012-04-18 10:06 PDT (History)
12 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch, v1 (8.30 KB, patch)
2012-01-20 13:23 PST, Ben Turner (not reading bugmail, use the needinfo flag!)
mounir: review+
Details | Diff | Review

Description Mounir Lamouri (:mounir) 2011-11-28 04:42:23 PST
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) 2011-11-28 04:57:52 PST
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] 2011-11-28 09:14:01 PST
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 2011-11-28 13:51:12 PST
It serves as a replacement for FileError: <http://dev.w3.org/2006/webapi/FileAPI/#dfn-error-codes>.
Comment 4 Olli Pettay [:smaug] 2011-11-29 00:27:40 PST
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] 2011-11-29 00:31:44 PST
http://www.w3.org/Bugs/Public/show_bug.cgi?id=14991
Comment 6 Mounir Lamouri (:mounir) 2012-01-14 14:47:33 PST
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!) 2012-01-14 19:05:23 PST
Stealing.
Comment 8 Ben Turner (not reading bugmail, use the needinfo flag!) 2012-01-20 13:23:05 PST
Created attachment 590317 [details] [diff] [review]
Patch, v1

Should suffice!
Comment 9 Mounir Lamouri (:mounir) 2012-01-20 15:38:02 PST
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!) 2012-01-20 16:10:55 PST
(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] 2012-01-20 21:20:08 PST
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 2012-01-21 01:02:17 PST
(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!) 2012-01-21 05:41:40 PST
(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 2012-01-21 06:29:26 PST
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!) 2012-01-21 07:20:41 PST
(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 17 Ben Turner (not reading bugmail, use the needinfo flag!) 2012-01-21 09:20:33 PST
(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!) 2012-01-21 09:27:33 PST
Mounir, waiting on you about this macros business. What was that about?
Comment 19 Mounir Lamouri (:mounir) 2012-01-21 15:19:03 PST
(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 21 Ben Turner (not reading bugmail, use the needinfo flag!) 2012-01-23 05:59:17 PST
Relanded with an include fix.

https://hg.mozilla.org/integration/mozilla-inbound/rev/143f01714f2b
Comment 22 Ed Morley [:emorley] 2012-01-23 06:29:46 PST
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!) 2012-01-24 02:15:56 PST
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) 2012-01-24 10:45:10 PST
https://hg.mozilla.org/mozilla-central/rev/117f6d70c584
Comment 25 Florian Scholz [:fscholz] (MDN) 2012-04-18 10:06:11 PDT
Updated https://developer.mozilla.org/en/DOM/DOMError
Mentioned on https://developer.mozilla.org/en/Firefox_12_for_developers#DOM

Note You need to log in before you can comment on or make changes to this bug.