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

NEW
Unassigned

Status

()

defect
5 years ago
4 months ago

People

(Reporter: jib, Unassigned)

Tracking

Trunk
x86
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

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.
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;
Blocks: 882145
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.  :(
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?
> 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.
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.
Blocks: 907352
No longer blocks: 882145
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']
(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.
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...
At least comment 8 applies to the situation described in comment 5.
Summary: webidl union as dictionary member doesn't compile (in cases where forward-declaring is insufficient) → can't nest webidl unions even of different types.
Blocks: 1006725
No longer blocks: 907352
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.
Flags: needinfo?(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?
Flags: needinfo?(bzbarsky)
Summary: can't nest webidl unions even of different types. → can't have webidl union containing a dictionary which has a union as a member
Depends on: 1068740
(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
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.
No longer blocks: 1006707
(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).
Blocks: 1290914
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.