The default bug view has changed. See this FAQ.

Implement code generator for simple DOM events

RESOLVED FIXED

Status

()

Core
DOM: Events
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: smaug, Assigned: smaug)

Tracking

Trunk
x86_64
All
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 8 obsolete attachments)

(Assignee)

Description

5 years ago
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.
(Assignee)

Comment 1

5 years ago
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)
(Assignee)

Comment 2

5 years ago
Created attachment 633932 [details]
generated .h
(Assignee)

Comment 3

5 years ago
Created attachment 633933 [details]
generated .cpp
(Assignee)

Comment 4

5 years ago
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.
Attachment #633931 - Attachment is obsolete: true
Attachment #633968 - Flags: feedback?(peterv)
(Assignee)

Comment 5

5 years ago
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)
(Assignee)

Comment 6

5 years ago
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:
(Assignee)

Comment 8

5 years ago
(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.
(Assignee)

Updated

5 years ago
Blocks: 765766
(Assignee)

Comment 9

5 years ago
Created attachment 634076 [details] [diff] [review]
fixed few nits
(Assignee)

Updated

5 years ago
Blocks: 765947
(Assignee)

Comment 10

5 years ago
Created attachment 634349 [details] [diff] [review]
up-to-date
Attachment #634076 - Attachment is obsolete: true
(Assignee)

Comment 11

5 years ago
Created attachment 634352 [details] [diff] [review]
up-to-date
Attachment #634349 - Attachment is obsolete: true
(Assignee)

Comment 12

5 years ago
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
(Assignee)

Comment 15

5 years ago
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).
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)
(Assignee)

Comment 16

5 years ago
Created attachment 639822 [details] [diff] [review]
up-to-date
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.
(Assignee)

Comment 18

5 years ago
(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.
(Assignee)

Comment 19

5 years ago
Created attachment 643098 [details] [diff] [review]
v7
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)
(Assignee)

Comment 20

5 years ago
Created attachment 643100 [details]
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+
(Assignee)

Comment 22

5 years ago
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+
(Assignee)

Comment 24

5 years ago
Created attachment 644371 [details] [diff] [review]
fix for the wrong undef
(Assignee)

Comment 25

5 years ago
https://hg.mozilla.org/mozilla-central/rev/4590a7ed92ab
https://hg.mozilla.org/mozilla-central/rev/d74c21aebfb6
Status: NEW → RESOLVED
Last Resolved: 5 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.
https://hg.mozilla.org/mozilla-central/rev/252f295c4664
You need to log in before you can comment on or make changes to this bug.