Switch from using internal constraints to use explicit PeerConnectionFactoryInterface::SetOptions t…

Switch from using internal constraints to use explicit PeerConnectionFactoryInterface::SetOptions to set command line options on new PeerConnections.

BUG=304024
R=fischman@chromium.org, jiayl@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=232702

[Ami GONE FROM CHROMIUM, 2013-10-20 20:32:11]

<drive-by>

https://codereview.chromium.org/30983002/diff/1/content/renderer/media/media_...
File content/renderer/media/media_stream_dependency_factory.cc (right):

https://codereview.chromium.org/30983002/diff/1/content/renderer/media/media_...
content/renderer/media/media_stream_dependency_factory.cc:563: return
pc_factory_.get() != NULL;
Isn't this unconditionally true now?

[perkj_chrome, 2013-10-21 09:56:57]

Addressed fishmans comment and rebased cl to latest.

https://codereview.chromium.org/30983002/diff/1/content/renderer/media/media_...
File content/renderer/media/media_stream_dependency_factory.cc (right):

https://codereview.chromium.org/30983002/diff/1/content/renderer/media/media_...
content/renderer/media/media_stream_dependency_factory.cc:563: return
pc_factory_.get() != NULL;
On 2013/10/20 20:32:11, Ami Fischman wrote:
> Isn't this unconditionally true now?

yes.... I rewrote the if statement.

[jiayl, 2013-10-23 16:40:51]

lgtm, but you need an owner to review, e.g. fischman

[Ami GONE FROM CHROMIUM, 2013-10-23 21:33:11]

LGTM % nits and clarifying question.

https://codereview.chromium.org/30983002/diff/50001/content/renderer/media/me...
File content/renderer/media/media_stream_dependency_factory.cc (right):

https://codereview.chromium.org/30983002/diff/50001/content/renderer/media/me...
content/renderer/media/media_stream_dependency_factory.cc:513: audio_device_ =
new WebRtcAudioDeviceImpl();
nit: it would be clearer in my opinion to make this a local  scoped_refptr, and
then assign the local value to audio_device_ around l.550, i.e. only if
pc_factory_ is being set to something non-NULL.

I'd also move this "new" statement to as close as the first use of the var (the
webrtc::CreatePeerConnectionFactory() at l.539) as possible.

https://codereview.chromium.org/30983002/diff/50001/content/renderer/media/me...
content/renderer/media/media_stream_dependency_factory.cc:537: 
delete extra \n ?

https://codereview.chromium.org/30983002/diff/50001/content/renderer/media/me...
content/renderer/media/media_stream_dependency_factory.cc:551:
PeerConnectionFactoryInterface::Options factory_options;
Probably obvious but this can't land until 55263923 rolls into codesite webrtc
and chromium DEPS rolls.

https://codereview.chromium.org/30983002/diff/50001/content/renderer/media/me...
content/renderer/media/media_stream_dependency_factory.cc:554:
factory_options.disable_sctp_data_channels=
s/=/ =/

https://codereview.chromium.org/30983002/diff/50001/content/renderer/media/rt...
File content/renderer/media/rtc_media_constraints.cc (left):

https://codereview.chromium.org/30983002/diff/50001/content/renderer/media/rt...
content/renderer/media/rtc_media_constraints.cc:30: // Ignore internal
constraints set by JS.
Just to make sure: you're ok with internal constraints being settable from JS
now?
(perhaps this filtering should only be dropped once libjingle no longer honors
any internal constraints?)

[perkj_chrome, 2013-11-01 09:12:54]

https://codereview.chromium.org/30983002/diff/50001/content/renderer/media/me...
File content/renderer/media/media_stream_dependency_factory.cc (right):

https://codereview.chromium.org/30983002/diff/50001/content/renderer/media/me...
content/renderer/media/media_stream_dependency_factory.cc:513: audio_device_ =
new WebRtcAudioDeviceImpl();
On 2013/10/23 21:33:11, Ami Fischman wrote:
> nit: it would be clearer in my opinion to make this a local  scoped_refptr,
and
> then assign the local value to audio_device_ around l.550, i.e. only if
> pc_factory_ is being set to something non-NULL.
> 
> I'd also move this "new" statement to as close as the first use of the var
(the
> webrtc::CreatePeerConnectionFactory() at l.539) as possible.

good points. Done.

https://codereview.chromium.org/30983002/diff/50001/content/renderer/media/me...
content/renderer/media/media_stream_dependency_factory.cc:537: 
On 2013/10/23 21:33:11, Ami Fischman wrote:
> delete extra \n ?

Done.

https://codereview.chromium.org/30983002/diff/50001/content/renderer/media/me...
content/renderer/media/media_stream_dependency_factory.cc:551:
PeerConnectionFactoryInterface::Options factory_options;
On 2013/10/23 21:33:11, Ami Fischman wrote:
> Probably obvious but this can't land until 55263923 rolls into codesite webrtc
> and chromium DEPS rolls.

Done.

https://codereview.chromium.org/30983002/diff/50001/content/renderer/media/me...
content/renderer/media/media_stream_dependency_factory.cc:554:
factory_options.disable_sctp_data_channels=
On 2013/10/23 21:33:11, Ami Fischman wrote:
> s/=/ =/

Done.

https://codereview.chromium.org/30983002/diff/50001/content/renderer/media/rt...
File content/renderer/media/rtc_media_constraints.cc (left):

https://codereview.chromium.org/30983002/diff/50001/content/renderer/media/rt...
content/renderer/media/rtc_media_constraints.cc:30: // Ignore internal
constraints set by JS.
On 2013/10/23 21:33:11, Ami Fischman wrote:
> Just to make sure: you're ok with internal constraints being settable from JS
> now?
> (perhaps this filtering should only be dropped once libjingle no longer honors
> any internal constraints?)

All internal constraints are removed as far as I know. But why not keep this
anyway for a while.

[commit-bot: I haz the power, 2013-11-01 09:13:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/perkj@chromium.org/30983002/130001

[commit-bot: I haz the power, 2013-11-01 10:02:19]

Sorry for I got bad news for ya.
Compile failed with a clobber build on win_x64_rel.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_x64_re...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-11-01 11:57:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/perkj@chromium.org/30983002/130001

[commit-bot: I haz the power, 2013-11-01 15:28:43]

Retried try job too often on win_rel for step(s) chrome_frame_tests,
content_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-11-01 19:10:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/perkj@chromium.org/30983002/130001

[commit-bot: I haz the power, 2013-11-01 22:31:58]

Retried try job too often on win_rel for step(s) content_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-11-02 06:18:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/perkj@chromium.org/30983002/130001

[commit-bot: I haz the power, 2013-11-02 09:27:31]

Retried try job too often on win_rel for step(s) content_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[perkj_chrome, 2013-11-04 12:35:38]

Committed patchset #3 manually as r232702 (presubmit successful).