995352 - can't have webidl union containing a dictionary which has a union as a member

Status: NEW

(Core :: DOM: Bindings (WebIDL), defect, P3)

Description

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

dom/webidl/MediaStreamTrack.webidl:

  dictionary MediaTrackConstraintSet {
    (long or ConstrainLongRange) width;
    (long or ConstrainLongRange) height;
    ...
  };

dom/webidl/Constraints.webidl (new):

  dictionary ConstrainLongRange {
    long min;
    long max;
  };

...
In file included from /Users/Jan/moz/mozilla-central/obj-x86_64-apple-darwin12.2.1-debug/dom/file/Unified_cpp_dom_file0.cpp:158:
In file included from /Users/Jan/moz/mozilla-central/dom/file/LockedFile.cpp:21:
In file included from ../../dist/include/mozilla/dom/UnionTypes.h:8:
../../dist/include/mozilla/dom/MediaStreamTrackBinding.h:50:34: error: field has incomplete type 'mozilla::dom::OwningLongOrConstrainLongRange'
  OwningLongOrConstrainLongRange mHeight;


Ironically, OwningLongOrConstrainLongRange is defined later down in UnionTypes.h. It is also forward-declared in MediaStreamTrackBinding.h itself:

  namespace mozilla {
  namespace dom {
  ...
  class OwningLongOrConstrainLongRange;
  }
  }
  ...
  struct MediaTrackConstraintSet : public DictionaryBase
  {
    Optional<VideoFacingModeEnum > mFacingMode;
    OwningLongOrConstrainLongRange mHeight;

but mHeight is embedded, so forward-declaring is not enough.

Comment 1

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

I haven't found a workaround. I tried making it a sequence for now:

  dictionary MediaTrackConstraintSet {
    sequence<ConstrainLong> width;
    sequence<ConstrainLong> height;
    ...
  };

and this got me farther I think (just looking ahead):

In file included from /Users/Jan/moz/mozilla-central/obj-x86_64-apple-darwin12.2.1-debug/dom/bindings/UnifiedBindings1.cpp:34:
In file included from ./AudioStreamTrackBinding.cpp:4:
In file included from ../../dist/include/mozilla/dom/MediaStreamTrackBinding.h:12:
../../dist/include/mozilla/dom/UnionTypes.h:444:33: error: no member named 'FastMediaTrackConstraints' in namespace 'mozilla::dom::binding_detail'; did you mean 'BooleanOrMediaTrackConstraints'?
    UnionMember<binding_detail::FastMediaTrackConstraints > mMediaTrackConstraints;

Comment 2

[Boris Zbarsky [:bzbarsky]]

So the real issue here is that MediaStreamTrackBinding.h has a dictionary containing a union, so has to include UnionTypes.h.

But it also defines a type (MediaTrackConstraints, to be exact) that gets used in a union.  So UnionTypes.h has to include MediaStreamTrackBinding.h to see the definition of that type.

That obviously can't work.  Yay C++.

The "right" solution is probably to move to having separate generated header files per dictionary.  In the short term, you could do that manually by moving the MediaTrackConstraints dictionary into a separate .webidl file.  :(

Comment 3

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

So we pretty much can't nest unions?

Note that in the new syntax MediaTrackConstraints is derived from MediaTrackConstraintSet:

  dictionary MediaTrackConstraints : MediaTrackConstraintSet {

(In reply to Boris Zbarsky [:bz] from comment #2)
> The "right" solution is probably to move to having separate generated header
> files per dictionary.  In the short term, you could do that manually by
> moving the MediaTrackConstraints dictionary into a separate .webidl file.  :(

Didn't work, because one includes the other:

In file included from /Users/Jan/moz/mozilla-central/obj-x86_64-apple-darwin12.2.1-debug/dom/file/Unified_cpp_dom_file0.cpp:158:
In file included from /Users/Jan/moz/mozilla-central/dom/file/LockedFile.cpp:21:
In file included from ../../dist/include/mozilla/dom/UnionTypes.h:8:
In file included from ../../dist/include/mozilla/dom/MediaStreamTrackBinding.h:7:
../../dist/include/mozilla/dom/MediaTrackConstraintSetBinding.h:33:34: error: field has incomplete type 'mozilla::dom::OwningLongOrConstrainLongRange'
  OwningLongOrConstrainLongRange mHeight;

Would a solution be to split up UnionTypes.h into a file for each union type?

Comment 4

[Boris Zbarsky [:bzbarsky]]

> So we pretty much can't nest unions?

Yes, if you mean a union containing a dictionary which contains another union.  :(  I'm pretty sure we have a bug on that already.

> Would a solution be to split up UnionTypes.h into a file for each union type?

Yes.

Comment 5

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Sorry, I marked this blocking the wrong bug. I meant to indicate I need this functionality for stuff I'm working on.

Constraints, the way they're defined, use unions at two levels:

1. { video: true }
2. { video: { width: 1280 } }
3. { video: { width: { min: 1280 } } }

My temporary workaround disallows #2, which is probably not good to ship. i.e. I force people to express #2 using #3 by setting the same min and max:

 #2 using #3: { video: { width: { min: 1280, max: 1280 } } }

I can give a patch a shot if need be.

Comment 6

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Same problem with our existing facingMode constraint. We don't support #2:

1. facingMode:'user',
2. facingMode:['user', 'environment'],

Workaround: #1 using #2: facingMode:['user']

Comment 7

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

(In reply to Jan-Ivar Bruaroey [:jib] from comment #6)
> Workaround: #1 using #2: facingMode:['user']
Sorry, I should have said there is no workaround in this case, since we currently support only #1, and can't easily change it without breaking compatibility.

Comment 8

[Boris Zbarsky [:bzbarsky]]

The simplest workaround for this, I suspect, is to make the outer union a union of boolean and object.  Then take that object and manually init a dictionary with it on the C++ side.  That won't give you quite per-spec behavior if someone passes a Date or a RegExp as the object, but I seriously doubt anyone will anyway, even in a test suite trying to be exhaustive...

Comment 9

[Boris Zbarsky [:bzbarsky]]

At least comment 8 applies to the situation described in comment 5.

Comment 10

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Any chance of a resolution to this soon? Bug 1006707 is targeted for 35 and I don't see a good workaround for that one.

I also need to get moving on Bug 1006725 and hate my options.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Your best hope for a resolution here is bug 1068740 getting fixed.  I'm somewhat dubious that will happen for 35, though Peter would know better.  I definitely won't have time work on this in what remains of the 35 cycle.

For bug 1006725, does the suggestion in comment 8 work?  I know it's ugly, but it shouldn't be too much work to do...

For bug 1006707, what does the relevant ideal IDL look like?

Comment 12

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

(In reply to Boris Zbarsky [:bz] from comment #11)
> For bug 1006725, does the suggestion in comment 8 work?

It should.

> For bug 1006707, what does the relevant ideal IDL look like?

It's gotten more complicated with recent spec changes [1]. It's:

dictionary MediaTrackConstraintSet {
    ConstrainDOMString facingMode;
};

typedef (DOMString or sequence<DOMString> or ConstrainDOMStringParameters) ConstrainDOMString;

dictionary ConstrainDOMStringParameters {
    (DOMString or sequence<DOMString>) exact;
    (DOMString or sequence<DOMString>) ideal;
};

Which makes for fun class-names. :-)

../../dist/include/mozilla/dom/MediaTrackConstraintSetBinding.h:61:62: error: field has incomplete type
'mozilla::dom::OwningStringOrStringSequenceOrConstrainDOMStringParameters'
  OwningStringOrStringSequenceOrConstrainDOMStringParameters mFacingMode;


[1] http://w3c.github.io/mediacapture-main/getusermedia.html#track-property-registrations

Comment 13

[Boris Zbarsky [:bzbarsky]]

I see.  That one is harder because it's got a dictionary and a sequence, and an object could be one or the other (though that's a _very_ recent spec change; before that this was just invalid IDL).

I don't have any clever suggestions here short of once again making the union (DOMString or object) and then copy/pasting the sort of code overload resolution between a sequence and a dictionary would produce in the object case....  It's definitely a PITA, but if you end up blocked on this part I'm happy to write that part of the code for you.

Comment 14

[Peter Van der Beken [:peterv]]

(In reply to Boris Zbarsky [:bz] from comment #11)
> Your best hope for a resolution here is bug 1068740 getting fixed.

Note that that only fixed it for unions that are used in only one idl file. The others stills go in UnionTypes.h (see https://bugzilla.mozilla.org/show_bug.cgi?id=1290914#c17).