Resolve addresses of STUN/Relay servers in JingleInfoRequest.

Resolve addresses of STUN/Relay servers in JingleInfoRequest.

BUG=83242
TEST=NAT traversal works

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=96060

[Sergey Ulanov, 2011-08-08 18:53:04]

This code depends on http://codereview.chromium.org/7599003/

[Wez, 2011-08-08 23:07:59]

http://codereview.chromium.org/7598004/diff/1/remoting/client/ipc_host_addres...
File remoting/client/ipc_host_address_resolver.cc (right):

http://codereview.chromium.org/7598004/diff/1/remoting/client/ipc_host_addres...
remoting/client/ipc_host_address_resolver.cc:23: request_->Cancel();
nit: I don't recall the other CL guaranteeing that the callback previously
supplied to |request_| will definitely not be called after Cancel().

http://codereview.chromium.org/7598004/diff/1/remoting/client/ipc_host_addres...
File remoting/client/ipc_host_address_resolver.h (right):

http://codereview.chromium.org/7598004/diff/1/remoting/client/ipc_host_addres...
remoting/client/ipc_host_address_resolver.h:16: // resolve host addresses.
nit: It uses a P2PSocketDispatcher, which uses IPC under-the-hood, but this
class doesn't strictly need to know that.

http://codereview.chromium.org/7598004/diff/1/remoting/client/ipc_host_addres...
remoting/client/ipc_host_address_resolver.h:19: // HostAddressResolver is moved
to libjingle.
Is this not very specific to libjingle, i.e. it belongs in jingle/glue, not
content/renderer?

http://codereview.chromium.org/7598004/diff/1/remoting/jingle_glue/host_addre...
File remoting/jingle_glue/host_address_resolver.h (right):

http://codereview.chromium.org/7598004/diff/1/remoting/jingle_glue/host_addre...
remoting/jingle_glue/host_address_resolver.h:19: class HostAddressRequest {
Isn't this just a HostResolver?

http://codereview.chromium.org/7598004/diff/1/remoting/jingle_glue/host_addre...
remoting/jingle_glue/host_address_resolver.h:27: const
talk_base::SocketAddress&> SignalDone;
This only supports a single resolved address, whereas a host may resolve to many
addresses, as well as to alternate names.  Is it a problem that this interface
only supports IPv4?

http://codereview.chromium.org/7598004/diff/1/remoting/jingle_glue/host_addre...
remoting/jingle_glue/host_address_resolver.h:33: class HostAddressResolver {
Isn't this a HostResolverFactory?

http://codereview.chromium.org/7598004/diff/1/remoting/jingle_glue/jingle_inf...
File remoting/jingle_glue/jingle_info_request.cc (right):

http://codereview.chromium.org/7598004/diff/1/remoting/jingle_glue/jingle_inf...
remoting/jingle_glue/jingle_info_request.cc:20: struct
JingleInfoRequest::PendingDnsRequest {
If you made the HostAddressResolver interface more like getaddrinfo(), i.e. have
it take the host name _and_ the port/service-name then you wouldn't need this
class; the port would be part of the information in the HostAddressResolver
instance, for it to return with the addresses.

http://codereview.chromium.org/7598004/diff/1/remoting/jingle_glue/jingle_inf...
remoting/jingle_glue/jingle_info_request.cc:78:
stun_hosts_.push_back(talk_base::SocketAddress(host, port));
It might be simpler to mandate that HostAddressRequest must cope with IP
literals, and implement this short-cut in that class, akin to the way
getaddrinfo() works?

http://codereview.chromium.org/7598004/diff/1/remoting/jingle_glue/jingle_inf...
remoting/jingle_glue/jingle_info_request.cc:103: relay_hosts_.push_back(host);
Same here.

[Sergey Ulanov, 2011-08-09 01:42:15]

http://codereview.chromium.org/7598004/diff/1/remoting/client/ipc_host_addres...
File remoting/client/ipc_host_address_resolver.cc (right):

http://codereview.chromium.org/7598004/diff/1/remoting/client/ipc_host_addres...
remoting/client/ipc_host_address_resolver.cc:23: request_->Cancel();
On 2011/08/08 23:07:59, Wez wrote:
> nit: I don't recall the other CL guaranteeing that the callback previously
> supplied to |request_| will definitely not be called after Cancel().

It does guarantee it, otherwise what's the point of the Cancel() method? I've
update comments in the previous CL.

http://codereview.chromium.org/7598004/diff/1/remoting/client/ipc_host_addres...
File remoting/client/ipc_host_address_resolver.h (right):

http://codereview.chromium.org/7598004/diff/1/remoting/client/ipc_host_addres...
remoting/client/ipc_host_address_resolver.h:16: // resolve host addresses.
On 2011/08/08 23:07:59, Wez wrote:
> nit: It uses a P2PSocketDispatcher, which uses IPC under-the-hood, but this
> class doesn't strictly need to know that.

Done.

http://codereview.chromium.org/7598004/diff/1/remoting/client/ipc_host_addres...
remoting/client/ipc_host_address_resolver.h:19: // HostAddressResolver is moved
to libjingle.
On 2011/08/08 23:07:59, Wez wrote:
> Is this not very specific to libjingle, i.e. it belongs in jingle/glue, not
> content/renderer?
This class depends on P2PSocketDispatcher and P2PSocketDispatcher depends on
chrome/content, but jingle/glue may not depend on anything in content/. Ideally
in the future this code will not be used anywhere beside P2PTransportImpl.

http://codereview.chromium.org/7598004/diff/1/remoting/jingle_glue/host_addre...
File remoting/jingle_glue/host_address_resolver.h (right):

http://codereview.chromium.org/7598004/diff/1/remoting/jingle_glue/host_addre...
remoting/jingle_glue/host_address_resolver.h:19: class HostAddressRequest {
On 2011/08/08 23:07:59, Wez wrote:
> Isn't this just a HostResolver?

Done.

http://codereview.chromium.org/7598004/diff/1/remoting/jingle_glue/host_addre...
remoting/jingle_glue/host_address_resolver.h:27: const
talk_base::SocketAddress&> SignalDone;
On 2011/08/08 23:07:59, Wez wrote:
> This only supports a single resolved address, whereas a host may resolve to
many
> addresses, as well as to alternate names. 
It reflects the current behavior implemented in libjingle. Normally names of
STUN servers resolve to single address, and I'm not sure will will ever need
multiple addresses for the same domain name. The server may always provide
multiple stun server names if it is necessary to have multiple STUN servers.

> Is it a problem that this interface only supports IPv4?
It will support IPv6 when IPv6 support is implemented in
talk_base::SocketAddress.

http://codereview.chromium.org/7598004/diff/1/remoting/jingle_glue/host_addre...
remoting/jingle_glue/host_address_resolver.h:33: class HostAddressResolver {
On 2011/08/08 23:07:59, Wez wrote:
> Isn't this a HostResolverFactory?

Done.

http://codereview.chromium.org/7598004/diff/1/remoting/jingle_glue/jingle_inf...
File remoting/jingle_glue/jingle_info_request.cc (right):

http://codereview.chromium.org/7598004/diff/1/remoting/jingle_glue/jingle_inf...
remoting/jingle_glue/jingle_info_request.cc:20: struct
JingleInfoRequest::PendingDnsRequest {
On 2011/08/08 23:07:59, Wez wrote:
> If you made the HostAddressResolver interface more like getaddrinfo(), i.e.
have
> it take the host name _and_ the port/service-name then you wouldn't need this
> class; the port would be part of the information in the HostAddressResolver
> instance, for it to return with the addresses.

Done.

http://codereview.chromium.org/7598004/diff/1/remoting/jingle_glue/jingle_inf...
remoting/jingle_glue/jingle_info_request.cc:78:
stun_hosts_.push_back(talk_base::SocketAddress(host, port));
On 2011/08/08 23:07:59, Wez wrote:
> It might be simpler to mandate that HostAddressRequest must cope with IP
> literals, and implement this short-cut in that class, akin to the way
> getaddrinfo() works?

Done.

http://codereview.chromium.org/7598004/diff/1/remoting/jingle_glue/jingle_inf...
remoting/jingle_glue/jingle_info_request.cc:103: relay_hosts_.push_back(host);
On 2011/08/08 23:07:59, Wez wrote:
> Same here.
Actually we don't need to resolve relay hosts.

[Wez, 2011-08-09 03:27:53]

LGTM