Closed Bug 767924 Opened 7 years ago Closed 5 years ago

Implement sequences as union member types for WebIDL

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla33

People

(Reporter: peterv, Assigned: bzbarsky)

References

(Blocks 2 open bugs)

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.
Depends on: 899972
Flags: needinfo?(bzbarsky)
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.
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)
Flags: needinfo?(bzbarsky)
(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).
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)
Flags: needinfo?(peterv)
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
No longer blocks: 882145
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
Blocks: 1006707
No longer blocks: 907352
Blocks: 1015796
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
No longer blocks: 1015796
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: nobody → bzbarsky
Status: NEW → ASSIGNED
Attachment #8442559 - Flags: review?(peterv)
Flags: needinfo?(peterv)
Nikhil, you're going to need mozmap in a union as well, right?  And distinguishability of sequences and mozmap?
Flags: needinfo?(nsm.nikhil)
Attachment #8442558 - Attachment is obsolete: true
Attachment #8442558 - Flags: review?(peterv)
Attachment #8442893 - Flags: review?(peterv)
Attachment #8442559 - Attachment is obsolete: true
Attachment #8442559 - Flags: review?(peterv)
Attachment #8442893 - Attachment is obsolete: true
Attachment #8442893 - Flags: review?(peterv)
Attachment #8442557 - Flags: review?(peterv) → review+
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+
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+
> 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.  ;)
Blocks: 899972
No longer depends on: 899972
Depends on: 1026080
Clearing ni? since this was discussed on IRC. Answer was yes to both.
Flags: needinfo?(nsm.nikhil)
Attachment #8442560 - Flags: review?(peterv) → review+
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?
> 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.
Just for posterity, this was a thing I tried that didn't work out.
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?
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.
No longer blocks: 1053033
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.