Implement MediaStreamTrack.getConstraints

Implement MediaStreamTrack.getConstraints

This stores the currently valid constraints inside a
MediaStreamTrack object, in the form of a WebMediaConstraints
variable, and returns it with the GetConstraints operation.

http://w3c.github.io/mediacapture-main/getusermedia.html#widl-MediaStreamTrack-getConstraints-MediaTrackConstraints

BUG=543997
Committed: https://crrev.com/1a9ce9415e528d0fe526d6cc7f766a497893265e
Cr-Commit-Position: refs/heads/master@{#390074}

[hta - Chromium, 2016-04-20 13:11:16]

Description was changed from

==========
Implement MediaStreamTrack.getConstraints

This stores the currently valid constraints inside a
MediaStreamTrack object, in the form of a WebMediaConstraints
variable, and returns it with the GetConstraints operation.

http://w3c.github.io/mediacapture-main/getusermedia.html#widl-MediaStreamTrac...

BUG=543997
==========

to

==========
Implement MediaStreamTrack.getConstraints

This stores the currently valid constraints inside a
MediaStreamTrack object, in the form of a WebMediaConstraints
variable, and returns it with the GetConstraints operation.

http://w3c.github.io/mediacapture-main/getusermedia.html#widl-MediaStreamTrac...

BUG=543997
==========

[hta - Chromium, 2016-04-20 13:11:16]

hta@chromium.org changed reviewers:
+ peter@chromium.org, tommi@chromium.org

[hta - Chromium, 2016-04-20 13:11:39]

One more part of the constraints puzzle ready to go.

[hta - Chromium, 2016-04-20 13:11:49]

The CQ bit was checked by hta@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-04-20 13:12:05]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1910463002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1910463002/1

[commit-bot: I haz the power, 2016-04-20 14:48:00]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-04-20 14:48:01]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[hta - Chromium, 2016-04-20 15:02:30]

The CQ bit was checked by hta@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-04-20 15:02:42]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1910463002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1910463002/20001

[commit-bot: I haz the power, 2016-04-20 16:50:57]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-04-20 16:50:57]

Dry run: This issue passed the CQ dry run.

[Peter Beverloo, 2016-04-21 14:11:46]

Blink lgtm, but please get Tommi's sign-off too.

Note that these tests would be an excellent candidate for regular GTest-based
unit tests too :)

https://codereview.chromium.org/1910463002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/mediastream/MediaStreamTrack-getConstraints.html
(right):

https://codereview.chromium.org/1910463002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/mediastream/MediaStreamTrack-getConstraints.html:17:
assert_equals(constraints.echoCancellation.exact, true, "constraints(2): "
+JSON.stringify(constraints));
Could make this a bit more explicit:

assert_true(constraints.hasOwnProperty('echoCancellation'));
assert_true(constraints.echoCancellation.exact);

https://codereview.chromium.org/1910463002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/mediastream/MediaStreamTrack-getConstraints.html:26:
assert_equals(constraints.echoCancellation.exact, true, "constraints(2): "
+JSON.stringify(constraints));
assert_false(constraints.hasOwnProperty('notKnownName'));

(You're testing the `echoCancellation` bit as part of the previous patch
already.)

https://codereview.chromium.org/1910463002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/mediastream/MediaStreamTrack-getConstraints.html:71:
complexConstraints.advanced = [ ignorableConstraintSet ];
nit: Please prefix these variables with `let` or `const` as appropriate, now
you're hoisting them on the global scope.

https://codereview.chromium.org/1910463002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp
(right):

https://codereview.chromium.org/1910463002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp:54:
struct NameValueStringConstraint {
Have you considered using something like the following instead?

using NameValueStringPair = std::pair<WebString, WebString>;

https://codereview.chromium.org/1910463002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp:192:
static bool parse(const Dictionary& constraintsDictionary,
WebVector<NameValueStringConstraint>& optional,
WebVector<NameValueStringConstraint>& mandatory)
Is there any reason to use a WebVector here and elsewhere w/ the
NameValueStringConstraint type rather than a Vector?

The WebVector is the exposed version of a Vector in the WebKit API, but since
you're using it with an internal type that doesn't seem necessary.

https://codereview.chromium.org/1910463002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp:599:
ConstrainLongRange convertLong(const LongConstraint& input)
While English is not my first language, it seems to me like the spec would want
to use the noun (Constraint) rather than the verb (Constrain) for these types?

https://codereview.chromium.org/1910463002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp:633:
Vector<WTF::String> buffer;
nit: no need to prefix with WTF::

https://codereview.chromium.org/1910463002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp:686: //
TODO(hta): Add the non-standard names (here and in IDL).
These are the goog* right, or also the hotword/sinkId and others that are being
parsed in parseOldStyleNames()?

As long as we don't add any new ones, I'm definitely in favor of properly and
explicitly documenting what we're doing here :-).

https://codereview.chromium.org/1910463002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/mediastream/MediaStreamTrack.h (right):

https://codereview.chromium.org/1910463002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/mediastream/MediaStreamTrack.h:76: // Internal
I see that this has to be exposed because UserMediaRequest uses it, but would
you please document what appropriate usage is rather than noting that it's
internal? (Which visibility already dictates it's not.)

https://codereview.chromium.org/1910463002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/mediastream/MediaStreamTrack.idl (right):

https://codereview.chromium.org/1910463002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/mediastream/MediaStreamTrack.idl:40:
[RuntimeEnabled=MediaConstraints] MediaTrackConstraints getConstraints();
More of a spec issue, but by returning a dictionary here the following equation
would return false:

  let track = ...;  // MediaStreamTrack instance
  track.getConstraints() === track.getConstraints()

A more modern version of doing this is by applying FrozenArray<>, which is being
introduced in crbug.com/515920. `constraints` can then just be a property and
equality would work as expected.

https://codereview.chromium.org/1910463002/diff/20001/third_party/WebKit/publ...
File third_party/WebKit/public/platform/WebMediaConstraints.h (right):

https://codereview.chromium.org/1910463002/diff/20001/third_party/WebKit/publ...
third_party/WebKit/public/platform/WebMediaConstraints.h:46: class
BLINK_PLATFORM_EXPORT BaseConstraint {
Not for this patch, but the naming rules of the Blink API indicate that all
types must have the Web* prefix. Please consider updating this at some point..

https://www.chromium.org/blink/public-c-api#TOC-Naming

[hta - Chromium, 2016-04-24 09:58:26]

I hope I got them all.... a couple of pushbacks, but mostly "done".

https://codereview.chromium.org/1910463002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/mediastream/MediaStreamTrack-getConstraints.html
(right):

https://codereview.chromium.org/1910463002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/mediastream/MediaStreamTrack-getConstraints.html:17:
assert_equals(constraints.echoCancellation.exact, true, "constraints(2): "
+JSON.stringify(constraints));
On 2016/04/21 14:11:45, Peter Beverloo wrote:
> Could make this a bit more explicit:
> 
> assert_true(constraints.hasOwnProperty('echoCancellation'));
> assert_true(constraints.echoCancellation.exact);

Done.

https://codereview.chromium.org/1910463002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/mediastream/MediaStreamTrack-getConstraints.html:26:
assert_equals(constraints.echoCancellation.exact, true, "constraints(2): "
+JSON.stringify(constraints));
On 2016/04/21 14:11:45, Peter Beverloo wrote:
> assert_false(constraints.hasOwnProperty('notKnownName'));
> 
> (You're testing the `echoCancellation` bit as part of the previous patch
> already.)

Done.

https://codereview.chromium.org/1910463002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/mediastream/MediaStreamTrack-getConstraints.html:71:
complexConstraints.advanced = [ ignorableConstraintSet ];
On 2016/04/21 14:11:45, Peter Beverloo wrote:
> nit: Please prefix these variables with `let` or `const` as appropriate, now
> you're hoisting them on the global scope.

Done.

https://codereview.chromium.org/1910463002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp
(right):

https://codereview.chromium.org/1910463002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp:54:
struct NameValueStringConstraint {
On 2016/04/21 14:11:46, Peter Beverloo wrote:
> Have you considered using something like the following instead?
> 
> using NameValueStringPair = std::pair<WebString, WebString>;

Not really - this was a struct that was exposed globally a few iterations ago; I
thought this was a good time to change the name to point out that it's *not*
part of the Web* interface any more, but I didn't want to spend time updating
the places it's used (which is all code that I hope to get to the dustbin one
day).

https://codereview.chromium.org/1910463002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp:192:
static bool parse(const Dictionary& constraintsDictionary,
WebVector<NameValueStringConstraint>& optional,
WebVector<NameValueStringConstraint>& mandatory)
On 2016/04/21 14:11:46, Peter Beverloo wrote:
> Is there any reason to use a WebVector here and elsewhere w/ the
> NameValueStringConstraint type rather than a Vector?
> 
> The WebVector is the exposed version of a Vector in the WebKit API, but since
> you're using it with an internal type that doesn't seem necessary.

Good point. It was used globally, but now it's just hassle.

https://codereview.chromium.org/1910463002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp:599:
ConstrainLongRange convertLong(const LongConstraint& input)
On 2016/04/21 14:11:46, Peter Beverloo wrote:
> While English is not my first language, it seems to me like the spec would
want
> to use the noun (Constraint) rather than the verb (Constrain) for these types?

Not my first language either, but it is the first language of the guy who picked
the name.... I think he was thinking that "this type serves to specify how to
constrain a Long to a Range". Don't want to revisit this piece of the spec.....
we're ready to call it a W3C CR just about now.

https://codereview.chromium.org/1910463002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp:633:
Vector<WTF::String> buffer;
On 2016/04/21 14:11:46, Peter Beverloo wrote:
> nit: no need to prefix with WTF::

Done.

https://codereview.chromium.org/1910463002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp:686: //
TODO(hta): Add the non-standard names (here and in IDL).
On 2016/04/21 14:11:46, Peter Beverloo wrote:
> These are the goog* right, or also the hotword/sinkId and others that are
being
> parsed in parseOldStyleNames()?
> 
> As long as we don't add any new ones, I'm definitely in favor of properly and
> explicitly documenting what we're doing here :-).

I think I need to moderate this remark. I'll file a bug, it's a better way of
having a discussion about it.
https://crbug.com/605673 filed.

https://codereview.chromium.org/1910463002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/mediastream/MediaStreamTrack.h (right):

https://codereview.chromium.org/1910463002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/mediastream/MediaStreamTrack.h:76: // Internal
On 2016/04/21 14:11:46, Peter Beverloo wrote:
> I see that this has to be exposed because UserMediaRequest uses it, but would
> you please document what appropriate usage is rather than noting that it's
> internal? (Which visibility already dictates it's not.)

More explanation added. There will be an applyConstraints call here eventually
too, which is IDL-exposed.

https://codereview.chromium.org/1910463002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/mediastream/MediaStreamTrack.idl (right):

https://codereview.chromium.org/1910463002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/mediastream/MediaStreamTrack.idl:40:
[RuntimeEnabled=MediaConstraints] MediaTrackConstraints getConstraints();
On 2016/04/21 14:11:46, Peter Beverloo wrote:
> More of a spec issue, but by returning a dictionary here the following
equation
> would return false:
> 
>   let track = ...;  // MediaStreamTrack instance
>   track.getConstraints() === track.getConstraints()
> 
> A more modern version of doing this is by applying FrozenArray<>, which is
being
> introduced in crbug.com/515920. `constraints` can then just be a property and
> equality would work as expected.

FrozenArray<> isn't a name/value map, though, and can't be used to contain a
dictionary (I think - haraken insists that dictionary types don't survive a
single cycle). I've grown to dislike dictionaries for many reasons, but they do
convert very nicely into Javascript objects.

https://codereview.chromium.org/1910463002/diff/20001/third_party/WebKit/publ...
File third_party/WebKit/public/platform/WebMediaConstraints.h (right):

https://codereview.chromium.org/1910463002/diff/20001/third_party/WebKit/publ...
third_party/WebKit/public/platform/WebMediaConstraints.h:46: class
BLINK_PLATFORM_EXPORT BaseConstraint {
On 2016/04/21 14:11:46, Peter Beverloo wrote:
> Not for this patch, but the naming rules of the Blink API indicate that all
> types must have the Web* prefix. Please consider updating this at some point..
> 
> https://www.chromium.org/blink/public-c-api#TOC-Naming

Acknowledged. crbug.com/605684 filed so that I do not forget.

[tommi (sloooow) - chröme, 2016-04-27 08:57:40]

lgtm. minor comments below.

https://codereview.chromium.org/1910463002/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/mediastream/MediaStreamTrack-getConstraints.html
(right):

https://codereview.chromium.org/1910463002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/mediastream/MediaStreamTrack-getConstraints.html:9:

nit: one empty line (or none)

https://codereview.chromium.org/1910463002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/mediastream/MediaStreamTrack-getConstraints.html:41:
continue;
this is kind of hard to read and easy to read incorrectly.  There's a return
statement above and then a continue. In general I'd prefer to have
return/break/continue statements that change program flow, on a separate line.

I'd go for something like:

for (var p in a) {
  if (!a.hasOwnProperty(p))
    continue;
  if (!b.hasOwnProperty(p))
    return false;
<etc>

When debugging, it can be useful to be able to put breakpoints on those branch
points and that's made hard if the condition and branch are on the same line.

https://codereview.chromium.org/1910463002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/mediastream/MediaStreamTrack-getConstraints.html:43:
if (a[p] !== b[p]) return false;;
nit: one semicolon should cut it ;)

https://codereview.chromium.org/1910463002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/mediastream/MediaStreamTrack-getConstraints.html:82:

nit: reduce whitespace

[hta - Chromium, 2016-04-27 09:32:36]

hta@chromium.org changed reviewers:
+ jochen@chromium.org

[hta - Chromium, 2016-04-27 09:32:37]

Adding Jochen for public/

Comments from Tommi addressed.

https://codereview.chromium.org/1910463002/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/mediastream/MediaStreamTrack-getConstraints.html
(right):

https://codereview.chromium.org/1910463002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/mediastream/MediaStreamTrack-getConstraints.html:9:

On 2016/04/27 08:57:40, tommi-chrömium wrote:
> nit: one empty line (or none)

Done.

https://codereview.chromium.org/1910463002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/mediastream/MediaStreamTrack-getConstraints.html:41:
continue;
On 2016/04/27 08:57:40, tommi-chrömium wrote:
> this is kind of hard to read and easy to read incorrectly.  There's a return
> statement above and then a continue. In general I'd prefer to have
> return/break/continue statements that change program flow, on a separate line.
> 
> I'd go for something like:
> 
> for (var p in a) {
>   if (!a.hasOwnProperty(p))
>     continue;
>   if (!b.hasOwnProperty(p))
>     return false;
> <etc>
> 
> When debugging, it can be useful to be able to put breakpoints on those branch
> points and that's made hard if the condition and branch are on the same line.

Done - consistent newlines imposed.

https://codereview.chromium.org/1910463002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/mediastream/MediaStreamTrack-getConstraints.html:43:
if (a[p] !== b[p]) return false;;
On 2016/04/27 08:57:40, tommi-chrömium wrote:
> nit: one semicolon should cut it ;)

Done.

https://codereview.chromium.org/1910463002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/mediastream/MediaStreamTrack-getConstraints.html:82:

On 2016/04/27 08:57:40, tommi-chrömium wrote:
> nit: reduce whitespace

Done.

[jochen (gone - plz use gerrit), 2016-04-27 13:16:08]

lgtm

[hta - Chromium, 2016-04-27 13:20:09]

The CQ bit was checked by hta@chromium.org

[hta - Chromium, 2016-04-27 13:20:10]

The patchset sent to the CQ was uploaded after l-g-t-m from peter@chromium.org,
tommi@chromium.org
Link to the patchset: https://codereview.chromium.org/1910463002/#ps80001
(title: "Address review comments on layout test")

[commit-bot: I haz the power, 2016-04-27 13:20:26]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1910463002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1910463002/80001

[commit-bot: I haz the power, 2016-04-27 14:56:20]

Description was changed from

==========
Implement MediaStreamTrack.getConstraints

This stores the currently valid constraints inside a
MediaStreamTrack object, in the form of a WebMediaConstraints
variable, and returns it with the GetConstraints operation.

http://w3c.github.io/mediacapture-main/getusermedia.html#widl-MediaStreamTrac...

BUG=543997
==========

to

==========
Implement MediaStreamTrack.getConstraints

This stores the currently valid constraints inside a
MediaStreamTrack object, in the form of a WebMediaConstraints
variable, and returns it with the GetConstraints operation.

http://w3c.github.io/mediacapture-main/getusermedia.html#widl-MediaStreamTrac...

BUG=543997
==========

[commit-bot: I haz the power, 2016-04-27 14:56:21]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-04-27 14:57:25]

Description was changed from

==========
Implement MediaStreamTrack.getConstraints

This stores the currently valid constraints inside a
MediaStreamTrack object, in the form of a WebMediaConstraints
variable, and returns it with the GetConstraints operation.

http://w3c.github.io/mediacapture-main/getusermedia.html#widl-MediaStreamTrac...

BUG=543997
==========

to

==========
Implement MediaStreamTrack.getConstraints

This stores the currently valid constraints inside a
MediaStreamTrack object, in the form of a WebMediaConstraints
variable, and returns it with the GetConstraints operation.

http://w3c.github.io/mediacapture-main/getusermedia.html#widl-MediaStreamTrac...

BUG=543997

Committed: https://crrev.com/1a9ce9415e528d0fe526d6cc7f766a497893265e
Cr-Commit-Position: refs/heads/master@{#390074}
==========

[commit-bot: I haz the power, 2016-04-27 14:57:26]

Patchset 5 (id:??) landed as
https://crrev.com/1a9ce9415e528d0fe526d6cc7f766a497893265e
Cr-Commit-Position: refs/heads/master@{#390074}