Refactor "track options" to be a dictionary. (issue 1318393002 by hta@chromium.org)

[h...@chromium.org]

Reviewers: tommi, Guido Urdaneta,

Message:
Time for review. Cannot be landed yet (commit of the union patch failed in
queue).


Description:
Refactor "track options" to be a dictionary.

This dictionary has members "optional" and "mandatory" for
backwards compatibility; the intent is that constraints using
these members will be treated like old-style constraints, while
constraints without them will be treated like spec-style.


BUG=522612

Please review this at https://codereview.chromium.org/1318393002/

Base URL: svn://svn.chromium.org/blink/trunk

Affected files (+90, -23 lines):
M third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.h
M third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp
M third_party/WebKit/Source/modules/mediastream/MediaStreamConstraints.idl
A
third_party/WebKit/Source/modules/mediastream/MediaTrackConstraintSet.idl
M third_party/WebKit/Source/modules/mediastream/UserMediaRequest.cpp
M third_party/WebKit/Source/modules/modules.gypi

[gui...@chromium.org]

https://codereview.chromium.org/1318393002/diff/40001/third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp
File
third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp
(right):

https://codereview.chromium.org/1318393002/diff/40001/third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp#newcode128
third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp:128:
for (; iter != mandatoryConstraintsHashMap.end(); ++iter)
Can you use range for here?

https://codereview.chromium.org/1318393002/diff/40001/third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp#newcode136
third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp:136:
for (size_t i = 0; i < optionalConstraints.size(); ++i) {
Range for?

https://codereview.chromium.org/1318393002/diff/40001/third_party/WebKit/Source/modules/mediastream/UserMediaRequest.cpp
File third_party/WebKit/Source/modules/mediastream/UserMediaRequest.cpp
(right):

https://codereview.chromium.org/1318393002/diff/40001/third_party/WebKit/Source/modules/mediastream/UserMediaRequest.cpp#newcode59
third_party/WebKit/Source/modules/mediastream/UserMediaRequest.cpp:59:
// Do nothing.
Is this possible?
Should there be a NOTREACHED here?

https://codereview.chromium.org/1318393002/

[h...@chromium.org]

I'd like some help on finding "range for".

https://codereview.chromium.org/1318393002/diff/40001/third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp
File
third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp
(right):

https://codereview.chromium.org/1318393002/diff/40001/third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp#newcode128
third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp:128:
for (; iter != mandatoryConstraintsHashMap.end(); ++iter)

On 2015/10/05 12:34:12, Guido Urdaneta wrote:
> Can you use range for here?

can you point to an examle of range for? I think that's a C++ 11 feature
I've never used.
This is copy/pasted code (I intend to remove the code I pasted from, but
it turned out to have one more user).

https://codereview.chromium.org/1318393002/diff/40001/third_party/WebKit/Source/modules/mediastream/UserMediaRequest.cpp
File third_party/WebKit/Source/modules/mediastream/UserMediaRequest.cpp
(right):

https://codereview.chromium.org/1318393002/diff/40001/third_party/WebKit/Source/modules/mediastream/UserMediaRequest.cpp#newcode59
third_party/WebKit/Source/modules/mediastream/UserMediaRequest.cpp:59:
// Do nothing.

On 2015/10/05 12:34:12, Guido Urdaneta wrote:
> Is this possible?
> Should there be a NOTREACHED here?

It is possible. When you call getUserMedia({video: true}), options will
be null for audio (not false). The tests actually caught this one.

I find the if/then(nothing)/elsif(something)/else structure awkward, but
didn't find a simpler way to write it.

https://codereview.chromium.org/1318393002/

[gui...@chromium.org]

https://codereview.chromium.org/1318393002/diff/40001/third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp
File
third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp
(right):

https://codereview.chromium.org/1318393002/diff/40001/third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp#newcode128
third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp:128:
for (; iter != mandatoryConstraintsHashMap.end(); ++iter)

On 2015/10/05 12:54:01, hta - Chromium wrote:
> On 2015/10/05 12:34:12, Guido Urdaneta wrote:
> > Can you use range for here?

> can you point to an examle of range for? I think that's a C++ 11
feature I've
> never used.
> This is copy/pasted code (I intend to remove the code I pasted from,
but it
> turned out to have one more user).

Since this is not an STL container I don't know if it range for actually
applies.
http://en.cppreference.com/w/cpp/language/range-for

https://codereview.chromium.org/1318393002/

[pe...@chromium.org]

https://codereview.chromium.org/1318393002/diff/40001/third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp
File
third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp
(right):

https://codereview.chromium.org/1318393002/diff/40001/third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp#newcode120
third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp:120:
Dictionary mandatoryConstraintsDictionary = constraintsIn.mandatory();
Parsing of |mandatoryConstraintsDictionary| and
|optionalConstraintsVector| in this method are identical to the parse()
function above.

Rather than duplicating the code, could we perhaps two static (or
anonymous) functions for parsing the "mandatory" and "_optional"
members, both of which are called directly by the create() methods
below?

https://codereview.chromium.org/1318393002/diff/40001/third_party/WebKit/Source/modules/mediastream/MediaStreamConstraints.idl
File
third_party/WebKit/Source/modules/mediastream/MediaStreamConstraints.idl
(right):

https://codereview.chromium.org/1318393002/diff/40001/third_party/WebKit/Source/modules/mediastream/MediaStreamConstraints.idl#newcode5
third_party/WebKit/Source/modules/mediastream/MediaStreamConstraints.idl:5:

micro nit: please link to the spec

//
https://w3c.github.io/mediacapture-main/getusermedia.html#idl-def-MediaStreamConstraints

https://codereview.chromium.org/1318393002/diff/40001/third_party/WebKit/Source/modules/mediastream/MediaStreamConstraints.idl#newcode7
third_party/WebKit/Source/modules/mediastream/MediaStreamConstraints.idl:7:
// Dictionary should be MediaTrackConstraintSet. crbug/524424 blocks
this.
nit: crbug.com/524424

You may want to phrase this as a TODO to resolve when the issue is
fixed. The information in lines 8-10 would be of more value in the bug.
(As the canonical place for information on the issue.)

https://codereview.chromium.org/1318393002/diff/40001/third_party/WebKit/Source/modules/mediastream/MediaTrackConstraintSet.idl
File
third_party/WebKit/Source/modules/mediastream/MediaTrackConstraintSet.idl
(right):

https://codereview.chromium.org/1318393002/diff/40001/third_party/WebKit/Source/modules/mediastream/MediaTrackConstraintSet.idl#newcode4
third_party/WebKit/Source/modules/mediastream/MediaTrackConstraintSet.idl:4:

micro nit: Please link to the spec:

//
https://w3c.github.io/mediacapture-main/getusermedia.html#idl-def-MediaTrackConstraintSet

https://codereview.chromium.org/1318393002/diff/40001/third_party/WebKit/Source/modules/mediastream/MediaTrackConstraintSet.idl#newcode8
third_party/WebKit/Source/modules/mediastream/MediaTrackConstraintSet.idl:8:
// Next line doesn't compile - crbug/524357
What does "Next line doesn't compile" mean? Try-bots are proving you
wrong :)

nit: crbug.com/524357

https://codereview.chromium.org/1318393002/diff/40001/third_party/WebKit/Source/modules/mediastream/MediaTrackConstraintSet.idl#newcode11
third_party/WebKit/Source/modules/mediastream/MediaTrackConstraintSet.idl:11:
DOMString foobar;
Please remove this line.

https://codereview.chromium.org/1318393002/

[h...@chromium.org]

Significant blush for the work-in-progress comments I'd forgotten to remove!

See if you like this split into helper functions better - there was less
common
code than I thought at first.

https://codereview.chromium.org/1318393002/diff/40001/third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp
File
third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp
(right):

https://codereview.chromium.org/1318393002/diff/40001/third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp#newcode120
third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp:120:
Dictionary mandatoryConstraintsDictionary = constraintsIn.mandatory();

On 2015/10/05 13:49:09, Peter Beverloo wrote:
> Parsing of |mandatoryConstraintsDictionary| and
|optionalConstraintsVector| in
> this method are identical to the parse() function above.

> Rather than duplicating the code, could we perhaps two static (or
anonymous)
> functions for parsing the "mandatory" and "_optional" members, both of
which are
> called directly by the create() methods below?

There are subtle differences in the control flow, but significant chunks
can be extracted. Does this look better?

https://codereview.chromium.org/1318393002/diff/40001/third_party/WebKit/Source/modules/mediastream/MediaStreamConstraints.idl
File
third_party/WebKit/Source/modules/mediastream/MediaStreamConstraints.idl
(right):

https://codereview.chromium.org/1318393002/diff/40001/third_party/WebKit/Source/modules/mediastream/MediaStreamConstraints.idl#newcode5
third_party/WebKit/Source/modules/mediastream/MediaStreamConstraints.idl:5:

On 2015/10/05 13:49:09, Peter Beverloo wrote:
> micro nit: please link to the spec

> //

https://w3c.github.io/mediacapture-main/getusermedia.html#idl-def-MediaStreamConstraints

Done.

https://codereview.chromium.org/1318393002/diff/40001/third_party/WebKit/Source/modules/mediastream/MediaStreamConstraints.idl#newcode7
third_party/WebKit/Source/modules/mediastream/MediaStreamConstraints.idl:7:
// Dictionary should be MediaTrackConstraintSet. crbug/524424 blocks
this.

On 2015/10/05 13:49:09, Peter Beverloo wrote:
> nit: crbug.com/524424

> You may want to phrase this as a TODO to resolve when the issue is
fixed. The
> information in lines 8-10 would be of more value in the bug. (As the
canonical
> place for information on the issue.)

This is embarassing ... 524424 is fixed, and I forgot to delete the
comment!

https://codereview.chromium.org/1318393002/diff/40001/third_party/WebKit/Source/modules/mediastream/MediaTrackConstraintSet.idl
File
third_party/WebKit/Source/modules/mediastream/MediaTrackConstraintSet.idl
(right):

https://codereview.chromium.org/1318393002/diff/40001/third_party/WebKit/Source/modules/mediastream/MediaTrackConstraintSet.idl#newcode4
third_party/WebKit/Source/modules/mediastream/MediaTrackConstraintSet.idl:4:

On 2015/10/05 13:49:09, Peter Beverloo wrote:
> micro nit: Please link to the spec:

> //

https://w3c.github.io/mediacapture-main/getusermedia.html#idl-def-MediaTrackConstraintSet

Done.

https://codereview.chromium.org/1318393002/diff/40001/third_party/WebKit/Source/modules/mediastream/MediaTrackConstraintSet.idl#newcode8
third_party/WebKit/Source/modules/mediastream/MediaTrackConstraintSet.idl:8:
// Next line doesn't compile - crbug/524357

On 2015/10/05 13:49:09, Peter Beverloo wrote:
> What does "Next line doesn't compile" mean? Try-bots are proving you
wrong :)

> nit: crbug.com/524357

That I'd forgotten to remove the comment :-( (embarraassed)

https://codereview.chromium.org/1318393002/diff/40001/third_party/WebKit/Source/modules/mediastream/MediaTrackConstraintSet.idl#newcode11
third_party/WebKit/Source/modules/mediastream/MediaTrackConstraintSet.idl:11:
DOMString foobar;

On 2015/10/05 13:49:09, Peter Beverloo wrote:
> Please remove this line.

Done.

https://codereview.chromium.org/1318393002/

[pe...@chromium.org]

lgtm % few nits


https://codereview.chromium.org/1318393002/diff/80001/third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp
File
third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp
(right):

https://codereview.chromium.org/1318393002/diff/80001/third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp#newcode58
third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp:58:
mandatoryConstraintsVector.append(WebMediaConstraint(iter->key,
iter->value));
nit: Why don't use add them to |mandatory| immediately rather than first
writing them to a temporary vector, then copying the contents to it?
There are no failure clauses after this loop, so that should be safe to
do.

https://codereview.chromium.org/1318393002/diff/80001/third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp#newcode66
third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp:66:
constraint.getPropertyNames(localNames);
Would it make sense to check for the return value here, as you do for
the function above (using |ok|)?

https://codereview.chromium.org/1318393002/diff/80001/third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp#newcode131
third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp:131:
static bool parse(const MediaTrackConstraintSet& constraintsIn,
WebVector<WebMediaConstraint>& optional, WebVector<WebMediaConstraint>&
mandatory)
I'm still not a huge fan of the overloading, but it matches the style of
this file.

If you, or anybody, would touch this file in the future, a good thing to
consider would be to refactor the interface in a way that would allow
you to write unit tests for it. The functions you extracted would
actually be perfect for that already.

https://codereview.chromium.org/1318393002/diff/80001/third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp#newcode142
third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp:142:
const Vector<Dictionary> optionalConstraints = constraintsIn.optional();
const T&, otherwise you're making a copy.

https://codereview.chromium.org/1318393002/diff/80001/third_party/WebKit/Source/modules/mediastream/MediaStreamConstraints.idl
File
third_party/WebKit/Source/modules/mediastream/MediaStreamConstraints.idl
(right):

https://codereview.chromium.org/1318393002/diff/80001/third_party/WebKit/Source/modules/mediastream/MediaStreamConstraints.idl#newcode6
third_party/WebKit/Source/modules/mediastream/MediaStreamConstraints.idl:6:
//
http://w3c.github.io/mediacapture-main/getusermedia.html#idl-def-MediaStreamConstraints
consistency micro nit: We usually use the following syntax in IDL files:

=====
// copyright

// https://...

[] idl statements
=====

(note the blank lines, absence of Specification: line.)

https://codereview.chromium.org/1318393002/diff/80001/third_party/WebKit/Source/modules/mediastream/MediaStreamConstraints.idl#newcode6
third_party/WebKit/Source/modules/mediastream/MediaStreamConstraints.idl:6:
//
http://w3c.github.io/mediacapture-main/getusermedia.html#idl-def-MediaStreamConstraints
https:// please

https://codereview.chromium.org/1318393002/diff/80001/third_party/WebKit/Source/modules/mediastream/MediaTrackConstraintSet.idl
File
third_party/WebKit/Source/modules/mediastream/MediaTrackConstraintSet.idl
(right):

https://codereview.chromium.org/1318393002/diff/80001/third_party/WebKit/Source/modules/mediastream/MediaTrackConstraintSet.idl#newcode9
third_party/WebKit/Source/modules/mediastream/MediaTrackConstraintSet.idl:9:
// The "mandatory" and "optional" members are retained for conformance
micro nit: optional -> _optional

https://codereview.chromium.org/1318393002/

[h...@chromium.org]

I think it's OK now... WebVector is a very limited vector type.

https://codereview.chromium.org/1318393002/diff/80001/third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp
File
third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp
(right):

https://codereview.chromium.org/1318393002/diff/80001/third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp#newcode58
third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp:58:
mandatoryConstraintsVector.append(WebMediaConstraint(iter->key,
iter->value));

On 2015/10/08 09:50:25, Peter Beverloo wrote:
> nit: Why don't use add them to |mandatory| immediately rather than
first writing
> them to a temporary vector, then copying the contents to it? There are
no
> failure clauses after this loop, so that should be safe to do.

It seems that mandatory is a WebVector, and I can't find any append
operation in WebVector.h. Seems that WebVectors can only be modified by
assign().

https://codereview.chromium.org/1318393002/diff/80001/third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp#newcode66
third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp:66:
constraint.getPropertyNames(localNames);

On 2015/10/08 09:50:25, Peter Beverloo wrote:
> Would it make sense to check for the return value here, as you do for
the
> function above (using |ok|)?

Done.

https://codereview.chromium.org/1318393002/diff/80001/third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp#newcode131
third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp:131:
static bool parse(const MediaTrackConstraintSet& constraintsIn,
WebVector<WebMediaConstraint>& optional, WebVector<WebMediaConstraint>&
mandatory)

On 2015/10/08 09:50:25, Peter Beverloo wrote:
> I'm still not a huge fan of the overloading, but it matches the style
of this
> file.

> If you, or anybody, would touch this file in the future, a good thing
to
> consider would be to refactor the interface in a way that would allow
you to
> write unit tests for it. The functions you extracted would actually be
perfect
> for that already.

I thought Blink only believed in JS-level tests... now I see that
there's actually a gunit user here too. Will definitely consider writing
gunit tests in the future!

A next step in the refactoring is to make the other caller of create()
use MediaTrackConstraintSet instead of Dictionary; this will delete the
overload. I overloaded these functions becaue I think the names of the
functions are good, and did not want to make them harder to read; I
agree about it generally being a confusing thing.

https://codereview.chromium.org/1318393002/diff/80001/third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp#newcode142
third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp:142:
const Vector<Dictionary> optionalConstraints = constraintsIn.optional();

On 2015/10/08 09:50:25, Peter Beverloo wrote:
> const T&, otherwise you're making a copy.

Done.

https://codereview.chromium.org/1318393002/diff/80001/third_party/WebKit/Source/modules/mediastream/MediaStreamConstraints.idl
File
third_party/WebKit/Source/modules/mediastream/MediaStreamConstraints.idl
(right):

https://codereview.chromium.org/1318393002/diff/80001/third_party/WebKit/Source/modules/mediastream/MediaStreamConstraints.idl#newcode6
third_party/WebKit/Source/modules/mediastream/MediaStreamConstraints.idl:6:
//
http://w3c.github.io/mediacapture-main/getusermedia.html#idl-def-MediaStreamConstraints

On 2015/10/08 09:50:25, Peter Beverloo wrote:
> https:// please

Done.

https://codereview.chromium.org/1318393002/diff/80001/third_party/WebKit/Source/modules/mediastream/MediaStreamConstraints.idl#newcode6
third_party/WebKit/Source/modules/mediastream/MediaStreamConstraints.idl:6:
//
http://w3c.github.io/mediacapture-main/getusermedia.html#idl-def-MediaStreamConstraints

On 2015/10/08 09:50:25, Peter Beverloo wrote:
> consistency micro nit: We usually use the following syntax in IDL
files:

> =====
> // copyright

> // https://...

> [] idl statements
> =====

> (note the blank lines, absence of Specification: line.)

Thanks - these are the kind of style issues that are hard to gather from
the documentation!

https://codereview.chromium.org/1318393002/diff/80001/third_party/WebKit/Source/modules/mediastream/MediaTrackConstraintSet.idl
File
third_party/WebKit/Source/modules/mediastream/MediaTrackConstraintSet.idl
(right):

https://codereview.chromium.org/1318393002/diff/80001/third_party/WebKit/Source/modules/mediastream/MediaTrackConstraintSet.idl#newcode9
third_party/WebKit/Source/modules/mediastream/MediaTrackConstraintSet.idl:9:
// The "mandatory" and "optional" members are retained for conformance

On 2015/10/08 09:50:25, Peter Beverloo wrote:
> micro nit: optional -> _optional

Done.

https://codereview.chromium.org/1318393002/

[commit-bot@chromium.org via codereview.chromium.org]

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


https://codereview.chromium.org/1318393002/

[commit-bot@chromium.org via codereview.chromium.org]

Try jobs failed on following builders:
chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presubmit/builds/108122)

https://codereview.chromium.org/1318393002/

[to...@chromium.org]

https://codereview.chromium.org/1318393002/diff/100001/third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp
File
third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp
(right):

https://codereview.chromium.org/1318393002/diff/100001/third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp#newcode57
third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp:57:

for (; iter != mandatoryConstraintsHashMap.end(); ++iter)

range based for loop? (looks like HashMap supports that)

https://codereview.chromium.org/1318393002/diff/100001/third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp#newcode71
third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp:71:
String key = localNames[0];
is this copy necessary? (const String&?)

https://codereview.chromium.org/1318393002/diff/100001/third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp#newcode149
third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp:149:
Dictionary constraint = optionalConstraints[i];
can we avoid creating a copy or is this not really a copy?

https://codereview.chromium.org/1318393002/diff/100001/third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp#newcode185
third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp:185:
constraints.initialize(optional, mandatory);
is the ownership of the vectors given to the constraints, or are they
copied?
In general I worry that the code is parsing and copying strings a lot
and that's not a good convention to start.

https://codereview.chromium.org/1318393002/

[h...@chromium.org]

A few more const & added. Tests still pass, so I guess it works.

Asking for more guidance on range loops, and pushing back on refactoring the
Web* classes to do less copying at this point.

https://codereview.chromium.org/1318393002/diff/100001/third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp
File
third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp
(right):

https://codereview.chromium.org/1318393002/diff/100001/third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp#newcode57
third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp:57:
for (; iter != mandatoryConstraintsHashMap.end(); ++iter)

On 2015/10/09 14:49:21, tommi wrote:
> range based for loop? (looks like HashMap supports that)

Guido suggested that a couple of cycles ago, but I found it a bit
confusing to figure out how to write one, so punted.

Is it obvious enough to write in a comment how it should look?

https://codereview.chromium.org/1318393002/diff/100001/third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp#newcode71
third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp:71:
String key = localNames[0];

On 2015/10/09 14:49:21, tommi wrote:
> is this copy necessary? (const String&?)

Done.

https://codereview.chromium.org/1318393002/diff/100001/third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp#newcode149
third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp:149:
Dictionary constraint = optionalConstraints[i];

On 2015/10/09 14:49:21, tommi wrote:
> can we avoid creating a copy or is this not really a copy?

The old code (above) used a getter that returned a Dictionary. Here
we're dealing with a Vector<Dictionary>, which should allow for
references.

Done.

https://codereview.chromium.org/1318393002/diff/100001/third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp#newcode185
third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp:185:
constraints.initialize(optional, mandatory);

On 2015/10/09 14:49:21, tommi wrote:
> is the ownership of the vectors given to the constraints, or are they
copied?
> In general I worry that the code is parsing and copying strings a lot
and that's
> not a good convention to start.

It's not a start, it's been that way for a long time.

I want to get the design better for the new constraints, but worry more
about not introducing changes in behavior for the old constraints. This
is still the old stuff.

WebMediaConstraints has WebVectors of WebMediaConstraint, which has
WebString members. Not a pointer in sight; it seems to be copying all
the way down.

My experience with the Web* types is that there's a lot of that.

https://codereview.chromium.org/1318393002/

[gui...@chromium.org]

https://codereview.chromium.org/1318393002/diff/100001/third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp
File
third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp
(right):

https://codereview.chromium.org/1318393002/diff/100001/third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp#newcode57
third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp:57:
for (; iter != mandatoryConstraintsHashMap.end(); ++iter)

On 2015/10/09 16:26:28, hta - Chromium wrote:
> On 2015/10/09 14:49:21, tommi wrote:
> > range based for loop? (looks like HashMap supports that)

> Guido suggested that a couple of cycles ago, but I found it a bit
confusing to
> figure out how to write one, so punted.

> Is it obvious enough to write in a comment how it should look?

Something like this should work:

for (const auto& entry : mandatoryConstraintsHashMap)
mandatoryConstraintsVector.append(WebMediaConstraint(entry.key,
entry.value))

https://codereview.chromium.org/1318393002/

[to...@chromium.org]

https://codereview.chromium.org/1318393002/diff/100001/third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp
File
third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp
(right):

https://codereview.chromium.org/1318393002/diff/100001/third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp#newcode57
third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp:57:
for (; iter != mandatoryConstraintsHashMap.end(); ++iter)
On 2015/10/09 16:26:28, hta - Chromium wrote:
> On 2015/10/09 14:49:21, tommi wrote:
> > range based for loop? (looks like HashMap supports that)

> Guido suggested that a couple of cycles ago, but I found it a bit
confusing to
> figure out how to write one, so punted.

> Is it obvious enough to write in a comment how it should look?

Assuming it works the same way as std maps, it'd look something like
this:

for (auto& iter : mandatoryConstraintsHashMap)
<same as what you have>

(possibly |const auto&|)

https://codereview.chromium.org/1318393002/diff/100001/third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp#newcode185
third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp:185:
constraints.initialize(optional, mandatory);

On 2015/10/09 16:26:28, hta - Chromium wrote:
> On 2015/10/09 14:49:21, tommi wrote:

> > is the ownership of the vectors given to the constraints, or are
they copied?
> > In general I worry that the code is parsing and copying strings a
lot and
> that's
> > not a good convention to start.

> It's not a start, it's been that way for a long time.

> I want to get the design better for the new constraints, but worry
more about
> not introducing changes in behavior for the old constraints. This is
still the
> old stuff.

> WebMediaConstraints has WebVectors of WebMediaConstraint, which has
WebString
> members. Not a pointer in sight; it seems to be copying all the way
down.

> My experience with the Web* types is that there's a lot of that.

Sorry, my 'to start' remark didn't come out the way it was intended. I
know it's not being introduced in this cl.
OK to leave this as is.

https://codereview.chromium.org/1318393002/

[h...@chromium.org]

Adopted the range based for loop. It's beautiful, not just shiny :-)

https://codereview.chromium.org/1318393002/diff/100001/third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp
File
third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp
(right):

https://codereview.chromium.org/1318393002/diff/100001/third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp#newcode57
third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp:57:
for (; iter != mandatoryConstraintsHashMap.end(); ++iter)

On 2015/10/12 09:02:28, tommi wrote:
> On 2015/10/09 16:26:28, hta - Chromium wrote:
> > On 2015/10/09 14:49:21, tommi wrote:
> > > range based for loop? (looks like HashMap supports that)
> >
> > Guido suggested that a couple of cycles ago, but I found it a bit
confusing to
> > figure out how to write one, so punted.
> >
> > Is it obvious enough to write in a comment how it should look?

> Assuming it works the same way as std maps, it'd look something like
this:

> for (auto& iter : mandatoryConstraintsHashMap)
> <same as what you have>

> (possibly |const auto&|)

This looked so beautiful, I'm adopting it :-)

https://codereview.chromium.org/1318393002/diff/100001/third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp#newcode185
third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp:185:
constraints.initialize(optional, mandatory);

Acknowledged.

https://codereview.chromium.org/1318393002/

[to...@chromium.org]

lgtm



https://codereview.chromium.org/1318393002/

[commit-bot@chromium.org via codereview.chromium.org]

CQ is trying da patch. Follow status at

https://chromium-cq-status.appspot.com/patch-status/1318393002/140001
View timeline at
https://chromium-cq-status.appspot.com/patch-timeline/1318393002/140001


https://codereview.chromium.org/1318393002/

[commit-bot@chromium.org via codereview.chromium.org]

Committed patchset #8 (id:140001)

https://codereview.chromium.org/1318393002/

[commit-bot@chromium.org via codereview.chromium.org]

Patchset 8 (id:??) landed as
https://crrev.com/0e6149ca421d75a825064359ac01b8a71f5c3c13
Cr-Commit-Position: refs/heads/master@{#353519}

https://codereview.chromium.org/1318393002/