Closed
Bug 767924
Opened 12 years ago
Closed 10 years ago
Implement sequences as union member types for WebIDL
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla33
People
(Reporter: peterv, Assigned: bzbarsky)
References
(Blocks 1 open bug)
Details
Attachments
(7 files, 3 obsolete files)
4.71 KB,
patch
|
peterv
:
review+
|
Details | Diff | Splinter Review |
9.18 KB,
patch
|
peterv
:
review+
|
Details | Diff | Splinter Review |
7.00 KB,
patch
|
peterv
:
review+
|
Details | Diff | Splinter Review |
17.36 KB,
patch
|
peterv
:
review+
|
Details | Diff | Splinter Review |
8.53 KB,
patch
|
Details | Diff | Splinter Review | |
3.73 KB,
patch
|
Details | Diff | Splinter Review | |
1.07 KB,
patch
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Updated•12 years ago
|
Blocks: ParisBindings
Updated•11 years ago
|
Blocks: web-animations
Assignee | ||
Updated•11 years ago
|
Flags: needinfo?(bzbarsky)
Comment 1•11 years ago
|
||
What's the ETA on this bug?
As mentioned in bug 767926, comment 18 this workaround didn't work:
dictionary MediaStreamConstraints {
// (boolean or MediaTrackConstraints) video;
(boolean or object) video;
};
dictionary MediaTrackConstraints { ... }
Which means I have no workaround for bug 882145 for FF26.
Assignee | ||
Comment 2•11 years ago
|
||
Making (boolean or object) work in a dictionary is almost certainly simpler than this bug. Either way, I can make this happen.
That said, there is always the sledgehammer workaround:
any video;
and then in the C++:
if (dict.mVideo.isObject()) {
// Work with dict.mVideo.toObject();
} else {
bool myBool = JS::ToBoolean(dict.mVideo);
// Now we have a bool
}
Flags: needinfo?(bzbarsky)
Assignee | ||
Updated•11 years ago
|
Flags: needinfo?(bzbarsky)
Comment 3•11 years ago
|
||
(In reply to Boris Zbarsky [:bz] from comment #2)
> Making (boolean or object) work in a dictionary is almost certainly simpler
> than this bug. Either way, I can make this happen.
Cool. (boolean or MediaTrackConstraints) would obviously be preferable but (boolean or object) would be a fine workaround for now.
Note that the MediaTrackConstraints dictionary contains an object
> That said, there is always the sledgehammer workaround:
>
> any video;
>
> and then in the C++:
>
> if (dict.mVideo.isObject()) {
> // Work with dict.mVideo.toObject();
> } else {
> bool myBool = JS::ToBoolean(dict.mVideo);
> // Now we have a bool
> }
Thanks, good to know! (saves me from backing out of using webidl for Ff26 worst case).
Assignee | ||
Comment 4•11 years ago
|
||
So I've run into an interesting problem here. Consider this:
dictionary Foo {};
dictionary Bar {
(long or Foo) member;
};
now the declaration of Bar needs the declaration of the LongOrFoo class, but that needs to know sizeof(Foo) to do the UnionMember bits. So if Foo and Bar are in the same webidl file there's just no way to make this work with C++ headers.
And even if Foo and Bar are in different webidl files we get a weird ordering/include dependency....
Peter, any bright ideas?
Flags: needinfo?(bzbarsky)
Assignee | ||
Updated•11 years ago
|
Flags: needinfo?(peterv)
Assignee | ||
Comment 5•11 years ago
|
||
I split off dictionaries to bug 918011.
Flags: needinfo?(peterv)
Summary: Implement sequences and dictionaries as union member types for WebIDL → Implement sequences as union member types for WebIDL
Comment 6•11 years ago
|
||
Looks like I'll need this, assuming our constraints proposal goes through http://lists.w3.org/Archives/Public/public-media-capture/2014Apr/0002.html :
enum VideoFacingModeEnum {
"user",
"environment",
"left",
"right"
};
dictionary MediaTrackConstraintSet {
(VideoFacingModeEnum or sequence<VideoFacingModeEnum>) facingMode;
};
File "/Users/Jan/moz/mozilla-central/dom/bindings/Codegen.py", line 7254, in getUnionTypeTemplateVars
raise TypeError("Can't handle sequences in unions")
TypeError: Can't handle sequences in unions
make[5]: *** [codegen.pp] Error 255
Updated•11 years ago
|
The fetch spec as currently defined depends on this for the definition of HeadersInit, though I would not consider it a true blocker as of now since a Headers() instance can be created and headers manually set.
Blocks: 995484
Assignee | ||
Comment 8•10 years ago
|
||
So I poked at this some more, and remembered why it's a pain.
For sequences of gcthings, we currently use a holder. But unions aren't set up very well to do holders that have a nontrivial constructor. That needs fixing. The other option is to stop using a holder for rooting, but then optional rooted sequences end up looking like Optional<RootedSequence<T>> instead of Optional<Sequence<T>>. Unlike the object/any cases, this is not easily fixed via specializations of Optional... It would be possible to fix it by writing custom Optional-handling code for sequences, but that's pretty annoying and complicated and fragile. So it seems easier to fix holders in unions to work better. Unless, of course, we're OK with leaking the rootedness of the sequence to callees. Peter, thoughts?
Blocks: 1015796
Flags: needinfo?(peterv)
Assignee | ||
Comment 9•10 years ago
|
||
Attachment #8442557 -
Flags: review?(peterv)
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Assignee | ||
Comment 10•10 years ago
|
||
Attachment #8442558 -
Flags: review?(peterv)
Assignee | ||
Comment 11•10 years ago
|
||
Attachment #8442559 -
Flags: review?(peterv)
Assignee | ||
Comment 12•10 years ago
|
||
Attachment #8442560 -
Flags: review?(peterv)
Assignee | ||
Updated•10 years ago
|
Flags: needinfo?(peterv)
Assignee | ||
Comment 13•10 years ago
|
||
Nikhil, you're going to need mozmap in a union as well, right? And distinguishability of sequences and mozmap?
Flags: needinfo?(nsm.nikhil)
Assignee | ||
Comment 14•10 years ago
|
||
Attachment #8442842 -
Flags: review?(peterv)
Assignee | ||
Updated•10 years ago
|
Attachment #8442558 -
Attachment is obsolete: true
Attachment #8442558 -
Flags: review?(peterv)
Assignee | ||
Comment 15•10 years ago
|
||
Attachment #8442893 -
Flags: review?(peterv)
Assignee | ||
Updated•10 years ago
|
Attachment #8442559 -
Attachment is obsolete: true
Attachment #8442559 -
Flags: review?(peterv)
Assignee | ||
Comment 16•10 years ago
|
||
Attachment #8442916 -
Flags: review?(peterv)
Assignee | ||
Updated•10 years ago
|
Attachment #8442893 -
Attachment is obsolete: true
Attachment #8442893 -
Flags: review?(peterv)
Reporter | ||
Updated•10 years ago
|
Attachment #8442557 -
Flags: review?(peterv) → review+
Reporter | ||
Comment 17•10 years ago
|
||
Comment on attachment 8442842 [details] [diff] [review]
part 2. Introduce both const and non-const versions of GetAs* methods on unions, with the former returning the public-facing type and the latter returning the internal type
Review of attachment 8442842 [details] [diff] [review]:
-----------------------------------------------------------------
::: dom/bindings/Codegen.py
@@ +7981,5 @@
> if type.isSpiderMonkeyInterface():
> typeName = CGGeneric(type.name)
> if type.nullable():
> typeName = CGTemplatedType("Nullable", typeName)
> + return CGWrapper(typeName, post=" const &")
Make this
@@ +8265,2 @@
> if self.ownsMembers:
> + getterReturnType = "%s const&" % vars["structType"]
and this consistent wrt spaces between |const| and |&|.
Attachment #8442842 -
Flags: review?(peterv) → review+
Reporter | ||
Comment 18•10 years ago
|
||
Comment on attachment 8442916 [details] [diff] [review]
part 3. Allow sequences in unions
Review of attachment 8442916 [details] [diff] [review]:
-----------------------------------------------------------------
::: dom/bindings/Codegen.py
@@ +3925,5 @@
> if len(arrayObjectMemberTypes) > 0:
> + assert len(arrayObjectMemberTypes) == 1
> + name = getUnionMemberName(arrayObjectMemberTypes[0])
> + arrayObject = CGGeneric(
> + "done = (failed = !%s.TrySetTo%s(cx, ${val}, ${mutableVal}, tryNext)) || tryNext;\n" %
!tryNext, no? Too bad we don't actually run our test codegen files :-).
@@ -8023,5 @@
> ownsMembers=False):
> - # For dictionaries and sequences we need to pass None as the failureCode
> - # for getJSToNativeConversionInfo.
> - # Also, for dictionaries we would need to handle conversion of
> - # null/undefined to the dictionary correctly.
Heh, these comments might have been obsolete for a while?
Attachment #8442916 -
Flags: review?(peterv) → review+
Assignee | ||
Comment 19•10 years ago
|
||
> and this consistent wrt spaces between |const| and |&|.
Done, with space between "const" and '&'.
> !tryNext, no? Too bad we don't actually run our test codegen files :-).
Good catch, and indeed!
> Heh, these comments might have been obsolete for a while?
Verily. ;)
Assignee | ||
Updated•10 years ago
|
Clearing ni? since this was discussed on IRC. Answer was yes to both.
Flags: needinfo?(nsm.nikhil)
Reporter | ||
Updated•10 years ago
|
Attachment #8442560 -
Flags: review?(peterv) → review+
Reporter | ||
Comment 21•10 years ago
|
||
Comment on attachment 8442560 [details] [diff] [review]
part 4. Allow [] as a default value for sequences in unions
Review of attachment 8442560 [details] [diff] [review]:
-----------------------------------------------------------------
::: dom/bindings/Codegen.py
@@ +4143,5 @@
> if tag in numericSuffixes or tag is IDLType.Tags.bool:
> defaultStr = getHandleDefault(defaultValue)
> value = declLoc + (".Value()" if nullable else "")
> default = CGGeneric("%s.RawSetAs%s() = %s;\n" %
> (value, defaultValue.type, defaultStr))
BTW, should |defaultValue.type| be |getUnionMemberName(defaultValue.type)| here too?
Assignee | ||
Comment 22•10 years ago
|
||
> BTW, should |defaultValue.type| be |getUnionMemberName(defaultValue.type)| here too?
Yes, it should. It happens to be the same thing for now, but better to be consistent. I'll fix.
Assignee | ||
Comment 23•10 years ago
|
||
remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/4b2ade04c59b
remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/4db2e66252d4
remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/5a7b033244ae
remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/d9ae4452d38f
Flags: in-testsuite+
Target Milestone: --- → mozilla33
Assignee | ||
Comment 24•10 years ago
|
||
Just for posterity, this was a thing I tried that didn't work out.
Assignee | ||
Comment 25•10 years ago
|
||
Comment 26•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/4b2ade04c59b
https://hg.mozilla.org/mozilla-central/rev/4db2e66252d4
https://hg.mozilla.org/mozilla-central/rev/5a7b033244ae
https://hg.mozilla.org/mozilla-central/rev/d9ae4452d38f
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Comment 27•10 years ago
|
||
Boris, this patch fails to compile because I'm using a sequence of enum. Is that a shortcoming of the current support or am I doing something illegal?
Assignee | ||
Comment 28•10 years ago
|
||
Some of both.
One reason it fails to compile is that unions screw up sequences of "wrapper" types. I filed bug 1063889 on that.
The second reason is that the union goes in UnionTypes.h, which makes that header include your binding's header to get the enum declaration. But your binding's header has to include UnionTypes.h to get the union definition for its dictionary declaration. This doesn't work, obviously. So once bug 1063889 is fixed you'll need to make sure your dictionary and your enum are in different .webidl files.
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•