Move PortAllocator creation out of LibjingleTransportFactory.

Move PortAllocator creation out of LibjingleTransportFactory.

Host will have host-specific port allocator, so PortAllocator creation
has to be moved out of protocol code. Since  PortAllocator needs to be 
configured with network settings (nat traversal and port range), I 
refactored how these settings are passed between layers.

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

[Wez, 2012-04-27 17:58:49]

http://codereview.chromium.org/10233021/diff/3001/remoting/host/chromoting_ho...
File remoting/host/chromoting_host.cc (right):

http://codereview.chromium.org/10233021/diff/3001/remoting/host/chromoting_ho...
remoting/host/chromoting_host.cc:113: // when it is used together with TCP the
performance is veryey bad
typo: veryey

http://codereview.chromium.org/10233021/diff/3001/remoting/host/chromoting_ho...
remoting/host/chromoting_host.cc:114: // so we explicitly disable TCP
connections.
nit: ... disable channels conencting over TCP.

http://codereview.chromium.org/10233021/diff/3001/remoting/host/chromoting_ho...
remoting/host/chromoting_host.cc:117: NetworkSettings::NAT_TRAVERSAL_ENABLED) {
Replace this if/else with a switch, so it's easier to read and we get
compile-time checks on the set of values.

http://codereview.chromium.org/10233021/diff/3001/remoting/host/chromoting_ho...
remoting/host/chromoting_host.cc:141: NetworkSettings::NAT_TRAVERSAL_ENABLED;
nit: Consider defining this false, before the port-allocator test of
nat_traversal_mode, so that there's just one block defining the
NAT_TRAVERSAL_ENABLED behaviour.

http://codereview.chromium.org/10233021/diff/3001/remoting/protocol/jingle_se...
File remoting/protocol/jingle_session_manager.h (right):

http://codereview.chromium.org/10233021/diff/3001/remoting/protocol/jingle_se...
remoting/protocol/jingle_session_manager.h:56: bool fetch_nat_traversal_config);
nit: At a glance this name suggests it's fetching _our_ NAT traversal
configuration. How about |fetch_relay_and_stun_config|?

http://codereview.chromium.org/10233021/diff/3001/remoting/protocol/jingle_se...
remoting/protocol/jingle_session_manager.h:98: bool nat_travesal_;
typo: nat_travesal_

http://codereview.chromium.org/10233021/diff/3001/remoting/protocol/jingle_se...
remoting/protocol/jingle_session_manager.h:98: bool nat_travesal_;
Name this |allow_nat_traversal_| or similar, to express what it means.

http://codereview.chromium.org/10233021/diff/3001/remoting/protocol/jingle_se...
remoting/protocol/jingle_session_manager.h:98: bool nat_travesal_;
Is this used?

http://codereview.chromium.org/10233021/diff/3001/remoting/protocol/libjingle...
File remoting/protocol/libjingle_transport_factory.cc (right):

http://codereview.chromium.org/10233021/diff/3001/remoting/protocol/libjingle...
remoting/protocol/libjingle_transport_factory.cc:69: cricket::PortAllocator*
port_allocator_;
So the LibjingleTransportFactory now owns the PortAllocator - do we have any
checks to verify that the LibjingleStreamTransports don't outlive it?

http://codereview.chromium.org/10233021/diff/3001/remoting/protocol/pepper_tr...
File remoting/protocol/pepper_transport_factory.cc (left):

http://codereview.chromium.org/10233021/diff/3001/remoting/protocol/pepper_tr...
remoting/protocol/pepper_transport_factory.cc:152: if
(config_.nat_traversal_mode == TransportConfig::NAT_TRAVERSAL_ENABLED) {
This test seems to be going away because TransportConfig no longer knows about
NAT traversal; NAT traversal has been pushed up to be a host-side setting.
What's the rationale for that?

[Sergey Ulanov, 2012-04-27 22:51:14]

http://codereview.chromium.org/10233021/diff/3001/remoting/host/chromoting_ho...
File remoting/host/chromoting_host.cc (right):

http://codereview.chromium.org/10233021/diff/3001/remoting/host/chromoting_ho...
remoting/host/chromoting_host.cc:113: // when it is used together with TCP the
performance is veryey bad
On 2012/04/27 17:58:51, Wez wrote:
> typo: veryey

Done.

http://codereview.chromium.org/10233021/diff/3001/remoting/host/chromoting_ho...
remoting/host/chromoting_host.cc:114: // so we explicitly disable TCP
connections.
On 2012/04/27 17:58:51, Wez wrote:
> nit: ... disable channels conencting over TCP.

Done.

http://codereview.chromium.org/10233021/diff/3001/remoting/host/chromoting_ho...
remoting/host/chromoting_host.cc:117: NetworkSettings::NAT_TRAVERSAL_ENABLED) {
On 2012/04/27 17:58:51, Wez wrote:
> Replace this if/else with a switch, so it's easier to read and we get
> compile-time checks on the set of values.

In one of my two other CLs this code is moved and changed to a single if without
else, so no need to change it here.

http://codereview.chromium.org/10233021/diff/3001/remoting/host/chromoting_ho...
remoting/host/chromoting_host.cc:141: NetworkSettings::NAT_TRAVERSAL_ENABLED;
On 2012/04/27 17:58:51, Wez wrote:
> nit: Consider defining this false, before the port-allocator test of
> nat_traversal_mode, so that there's just one block defining the
> NAT_TRAVERSAL_ENABLED behaviour.

This code above is moved to a separate file in one of my other two CLs.

http://codereview.chromium.org/10233021/diff/3001/remoting/protocol/jingle_se...
File remoting/protocol/jingle_session_manager.h (right):

http://codereview.chromium.org/10233021/diff/3001/remoting/protocol/jingle_se...
remoting/protocol/jingle_session_manager.h:56: bool fetch_nat_traversal_config);
On 2012/04/27 17:58:51, Wez wrote:
> nit: At a glance this name suggests it's fetching _our_ NAT traversal
> configuration. How about |fetch_relay_and_stun_config|?

Renamed to fetch_stun_relay_config

http://codereview.chromium.org/10233021/diff/3001/remoting/protocol/jingle_se...
remoting/protocol/jingle_session_manager.h:98: bool nat_travesal_;
On 2012/04/27 17:58:51, Wez wrote:
> typo: nat_travesal_

Done.

http://codereview.chromium.org/10233021/diff/3001/remoting/protocol/jingle_se...
remoting/protocol/jingle_session_manager.h:98: bool nat_travesal_;
On 2012/04/27 17:58:51, Wez wrote:
> Name this |allow_nat_traversal_| or similar, to express what it means.

Done.

http://codereview.chromium.org/10233021/diff/3001/remoting/protocol/jingle_se...
remoting/protocol/jingle_session_manager.h:98: bool nat_travesal_;
On 2012/04/27 17:58:51, Wez wrote:
> Is this used?

No, removed it.

http://codereview.chromium.org/10233021/diff/3001/remoting/protocol/libjingle...
File remoting/protocol/libjingle_transport_factory.cc (right):

http://codereview.chromium.org/10233021/diff/3001/remoting/protocol/libjingle...
remoting/protocol/libjingle_transport_factory.cc:69: cricket::PortAllocator*
port_allocator_;
On 2012/04/27 17:58:51, Wez wrote:
> So the LibjingleTransportFactory now owns the PortAllocator - do we have any
> checks to verify that the LibjingleStreamTransports don't outlive it?

No. Don't think we need it - it would crash it quickly if the object are
destroyed in incorrect order.

http://codereview.chromium.org/10233021/diff/3001/remoting/protocol/pepper_tr...
File remoting/protocol/pepper_transport_factory.cc (left):

http://codereview.chromium.org/10233021/diff/3001/remoting/protocol/pepper_tr...
remoting/protocol/pepper_transport_factory.cc:152: if
(config_.nat_traversal_mode == TransportConfig::NAT_TRAVERSAL_ENABLED) {
On 2012/04/27 17:58:51, Wez wrote:
> This test seems to be going away because TransportConfig no longer knows about
> NAT traversal; NAT traversal has been pushed up to be a host-side setting.
> What's the rationale for that?

Currently TransportConfig comes from the signaling layer when creating a
transport, and I don't think that the signaling layer needs to worry about NAT
traversal. It is still possible to enable/disable NAT traversal when creating
transport layer. This particular code is used on client side only and I plan to
remove it anyway (after we migrate to the UDP API), so it's not necessary to be
able to disable NAT traversal here. Ideally STUN/Relay configuration should be
fetched outside of JingleSessionManager - I added TODO to fix that.

[Wez, 2012-04-30 20:47:29]

lgtm

[commit-bot: I haz the power, 2012-04-30 20:56:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sergeyu@chromium.org/10233021/11002

[commit-bot: I haz the power, 2012-04-30 21:08:50]

Try job failure for 10233021-11002 on win_rel for step "update".
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

Step "update" is always a major failure.
Look at the try server FAQ for more details.