Implement HostPortAllocator.

Implement HostPortAllocator.

HostPortAllocator is a PortAllocator specific to Chromoting host.

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

[Wez, 2012-04-27 23:23:20]

http://codereview.chromium.org/10160013/diff/1/remoting/host/host_port_alloca...
File remoting/host/host_port_allocator.cc (right):

http://codereview.chromium.org/10160013/diff/1/remoting/host/host_port_alloca...
remoting/host/host_port_allocator.cc:36: // Filter out non-UDP relay ports, so
that we don't try to use them.
This is filtering out non-UDP relay ports from the peer-supplied port
configuration, so we won't try to make TCP based outgoing connections?

http://codereview.chromium.org/10160013/diff/1/remoting/host/host_port_alloca...
remoting/host/host_port_allocator.cc:39: cricket::PortConfiguration::PortList
filtered_ports;
If PortList is actually an stl::list then can't you filter out the ports
in-place by erase()ing them?

http://codereview.chromium.org/10160013/diff/1/remoting/host/host_port_alloca...
remoting/host/host_port_allocator.cc:59: url_fetcher->SetHeader("X-Stream-Type",
channel_name());
nit: May as well set the headers in order.

http://codereview.chromium.org/10160013/diff/1/remoting/host/host_port_alloca...
remoting/host/host_port_allocator.cc:76: TryCreateRelaySession();
Doesn't this mean we'll keep trying to create relay sessions if it keeps
failing?

http://codereview.chromium.org/10160013/diff/1/remoting/host/host_port_alloca...
remoting/host/host_port_allocator.cc:95: // when it is used together with TCP
the performance is veryey bad
typo: veryey

http://codereview.chromium.org/10160013/diff/1/remoting/host/host_port_alloca...
File remoting/host/host_port_allocator.h (right):

http://codereview.chromium.org/10160013/diff/1/remoting/host/host_port_alloca...
remoting/host/host_port_allocator.h:27: class HostPortAllocatorSession
Does this need to be defined in the header?  It's not a public part of the
interface, is it?

http://codereview.chromium.org/10160013/diff/1/remoting/host/host_port_alloca...
remoting/host/host_port_allocator.h:48: const std::string& response);
OnRequestDone -> OnSessionRequestDone

http://codereview.chromium.org/10160013/diff/1/remoting/host/host_port_alloca...
remoting/host/host_port_allocator.h:60: const NetworkSettings&
network_settings);
Add a comment explaining what this class does that a non-Host-specific one
wouldn't.

http://codereview.chromium.org/10160013/diff/1/remoting/host/network_settings.h
File remoting/host/network_settings.h (right):

http://codereview.chromium.org/10160013/diff/1/remoting/host/network_settings...
remoting/host/network_settings.h:22: NAT_TRAVERSAL_ENABLED,
nit: Consider re-ordering these so that ENABLED appears first, since it's the
most common configuration.

http://codereview.chromium.org/10160013/diff/1/remoting/host/network_settings...
remoting/host/network_settings.h:36: max_port(0) {
Can we do without this ugly ctor, please? :)

http://codereview.chromium.org/10160013/diff/1/remoting/protocol/libjingle_tr...
File remoting/protocol/libjingle_transport_factory.h (right):

http://codereview.chromium.org/10160013/diff/1/remoting/protocol/libjingle_tr...
remoting/protocol/libjingle_transport_factory.h:26:
scoped_ptr<cricket::HttpPortAllocatorBase> port_allocator,
It looks wrong for the caller to pass in HttpPortAllocatorBase rather than
HttpPortAllocator; clarify why we do that in a comment on the ctor?

[Sergey Ulanov, 2012-04-28 00:05:26]

http://codereview.chromium.org/10160013/diff/1/remoting/host/host_port_alloca...
File remoting/host/host_port_allocator.cc (right):

http://codereview.chromium.org/10160013/diff/1/remoting/host/host_port_alloca...
remoting/host/host_port_allocator.cc:36: // Filter out non-UDP relay ports, so
that we don't try to use them.
On 2012/04/27 23:23:21, Wez wrote:
> This is filtering out non-UDP relay ports from the peer-supplied port
> configuration, so we won't try to make TCP based outgoing connections?

Correct. Clarified the comment.

http://codereview.chromium.org/10160013/diff/1/remoting/host/host_port_alloca...
remoting/host/host_port_allocator.cc:39: cricket::PortConfiguration::PortList
filtered_ports;
On 2012/04/27 23:23:21, Wez wrote:
> If PortList is actually an stl::list then can't you filter out the ports
> in-place by erase()ing them?

it is stl::vector

http://codereview.chromium.org/10160013/diff/1/remoting/host/host_port_alloca...
remoting/host/host_port_allocator.cc:59: url_fetcher->SetHeader("X-Stream-Type",
channel_name());
On 2012/04/27 23:23:21, Wez wrote:
> nit: May as well set the headers in order.

That's the order in which they are specified in other port allocators. If I
order them alphabetically then X-Talk-Google-Relay-Auth and X-Google-Relay-Auth
won't be next to each other.

http://codereview.chromium.org/10160013/diff/1/remoting/host/host_port_alloca...
remoting/host/host_port_allocator.cc:76: TryCreateRelaySession();
On 2012/04/27 23:23:21, Wez wrote:
> Doesn't this mean we'll keep trying to create relay sessions if it keeps
> failing?

No HttpPortAllocatorBase limits number of retries.

http://codereview.chromium.org/10160013/diff/1/remoting/host/host_port_alloca...
remoting/host/host_port_allocator.cc:95: // when it is used together with TCP
the performance is veryey bad
On 2012/04/27 23:23:21, Wez wrote:
> typo: veryey

Done.

http://codereview.chromium.org/10160013/diff/1/remoting/host/host_port_alloca...
File remoting/host/host_port_allocator.h (right):

http://codereview.chromium.org/10160013/diff/1/remoting/host/host_port_alloca...
remoting/host/host_port_allocator.h:27: class HostPortAllocatorSession
On 2012/04/27 23:23:21, Wez wrote:
> Does this need to be defined in the header?  It's not a public part of the
> interface, is it?

Done.

http://codereview.chromium.org/10160013/diff/1/remoting/host/host_port_alloca...
remoting/host/host_port_allocator.h:48: const std::string& response);
On 2012/04/27 23:23:21, Wez wrote:
> OnRequestDone -> OnSessionRequestDone

Done.

http://codereview.chromium.org/10160013/diff/1/remoting/host/host_port_alloca...
remoting/host/host_port_allocator.h:60: const NetworkSettings&
network_settings);
On 2012/04/27 23:23:21, Wez wrote:
> Add a comment explaining what this class does that a non-Host-specific one
> wouldn't.

There was already a comment about it at the top of the file. Moved it here.

http://codereview.chromium.org/10160013/diff/1/remoting/host/network_settings.h
File remoting/host/network_settings.h (right):

http://codereview.chromium.org/10160013/diff/1/remoting/host/network_settings...
remoting/host/network_settings.h:22: NAT_TRAVERSAL_ENABLED,
On 2012/04/27 23:23:21, Wez wrote:
> nit: Consider re-ordering these so that ENABLED appears first, since it's the
> most common configuration.

Done.

http://codereview.chromium.org/10160013/diff/1/remoting/host/network_settings...
remoting/host/network_settings.h:36: max_port(0) {
On 2012/04/27 23:23:21, Wez wrote:
> Can we do without this ugly ctor, please? :)

Done.

http://codereview.chromium.org/10160013/diff/1/remoting/protocol/libjingle_tr...
File remoting/protocol/libjingle_transport_factory.h (right):

http://codereview.chromium.org/10160013/diff/1/remoting/protocol/libjingle_tr...
remoting/protocol/libjingle_transport_factory.h:26:
scoped_ptr<cricket::HttpPortAllocatorBase> port_allocator,
On 2012/04/27 23:23:21, Wez wrote:
> It looks wrong for the caller to pass in HttpPortAllocatorBase rather than
> HttpPortAllocator; clarify why we do that in a comment on the ctor?

Done. Also added TODO to fix it.

[Wez, 2012-04-30 19:38:38]

lgtm

http://codereview.chromium.org/10160013/diff/13002/remoting/host/host_port_al...
File remoting/host/host_port_allocator.h (right):

http://codereview.chromium.org/10160013/diff/13002/remoting/host/host_port_al...
remoting/host/host_port_allocator.h:25: // when creating relay sessions. It also
configures the itself
typo: ... the itself ...

[Sergey Ulanov, 2012-04-30 22:42:45]

http://codereview.chromium.org/10160013/diff/13002/remoting/host/host_port_al...
File remoting/host/host_port_allocator.h (right):

http://codereview.chromium.org/10160013/diff/13002/remoting/host/host_port_al...
remoting/host/host_port_allocator.h:25: // when creating relay sessions. It also
configures the itself
On 2012/04/30 19:38:38, Wez wrote:
> typo: ... the itself ...

Done.

[commit-bot: I haz the power, 2012-04-30 23:43:24]

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