Adding the ability to change ICE servers through SetConfiguration.

Adding the ability to change ICE servers through SetConfiguration.

Added a SetIceServers method to PortAllocator. Also added a new
PeerConnection Initialize method that takes a PortAllocator, in the
hope that we can get rid of PortAllocatorFactoryInterface, since the
only substantial thing a factory does is convert the webrtc:: ICE
servers to cricket:: versions.

Committed: https://crrev.com/d3b26d94399ff539db375a9b84010ee75479d4cf
Cr-Commit-Position: refs/heads/master@{#10420}

[Taylor Brandstetter, 2015-10-14 18:33:39]

deadbeef@webrtc.org changed reviewers:
+ pthatcher@webrtc.org

[pthatcher1, 2015-10-15 05:36:58]

https://codereview.webrtc.org/1391013007/diff/1/talk/app/webrtc/peerconnectio...
File talk/app/webrtc/peerconnection.cc (right):

https://codereview.webrtc.org/1391013007/diff/1/talk/app/webrtc/peerconnectio...
talk/app/webrtc/peerconnection.cc:309: std::vector<cricket::RelayServerConfig>*
cricket_turns) {
How feasible is it to simply put StunConfiguration and TurnConfiguration into
portallocator.h and push those down the whole way?  Or the other way around: use
the cricket stuff all the way up?  It's ugly that we have to convert between the
two?  

If it's not feasible right now, please leave a TODO explaining what would need
to happen to make it feasible.

https://codereview.webrtc.org/1391013007/diff/1/talk/app/webrtc/peerconnectio...
talk/app/webrtc/peerconnection.cc:417: RTC_DCHECK(observer != nullptr);
Can it just be RTC_DCHECK(observer)?

https://codereview.webrtc.org/1391013007/diff/1/talk/app/webrtc/peerconnectio...
talk/app/webrtc/peerconnection.cc:430: dtls_identity_store.Pass(), observer);
Does this mean the ice servers are passed twice and pass into the port allocator
twice?  That seems strange.  Can we make it do it just once?

https://codereview.webrtc.org/1391013007/diff/1/webrtc/p2p/client/fakeportall...
File webrtc/p2p/client/fakeportallocator.h (right):

https://codereview.webrtc.org/1391013007/diff/1/webrtc/p2p/client/fakeportall...
webrtc/p2p/client/fakeportallocator.h:108: relays_ = turn_servers;
Can we be consistent with naming?  I recommend we use "relay_servers" and
"relay_servers_".  "turn_servers" is a misnomer since it's RelayServerConfig,
not TurnServerConfig.  And "relays_" isn't consistent with "stuns_servers_".

[Taylor Brandstetter, 2015-10-15 17:13:37]

https://codereview.webrtc.org/1391013007/diff/1/talk/app/webrtc/peerconnectio...
File talk/app/webrtc/peerconnection.cc (right):

https://codereview.webrtc.org/1391013007/diff/1/talk/app/webrtc/peerconnectio...
talk/app/webrtc/peerconnection.cc:309: std::vector<cricket::RelayServerConfig>*
cricket_turns) {
On 2015/10/15 05:36:58, pthatcher1 wrote:
> How feasible is it to simply put StunConfiguration and TurnConfiguration into
> portallocator.h and push those down the whole way?  Or the other way around:
use
> the cricket stuff all the way up?  It's ugly that we have to convert between
the
> two?  
> 
> If it's not feasible right now, please leave a TODO explaining what would need
> to happen to make it feasible.

Right now, the Chromium "PortAllocator factories" use StunConfiguration and
TurnConfiguration, so it would be a little messy to transplant them. I think the
easiest thing to do would be to wait until the factory classes are removed
(assuming you're ok with that). Then ParseIceServers can just convert directly
between URLs and the cricket structures. That would be 2 CLs from now.

https://codereview.webrtc.org/1391013007/diff/1/talk/app/webrtc/peerconnectio...
talk/app/webrtc/peerconnection.cc:417: RTC_DCHECK(observer != nullptr);
On 2015/10/15 05:36:58, pthatcher1 wrote:
> Can it just be RTC_DCHECK(observer)?

"observer != nullptr" is used everywhere else in this file, so I figured I
should be consistent.

https://codereview.webrtc.org/1391013007/diff/1/talk/app/webrtc/peerconnectio...
talk/app/webrtc/peerconnection.cc:430: dtls_identity_store.Pass(), observer);
On 2015/10/15 05:36:58, pthatcher1 wrote:
> Does this mean the ice servers are passed twice and pass into the port
allocator
> twice?  That seems strange.  Can we make it do it just once?

Right now the Chromium P2PPortAllocator doesn't support SetIceServers. So until
it does (which will be the next CL in this series) we need to pass in the
servers through CreatePortAllocator.

https://codereview.webrtc.org/1391013007/diff/1/webrtc/p2p/client/fakeportall...
File webrtc/p2p/client/fakeportallocator.h (right):

https://codereview.webrtc.org/1391013007/diff/1/webrtc/p2p/client/fakeportall...
webrtc/p2p/client/fakeportallocator.h:108: relays_ = turn_servers;
On 2015/10/15 05:36:58, pthatcher1 wrote:
> Can we be consistent with naming?  I recommend we use "relay_servers" and
> "relay_servers_".  "turn_servers" is a misnomer since it's RelayServerConfig,
> not TurnServerConfig.  And "relays_" isn't consistent with "stuns_servers_".

Done.

[pthatcher1, 2015-10-20 18:31:27]

lgtm, with nits

https://codereview.webrtc.org/1391013007/diff/1/talk/app/webrtc/peerconnectio...
File talk/app/webrtc/peerconnection.cc (right):

https://codereview.webrtc.org/1391013007/diff/1/talk/app/webrtc/peerconnectio...
talk/app/webrtc/peerconnection.cc:430: dtls_identity_store.Pass(), observer);
On 2015/10/15 17:13:37, Taylor Brandstetter wrote:
> On 2015/10/15 05:36:58, pthatcher1 wrote:
> > Does this mean the ice servers are passed twice and pass into the port
> allocator
> > twice?  That seems strange.  Can we make it do it just once?
> 
> Right now the Chromium P2PPortAllocator doesn't support SetIceServers. So
until
> it does (which will be the next CL in this series) we need to pass in the
> servers through CreatePortAllocator.

Can you put a comment, then, that says "this Initialize function parses ICE
servers a second time, but it will be removed once all port allocators support
SetIceServers."?

https://codereview.webrtc.org/1391013007/diff/1/webrtc/p2p/base/portallocator.h
File webrtc/p2p/base/portallocator.h (right):

https://codereview.webrtc.org/1391013007/diff/1/webrtc/p2p/base/portallocator...
webrtc/p2p/base/portallocator.h:163: const std::vector<RelayServerConfig>&
turn_servers) = 0;
relay_servers

https://codereview.webrtc.org/1391013007/diff/1/webrtc/p2p/client/basicportal...
File webrtc/p2p/client/basicportallocator.cc (right):

https://codereview.webrtc.org/1391013007/diff/1/webrtc/p2p/client/basicportal...
webrtc/p2p/client/basicportallocator.cc:244: for (const RelayServerConfig& relay
: allocator_->relays()) {
relay_server

https://codereview.webrtc.org/1391013007/diff/1/webrtc/p2p/client/basicportal...
File webrtc/p2p/client/basicportallocator.h (right):

https://codereview.webrtc.org/1391013007/diff/1/webrtc/p2p/client/basicportal...
webrtc/p2p/client/basicportallocator.h:42: const std::vector<RelayServerConfig>&
turn_servers) override {
relay_servers

https://codereview.webrtc.org/1391013007/diff/1/webrtc/p2p/client/basicportal...
webrtc/p2p/client/basicportallocator.h:76: std::vector<RelayServerConfig>
relays_;
relay_servers_

https://codereview.webrtc.org/1391013007/diff/1/webrtc/p2p/client/fakeportall...
File webrtc/p2p/client/fakeportallocator.h (right):

https://codereview.webrtc.org/1391013007/diff/1/webrtc/p2p/client/fakeportall...
webrtc/p2p/client/fakeportallocator.h:113: const std::vector<RelayServerConfig>&
relays() const { return relays_; }
relay_servers()

https://codereview.webrtc.org/1391013007/diff/1/webrtc/p2p/client/fakeportall...
webrtc/p2p/client/fakeportallocator.h:129: std::vector<RelayServerConfig>
relays_;
relay_servers_

[Taylor Brandstetter, 2015-10-20 22:01:33]

https://codereview.webrtc.org/1391013007/diff/1/talk/app/webrtc/peerconnectio...
File talk/app/webrtc/peerconnection.cc (right):

https://codereview.webrtc.org/1391013007/diff/1/talk/app/webrtc/peerconnectio...
talk/app/webrtc/peerconnection.cc:430: dtls_identity_store.Pass(), observer);
On 2015/10/20 18:31:26, pthatcher1 wrote:
> On 2015/10/15 17:13:37, Taylor Brandstetter wrote:
> > On 2015/10/15 05:36:58, pthatcher1 wrote:
> > > Does this mean the ice servers are passed twice and pass into the port
> > allocator
> > > twice?  That seems strange.  Can we make it do it just once?
> > 
> > Right now the Chromium P2PPortAllocator doesn't support SetIceServers. So
> until
> > it does (which will be the next CL in this series) we need to pass in the
> > servers through CreatePortAllocator.
> 
> Can you put a comment, then, that says "this Initialize function parses ICE
> servers a second time, but it will be removed once all port allocators support
> SetIceServers."?

Done.

https://codereview.webrtc.org/1391013007/diff/1/webrtc/p2p/base/portallocator.h
File webrtc/p2p/base/portallocator.h (right):

https://codereview.webrtc.org/1391013007/diff/1/webrtc/p2p/base/portallocator...
webrtc/p2p/base/portallocator.h:163: const std::vector<RelayServerConfig>&
turn_servers) = 0;
On 2015/10/20 18:31:26, pthatcher1 wrote:
> relay_servers

I decided to go the other route and change everything to turn_servers. I'll
rename the structures later (that will require a Chromium CL).

[pthatcher1, 2015-10-20 22:22:00]

lgtm

https://codereview.webrtc.org/1391013007/diff/80001/webrtc/p2p/base/portalloc...
File webrtc/p2p/base/portallocator.h (right):

https://codereview.webrtc.org/1391013007/diff/80001/webrtc/p2p/base/portalloc...
webrtc/p2p/base/portallocator.h:85: // TODO(deadbeef): Rename to
TurnServerConfig.
And RelayCredentials to TurnCredentials?

[Taylor Brandstetter, 2015-10-20 23:45:30]

https://codereview.webrtc.org/1391013007/diff/80001/webrtc/p2p/base/portalloc...
File webrtc/p2p/base/portallocator.h (right):

https://codereview.webrtc.org/1391013007/diff/80001/webrtc/p2p/base/portalloc...
webrtc/p2p/base/portallocator.h:85: // TODO(deadbeef): Rename to
TurnServerConfig.
On 2015/10/20 22:22:00, pthatcher1 wrote:
> And RelayCredentials to TurnCredentials?

Added TODO there as well.

[Taylor Brandstetter, 2015-10-22 18:15:37]

The CQ bit was checked by deadbeef@webrtc.org

[Taylor Brandstetter, 2015-10-22 18:15:37]

The patchset sent to the CQ was uploaded after l-g-t-m from pthatcher@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1391013007/#ps100001 (title:
"Adding a renaming TODO.")

[commit-bot: I haz the power, 2015-10-22 18:15:46]

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

[commit-bot: I haz the power, 2015-10-22 20:15:54]

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

[commit-bot: I haz the power, 2015-10-22 20:15:55]

Try jobs failed on following builders:
  android_arm64_rel on tryserver.webrtc (JOB_TIMED_OUT, no build URL)

[Taylor Brandstetter, 2015-10-26 22:43:26]

The CQ bit was checked by deadbeef@webrtc.org

[Taylor Brandstetter, 2015-10-26 22:43:26]

The patchset sent to the CQ was uploaded after l-g-t-m from pthatcher@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1391013007/#ps120001 (title:
"Fixing patch conflict.")

[commit-bot: I haz the power, 2015-10-26 22:43:39]

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

[commit-bot: I haz the power, 2015-10-27 00:55:25]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-10-27 00:55:37]

Patchset 7 (id:??) landed as
https://crrev.com/d3b26d94399ff539db375a9b84010ee75479d4cf
Cr-Commit-Position: refs/heads/master@{#10420}

[Taylor Brandstetter, 2015-10-27 02:21:26]

A revert of this CL (patchset #7 id:120001) has been created in
https://codereview.webrtc.org/1424803004/ by deadbeef@webrtc.org.

The reason for reverting is: Caused compiler warning, breaking Chrome FYI bots..