Pass TransportConfig to TransportFactory instead of Transport.

Pass TransportConfig to TransportFactory instead of Transport.

LibjingleTransport transport instances will be sharing a single
PortAllocator, so config needs to be passed to TransportFactory.

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

[Wez, 2012-04-27 23:49:14]

http://codereview.chromium.org/10160012/diff/5002/remoting/protocol/jingle_se...
File remoting/protocol/jingle_session_manager.cc (right):

http://codereview.chromium.org/10160012/diff/5002/remoting/protocol/jingle_se...
remoting/protocol/jingle_session_manager.cc:57: TransportConfig config;
nit: Move this below the comment, to immediately precede the setters below.

http://codereview.chromium.org/10160012/diff/5002/remoting/protocol/jingle_se...
remoting/protocol/jingle_session_manager.cc:68:
transport_factory_->SetConfig(config);
nit: Move this to immediately follow the setters, so that the VLOG() is the
block on it's own, rather than breaking up the TransportConfig setting code.

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

http://codereview.chromium.org/10160012/diff/5002/remoting/protocol/libjingle...
remoting/protocol/libjingle_transport_factory.cc:327:
stun_hosts.push_back(talk_base::SocketAddress());
Don't you mean to push |stun_address|?

http://codereview.chromium.org/10160012/diff/5002/remoting/protocol/libjingle...
remoting/protocol/libjingle_transport_factory.cc:328:
http_port_allocator_->SetStunHosts(stun_hosts);
Doesn't this mean we have only a single STUN host?

http://codereview.chromium.org/10160012/diff/5002/remoting/protocol/pepper_tr...
File remoting/protocol/pepper_transport_factory.h (right):

http://codereview.chromium.org/10160012/diff/5002/remoting/protocol/pepper_tr...
remoting/protocol/pepper_transport_factory.h:23: virtual void SetConfig(const
TransportConfig& config) OVERRIDE;
nit: Precede this with // TransportFactory interface, and there's no real reason
to have a blank line between SetConfig() and the Create functions.

http://codereview.chromium.org/10160012/diff/5002/remoting/protocol/transport.h
File remoting/protocol/transport.h (right):

http://codereview.chromium.org/10160012/diff/5002/remoting/protocol/transport...
remoting/protocol/transport.h:143: virtual void SetConfig(const TransportConfig&
config) = 0;
Add some comments to explain this interface, please. :)

http://codereview.chromium.org/10160012/diff/5002/remoting/protocol/transport...
remoting/protocol/transport.h:143: virtual void SetConfig(const TransportConfig&
config) = 0;
SetConfig -> SetTransportConfig

http://codereview.chromium.org/10160012/diff/5002/remoting/protocol/transport...
remoting/protocol/transport.h:143: virtual void SetConfig(const TransportConfig&
config) = 0;
Why do you want the TransportConfig to apply per-TransportFactory rather than
per-Transport?  What if we want to e.g. have different sessions have different
transport properties?

[Sergey Ulanov, 2012-04-28 00:18:18]

http://codereview.chromium.org/10160012/diff/5002/remoting/protocol/jingle_se...
File remoting/protocol/jingle_session_manager.cc (right):

http://codereview.chromium.org/10160012/diff/5002/remoting/protocol/jingle_se...
remoting/protocol/jingle_session_manager.cc:57: TransportConfig config;
On 2012/04/27 23:49:15, Wez wrote:
> nit: Move this below the comment, to immediately precede the setters below.

Done.

http://codereview.chromium.org/10160012/diff/5002/remoting/protocol/jingle_se...
remoting/protocol/jingle_session_manager.cc:68:
transport_factory_->SetConfig(config);
On 2012/04/27 23:49:15, Wez wrote:
> nit: Move this to immediately follow the setters, so that the VLOG() is the
> block on it's own, rather than breaking up the TransportConfig setting code.

Done.

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

http://codereview.chromium.org/10160012/diff/5002/remoting/protocol/libjingle...
remoting/protocol/libjingle_transport_factory.cc:327:
stun_hosts.push_back(talk_base::SocketAddress());
On 2012/04/27 23:49:15, Wez wrote:
> Don't you mean to push |stun_address|?

Done.

http://codereview.chromium.org/10160012/diff/5002/remoting/protocol/libjingle...
remoting/protocol/libjingle_transport_factory.cc:328:
http_port_allocator_->SetStunHosts(stun_hosts);
On 2012/04/27 23:49:15, Wez wrote:
> Doesn't this mean we have only a single STUN host?

Yes. Even if we provide more than one STUN host here, it will use only the first
one (limitation of the libjingle's BasicPortAllocator that HttpPortAllocator
inherits from).

http://codereview.chromium.org/10160012/diff/5002/remoting/protocol/pepper_tr...
File remoting/protocol/pepper_transport_factory.h (right):

http://codereview.chromium.org/10160012/diff/5002/remoting/protocol/pepper_tr...
remoting/protocol/pepper_transport_factory.h:23: virtual void SetConfig(const
TransportConfig& config) OVERRIDE;
On 2012/04/27 23:49:15, Wez wrote:
> nit: Precede this with // TransportFactory interface, and there's no real
reason
> to have a blank line between SetConfig() and the Create functions.

Done.

http://codereview.chromium.org/10160012/diff/5002/remoting/protocol/transport.h
File remoting/protocol/transport.h (right):

http://codereview.chromium.org/10160012/diff/5002/remoting/protocol/transport...
remoting/protocol/transport.h:143: virtual void SetConfig(const TransportConfig&
config) = 0;
On 2012/04/27 23:49:15, Wez wrote:
> Add some comments to explain this interface, please. :)

Done.

http://codereview.chromium.org/10160012/diff/5002/remoting/protocol/transport...
remoting/protocol/transport.h:143: virtual void SetConfig(const TransportConfig&
config) = 0;
On 2012/04/27 23:49:15, Wez wrote:
> SetConfig -> SetTransportConfig

Done.

http://codereview.chromium.org/10160012/diff/5002/remoting/protocol/transport...
remoting/protocol/transport.h:143: virtual void SetConfig(const TransportConfig&
config) = 0;
On 2012/04/27 23:49:15, Wez wrote:
> Why do you want the TransportConfig to apply per-TransportFactory rather than
> per-Transport?  What if we want to e.g. have different sessions have different
> transport properties?

That's in order to be able to share PortAllocator between Transports. The
transport config specified stun/relay configuration, and I don't think we would
want to use different stun/relay servers for different transport instances.

[Wez, 2012-04-30 19:33:13]

http://codereview.chromium.org/10160012/diff/5002/remoting/protocol/transport.h
File remoting/protocol/transport.h (right):

http://codereview.chromium.org/10160012/diff/5002/remoting/protocol/transport...
remoting/protocol/transport.h:143: virtual void SetConfig(const TransportConfig&
config) = 0;
On 2012/04/28 00:18:19, sergeyu wrote:
> On 2012/04/27 23:49:15, Wez wrote:
> > Why do you want the TransportConfig to apply per-TransportFactory rather
than
> > per-Transport?  What if we want to e.g. have different sessions have
different
> > transport properties?
> 
> That's in order to be able to share PortAllocator between Transports. The
> transport config specified stun/relay configuration, and I don't think we
would
> want to use different stun/relay servers for different transport instances.

We will want to be able to enable/disable NAT traversal & "incoming" mode on a
per-session basis, which will require per-Transport configuration, though?

[Sergey Ulanov, 2012-04-30 20:14:28]

http://codereview.chromium.org/10160012/diff/5002/remoting/protocol/transport.h
File remoting/protocol/transport.h (right):

http://codereview.chromium.org/10160012/diff/5002/remoting/protocol/transport...
remoting/protocol/transport.h:143: virtual void SetConfig(const TransportConfig&
config) = 0;
On 2012/04/30 19:33:13, Wez wrote:
> On 2012/04/28 00:18:19, sergeyu wrote:
> > On 2012/04/27 23:49:15, Wez wrote:
> > > Why do you want the TransportConfig to apply per-TransportFactory rather
> than
> > > per-Transport?  What if we want to e.g. have different sessions have
> different
> > > transport properties?
> > 
> > That's in order to be able to share PortAllocator between Transports. The
> > transport config specified stun/relay configuration, and I don't think we
> would
> > want to use different stun/relay servers for different transport instances.
> 
> We will want to be able to enable/disable NAT traversal & "incoming" mode on a
> per-session basis, which will require per-Transport configuration, though?

As we discussed we don't really need to be able to enable/disable NAT traversal
per-session.

[Wez, 2012-04-30 20:16:27]

lgtm

[commit-bot: I haz the power, 2012-04-30 22:10:02]

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