Not sure whether this needs to be split to several parts

If I'm reading the patch correctly, this doesn't let us define WebIDL-only generated events. Is that correct? I'd like to have that for Web Audio events -- do you expect any major difficulties in implementing that?

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #5) > If I'm reading the patch correctly, this doesn't let us define WebIDL-only > generated events. Is that correct? Yes, that is correct. > I'd like to have that for Web Audio > events -- do you expect any major difficulties in implementing that? Well, such codegenerator will need to use webidl parser as basis. This bug is about existing generated events (of which most are used in addons too, so we need .idl interface anyway)

Also, we don't have webidl interfaces for events which doesn't have corresponding idl interface. So before building codegen only for webidl events, would be good to add one such event manually to see if there are any problems.

(In reply to comment #7) > Also, we don't have webidl interfaces for events which doesn't have > corresponding idl interface. > So before building codegen only for webidl events, would be good to add one > such event manually > to see if there are any problems. Makes sense, thanks!

https://tbpl.mozilla.org/?tree=Try&rev=21bcea690efa This has a workaround for Bug 861493, at least in two cases

Some tests are now passing, and had to fix an assertion (SetIsDOMBinding() should allow recall ) https://tbpl.mozilla.org/?tree=Try&rev=26b05b08f960

GeneratedEventClasses.h is a new file. It contains the class declarations.

I guess I could split this, since this contains all the new .webidl files (which are generated from .idl files) and the changes to codegen. But in practice this all needs to go in at once.

Comment on attachment 744826 [details] [diff] [review] patch Uh, I thought I asked for a review

StyleSheetAddedEvent and StyleSheetRemovedEvent have been merged into StyleSheetChangeEvent and it shouldn't have an interface object. See bug 839103 and bug 872934.

Peter, how could I ease the reviewing? Splitting the patch wouldn't really make it much easier, but I could do that if wanted. I'd like to get this landed before we get some webidl-only code generator, though these codegenerators should be orthogonal.

Comment on attachment 744826 [details] [diff] [review] patch Review of attachment 744826 [details] [diff] [review]: ----------------------------------------------------------------- You should add comments in the WebIDL files referring to the relevant specs. I didn't compare all of them to their spec, but I did for some and it was a pain to have to look up the spec URLs. Can we avoid having duplicate dictionaries? If you really don't want to do that here then do it in a followup, but I wish we did it right away. We should avoid having two almost identical classes with the same name, just in a different namespace. Can you undo the mapping to Variant in the WebIDL files that should have 'any'? The WebIDL should have 'any' where xpidl had nsIVariant and the generated code should map 'any' to the right variant conversion under the hood. Note that this is slightly related to the previous point, since the two dictionary implementations use a different storage type for any. I'm fine with using XPCVariant to store the JS::Value in the event objects, though I think we should eventually store a JS::Value and expose it to the CC. The WebIDL conversion doesn't handle constants. I noticed this because of SpeechRecognitionError, but there might be others? Either make the conversion deal with them or spit out a warning that we're losing them so that we notice it. And do the same for methods? -'ing for these issues, though most of the patch looks good to me. ::: dom/webidl/CloseEvent.webidl @@ +1,4 @@ > +/* -*- Mode: IDL; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */ > +/* This Source Code Form is subject to the terms of the Mozilla Public > + * License, v. 2.0. If a copy of the MPL was not distributed with this file, > + * You can obtain one at http://mozilla.org/MPL/2.0/. Probably should keep the reference to http://dev.w3.org/html5/websockets/#closeevent @@ +16,5 @@ > + boolean canBubble, > + boolean cancelable, > + boolean wasClean, > + unsigned short code, > + DOMString? reason); Make a note that this is Mozilla-specific. ::: dom/webidl/CustomEvent.webidl @@ -6,5 @@ > - * The origin of this IDL file is > - * http://www.w3.org/TR/2012/WD-dom-20120105/ > - * > - * Copyright © 2012 W3C® (MIT, ERCIM, Keio), All Rights Reserved. W3C > - * liability, trademark and document use rules apply. It doesn't seem right to remove this (though the url could be updated). @@ +13,5 @@ > + [Throws] > + void initCustomEvent(DOMString type, > + boolean canBubble, > + boolean cancelable, > + any detail); Make a note that this is Mozilla-specific. ::: dom/webidl/DeviceOrientationEvent.webidl @@ +18,5 @@ > + boolean cancelable, > + double alpha, > + double beta, > + double gamma, > + boolean absolute); Make a note that this is Mozilla-specific. ::: dom/webidl/ElementReplaceEvent.webidl @@ +12,5 @@ > + [Throws] > + void initElementReplaceEvent(DOMString type, > + boolean canBubble, > + boolean cancelable, > + Element? upgrade); Make a note that this is Mozilla-specific. ::: dom/webidl/HashChangeEvent.webidl @@ +14,5 @@ > + void initHashChangeEvent(DOMString type, > + boolean canBubble, > + boolean cancelable, > + DOMString? oldURL, > + DOMString? newURL); Make a note that this is Mozilla-specific. ::: dom/webidl/PageTransitionEvent.webidl @@ +12,5 @@ > + [Throws] > + void initPageTransitionEvent(DOMString type, > + boolean canBubble, > + boolean cancelable, > + boolean persisted); Make a note that this is Mozilla-specific. ::: dom/webidl/PopStateEvent.webidl @@ +13,5 @@ > + [Throws] > + void initPopStateEvent(DOMString type, > + boolean canBubble, > + boolean cancelable, > + any state); Make a note that this is Mozilla-specific. ::: dom/webidl/SpeechRecognitionError.webidl @@ +5,5 @@ > + */ > + > +[Constructor(DOMString type, optional SpeechRecognitionErrorInit eventInitDict), HeaderFile="GeneratedEventClasses.h"] > +interface SpeechRecognitionError : Event > +{ What about the constants? @@ +7,5 @@ > +[Constructor(DOMString type, optional SpeechRecognitionErrorInit eventInitDict), HeaderFile="GeneratedEventClasses.h"] > +interface SpeechRecognitionError : Event > +{ > + readonly attribute unsigned long error; > + readonly attribute DOMString? message; Conscious change? ::: dom/webidl/SpeechRecognitionEvent.webidl @@ +9,5 @@ > +interface SpeechRecognitionEvent : Event > +{ > + readonly attribute unsigned long resultIndex; > + readonly attribute nsISupports? results; > + readonly attribute DOMString? interpretation; Conscious change? ::: dom/webidl/StorageEvent.webidl @@ +1,5 @@ > +/* -*- Mode: IDL; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */ > +/* This Source Code Form is subject to the terms of the Mozilla Public > + * License, v. 2.0. If a copy of the MPL was not distributed with this file, > + * You can obtain one at http://mozilla.org/MPL/2.0/. > + */ Lost the reference to the spec. @@ +10,5 @@ > +{ > + readonly attribute DOMString? key; > + readonly attribute DOMString? oldValue; > + readonly attribute DOMString? newValue; > + readonly attribute DOMString? url; Shouldn't url be non-nullable? @@ +21,5 @@ > + DOMString? key, > + DOMString? oldValue, > + DOMString? newValue, > + DOMString? url, > + Storage? storageArea); Make a note that this is Mozilla-specific. ::: dom/webidl/WebIDL.mk @@ +359,5 @@ > $(NULL) > endif > > +webidl_files += \ > + ProgressEvent.webidl \ Wrong indentations. ::: js/xpconnect/src/event_impl_gen.py @@ +115,5 @@ > + fd.write("namespace dom {\n") > + for e in conf.simple_events: > + idlname = ("nsIDOM%s.idl" % e) > + idl = p.parse(open(findIDL(options.incdirs, idlname)).read(), idlname) > + idl.resolve(options.incdirs, p) This seems to parse all idls twice. Wouldn't it be better to keep the earlier results of loadEventIDL in an array of tuples or a dict and reuse them here? @@ +143,5 @@ > + baseinterfaces.append(baseiface) > + baseiface = baseiface.idl.getName(baseiface.base, baseiface.location) > + baseinterfaces.reverse() > + > + baseattributes = [] You never use just the bases' attributes. Make this allattributes and add attributes to it. @@ +162,5 @@ > + > + for baseiface in baseinterfaces: > + baseimpl = ("%s" % baseiface.name[6:]) > + if (baseimpl == "Event"): > + baseimpl = "nsDOMEvent" Make a function of these 3 lines and use it wherever you do this. @@ +169,5 @@ > + fd.write(" NS_DECL_%s\n" % iface.name.upper()) > + fd.write(" virtual nsresult InitFromCtor(const nsAString& aType, JSContext* aCx, jsval* aVal);\n\n") > + > + hasVariant = False > + for a in baseattributes + attributes: allattributes @@ +190,5 @@ > + """xpidl methods take care of string member variables!""" > + if a.realtype.nativeType('in').count("nsAString"): > + continue > + elif a.realtype.nativeType('in').endswith('*'): > + fd.write(" already_AddRefed<%s> Get%s()\n" % (xpidl_to_native(a.realtype.nativeType('in').strip('* '), conf), firstCap(a.name))) Store |a.realtype.nativeType('in').strip('* ')| in a local var and use it. I'd also do the same for |firstCap(a.name)| @@ +193,5 @@ > + elif a.realtype.nativeType('in').endswith('*'): > + fd.write(" already_AddRefed<%s> Get%s()\n" % (xpidl_to_native(a.realtype.nativeType('in').strip('* '), conf), firstCap(a.name))) > + fd.write(" {\n"); > + fd.write(" nsCOMPtr<%s> %s = do_QueryInterface(m%s);\n" % (xpidl_to_canonical(a.realtype.nativeType('in').strip('* '), conf), a.name, firstCap(a.name))) > + fd.write(" return static_cast<%s*>(%s.forget().get());\n" % (xpidl_to_native(a.realtype.nativeType('in').strip('* '), conf), a.name)) This doesn't work on OS X, the already_Addrefed constructor is explicit. Just doing |return %s.forget();| mostly works, except for nsCSSStyleSheet. So: " return %s.forget().downcast<%s>();\n" % (a.name, xpidl_to_native(a.realtype.nativeType('in').strip('* '), conf)) ? Or explicitly construct the alread_AddRefed. @@ +206,5 @@ > + fd.write("Init%s(" % eventname) > + if hasVariant: > + fd.write("JSContext* aCx, ") > + fd.write("const nsAString& aType, bool aCanBubble, bool aCancelable") > + for a in baseattributes + attributes: allattributes @@ +358,3 @@ > baseinterfaces.reverse() > > baseattributes = [] See above, allattributes would make sense here too. @@ +468,5 @@ > + for a in baseattributes + attributes: > + if a.type == "nsIVariant": > + fd.write(" nsCOMPtr<nsIVariant> %s;\n" % a.name) > + fd.write(" aRv = nsContentUtils::XPConnect() ?\n") > + fd.write(" nsContentUtils::XPConnect()->JSToVariant(aCx, a%s, getter_AddRefs(%s)) :\n" % (firstCap(a.name), a.name)) Couldn't this simply use XPCVariant::newVariant?

(In reply to Peter Van der Beken [:peterv] from comment #20) > Can we avoid having duplicate dictionaries? If you really don't want to do > that here then do it in a followup, I was planning to remove the old dictionaries in a followup. > Can you undo the mapping to Variant in the WebIDL files that should have > 'any'? No. As far as I know, we webidl bindings can't handle 'any' always. > > The WebIDL conversion doesn't handle constants. I noticed this because of > SpeechRecognitionError, but there might be others? Either make the > conversion deal with them or spit out a warning that we're losing them so > that we notice it. And do the same for methods? Methods? generated events can't have methods. But need to deal with constants.

(In reply to Olli Pettay [:smaug] from comment #21) > > Can you undo the mapping to Variant in the WebIDL files that should have > > 'any'? > No. As far as I know, we webidl bindings can't handle 'any' always. WebIDL bindings handle 'any'. https://mxr.mozilla.org/mozilla-central/search?find=%2Fdom%2Fwebidl%2F&string=any It will be mapped to jsval.

(In reply to Olli Pettay [:smaug] from comment #21) > No. As far as I know, we webidl bindings can't handle 'any' always. What isn't working? > Methods? generated events can't have methods. Let's just make sure everyone is aware of that by asserting it :-).

Ok, I'll fix the variant handling. for the methods...well, the generated code doesn't even compile if the even interface has other methods than init*Event. That is rather strong assertion.

DOMString? is there for reason. xpidl by default supports passing null via string attributes.

(In reply to Olli Pettay [:smaug] from comment #25) > DOMString? is there for reason. xpidl by default supports passing null via > string attributes. Please file a followup bug to make our implementation spec-compliant.

Fix the assertion which got removed while merging. And couldn't enable one test after all. https://tbpl.mozilla.org/?tree=Try&rev=bac60975f160

And in fact it seems that the test for which we have "CustomEvent interface: existence and properties of interface object": true, is invalid.

Comment on attachment 764044 [details] [diff] [review] v8 Review of attachment 764044 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/webidl/SpeechRecognitionError.webidl @@ +13,5 @@ > + const unsigned long NETWORK = 3; > + const unsigned long NOT_ALLOWED = 4; > + const unsigned long SERVICE_NOT_ALLOWED = 5; > + const unsigned long BAD_GRAMMAR = 6; > + const unsigned long LANGUAGE_NOT_SUPPORTED = 7; Can you file a followup to convert this to an enum (I think the spec changed to that?). ::: js/xpconnect/src/event_impl_gen.py @@ +328,5 @@ > + fd.write("{\n") > + fd.write(" JS::Rooted<JS::Value> retVal(aCx, JS::NullValue());\n"); > + fd.write(" nsresult rv = NS_ERROR_UNEXPECTED;\n") > + fd.write(" if (m%s && !XPCVariant::VariantDataToJS(m%s, &rv, retVal.address())) {\n" % (firstCap(a.name), firstCap(a.name))) > + fd.write(" if (!JS_IsExceptionPending(aCx)) {\n") After discussion with bz I think we should just drop this check for pending exceptions. @@ +491,5 @@ > + for a in allattributes: > + if a.type == "nsIVariant": > + fd.write(" nsCOMPtr<nsIVariant> %s = dont_AddRef(XPCVariant::newVariant(aCx, a%s));\n" % (a.name, firstCap(a.name))) > + fd.write(" if (!%s) {\n" % a.name) > + fd.write(" aRv = NS_ERROR_FAILURE;\n") aRv.Throw(NS_ERROR_FAILURE);

This fails with a syntax error in my build: changeset: 135494:3e48ae462b0d parent: 135492:dd9245e21812 user: Olli Pettay <Olli.Pettay@helsinki.fi> date: Tue Jun 18 21:48:45 2013 +0300 summary: Bug 847611 - Paris bindings for autogenerated events, r=peterv diff -r dd9245e21812 -r 3e48ae462b0d dom/interfaces/events/nsIDOMDeviceLightEvent.idl --- a/dom/interfaces/events/nsIDOMDeviceLightEvent.idl Tue Jun 18 14:46:00 2013 -0400 +++ b/dom/interfaces/events/nsIDOMDeviceLightEvent.idl Tue Jun 18 21:48:45 2013 +0300 @@ -17,5 +17,5 @@ dictionary DeviceLightEventInit : EventInit { - double value; + double value = Infinity; };

Remove any *.pyc files in xpcom/idl-parser/ in your *source* dir, and file a bug if it still happens after that.

(In reply to :Ms2ger from comment #34) > Remove any *.pyc files in xpcom/idl-parser/ in your *source* dir, and file a > bug if it still happens after that. That seems to have had no effect. Will file a new bug. And btw, having generated files like this in the source directory is seriously counter-intuitive.

See bug 890748.