Closed
Bug 915419
Opened 11 years ago
Closed 11 years ago
Support "object" in union return values and unions inside dictionaries/sequences
Categories
(Core :: DOM: Core & HTML, defect)
Tracking
()
RESOLVED
FIXED
mozilla26
People
(Reporter: bzbarsky, Assigned: bzbarsky)
References
(Blocks 1 open bug)
Details
Attachments
(2 files, 2 obsolete files)
Patch coming up.
Assignee | ||
Comment 1•11 years ago
|
||
Adds RootedUnion and NullableRootedUnion structs that are used on the stack for union return values when the union needs rooting. It also adds a TraceUnion method on union return values, which is called by (Nullable)RootedUnion or by dictionary tracing. TraceUnion traces typed array and object members of unions (the only things unions can contain so far that might need tracing). Union return values are changed to store raw JSObject* or typed array structs instead of Rooted versions of both; the tracing is now handled via TraceUnion. The wrapping code for dictionary arguments to constructors is fixed to actually work. This required adding GetAs* methods to union return values that return references to the internal data. The SetToObject method is adjusted to actually work for union return value structs and not assume it's being generated for a union-conversion struct, and we now generate SetToObject methods as needed for union return values.
Attachment #803331 -
Flags: review?(dzbarsky)
Attachment #803331 -
Flags: review?(bugs)
Comment 2•11 years ago
|
||
Comment on attachment 803331 [details] [diff] [review] Add support for "object" types in owning unions (so union return values and unions in dictionaries and sequences. smaug Review of attachment 803331 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/bindings/BindingUtils.h @@ +1816,5 @@ > + { > + this->TraceUnion(trc); > + } > +}; > + Shouldn't this disallow copy constructor/operator=? ::: dom/bindings/test/TestCodeGen.webidl @@ +498,1 @@ > (CanvasPattern? or CanvasGradient) receiveUnionContainingNull(); Do you have tests for nullable unions with objects?
Assignee | ||
Comment 3•11 years ago
|
||
> Shouldn't this disallow copy constructor/operator=? It gets that automatically from AutoGCRooter disallowing it, right? > Do you have tests for nullable unions with objects? Good catch. Added some. Also, note that for now we don't support unions containing gcthings inside a sequence because I didn't add a SequenceTracer specialization for them. We can worry about it if we ever need them.
Assignee | ||
Comment 4•11 years ago
|
||
Attachment #803419 -
Flags: review?(dzbarsky)
Attachment #803419 -
Flags: review?(bugs)
Assignee | ||
Updated•11 years ago
|
Attachment #803331 -
Attachment is obsolete: true
Attachment #803331 -
Flags: review?(dzbarsky)
Attachment #803331 -
Flags: review?(bugs)
Comment 5•11 years ago
|
||
(In reply to Boris Zbarsky [:bz] from comment #3) > > Shouldn't this disallow copy constructor/operator=? > > It gets that automatically from AutoGCRooter disallowing it, right? > Oh right, I guess the default copy ctor calls the superclass so its ok.
Comment 6•11 years ago
|
||
Comment on attachment 803419 [details] [diff] [review] With more tests Review of attachment 803419 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/bindings/Codegen.py @@ +4729,1 @@ > memberWraps.append(memberWrap) Yeah, this makes more sense than what I had before ;) @@ +6314,1 @@ > unionValues.append(string.Template("UnionMember<${structType} > " It would probably be clearer if you collapsed these and made the return type depend on isReturnValue
Assignee | ||
Comment 7•11 years ago
|
||
Good idea!
Attachment #803462 -
Flags: review?(dzbarsky)
Attachment #803462 -
Flags: review?(bugs)
Assignee | ||
Updated•11 years ago
|
Attachment #803419 -
Attachment is obsolete: true
Attachment #803419 -
Flags: review?(dzbarsky)
Attachment #803419 -
Flags: review?(bugs)
Comment 8•11 years ago
|
||
Comment on attachment 803462 [details] [diff] [review] With that changed Review of attachment 803462 [details] [diff] [review]: ----------------------------------------------------------------- I don't know how JS_CallObjectTracer works but I'm assuming you got that part right. ::: dom/bindings/Codegen.py @@ +3304,1 @@ > # This is a bit annoying. In a union we don't want to have a Want to add a TypedArray in union test? @@ +4492,1 @@ > resultArgs = None Why do you need this?
Attachment #803462 -
Flags: review?(dzbarsky) → review+
Assignee | ||
Comment 9•11 years ago
|
||
> Want to add a TypedArray in union test? Not convinced about building and linking more test-only union code... > Why do you need this? So I don't get "referenced before assignment" exceptions when I return resultArgs?
Comment 10•11 years ago
|
||
(In reply to Boris Zbarsky [:bz] from comment #9) > > Want to add a TypedArray in union test? > > Not convinced about building and linking more test-only union code... > Sure, I guess this won't be used in real idl. I wonder if we can write a fuzzer that generates valid WebIDL to make sure we at least codegen things that compile. > > Why do you need this? > > So I don't get "referenced before assignment" exceptions when I return > resultArgs? Ah, true.
Comment 11•11 years ago
|
||
Any chance to see some generated code. This is dealing with tracing in a bit complicated setup and it just need to be right.
Assignee | ||
Comment 12•11 years ago
|
||
Olli, does this help? Or should I attach the entirety of TestCodeGen and UnionTypes?
Updated•11 years ago
|
Attachment #803462 -
Flags: review?(bugs) → review+
Assignee | ||
Comment 13•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/54247b7b87e6
Flags: in-testsuite+
Whiteboard: [need review]
Target Milestone: --- → mozilla26
Comment 14•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/54247b7b87e6
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•