Last Comment Bug 765163 - Implement code generator for simple DOM events
: Implement code generator for simple DOM events
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM: Events (show other bugs)
: Trunk
: x86_64 All
: -- normal (vote)
: ---
Assigned To: Olli Pettay [:smaug] (way behind * queues, especially ni? queue)
:
:
Mentors:
Depends on:
Blocks: 765766 765947
  Show dependency treegraph
 
Reported: 2012-06-15 02:24 PDT by Olli Pettay [:smaug] (way behind * queues, especially ni? queue)
Modified: 2012-07-22 10:40 PDT (History)
12 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
WIP (87.46 KB, patch)
2012-06-17 16:55 PDT, Olli Pettay [:smaug] (way behind * queues, especially ni? queue)
no flags Details | Diff | Splinter Review
generated .h (2.06 KB, text/plain)
2012-06-17 16:55 PDT, Olli Pettay [:smaug] (way behind * queues, especially ni? queue)
no flags Details
generated .cpp (6.31 KB, text/plain)
2012-06-17 16:56 PDT, Olli Pettay [:smaug] (way behind * queues, especially ni? queue)
no flags Details
patch (87.50 KB, patch)
2012-06-18 00:54 PDT, Olli Pettay [:smaug] (way behind * queues, especially ni? queue)
khuey: review-
Details | Diff | Splinter Review
fixed few nits (87.39 KB, patch)
2012-06-18 09:39 PDT, Olli Pettay [:smaug] (way behind * queues, especially ni? queue)
no flags Details | Diff | Splinter Review
up-to-date (88.26 KB, patch)
2012-06-19 04:17 PDT, Olli Pettay [:smaug] (way behind * queues, especially ni? queue)
no flags Details | Diff | Splinter Review
up-to-date (90.96 KB, patch)
2012-06-19 04:56 PDT, Olli Pettay [:smaug] (way behind * queues, especially ni? queue)
no flags Details | Diff | Splinter Review
up-to-date (56.68 KB, patch)
2012-07-06 15:00 PDT, Olli Pettay [:smaug] (way behind * queues, especially ni? queue)
no flags Details | Diff | Splinter Review
up-to-date (73.46 KB, patch)
2012-07-06 15:04 PDT, Olli Pettay [:smaug] (way behind * queues, especially ni? queue)
no flags Details | Diff | Splinter Review
v7 (73.34 KB, patch)
2012-07-17 12:50 PDT, Olli Pettay [:smaug] (way behind * queues, especially ni? queue)
khuey: review+
jst: review+
Details | Diff | Splinter Review
generated .h (882 bytes, text/plain)
2012-07-17 12:52 PDT, Olli Pettay [:smaug] (way behind * queues, especially ni? queue)
no flags Details
fix for the wrong undef (73.33 KB, patch)
2012-07-20 09:42 PDT, Olli Pettay [:smaug] (way behind * queues, especially ni? queue)
no flags Details | Diff | Splinter Review

Description Olli Pettay [:smaug] (way behind * queues, especially ni? queue) 2012-06-15 02:24:22 PDT
There are plenty of quite simple DOM event implementations.
Basically something which just add a property or two to nsIDOMEvent.
We could automatically generate the code for such cases.
Comment 1 Olli Pettay [:smaug] (way behind * queues, especially ni? queue) 2012-06-17 16:55:07 PDT
Created attachment 633931 [details] [diff] [review]
WIP

This converts DeviceProximityEvent, UserProximityEvent and CustomEvent
to use the auto generated code.
The idea is that for simple events extending nsIDOMEvent it should be enough
to write .idl, and add .idl to the Makefile.in and add the event name to the
event_impl_gen.conf

Code generator can't handle jsval, but normal XPCOM member variables, numbers,
booleans and strings should be ok.
If there are XPCOM member variables in the event, they are automatically
added to the cycle collection.
It is expected that each event interfaces has a dictionary for ctor.

(Try is now closed so I'll push the patch to try tomorrow)
Comment 2 Olli Pettay [:smaug] (way behind * queues, especially ni? queue) 2012-06-17 16:55:58 PDT
Created attachment 633932 [details]
generated .h
Comment 3 Olli Pettay [:smaug] (way behind * queues, especially ni? queue) 2012-06-17 16:56:24 PDT
Created attachment 633933 [details]
generated .cpp
Comment 4 Olli Pettay [:smaug] (way behind * queues, especially ni? queue) 2012-06-18 00:54:50 PDT
Created attachment 633968 [details] [diff] [review]
patch

Hopefully this doesn't cause random build failures. DictionaryHelper.h
must be there before GeneratedEvents.cpp

https://tbpl.mozilla.org/?tree=Try&rev=4b39a97fceca

Peter, what do you think about the changes to dom/base/nsDOMClassInfo.cpp 
and especially to dom/base/nsDOMClassInfoClasses.h

This is still all about the old bindings, but the point is to just make it easier to add new event interfaces.

The patch converts 3 event interface implementations to use codegen. There are several more which can be done
in followups.

I think there can be need to add support for #ifdefs (for B2G at least) in the config, but that also could be done
in a followup.
Comment 5 Olli Pettay [:smaug] (way behind * queues, especially ni? queue) 2012-06-18 01:40:01 PDT
Comment on attachment 633968 [details] [diff] [review]
patch

Kyle, could you check the python part of the patch.
It is copy-pasted-modified from the dictionary codegen.
Comment 6 Olli Pettay [:smaug] (way behind * queues, especially ni? queue) 2012-06-18 01:41:40 PDT
Comment on attachment 633968 [details] [diff] [review]
patch

Jst, could you look at the generated files (.h/.cpp attached to this bug)
and make sure they look ok. Comparing them to the removed files could be useful.
Comment 7 :Ms2ger (⌚ UTC+1/+2) 2012-06-18 02:18:59 PDT
Comment on attachment 633968 [details] [diff] [review]
patch

Review of attachment 633968 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/base/public/nsContentUtils.h
@@ -33,5 @@
>  #include "nsIStatefulFrame.h"
>  #include "nsINodeInfo.h"
>  #include "nsNodeInfoManager.h"
>  #include "nsContentList.h"
> -#include "nsDOMClassInfoID.h"

Did this need to be in this patch?

::: js/xpconnect/src/dictionary_helper_gen.py
@@ +447,5 @@
>      p = xpidl.IDLParser(outputdir=options.cachedir)
>  
>      conf = readConfigFile(filename)
>  
> +    if (len(filenames) > 1):

No need for the parens

@@ +453,5 @@
> +        execfile(filenames[1], eventconfig)
> +        simple_events = eventconfig.get('simple_events', [])
> +        for e in simple_events:
> +            eventdict = ("%sInit" % e)
> +            eventidl = ("nsIDOM%s.idl" % e)

Not here either

::: js/xpconnect/src/event_impl_gen.conf
@@ +20,5 @@
> +
> +# name of the type to not include using #include "typename.h"
> +exclude_automatic_type_include = [
> +    'nsISupports'
> +  ]

Indentation is inconsistent here

::: js/xpconnect/src/event_impl_gen.py
@@ +11,5 @@
> +# --makedepend-output support.
> +make_dependencies = []
> +make_targets = []
> +
> +def strip_begin(text, suffix):

prefix?

@@ +95,5 @@
> +
> +def print_header_file(fd, conf):
> +    fd.write("#if defined MOZ_GENERATED_EVENTS_CTORS\n")
> +    for e in conf.simple_events:
> +        fd.write("NS_DEFINE_EVENT_CTOR(%s)\n" % e);

fd.writelines(("NS_DEFINE_EVENT_CTOR(%s)\n" % e) for e in conf.simple_events), maybe

@@ +146,5 @@
> +
> +            for iface in interfaces:
> +                collect_names_and_non_primitive_attribute_types_from_interface(iface, attrnames, forwards)
> +
> +def collect_names_and_non_primitive_attribute_types_from_interface(iface, attrnames, forwards):

You like long names, don't you?

@@ +285,5 @@
> +    for a in attributes:
> +        fd.write("  %s\n" % attributeVariableTypeAndName(a))
> +    fd.write("};\n\n")
> +
> +    if len(ccattributes) > 0:

if ccattributes:
Comment 8 Olli Pettay [:smaug] (way behind * queues, especially ni? queue) 2012-06-18 06:07:02 PDT
(In reply to :Ms2ger from comment #7)
> ::: content/base/public/nsContentUtils.h
> @@ -33,5 @@
> >  #include "nsIStatefulFrame.h"
> >  #include "nsINodeInfo.h"
> >  #include "nsNodeInfoManager.h"
> >  #include "nsContentList.h"
> > -#include "nsDOMClassInfoID.h"
> 
> Did this need to be in this patch?
Yes.
nsDOMClassInfoID.h uses generated data, so using nsContentUtils.h isn't possible 
before that data (.h file) is created.

> 
> No need for the parens
Hey, there is no coding style in my python code.
If it doesn't crash nor raise errors, I'm happy :)

> @@ +20,5 @@
> > +
> > +# name of the type to not include using #include "typename.h"
> > +exclude_automatic_type_include = [
> > +    'nsISupports'
> > +  ]
> 
> Indentation is inconsistent here
Indeed

> > +def strip_begin(text, suffix):
> 
> prefix?
Actually, I can remove the method.

> 
> @@ +95,5 @@
> > +
> > +def print_header_file(fd, conf):
> > +    fd.write("#if defined MOZ_GENERATED_EVENTS_CTORS\n")
> > +    for e in conf.simple_events:
> > +        fd.write("NS_DEFINE_EVENT_CTOR(%s)\n" % e);
> 
> fd.writelines(("NS_DEFINE_EVENT_CTOR(%s)\n" % e) for e in
> conf.simple_events), maybe
That is horrible way. (I didn't know python lets one write that way)

> > +def collect_names_and_non_primitive_attribute_types_from_interface(iface, attrnames, forwards):
> 
> You like long names, don't you?
Yes, and I like copy-pasting from the dictionary generator :)

> > +    for a in attributes:
> > +        fd.write("  %s\n" % attributeVariableTypeAndName(a))
> > +    fd.write("};\n\n")
> > +
> > +    if len(ccattributes) > 0:
> 
> if ccattributes:
oh, that works. ok.
Comment 9 Olli Pettay [:smaug] (way behind * queues, especially ni? queue) 2012-06-18 09:39:03 PDT
Created attachment 634076 [details] [diff] [review]
fixed few nits
Comment 10 Olli Pettay [:smaug] (way behind * queues, especially ni? queue) 2012-06-19 04:17:05 PDT
Created attachment 634349 [details] [diff] [review]
up-to-date
Comment 11 Olli Pettay [:smaug] (way behind * queues, especially ni? queue) 2012-06-19 04:56:06 PDT
Created attachment 634352 [details] [diff] [review]
up-to-date
Comment 12 Olli Pettay [:smaug] (way behind * queues, especially ni? queue) 2012-06-19 08:46:28 PDT
Kyle, bug 765947 has some fixes to Makefile.in.
Comment 13 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-07-06 11:25:46 PDT
Comment on attachment 633968 [details] [diff] [review]
patch

Review of attachment 633968 [details] [diff] [review]:
-----------------------------------------------------------------

Please split the nsDOMClassInfoId.h stuff into a separate patch.  That makes this very painful to review.
Comment 14 David Zbarsky (:dzbarsky) 2012-07-06 11:28:13 PDT
I think that part is no longer needed after bug 763350
Comment 15 Olli Pettay [:smaug] (way behind * queues, especially ni? queue) 2012-07-06 15:00:21 PDT
Created attachment 639819 [details] [diff] [review]
up-to-date

Kyle, could you look at js/xpconnect/src/*
Jst, could you review especially dom/* and check also the generated code
(attached as separate attachments in this bug).
Comment 16 Olli Pettay [:smaug] (way behind * queues, especially ni? queue) 2012-07-06 15:04:33 PDT
Created attachment 639822 [details] [diff] [review]
up-to-date
Comment 17 Johnny Stenback (:jst, jst@mozilla.com) 2012-07-16 20:00:38 PDT
Olli, what would you think about flipping the approach of using GeneratedEvents.h from the current approach where you #define something that selectively enables a part of the included header to an approach where the generated header simply contains a list that invokes a macro for each generated event type, and the code that #includes the file is responsible of #define'ing what that macro does?

The primary reason I'd prefer that approach over what you wrote is that if I change how nsDOMClassInfo does its thing, rename macros there, or whatever, I don't need to go edit the event code generator to make stuff compile, I simply update the relevant macro that's defined in the places where I #include GeneratedEvents.h.

Other than that this looks good to me.
Comment 18 Olli Pettay [:smaug] (way behind * queues, especially ni? queue) 2012-07-17 05:13:28 PDT
(In reply to Johnny Stenback (:jst, jst@mozilla.com) from comment #17)
> Olli, what would you think about flipping the approach of using
> GeneratedEvents.h from the current approach where you #define something that
> selectively enables a part of the included header to an approach where the
> generated header simply contains a list that invokes a macro for each
> generated event type, and the code that #includes the file is responsible of
> #define'ing what that macro does?
I could do that.
Comment 19 Olli Pettay [:smaug] (way behind * queues, especially ni? queue) 2012-07-17 12:50:29 PDT
Created attachment 643098 [details] [diff] [review]
v7
Comment 20 Olli Pettay [:smaug] (way behind * queues, especially ni? queue) 2012-07-17 12:52:19 PDT
Created attachment 643100 [details]
generated .h

Need to still have special case for includes, since macros can't
handle that case.
Comment 21 Johnny Stenback (:jst, jst@mozilla.com) 2012-07-17 16:37:51 PDT
Comment on attachment 643098 [details] [diff] [review]
v7

- In dom/base/nsDOMClassInfo.cpp:

+#define MOZ_GENERATED_EVENT_LIST
+#define MOZ_GENERATED_EVENT(_event_interface)                         \
+  DOM_CLASSINFO_MAP_BEGIN(_event_interface, nsIDOM##_event_interface) \
+    DOM_CLASSINFO_MAP_ENTRY(nsIDOM##_event_interface)                 \
+    DOM_CLASSINFO_EVENT_MAP_ENTRIES                                   \
+  DOM_CLASSINFO_MAP_END
+#include "GeneratedEvents.h"
+#undef MOZ_GENERATED_EVENTS_CLASSINFOS

Make that #undef MOZ_GENERATED_EVENT_LIST

I explicitly did *not* look at the build system changes here, so Kyle should look at that as well as the code generator.

r=jst
Comment 22 Olli Pettay [:smaug] (way behind * queues, especially ni? queue) 2012-07-17 16:42:58 PDT
Yikes, a left-over from previous patch.
Comment 23 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-07-20 07:19:02 PDT
Comment on attachment 643098 [details] [diff] [review]
v7

Review of attachment 643098 [details] [diff] [review]:
-----------------------------------------------------------------

I'm not thrilled about the build changes here, but js/xpconnect/src/Makefile.in is already a disaster and this doesn't really make it worse.
Comment 24 Olli Pettay [:smaug] (way behind * queues, especially ni? queue) 2012-07-20 09:42:43 PDT
Created attachment 644371 [details] [diff] [review]
fix for the wrong undef
Comment 25 Olli Pettay [:smaug] (way behind * queues, especially ni? queue) 2012-07-20 14:06:31 PDT
https://hg.mozilla.org/mozilla-central/rev/4590a7ed92ab
https://hg.mozilla.org/mozilla-central/rev/d74c21aebfb6
Comment 26 Boris Zbarsky [:bz] (still a bit busy) 2012-07-21 20:43:09 PDT
This seems to be failing in some subtle way on incremental builds.  Right now the tree is all read because GeneratedEvents.cpp doesn't seem to see a bunch of the type defined in DictionaryHelpers.h.
Comment 27 Boris Zbarsky [:bz] (still a bit busy) 2012-07-21 22:19:27 PDT
I think I see what the problem is.  The rule for codegenning DictionaryHelpers.cpp _also_ writes DictionaryHelpers.h.

So in a parallel build, if DictionaryHelpers.h gets codegenned by the rule for it first (as part of the requirements for codegen of GeneratedEvents.cpp) and then the codegen of DictionaryHelpers.cpp races the compilation of GeneratedEvents.cpp, then it's possible for GeneratedEvents.cpp to read DictionaryHelpers.h after it's been truncated by the DictionaryHelpers.cpp codegen but before it's been written again.  Or something.

I went ahead and landed https://hg.mozilla.org/integration/mozilla-inbound/rev/252f295c4664 in an attempt to fix the above (by not writing DictionaryHelpers.h when codegenning DictionaryHelpers.cpp) and get the tree green.
Comment 28 Ryan VanderMeulen [:RyanVM] 2012-07-22 10:40:16 PDT
https://hg.mozilla.org/mozilla-central/rev/252f295c4664

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