Remove old-style constraints from Chrome internals

Remove all usage of "string=string" constraints in Chrome.

This removes the RTCMediaConstraints class (used to send
string constraints to WebRTC) and the "string=string"
accessors in WebMediaConstraints.h.

BUG=webrtc:4096
Committed: https://crrev.com/0a83ae755599515e677e12b78cfa07d798af705b
Cr-Commit-Position: refs/heads/master@{#381236}

[hta - Chromium, 2016-03-04 15:43:16]

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

[commit-bot: I haz the power, 2016-03-04 15:43:29]

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

[commit-bot: I haz the power, 2016-03-04 16:16:52]

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

[commit-bot: I haz the power, 2016-03-04 16:16:53]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[hta - Chromium, 2016-03-11 09:51:32]

Description was changed from

==========
Remove RTCMediaConstraints class and all usages.

DOES NOT COMPILE.
REQUIRES WEBRTC CL 1717583002.
NOT READY FOR REVIEW.

BUG=
==========

to

==========
Remove all usage of "string=string" constraints in Chrome.

This removes the RTCMediaConstraints class (used to send
string constraints to WebRTC) and the "string=string"
accessors in WebMediaConstraints.h.

Note: Depends on a webrtc roll.

BUG=webrtc:4096
==========

[hta - Chromium, 2016-03-11 09:51:32]

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

[hta - Chromium, 2016-03-11 09:52:42]

Patchset #1 (id:1) has been deleted

[hta - Chromium, 2016-03-11 09:52:53]

Patchset #1 (id:20001) has been deleted

[hta - Chromium, 2016-03-11 09:53:06]

Patchset #1 (id:40001) has been deleted

[Guido Urdaneta, 2016-03-11 14:53:30]

Not finished yet with all the files, but here's a comment.

https://codereview.chromium.org/1729683002/diff/60001/content/renderer/media/...
File content/renderer/media/media_stream_audio_processor_options.cc (right):

https://codereview.chromium.org/1729683002/diff/60001/content/renderer/media/...
content/renderer/media/media_stream_audio_processor_options.cc:140: if (!*field)
{
should this be !field? Otherwise you are testing the the (already set) value is
false.

[hta - Chromium, 2016-03-11 15:06:44]

Replied to comment (yes, reading code with rtc::optional can be a bit confusing
until you get used to it).

https://codereview.chromium.org/1729683002/diff/60001/content/renderer/media/...
File content/renderer/media/media_stream_audio_processor_options.cc (right):

https://codereview.chromium.org/1729683002/diff/60001/content/renderer/media/...
content/renderer/media/media_stream_audio_processor_options.cc:140: if (!*field)
{
On 2016/03/11 14:53:30, Guido Urdaneta wrote:
> should this be !field? Otherwise you are testing the the (already set) value
is
> false.

No, field is a pointer (and will always be non-null). *field checks if the
rtc::optional has a value or not - returns false if it's rtc::optional() and
true if it has a value (rtc::optional(false) or rtc::optional(true)).

[hta - Chromium, 2016-03-12 06:05:16]

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

[commit-bot: I haz the power, 2016-03-12 06:05:22]

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

[commit-bot: I haz the power, 2016-03-12 09:01:24]

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

[commit-bot: I haz the power, 2016-03-12 09:01:25]

Dry run: This issue passed the CQ dry run.

[hta - Chromium, 2016-03-14 14:58:51]

Description was changed from

==========
Remove all usage of "string=string" constraints in Chrome.

This removes the RTCMediaConstraints class (used to send
string constraints to WebRTC) and the "string=string"
accessors in WebMediaConstraints.h.

Note: Depends on a webrtc roll.

BUG=webrtc:4096
==========

to

==========
Remove all usage of "string=string" constraints in Chrome.

This removes the RTCMediaConstraints class (used to send
string constraints to WebRTC) and the "string=string"
accessors in WebMediaConstraints.h.

BUG=webrtc:4096
==========

[Guido Urdaneta, 2016-03-14 16:28:21]

https://codereview.chromium.org/1729683002/diff/60001/content/renderer/media/...
File content/renderer/media/rtc_peer_connection_handler.cc (right):

https://codereview.chromium.org/1729683002/diff/60001/content/renderer/media/...
content/renderer/media/rtc_peer_connection_handler.cc:939: // TODO(hta): Reject
the calling operation with "constraint error"
crbug?

https://codereview.chromium.org/1729683002/diff/60001/content/renderer/media/...
content/renderer/media/rtc_peer_connection_handler.cc:1036: // TODO(hta):
Convert
Convert what?

https://codereview.chromium.org/1729683002/diff/60001/content/renderer/media/...
content/renderer/media/rtc_peer_connection_handler.cc:1245:
peer_connection_tracker_->TrackUpdateIce(this, config, options);
should options be removed from here too?
should it be removed from updateICE?

[tommi (sloooow) - chröme, 2016-03-14 18:37:47]

great stuff!  Since we seem to be adding a dependency on a cricket:: type, I
have to ask about it.  If we have to add the dependency, then that's OK but I
would like to consider if we can reduce the scope of such a type and e.g. have a
Chromium/blink side type that we use in our methods and only translate to the
cricket type when we absolutely have to.

https://codereview.chromium.org/1729683002/diff/60001/content/renderer/media/...
File content/renderer/media/media_stream_audio_processor_options.h (right):

https://codereview.chromium.org/1729683002/diff/60001/content/renderer/media/...
content/renderer/media/media_stream_audio_processor_options.h:54: static void
ApplyFixedAudioConstraints(cricket::AudioOptions* options);
depending on a type from 'cricket' looks like a regression (new code should not
use it and we're cleaning it up elsewhere).  Is there a more appropriate type we
can use?

https://codereview.chromium.org/1729683002/diff/60001/content/renderer/media/...
File content/renderer/media/rtc_peer_connection_handler.cc (right):

https://codereview.chromium.org/1729683002/diff/60001/content/renderer/media/...
content/renderer/media/rtc_peer_connection_handler.cc:929: std::string
failing_name;
move scope to where it's needed

https://codereview.chromium.org/1729683002/diff/60001/content/renderer/media/...
content/renderer/media/rtc_peer_connection_handler.cc:937:
constraints.basic().iceRestart.name()},
just curious - did |git cl format| format this?  (I think it's fine as is, just
curious since I would have expected this to be a curveball)

https://codereview.chromium.org/1729683002/diff/60001/content/renderer/media/...
File content/renderer/media/webrtc/media_stream_video_webrtc_sink.cc (right):

https://codereview.chromium.org/1729683002/diff/60001/content/renderer/media/...
content/renderer/media/webrtc/media_stream_video_webrtc_sink.cc:134: &value);
add a comment that explains why we don't use |value|?

https://codereview.chromium.org/1729683002/diff/60001/content/renderer/media/...
File content/renderer/media/webrtc/mock_peer_connection_dependency_factory.h
(right):

https://codereview.chromium.org/1729683002/diff/60001/content/renderer/media/...
content/renderer/media/webrtc/mock_peer_connection_dependency_factory.h:38:
explicit MockAudioSource(const cricket::AudioOptions& options, bool remote);
would be great if we don't need to use a cricket:: type.

https://codereview.chromium.org/1729683002/diff/60001/content/renderer/media/...
File content/renderer/media/webrtc/peer_connection_dependency_factory.cc
(right):

https://codereview.chromium.org/1729683002/diff/60001/content/renderer/media/...
content/renderer/media/webrtc/peer_connection_dependency_factory.cc:176: //
Previous code had googEchoCancellation only.
possibly.  The echoCancellation constant controls what the default value is for
the other 'goog' constants that relate to webrtc.
E.g. if echoCancellation is false, then the default value for
googExperimentalNoiseSuppression should be false.  This might be sort of how
things work already though (as in, if echoCancellation is off, the others aren't
checked).

[hta - Chromium, 2016-03-15 06:46:05]

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

[hta - Chromium, 2016-03-15 06:47:33]

All comments addressed, I think. I think we don't have a suitable record to
replace the "cricket::" type we're using, so this will have to wait for a webrtc
CL to add one.

Adding jochen as reviewer for the Blink parts.

https://codereview.chromium.org/1729683002/diff/60001/content/renderer/media/...
File content/renderer/media/media_stream_audio_processor_options.h (right):

https://codereview.chromium.org/1729683002/diff/60001/content/renderer/media/...
content/renderer/media/media_stream_audio_processor_options.h:54: static void
ApplyFixedAudioConstraints(cricket::AudioOptions* options);
On 2016/03/14 18:37:47, tommi-chromium wrote:
> depending on a type from 'cricket' looks like a regression (new code should
not
> use it and we're cleaning it up elsewhere).  Is there a more appropriate type
we
> can use?

Not that I know of. The third_party/webrtc/api/localaudiosource.h "Create"
function (where this ends up) has two instances, one with cricket::AudioOptions
and the other with MediaConstraintsInterface - the form we're trying to get away
from.

I can add a TODO to switch to a non-cricket interface when available.

https://codereview.chromium.org/1729683002/diff/60001/content/renderer/media/...
File content/renderer/media/rtc_peer_connection_handler.cc (right):

https://codereview.chromium.org/1729683002/diff/60001/content/renderer/media/...
content/renderer/media/rtc_peer_connection_handler.cc:929: std::string
failing_name;
On 2016/03/14 18:37:47, tommi-chromium wrote:
> move scope to where it's needed

Done.

https://codereview.chromium.org/1729683002/diff/60001/content/renderer/media/...
content/renderer/media/rtc_peer_connection_handler.cc:937:
constraints.basic().iceRestart.name()},
On 2016/03/14 18:37:47, tommi-chromium wrote:
> just curious - did |git cl format| format this?  (I think it's fine as is,
just
> curious since I would have expected this to be a curveball)

Yes, it did - I just wrote it and thought "huh", and let git cl do its thing :-)

https://codereview.chromium.org/1729683002/diff/60001/content/renderer/media/...
content/renderer/media/rtc_peer_connection_handler.cc:939: // TODO(hta): Reject
the calling operation with "constraint error"
On 2016/03/14 16:28:21, Guido Urdaneta wrote:
> crbug?

Filed 594894.

https://codereview.chromium.org/1729683002/diff/60001/content/renderer/media/...
content/renderer/media/rtc_peer_connection_handler.cc:1036: // TODO(hta):
Convert
On 2016/03/14 16:28:21, Guido Urdaneta wrote:
> Convert what?

Leftover comment. Removing.

https://codereview.chromium.org/1729683002/diff/60001/content/renderer/media/...
content/renderer/media/rtc_peer_connection_handler.cc:1245:
peer_connection_tracker_->TrackUpdateIce(this, config, options);
On 2016/03/14 16:28:21, Guido Urdaneta wrote:
> should options be removed from here too?
> should it be removed from updateICE?

Yes, it should. Removing.

https://codereview.chromium.org/1729683002/diff/60001/content/renderer/media/...
File content/renderer/media/webrtc/media_stream_video_webrtc_sink.cc (right):

https://codereview.chromium.org/1729683002/diff/60001/content/renderer/media/...
content/renderer/media/webrtc/media_stream_video_webrtc_sink.cc:134: &value);
On 2016/03/14 18:37:47, tommi-chromium wrote:
> add a comment that explains why we don't use |value|?

Done.

https://codereview.chromium.org/1729683002/diff/60001/content/renderer/media/...
File content/renderer/media/webrtc/mock_peer_connection_dependency_factory.h
(right):

https://codereview.chromium.org/1729683002/diff/60001/content/renderer/media/...
content/renderer/media/webrtc/mock_peer_connection_dependency_factory.h:38:
explicit MockAudioSource(const cricket::AudioOptions& options, bool remote);
On 2016/03/14 18:37:47, tommi-chromium wrote:
> would be great if we don't need to use a cricket:: type.

Yes. See TODO in other file.

https://codereview.chromium.org/1729683002/diff/60001/content/renderer/media/...
File content/renderer/media/webrtc/peer_connection_dependency_factory.cc
(right):

https://codereview.chromium.org/1729683002/diff/60001/content/renderer/media/...
content/renderer/media/webrtc/peer_connection_dependency_factory.cc:176: //
Previous code had googEchoCancellation only.
On 2016/03/14 18:37:47, tommi-chromium wrote:
> possibly.  The echoCancellation constant controls what the default value is
for
> the other 'goog' constants that relate to webrtc.
> E.g. if echoCancellation is false, then the default value for
> googExperimentalNoiseSuppression should be false.  This might be sort of how
> things work already though (as in, if echoCancellation is off, the others
aren't
> checked).

It's more complex than that .... if echoCancellation is present, we don't check
googEchoCancellation; if it isn't, we do.

I've shared a doc with you where more info and links are found.

[hta - Chromium, 2016-03-15 06:47:48]

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

[commit-bot: I haz the power, 2016-03-15 06:47:56]

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

[commit-bot: I haz the power, 2016-03-15 06:59:27]

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

[commit-bot: I haz the power, 2016-03-15 06:59:28]

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

[hta - Chromium, 2016-03-15 07:19:26]

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

[commit-bot: I haz the power, 2016-03-15 07:19:39]

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

[commit-bot: I haz the power, 2016-03-15 09:03:54]

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

[commit-bot: I haz the power, 2016-03-15 09:03:56]

Dry run: This issue passed the CQ dry run.

[Guido Urdaneta, 2016-03-15 09:56:02]

lgtm

[jochen (gone - plz use gerrit), 2016-03-15 15:28:14]

lgtm

[jochen (gone - plz use gerrit), 2016-03-15 15:28:22]

lgtm

[tommi (sloooow) - chröme, 2016-03-15 16:57:43]

lgtm. I'll take a look at the localaudiosource stuff in webrtc later.

[hta - Chromium, 2016-03-15 16:58:40]

The CQ bit was checked by hta@chromium.org

[commit-bot: I haz the power, 2016-03-15 16:59:08]

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

[commit-bot: I haz the power, 2016-03-15 17:04:27]

Description was changed from

==========
Remove all usage of "string=string" constraints in Chrome.

This removes the RTCMediaConstraints class (used to send
string constraints to WebRTC) and the "string=string"
accessors in WebMediaConstraints.h.

BUG=webrtc:4096
==========

to

==========
Remove all usage of "string=string" constraints in Chrome.

This removes the RTCMediaConstraints class (used to send
string constraints to WebRTC) and the "string=string"
accessors in WebMediaConstraints.h.

BUG=webrtc:4096
==========

[commit-bot: I haz the power, 2016-03-15 17:04:28]

Committed patchset #3 (id:100001)

[commit-bot: I haz the power, 2016-03-15 17:05:35]

Description was changed from

==========
Remove all usage of "string=string" constraints in Chrome.

This removes the RTCMediaConstraints class (used to send
string constraints to WebRTC) and the "string=string"
accessors in WebMediaConstraints.h.

BUG=webrtc:4096
==========

to

==========
Remove all usage of "string=string" constraints in Chrome.

This removes the RTCMediaConstraints class (used to send
string constraints to WebRTC) and the "string=string"
accessors in WebMediaConstraints.h.

BUG=webrtc:4096

Committed: https://crrev.com/0a83ae755599515e677e12b78cfa07d798af705b
Cr-Commit-Position: refs/heads/master@{#381236}
==========

[commit-bot: I haz the power, 2016-03-15 17:05:37]

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