Closed Bug 847611 Opened 11 years ago Closed 11 years ago

Paris bindings for autogenerated events

Categories

(Core :: DOM: Events, defect)

Product:
Core
Component:
DOM: Events
Platform:
x86_64
Linux
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla24

People

(Reporter: smaug, Assigned: smaug)

References

(Blocks 2 open bugs)

Details

Attachments

(6 files, 6 obsolete files)

v4
79.09 KB, patch
Details | Diff | Splinter Review
GeneratedEventClasses.h - example
38.66 KB, text/plain
Details
GeneratedEvents.h - example
6.79 KB, text/plain
Details
GeneratedEvents.cpp - example
91.82 KB, text/plain
Details
patch
83.35 KB, patch
peterv
: review-
Details | Diff | Splinter Review
v8
86.41 KB, patch
peterv
: review+
Details | Diff | Splinter Review 
Not sure whether this needs to be split to several parts
Assignee: nobody → bugs
Blocks: 860979
Attached patch . (obsolete) — — Splinter Review 
https://tbpl.mozilla.org/?tree=Try&rev=95bcc358ae42
Attachment #736581 - Attachment is obsolete: true
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!
Depends on: 861493
Attached patch . (obsolete) — — Splinter Review 
https://tbpl.mozilla.org/?tree=Try&rev=21bcea690efa

This has a workaround for Bug 861493, at least in two cases
Attached patch v4 — — Splinter Review 
https://tbpl.mozilla.org/?tree=Try&rev=f0fcee822758
Attachment #736583 - Attachment is obsolete: true
Attachment #737145 - Attachment is obsolete: true
Some tests are now passing, and had to fix an assertion
(SetIsDOMBinding() should allow recall )

https://tbpl.mozilla.org/?tree=Try&rev=26b05b08f960
Attachment #744823 - Attachment is patch: false
GeneratedEventClasses.h is a new file. It contains the class declarations.
Attached patch patch — — Splinter Review 
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
Attachment #744826 - Flags: review?(peterv)
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?
Attachment #744826 - Flags: review?(peterv) → review-
(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.
Depends on: 884102
Attached patch v8 — — Splinter Review 
Fix the assertion which got removed while merging.
And couldn't enable one test after all.
https://tbpl.mozilla.org/?tree=Try&rev=bac60975f160
Attachment #763842 - Attachment is obsolete: true
Attachment #763842 - Flags: review?(peterv)
Attachment #764044 - Flags: review?(peterv)
And in fact it seems that the test for which we have "CustomEvent interface: existence and properties of interface object": true, is invalid.
Blocks: 884102
No longer depends on: 884102
Blocks: 884296
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);
Attachment #764044 - Flags: review?(peterv) → review+
Blocks: 884407
https://hg.mozilla.org/mozilla-central/rev/3e48ae462b0d
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
Blocks: 885611
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.
Blocks: 898874
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: