Closed
Bug 765163
Opened 12 years ago
Closed 12 years ago
Implement code generator for simple DOM events
Categories
(Core :: DOM: Events, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: smaug, Assigned: smaug)
References
Details
Attachments
(4 files, 8 obsolete files)
6.31 KB,
text/plain
|
Details | |
73.34 KB,
patch
|
khuey
:
review+
jst
:
review+
|
Details | Diff | Splinter Review |
882 bytes,
text/plain
|
Details | |
73.33 KB,
patch
|
Details | Diff | Splinter Review |
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•12 years ago
|
||
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•12 years ago
|
||
Assignee | ||
Comment 3•12 years ago
|
||
Assignee | ||
Comment 4•12 years ago
|
||
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•12 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•12 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 7•12 years ago
|
||
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•12 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 | ||
Comment 9•12 years ago
|
||
Assignee | ||
Comment 10•12 years ago
|
||
Attachment #634076 -
Attachment is obsolete: true
Assignee | ||
Comment 11•12 years ago
|
||
Attachment #634349 -
Attachment is obsolete: true
Assignee | ||
Comment 12•12 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-
Comment 14•12 years ago
|
||
I think that part is no longer needed after bug 763350
Assignee | ||
Comment 15•12 years ago
|
||
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•12 years ago
|
||
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)
Comment 17•12 years ago
|
||
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•12 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•12 years ago
|
||
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•12 years ago
|
||
Need to still have special case for includes, since macros can't handle that case.
Attachment #633932 -
Attachment is obsolete: true
Comment 21•12 years ago
|
||
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•12 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•12 years ago
|
||
Assignee | ||
Comment 25•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/4590a7ed92ab https://hg.mozilla.org/mozilla-central/rev/d74c21aebfb6
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Comment 26•12 years ago
|
||
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•12 years ago
|
||
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.
Description
•