Fix PepperPortAllocator to resolve STUN addresses using HostResolverPrivate.

Fix PepperPortAllocator to resolve STUN addresses using HostResolverPrivate.

Also the port allocator now disables TCP ports as it is supposed to.

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

[Wez, 2012-05-15 23:11:29]

http://codereview.chromium.org/10384171/diff/1/remoting/client/plugin/pepper_...
File remoting/client/plugin/pepper_port_allocator.cc (right):

http://codereview.chromium.org/10384171/diff/1/remoting/client/plugin/pepper_...
remoting/client/plugin/pepper_port_allocator.cc:67: bool stun_address_resolved_;
Why do you need |unresolved_stun_address_| and |stun_address_resolved_| rather
than just testing |stun_address_.IsResolved()|, and potentially
|stun_address_.IsNil()|?

http://codereview.chromium.org/10384171/diff/1/remoting/client/plugin/pepper_...
remoting/client/plugin/pepper_port_allocator.cc:93:
set_flags(cricket::PORTALLOCATOR_DISABLE_TCP);
This doesn't look related to this CL's description?

http://codereview.chromium.org/10384171/diff/1/remoting/client/plugin/pepper_...
remoting/client/plugin/pepper_port_allocator.cc:185: if (!stun_address_.IsNil())
|!stun_address_.IsUnresolved()|?

http://codereview.chromium.org/10384171/diff/1/remoting/client/plugin/pepper_...
remoting/client/plugin/pepper_port_allocator.cc:185: if (!stun_address_.IsNil())
Use { } consistently with the other if(), below.

http://codereview.chromium.org/10384171/diff/1/remoting/client/plugin/pepper_...
remoting/client/plugin/pepper_port_allocator.cc:188: if
(relay_response_received_) {
This never gets set anywhere.  Do you actually need it; would it be sufficient
to test for a non-empty |relay_response_body_|?

http://codereview.chromium.org/10384171/diff/1/remoting/client/plugin/pepper_...
File remoting/client/plugin/pepper_port_allocator.h (right):

http://codereview.chromium.org/10384171/diff/1/remoting/client/plugin/pepper_...
remoting/client/plugin/pepper_port_allocator.h:20: //   * PepperPortAllocator
resolves STUN IP addresses before passing
nit: Presumably it resolves STUN host DNS names, not IP addresses?

http://codereview.chromium.org/10384171/diff/1/remoting/client/plugin/pepper_...
File remoting/client/plugin/pepper_util.h (right):

http://codereview.chromium.org/10384171/diff/1/remoting/client/plugin/pepper_...
remoting/client/plugin/pepper_util.h:26: bool
SocketAddressToPpAddressWithPort(const talk_base::SocketAddress& address,
nit: Add short explanatory comment(s) so these methods are less
orphaned-looking.

[Sergey Ulanov, 2012-05-16 00:22:30]

http://codereview.chromium.org/10384171/diff/1/remoting/client/plugin/pepper_...
File remoting/client/plugin/pepper_port_allocator.cc (right):

http://codereview.chromium.org/10384171/diff/1/remoting/client/plugin/pepper_...
remoting/client/plugin/pepper_port_allocator.cc:67: bool stun_address_resolved_;
On 2012/05/15 23:11:29, Wez wrote:
> Why do you need |unresolved_stun_address_| and |stun_address_resolved_| rather
> than just testing |stun_address_.IsResolved()|, and potentially
> |stun_address_.IsNil()|?

Removed both unresolved_stun_address_ and stun_address_resolved_.

http://codereview.chromium.org/10384171/diff/1/remoting/client/plugin/pepper_...
remoting/client/plugin/pepper_port_allocator.cc:93:
set_flags(cricket::PORTALLOCATOR_DISABLE_TCP);
On 2012/05/15 23:11:29, Wez wrote:
> This doesn't look related to this CL's description?

Updated the description.

http://codereview.chromium.org/10384171/diff/1/remoting/client/plugin/pepper_...
remoting/client/plugin/pepper_port_allocator.cc:185: if (!stun_address_.IsNil())
On 2012/05/15 23:11:29, Wez wrote:
> |!stun_address_.IsUnresolved()|?

STUN address may not be set. IsUnresolved returns false for Nil addresses.

http://codereview.chromium.org/10384171/diff/1/remoting/client/plugin/pepper_...
remoting/client/plugin/pepper_port_allocator.cc:185: if (!stun_address_.IsNil())
On 2012/05/15 23:11:29, Wez wrote:
> Use { } consistently with the other if(), below.

Done.

http://codereview.chromium.org/10384171/diff/1/remoting/client/plugin/pepper_...
remoting/client/plugin/pepper_port_allocator.cc:188: if
(relay_response_received_) {
On 2012/05/15 23:11:29, Wez wrote:
> This never gets set anywhere.

It should be set in OnResponseBodyRead(). Fixed now.
 
> Do you actually need it; would it be sufficient
> to test for a non-empty |relay_response_body_|?

Yes, we do need it. relay_response_body_ may be non-empty while we are in
process of reading server response, but haven't finished reading it yet.

http://codereview.chromium.org/10384171/diff/1/remoting/client/plugin/pepper_...
File remoting/client/plugin/pepper_port_allocator.h (right):

http://codereview.chromium.org/10384171/diff/1/remoting/client/plugin/pepper_...
remoting/client/plugin/pepper_port_allocator.h:20: //   * PepperPortAllocator
resolves STUN IP addresses before passing
On 2012/05/15 23:11:29, Wez wrote:
> nit: Presumably it resolves STUN host DNS names, not IP addresses?

Done.

http://codereview.chromium.org/10384171/diff/1/remoting/client/plugin/pepper_...
File remoting/client/plugin/pepper_util.h (right):

http://codereview.chromium.org/10384171/diff/1/remoting/client/plugin/pepper_...
remoting/client/plugin/pepper_util.h:26: bool
SocketAddressToPpAddressWithPort(const talk_base::SocketAddress& address,
On 2012/05/15 23:11:29, Wez wrote:
> nit: Add short explanatory comment(s) so these methods are less
> orphaned-looking.

Done.

[Wez, 2012-05-16 00:51:53]

lgtm

http://codereview.chromium.org/10384171/diff/1/remoting/client/plugin/pepper_...
File remoting/client/plugin/pepper_port_allocator.cc (right):

http://codereview.chromium.org/10384171/diff/1/remoting/client/plugin/pepper_...
remoting/client/plugin/pepper_port_allocator.cc:185: if (!stun_address_.IsNil())
On 2012/05/16 00:22:30, sergeyu wrote:
> On 2012/05/15 23:11:29, Wez wrote:
> > |!stun_address_.IsUnresolved()|?
> 
> STUN address may not be set. IsUnresolved returns false for Nil addresses.

OK; consider adding a DCHECK() or even CHECK() for !IsUnresolved() on
stun_address_, at this point.

[Sergey Ulanov, 2012-05-16 01:02:15]

http://codereview.chromium.org/10384171/diff/1/remoting/client/plugin/pepper_...
File remoting/client/plugin/pepper_port_allocator.cc (right):

http://codereview.chromium.org/10384171/diff/1/remoting/client/plugin/pepper_...
remoting/client/plugin/pepper_port_allocator.cc:185: if (!stun_address_.IsNil())
On 2012/05/16 00:51:53, Wez wrote:
> On 2012/05/16 00:22:30, sergeyu wrote:
> > On 2012/05/15 23:11:29, Wez wrote:
> > > |!stun_address_.IsUnresolved()|?
> > 
> > STUN address may not be set. IsUnresolved returns false for Nil addresses.
> 
> OK; consider adding a DCHECK() or even CHECK() for !IsUnresolved() on
> stun_address_, at this point.

Done. Actually we don't need this method anymore, so I simplified this code
further.

[commit-bot: I haz the power, 2012-05-16 01:03:35]

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

[commit-bot: I haz the power, 2012-05-16 01:37:20]

Try job failure for 10384171-4003 (retry) on win_rel for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2012-05-16 02:04:16]

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

[commit-bot: I haz the power, 2012-05-16 03:50:59]

Change committed as 137355