Re: gUM constraints webidl implementer feedback

On 10/15/2013 08:28 PM, Jan-Ivar Bruaroey wrote:
> Hi all,
>
> I took notes while implementing the beginnings of gUM constraints in 
> Firefox (just one: facingMode, at the moment), and I've been meaning 
> to post my feedback. They're in engineering-style bullet-points, so my 
> apologies in advance for the terseness (these are all requests for 
> spec changes):
>
>  1. Maybe list audio before video? dictionaries get alphabetized, so
>     why fight it :-) i.e.
>       * (boolean or MediaTrackConstraints) audio = false;
>       * (boolean or MediaTrackConstraints) video = false;
>

This shouldn't matter, so either way is fine.
>
>      *
>
>
>  1. Our WebIDL compiler complained about this in the spec (so I had to
>     remove the '?'):
>       * error: An argument cannot be a nullable dictionary or nullable
>         union containing a dictionary
>          1. void mozGetUserMedia(MediaStreamConstraints? options,
>

This seems like a bug in the spec.
>
>         1.
>
>
>  1. And this:
>       * error: Dictionary MediaTrackConstraints has member with
>         nullable dictionary type
>           o MediaTrackConstraintSet? mandatory = null;
>      1. Dictionary entries are inherently optional, so having them
>         nullable as well seems redundant. Ditto for default value.
>      2. Same for optional
>

Yup.
>
>     1.
>
>
>  1. Why not THROW on unsupported mandatory constraints? Our generated
>     webidl bindings already throw for us on various syntax and type
>     errors, and this way, should webidl ever grow, say, an extended
>     attribute feature that make a dictionary throw on unknown entries,
>     then we could use it out of the box (right now they're ignored,
>     which is a problem - see below).
>

I believe a dictionary is designed to allow unknown members by default 
(I could be wrong).
In our implementation, the place that checks whether constraints are 
supported or not is a couple of process switches away from the place 
where the IDL checks get done, so it's convenient for us to use the 
error callback.

> 1.
>
>
>  2. Why not allow multiple keys in each object in the optional array?
>       * When implementing this, it became obvious pretty soon that
>         reusing the sameapply() function for optional constraint
>         entries as for the mandatory constraints was desirable.There's
>         really no cost to implementation of doing this. In fact, I
>         found there to be a high cost of implementation to consider
>         anything else. That's because it would need to be spec'ed if
>         you ask webidl guys. These guys are extremely pedantic about
>         processing models in JS (for good reason: two ways of doing
>         things in JS are never the same, which means side-effects and
>         security issues w.r.t. malignant objects must be nailed down).
>         So reusing the established dictionary processing model here
>         seems like a win.
>
>       * Here's how I implement it today in webidl:
>           o // MediaTrackConstraint = single-property-subset of
>             MediaTrackConstraintSet
>             // Implemented as full set. Test Object.keys(pair).length == 1
>             typedef MediaTrackConstraintSet MediaTrackConstraint;
>

The only reason I remember for not having multiple keys in each object 
in the optional array is that if you have more than one constraint in an 
optional array element, it's more confusing if one of them can be 
satisfied and the other one can't. Does the one that can be satisfied 
get ignored because of the failing one, or is it one applied and one 
ignored?

Others may have other memories.

> 1.
>          o
>
>  2. Mandatory constraints aren't implementable as webidl dictionaries.
>       * We're supposed to detect unknown mandatory constraints and
>         fail on them, but webidl dictionaries /silently ignore/
>         unknown keys!
>

I thought this was a feature... don't you get to see the ignored keys in 
the engine?

> 1.
>       * To work around this problem, I had to alter the API to take
>         the mandatory member as a plain object (which makes our webidl
>         security guys' neck-hair stand up, but it's ok), like this:
>           o dictionary MediaTrackConstraints {
>                 object mandatory; // so we can see unknown +
>             unsupported constraints
>                 sequence<MediaTrackConstraint> _optional;
>             };
>       * I then convert it internally to this structure after scanning
>         for unknown keys, which should be safe:
>           o dictionary MediaTrackConstraintsInternal {
>                 MediaTrackConstraintSet mandatory; // holds only
>             supported constraints
>                 sequence<MediaTrackConstraint> _optional;
>             };
>
>
> In lieu of alternative solutions to number 6, I'd like to see the spec 
> be explicit about what's needed to implement this as it stands.

Seems to me we have a differing understanding of how the engine should 
deal with dictionaries.


>
> .: Jan-Ivar :.
>