Peter Van der Beken [:peterv] Reporter
	Description • 12 years ago

      No description provided.

	Jan-Ivar Bruaroey [:jib] (needinfo? me)
	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.

	Boris Zbarsky [:bzbarsky] 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
  }

	Jan-Ivar Bruaroey [:jib] (needinfo? me)
	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).

	Boris Zbarsky [:bzbarsky] 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?

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 5 • 11 years ago

I split off dictionaries to bug 918011.

	Jan-Ivar Bruaroey [:jib] (needinfo? me)
	Comment 6 • 10 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

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)
	Comment 7 • 10 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.

	Boris Zbarsky [:bzbarsky] 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?

	Boris Zbarsky [:bzbarsky] 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?

	Peter Van der Beken [:peterv] 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 |&|.

	Peter Van der Beken [:peterv] 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?

	Boris Zbarsky [:bzbarsky] 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.  ;)

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)
	Comment 20 • 10 years ago

Clearing ni? since this was discussed on IRC. Answer was yes to both.

	Peter Van der Beken [:peterv] 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?

	Boris Zbarsky [:bzbarsky] 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.

	Boris Zbarsky [:bzbarsky] 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

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 24 • 10 years ago

Just for posterity, this was a thing I tried that didn't work out.

	Ed Morley [:emorley]
	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

	[:fabrice] Fabrice Desré
	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?

	Boris Zbarsky [:bzbarsky] 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.