We need this, if we're tracing them.

Comment on attachment 752002 [details] [diff] [review] Building on Android too Review of attachment 752002 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/bindings/BindingDeclarations.h @@ +332,5 @@ > + Optional_base<JSObject*, JSObject*>::Construct(nullptr); > + } > + > + template <class T1> > + void Construct(const T1 &t1) & to the left (same below) ::: dom/bindings/Codegen.py @@ +7712,5 @@ > self.getMemberType(m), > visibility="public") for m in self.memberInfo] > + memberConstructs = (self.getMemberConstructor(m) for m in > + self.dictionary.members) > + memberConstructs = [ c for c in memberConstructs if c is not None ] No spaces within []. I guess this could be [c for c in (self.getMemberConstructor(m) for m in self.dictionary.members) if c is not None] if you could fit it in 80 columns... Meh. But actually... Why not use the body argument to the ClassMember constructor, like so: members = [ClassMember(self.makeMemberName(m[0].identifier.name), self.getMemberType(m), visibility="public", body=self.getMemberConstructor(m[0])) for m in self.memberInfo] ? Looks like that handles None fine. @@ +7934,5 @@ > trace = CGIfWrapper(trace, "%s.WasPassed()" % memberLoc) > > return trace.define() > > + def getMemberConstructor(self, member): A little docstring, perhaps?

> But actually... Why not use the body argument to the ClassMember constructor, like so: An excellent idea. Fixed the other bits too.

Comment on attachment 752216 [details] [diff] [review] Updated to comments Review of attachment 752216 [details] [diff] [review]: ----------------------------------------------------------------- Much nicer, thanks. ::: dom/bindings/Codegen.py @@ +7938,5 @@ > + Get the right initializer for the member. Most members don't need one, > + but we need to pre-initialize 'any' and 'object' that have a default > + value, so they're safe to trace at all times. > + """ > + (member, _) = memberInfo You should be able to drop the parens here

Comment on attachment 752219 [details] [diff] [review] Aurora patch updated to same comments Review of attachment 752219 [details] [diff] [review]: ----------------------------------------------------------------- The trunk patch does end up looking quite a bit nicer. ::: dom/bindings/Codegen.py @@ +7500,5 @@ > + memberConstructs = CGWrapper( > + CGIndenter(CGList(memberConstructs, ",\n"), 4), > + pre=":\n", post="\n ").define() > + else: > + memberConstructs = "" Not sure if I like using the same variable name for a list and a string. @@ +7528,5 @@ > " Init(nullptr, JS::NullHandleValue);\n" > " }\n" > "};").substitute( { "selfName": self.makeClassName(d), > + "inheritance": inheritance, > + "memberConstructs" : memberConstructs})) Spacing seems a little strange near : and }

> You should be able to drop the parens here Hmm. I like the clarity of having the parens, I think. Thanks for the reviews!

Comment on attachment 752230 [details] [diff] [review] Aurora patch updated to all comments [Approval Request Comment] Bug caused by (feature/regressing bug #): Bug 868715 and various WebIDL conversions. User impact if declined: Possible crashes due to dereferencing uninitialized memory during GC. Testing completed (on m-c, etc.): Risk to taking this patch (and alternatives if risky): Low risk. Just initializes the relevant variables. String or IDL/UUID changes made by this patch: None