Closed Bug 765163 Opened 7 years ago Closed 7 years ago

Implement code generator for simple DOM events

Categories

(Core :: DOM: Events, defect)

x86_64
All
defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: smaug, Assigned: smaug)

References

Details

Attachments

(4 files, 8 obsolete files)

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.
Attached patch WIP (obsolete) — Splinter Review
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)
Attached file generated .h (obsolete) —
Attached file generated .cpp
Attached patch patch (obsolete) — Splinter Review
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.
Attachment #633931 - Attachment is obsolete: true
Attachment #633968 - Flags: feedback?(peterv)
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.
Attachment #633968 - Flags: review?(khuey)
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.
Attachment #633968 - Flags: review?(jst)
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:
(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.
Blocks: 765766
Attached patch fixed few nits (obsolete) — Splinter Review
Blocks: 765947
Attached patch up-to-date (obsolete) — Splinter Review
Attachment #634076 - Attachment is obsolete: true
Attached patch up-to-date (obsolete) — Splinter Review
Attachment #634349 - Attachment is obsolete: true
Kyle, bug 765947 has some fixes to Makefile.in.
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.
Attachment #633968 - Flags: review?(khuey) → review-
I think that part is no longer needed after bug 763350
Attached patch up-to-date (obsolete) — Splinter Review
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).
Attachment #633968 - Attachment is obsolete: true
Attachment #633968 - Flags: review?(jst)
Attachment #633968 - Flags: feedback?(peterv)
Attachment #639819 - Flags: review?(khuey)
Attachment #639819 - Flags: review?(jst)
Attached patch up-to-date (obsolete) — Splinter Review
Attachment #634352 - Attachment is obsolete: true
Attachment #639819 - Attachment is obsolete: true
Attachment #639819 - Flags: review?(khuey)
Attachment #639819 - Flags: review?(jst)
Attachment #639822 - Flags: review?(khuey)
Attachment #639822 - Flags: review?(jst)
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.
(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.
Attached patch v7Splinter Review
Attachment #639822 - Attachment is obsolete: true
Attachment #639822 - Flags: review?(khuey)
Attachment #639822 - Flags: review?(jst)
Attachment #643098 - Flags: review?(khuey)
Attachment #643098 - Flags: review?(jst)
Attached file generated .h
Need to still have special case for includes, since macros can't
handle that case.
Attachment #633932 - Attachment is obsolete: true
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
Attachment #643098 - Flags: review?(jst) → review+
Yikes, a left-over from previous patch.
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.
Attachment #643098 - Flags: review?(khuey) → review+
https://hg.mozilla.org/mozilla-central/rev/4590a7ed92ab
https://hg.mozilla.org/mozilla-central/rev/d74c21aebfb6
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
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.
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.
You need to log in before you can comment on or make changes to this bug.