Changed the jingle network code in ChromeAsyncSocket to use the client socket pool. This also all...

Changed the jingle network code in ChromeAsyncSocket to use the client socket pool. This also allows the connection to be able to tunnel through proxies.

BUG=77430
TEST=Unit-tests, sync unit-tests, test Cloud Print and Sync behind procy servers.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=81820

[sanjeevr, 2011-04-13 19:21:10]

willchan, please review the net/ changes.

akalin, please review everything.

[akalin, 2011-04-13 20:49:32]

willchan@, do you have any suggestions for the best way for sanjeevr to proceed
is?  Basically, he's trying to add support for proxy resolution, and it seems to
me that it can be achieved by changing the ClientSocketFactory into a
ClientSocketPool(Base) which handles all those details, and ChromeAsyncSocket
shouldn't need to know the details of the proxy resolution, fake ssl stuff, etc.

http://codereview.chromium.org/6833031/diff/2004/jingle/notifier/base/chrome_...
File jingle/notifier/base/chrome_async_socket.cc (right):

http://codereview.chromium.org/6833031/diff/2004/jingle/notifier/base/chrome_...
jingle/notifier/base/chrome_async_socket.cc:42: const
scoped_refptr<net::URLRequestContextGetter>& request_context_getter,
It seems to me that a socket shouldn't need to know about the full
URLRequestContextGetter.  In fact, shouldn't it suffice to pass in a
ClientSocketPool that it can request sockets from?  Or maybe
ClientSocketPoolBase/ConnectJob.  Basically, I think ChromeAsyncSocket should
simply call some magical "Connect" method that handles proxy resolution, ssl
handshake, etc. behind the scenes.

http://codereview.chromium.org/6833031/diff/2004/jingle/notifier/base/chrome_...
jingle/notifier/base/chrome_async_socket.cc:57:
fake_ssl_handshake_callback_(ALLOW_THIS_IN_INITIALIZER_LIST(this),
i don't think ChromeAsyncSocket should know anything about FakeSSLClientSocket
-- it should get a ClientSocket(Handle) from somewhere and use that interface.

http://codereview.chromium.org/6833031/diff/2004/jingle/notifier/base/chrome_...
jingle/notifier/base/chrome_async_socket.cc:81: net::HttpNetworkSession::Params
session_params;
It seems to me that you can at least construct the HttpNetworkSession outside of
the constructor and pass that in instead of the URLRequestContextGetter.

http://codereview.chromium.org/6833031/diff/2004/jingle/notifier/base/chrome_...
jingle/notifier/base/chrome_async_socket.cc:275: connection_.reset(new
net::ClientSocketHandle);
Yeah this part is weird.  Shouldn't we use a socket pool and call that, instead
of having to create sockets ourselves?

[sanjeevr, 2011-04-13 21:20:14]

Will and I had spoken about this a couple of weeks back before I started and
this was the tentative approach we decided on. I agree that it would be nice to
get the proxy resolution also into a separate class.

On 2011/04/13 20:49:32, akalin wrote:
> willchan@, do you have any suggestions for the best way for sanjeevr to
proceed
> is?  Basically, he's trying to add support for proxy resolution, and it seems
to
> me that it can be achieved by changing the ClientSocketFactory into a
> ClientSocketPool(Base) which handles all those details, and ChromeAsyncSocket
> shouldn't need to know the details of the proxy resolution, fake ssl stuff,
etc.
> 
>
http://codereview.chromium.org/6833031/diff/2004/jingle/notifier/base/chrome_...
> File jingle/notifier/base/chrome_async_socket.cc (right):
> 
>
http://codereview.chromium.org/6833031/diff/2004/jingle/notifier/base/chrome_...
> jingle/notifier/base/chrome_async_socket.cc:42: const
> scoped_refptr<net::URLRequestContextGetter>& request_context_getter,
> It seems to me that a socket shouldn't need to know about the full
> URLRequestContextGetter.  In fact, shouldn't it suffice to pass in a
> ClientSocketPool that it can request sockets from?  Or maybe
> ClientSocketPoolBase/ConnectJob.  Basically, I think ChromeAsyncSocket should
> simply call some magical "Connect" method that handles proxy resolution, ssl
> handshake, etc. behind the scenes.
> 
>
http://codereview.chromium.org/6833031/diff/2004/jingle/notifier/base/chrome_...
> jingle/notifier/base/chrome_async_socket.cc:57:
> fake_ssl_handshake_callback_(ALLOW_THIS_IN_INITIALIZER_LIST(this),
> i don't think ChromeAsyncSocket should know anything about FakeSSLClientSocket
> -- it should get a ClientSocket(Handle) from somewhere and use that interface.
> 
>
http://codereview.chromium.org/6833031/diff/2004/jingle/notifier/base/chrome_...
> jingle/notifier/base/chrome_async_socket.cc:81:
net::HttpNetworkSession::Params
> session_params;
> It seems to me that you can at least construct the HttpNetworkSession outside
of
> the constructor and pass that in instead of the URLRequestContextGetter.
> 
>
http://codereview.chromium.org/6833031/diff/2004/jingle/notifier/base/chrome_...
> jingle/notifier/base/chrome_async_socket.cc:275: connection_.reset(new
> net::ClientSocketHandle);
> Yeah this part is weird.  Shouldn't we use a socket pool and call that,
instead
> of having to create sockets ourselves?

[sanjeevr, 2011-04-13 21:20:21]

http://codereview.chromium.org/6833031/diff/2004/jingle/notifier/base/chrome_...
File jingle/notifier/base/chrome_async_socket.cc (right):

http://codereview.chromium.org/6833031/diff/2004/jingle/notifier/base/chrome_...
jingle/notifier/base/chrome_async_socket.cc:42: const
scoped_refptr<net::URLRequestContextGetter>& request_context_getter,
On 2011/04/13 20:49:32, akalin wrote:
> It seems to me that a socket shouldn't need to know about the full
> URLRequestContextGetter.  In fact, shouldn't it suffice to pass in a
> ClientSocketPool that it can request sockets from?  Or maybe
> ClientSocketPoolBase/ConnectJob.  Basically, I think ChromeAsyncSocket should
> simply call some magical "Connect" method that handles proxy resolution, ssl
> handshake, etc. behind the scenes.

This is not straightforward. Basically there is complicated logic about which
socket pools to request sockets from and the sockets are layered on top of each
other.

http://codereview.chromium.org/6833031/diff/2004/jingle/notifier/base/chrome_...
jingle/notifier/base/chrome_async_socket.cc:57:
fake_ssl_handshake_callback_(ALLOW_THIS_IN_INITIALIZER_LIST(this),
On 2011/04/13 20:49:32, akalin wrote:
> i don't think ChromeAsyncSocket should know anything about FakeSSLClientSocket
> -- it should get a ClientSocket(Handle) from somewhere and use that interface.

Yes, we can supply this layer externally.

http://codereview.chromium.org/6833031/diff/2004/jingle/notifier/base/chrome_...
jingle/notifier/base/chrome_async_socket.cc:81: net::HttpNetworkSession::Params
session_params;
On 2011/04/13 20:49:32, akalin wrote:
> It seems to me that you can at least construct the HttpNetworkSession outside
of
> the constructor and pass that in instead of the URLRequestContextGetter.

This is also doable (though it is almost the same thing)

http://codereview.chromium.org/6833031/diff/2004/jingle/notifier/base/chrome_...
jingle/notifier/base/chrome_async_socket.cc:275: connection_.reset(new
net::ClientSocketHandle);
On 2011/04/13 20:49:32, akalin wrote:
> Yeah this part is weird.  Shouldn't we use a socket pool and call that,
instead
> of having to create sockets ourselves?

We are not creating the socket ourselves. InitSocketHandleForRawConnect creates
the required layers of sockets and initializes the socket handle for us.

[sanjeevr, 2011-04-14 17:02:11]

Created a ProxyResolvingClientSocket to hide the details of the proxy resolution
and the socket pool usage. This does make it much cleaner and easier to
unit-test.

[akalin, 2011-04-14 21:54:50]

I like this much better!  Some initial comments.

http://codereview.chromium.org/6833031/diff/8018/jingle/notifier/base/proxy_r...
File jingle/notifier/base/proxy_resolving_client_socket.cc (right):

http://codereview.chromium.org/6833031/diff/8018/jingle/notifier/base/proxy_r...
jingle/notifier/base/proxy_resolving_client_socket.cc:17: 
Can you add some basic unit tests for this class?  Nothing too detailed, but
maybe one or two so that the test fixture is already there for future unit
tests.

http://codereview.chromium.org/6833031/diff/8018/jingle/notifier/base/proxy_r...
jingle/notifier/base/proxy_resolving_client_socket.cc:58:
DCHECK(transport_.get() && transport_->socket());
probably should be:

if (transport_.get() ...)
  return ...;
NOTREACHED();

return ?some error?;

or changed to CHECK (same for other functions)

And yeah, I know you probably copied this from the other socket wrappers (which
one of the net guys should really fix).

http://codereview.chromium.org/6833031/diff/8018/jingle/notifier/base/proxy_r...
jingle/notifier/base/proxy_resolving_client_socket.cc:91: // directly here as
the caller may not expect an error/close to
this comment doesn't apply (since there's no Error()/Close() method like in
ChromeAsyncSocket).  Say something like we always defer execution for
simplicity.

http://codereview.chromium.org/6833031/diff/8018/jingle/notifier/base/proxy_r...
jingle/notifier/base/proxy_resolving_client_socket.cc:219: GURL url =
GURL("http://" + dest_host_port_pair_.ToString());
need #include for gurl

http://codereview.chromium.org/6833031/diff/8018/jingle/notifier/base/proxy_r...
jingle/notifier/base/proxy_resolving_client_socket.cc:236: MessageLoop*
message_loop = MessageLoop::current();
The semantics of this function are a bit confusing, especially if we hit this
branch.  Say ReconsiderProxyAfterError() is called with some proxy error (that
hits the break case in the switch above).  Also say ReconsiderProxyAfterError()
also fails so rv = error.  Then we post a task and return rv.

Since this is only called by ProcessConnectDone, it sees the error return value,
then closes the transport socket and runs the user connect callback.  But then
the posted task (OnProcessProxyResolveDone) runs.  If tried_direct_connect_ is
true, then we call the user connect callback again (crash).  This may end up not
happening, but it's hard to reason through.  Maybe this function should either
return the original error, or return ERR_IO_PENDING (passing the original error
down)?

http://codereview.chromium.org/6833031/diff/8018/jingle/notifier/base/proxy_r...
jingle/notifier/base/proxy_resolving_client_socket.cc:265: return
transport_->socket()->GetPeerAddress(address);
if (transport.get() ... )
  return ...;
return ?error?

http://codereview.chromium.org/6833031/diff/8018/jingle/notifier/base/proxy_r...
jingle/notifier/base/proxy_resolving_client_socket.cc:269: return
transport_->socket()->NetLog();
if (transport_.get() ... )
  return ...;
NOTREACHED();
return bound_net_log_;

http://codereview.chromium.org/6833031/diff/8018/jingle/notifier/base/proxy_r...
jingle/notifier/base/proxy_resolving_client_socket.cc:301: if (transport_.get()
&& transport_->socket())
shouldn't this be:

if (transport_.get()) {
  if (transport_->socket()) {
    transport_->socket()->Disconnect();
  }
  transport_->Reset();
}
transport_.reset();

http://codereview.chromium.org/6833031/diff/8018/jingle/notifier/base/proxy_r...
File jingle/notifier/base/proxy_resolving_client_socket.h (right):

http://codereview.chromium.org/6833031/diff/8018/jingle/notifier/base/proxy_r...
jingle/notifier/base/proxy_resolving_client_socket.h:30: 
Add TODO to move this to net/

http://codereview.chromium.org/6833031/diff/8018/jingle/notifier/base/proxy_r...
jingle/notifier/base/proxy_resolving_client_socket.h:42: virtual int
Read(net::IOBuffer* buf, int buf_len,
Add OVERRIDE to overridden methods

http://codereview.chromium.org/6833031/diff/8018/jingle/notifier/base/xmpp_cl...
File jingle/notifier/base/xmpp_client_socket_factory.h (left):

http://codereview.chromium.org/6833031/diff/8018/jingle/notifier/base/xmpp_cl...
jingle/notifier/base/xmpp_client_socket_factory.h:1: // Copyright (c) 2010 The
Chromium Authors. All rights reserved.
I notice that this file is still listed as 'D' from the index.  Is that because
it was originally deleted?  It seems to work, just wondering if it's a harmless
error.

http://codereview.chromium.org/6833031/diff/8018/jingle/notifier/base/xmpp_cl...
File jingle/notifier/base/xmpp_client_socket_factory.h (right):

http://codereview.chromium.org/6833031/diff/8018/jingle/notifier/base/xmpp_cl...
jingle/notifier/base/xmpp_client_socket_factory.h:35: #if defined(UNIT_TEST)
Instead of doing this, can you create an interface (say,
ResolvingClientSocketFactory) and make this class and the mock class inherit
from it?

http://codereview.chromium.org/6833031/diff/8018/jingle/notifier/communicator...
File jingle/notifier/communicator/xmpp_connection_generator.h (right):

http://codereview.chromium.org/6833031/diff/8018/jingle/notifier/communicator...
jingle/notifier/communicator/xmpp_connection_generator.h:60: void
SetShouldResolveDNS(bool should_resolve_dns) {
Add TODO to always not resolve and rip this (and related code) out.

[sanjeevr, 2011-04-14 23:15:43]

Thanks for the exhaustive review. Made all the changes except adding the
unit-test. Working on that now. Can you take a look at the other stuff in the
meantime?

http://codereview.chromium.org/6833031/diff/8018/jingle/notifier/base/proxy_r...
File jingle/notifier/base/proxy_resolving_client_socket.cc (right):

http://codereview.chromium.org/6833031/diff/8018/jingle/notifier/base/proxy_r...
jingle/notifier/base/proxy_resolving_client_socket.cc:58:
DCHECK(transport_.get() && transport_->socket());
On 2011/04/14 21:54:50, akalin wrote:
> probably should be:
> 
> if (transport_.get() ...)
>   return ...;
> NOTREACHED();
> 
> return ?some error?;
> 
> or changed to CHECK (same for other functions)
> 
> And yeah, I know you probably copied this from the other socket wrappers
(which
> one of the net guys should really fix).

Yeah I wanted to do this, but I wasn't sure what error to return. So I copied
from what the SSLClientSocket wrapper was doing. Done.

http://codereview.chromium.org/6833031/diff/8018/jingle/notifier/base/proxy_r...
jingle/notifier/base/proxy_resolving_client_socket.cc:91: // directly here as
the caller may not expect an error/close to
On 2011/04/14 21:54:50, akalin wrote:
> this comment doesn't apply (since there's no Error()/Close() method like in
> ChromeAsyncSocket).  Say something like we always defer execution for
> simplicity.

Done.

http://codereview.chromium.org/6833031/diff/8018/jingle/notifier/base/proxy_r...
jingle/notifier/base/proxy_resolving_client_socket.cc:219: GURL url =
GURL("http://" + dest_host_port_pair_.ToString());
On 2011/04/14 21:54:50, akalin wrote:
> need #include for gurl

Done.

http://codereview.chromium.org/6833031/diff/8018/jingle/notifier/base/proxy_r...
jingle/notifier/base/proxy_resolving_client_socket.cc:236: MessageLoop*
message_loop = MessageLoop::current();
On 2011/04/14 21:54:50, akalin wrote:
> The semantics of this function are a bit confusing, especially if we hit this
> branch.  Say ReconsiderProxyAfterError() is called with some proxy error (that
> hits the break case in the switch above).  Also say
ReconsiderProxyAfterError()
> also fails so rv = error.  Then we post a task and return rv.
> 
> Since this is only called by ProcessConnectDone, it sees the error return
value,
> then closes the transport socket and runs the user connect callback.  But then
> the posted task (OnProcessProxyResolveDone) runs.  If tried_direct_connect_ is
> true, then we call the user connect callback again (crash).  This may end up
not
> happening, but it's hard to reason through.  Maybe this function should either
> return the original error, or return ERR_IO_PENDING (passing the original
error
> down)?

Done.

http://codereview.chromium.org/6833031/diff/8018/jingle/notifier/base/proxy_r...
jingle/notifier/base/proxy_resolving_client_socket.cc:265: return
transport_->socket()->GetPeerAddress(address);
On 2011/04/14 21:54:50, akalin wrote:
> if (transport.get() ... )
>   return ...;
> return ?error?

Done.

http://codereview.chromium.org/6833031/diff/8018/jingle/notifier/base/proxy_r...
jingle/notifier/base/proxy_resolving_client_socket.cc:269: return
transport_->socket()->NetLog();
On 2011/04/14 21:54:50, akalin wrote:
> if (transport_.get() ... )
>   return ...;
> NOTREACHED();
> return bound_net_log_;

Done.

http://codereview.chromium.org/6833031/diff/8018/jingle/notifier/base/proxy_r...
jingle/notifier/base/proxy_resolving_client_socket.cc:301: if (transport_.get()
&& transport_->socket())
On 2011/04/14 21:54:50, akalin wrote:
> shouldn't this be:
> 
> if (transport_.get()) {
>   if (transport_->socket()) {
>     transport_->socket()->Disconnect();
>   }
>   transport_->Reset();
> }
> transport_.reset();

Done.

http://codereview.chromium.org/6833031/diff/8018/jingle/notifier/base/proxy_r...
File jingle/notifier/base/proxy_resolving_client_socket.h (right):

http://codereview.chromium.org/6833031/diff/8018/jingle/notifier/base/proxy_r...
jingle/notifier/base/proxy_resolving_client_socket.h:30: 
On 2011/04/14 21:54:50, akalin wrote:
> Add TODO to move this to net/

Done.

http://codereview.chromium.org/6833031/diff/8018/jingle/notifier/base/proxy_r...
jingle/notifier/base/proxy_resolving_client_socket.h:42: virtual int
Read(net::IOBuffer* buf, int buf_len,
On 2011/04/14 21:54:50, akalin wrote:
> Add OVERRIDE to overridden methods

Done.

http://codereview.chromium.org/6833031/diff/8018/jingle/notifier/base/xmpp_cl...
File jingle/notifier/base/xmpp_client_socket_factory.h (left):

http://codereview.chromium.org/6833031/diff/8018/jingle/notifier/base/xmpp_cl...
jingle/notifier/base/xmpp_client_socket_factory.h:1: // Copyright (c) 2010 The
Chromium Authors. All rights reserved.
On 2011/04/14 21:54:50, akalin wrote:
> I notice that this file is still listed as 'D' from the index.  Is that
because
> it was originally deleted?  It seems to work, just wondering if it's a
harmless
> error.

Yeah I noticed that too, was not sure why. My svn diff shows it correctly as M.

http://codereview.chromium.org/6833031/diff/8018/jingle/notifier/base/xmpp_cl...
File jingle/notifier/base/xmpp_client_socket_factory.h (right):

http://codereview.chromium.org/6833031/diff/8018/jingle/notifier/base/xmpp_cl...
jingle/notifier/base/xmpp_client_socket_factory.h:35: #if defined(UNIT_TEST)
On 2011/04/14 21:54:50, akalin wrote:
> Instead of doing this, can you create an interface (say,
> ResolvingClientSocketFactory) and make this class and the mock class inherit
> from it?

Done.

http://codereview.chromium.org/6833031/diff/8018/jingle/notifier/communicator...
File jingle/notifier/communicator/xmpp_connection_generator.h (right):

http://codereview.chromium.org/6833031/diff/8018/jingle/notifier/communicator...
jingle/notifier/communicator/xmpp_connection_generator.h:60: void
SetShouldResolveDNS(bool should_resolve_dns) {
On 2011/04/14 21:54:50, akalin wrote:
> Add TODO to always not resolve and rip this (and related code) out.

Done.

[akalin, 2011-04-15 01:47:33]

Few more comments

http://codereview.chromium.org/6833031/diff/15001/jingle/notifier/base/proxy_...
File jingle/notifier/base/proxy_resolving_client_socket.cc (right):

http://codereview.chromium.org/6833031/diff/15001/jingle/notifier/base/proxy_...
jingle/notifier/base/proxy_resolving_client_socket.cc:33: bound_net_log_(
need to init tried_direct_connect_

http://codereview.chromium.org/6833031/diff/15001/jingle/notifier/base/proxy_...
jingle/notifier/base/proxy_resolving_client_socket.cc:40:
request_context_getter->GetURLRequestContext()->host_resolver();
seems cleaner to have an intermediate var for the URLRequestContext

http://codereview.chromium.org/6833031/diff/15001/jingle/notifier/base/proxy_...
jingle/notifier/base/proxy_resolving_client_socket.cc:56: if (pac_request_)
I think this clause belongs to Disconnect(), since pac_request_ is generated
there, and it's possible someone may call Disconnect() before it's finished. 
Then you can replace this clause here with Disconnect().

http://codereview.chromium.org/6833031/diff/15001/jingle/notifier/base/proxy_...
jingle/notifier/base/proxy_resolving_client_socket.cc:177: if ((status ==
net::OK) || (status == net::ERR_IO_PENDING)) {
DCHECK that status is either the original status or PENDING, then change the if
to:

if (status == PENDING) {
  return;
}

http://codereview.chromium.org/6833031/diff/15001/jingle/notifier/base/proxy_...
jingle/notifier/base/proxy_resolving_client_socket.cc:182: if (status !=
net::OK)
combine with previous clause

http://codereview.chromium.org/6833031/diff/15001/jingle/notifier/base/proxy_...
jingle/notifier/base/proxy_resolving_client_socket.cc:187: // TODO(sanjeevr):
This has largely been copied from
Can you document the expected behavior of this fn?  It looks like error is
expected to be != OK and != PENDING, and this function either returns the error
directly if there's nothing to be done, or kicks off another proxy request and
returns IO_PENDING.

http://codereview.chromium.org/6833031/diff/15001/jingle/notifier/base/proxy_...
jingle/notifier/base/proxy_resolving_client_socket.cc:191:
DCHECK(!pac_request_);
add DCHECK error != OK, DCHECK error != PENDING

http://codereview.chromium.org/6833031/diff/15001/jingle/notifier/base/proxy_...
File jingle/notifier/base/proxy_resolving_client_socket.h (right):

http://codereview.chromium.org/6833031/diff/15001/jingle/notifier/base/proxy_...
jingle/notifier/base/proxy_resolving_client_socket.h:76: // The transport
socket.
Move transport_ to be below this comment and separate from the other vars

http://codereview.chromium.org/6833031/diff/15001/jingle/notifier/base/xmpp_c...
File jingle/notifier/base/xmpp_client_socket_factory.h (right):

http://codereview.chromium.org/6833031/diff/15001/jingle/notifier/base/xmpp_c...
jingle/notifier/base/xmpp_client_socket_factory.h:27: // resolve host names and
tunnel through proxies.
can you put this into its own file, and add a TODO to move it to net/?  (We
don't want to move XmppClientSocketFactory)

http://codereview.chromium.org/6833031/diff/15001/jingle/notifier/base/xmpp_c...
File jingle/notifier/base/xmpp_connection.cc (left):

http://codereview.chromium.org/6833031/diff/15001/jingle/notifier/base/xmpp_c...
jingle/notifier/base/xmpp_connection.cc:38: net::NetLog* const net_log = NULL;
remove TODO, use netlog from URLRequestContext

http://codereview.chromium.org/6833031/diff/15001/jingle/notifier/base/xmpp_c...
File jingle/notifier/base/xmpp_connection_unittest.cc (right):

http://codereview.chromium.org/6833031/diff/15001/jingle/notifier/base/xmpp_c...
jingle/notifier/base/xmpp_connection_unittest.cc:56: }
}  // namespace

[sanjeevr, 2011-04-15 02:01:40]

Added the unit-tests for ProxyResolvingClientSocket as well. PTAL.

[sanjeevr, 2011-04-15 04:23:34]

Addressed all your comments (no more please, I beg of you! ;) )

http://codereview.chromium.org/6833031/diff/15001/jingle/notifier/base/proxy_...
File jingle/notifier/base/proxy_resolving_client_socket.cc (right):

http://codereview.chromium.org/6833031/diff/15001/jingle/notifier/base/proxy_...
jingle/notifier/base/proxy_resolving_client_socket.cc:33: bound_net_log_(
On 2011/04/15 01:47:33, akalin wrote:
> need to init tried_direct_connect_

Done.

http://codereview.chromium.org/6833031/diff/15001/jingle/notifier/base/proxy_...
jingle/notifier/base/proxy_resolving_client_socket.cc:40:
request_context_getter->GetURLRequestContext()->host_resolver();
On 2011/04/15 01:47:33, akalin wrote:
> seems cleaner to have an intermediate var for the URLRequestContext

Done.

http://codereview.chromium.org/6833031/diff/15001/jingle/notifier/base/proxy_...
jingle/notifier/base/proxy_resolving_client_socket.cc:56: if (pac_request_)
On 2011/04/15 01:47:33, akalin wrote:
> I think this clause belongs to Disconnect(), since pac_request_ is generated
> there, and it's possible someone may call Disconnect() before it's finished. 
> Then you can replace this clause here with Disconnect().

Done.

http://codereview.chromium.org/6833031/diff/15001/jingle/notifier/base/proxy_...
jingle/notifier/base/proxy_resolving_client_socket.cc:177: if ((status ==
net::OK) || (status == net::ERR_IO_PENDING)) {
On 2011/04/15 01:47:33, akalin wrote:
> DCHECK that status is either the original status or PENDING, then change the
if
> to:
> 
> if (status == PENDING) {
>   return;
> }

Actually it does not always preserve the original error (see the SOCKS case). I
added a DCHECK(status != net::OK);

http://codereview.chromium.org/6833031/diff/15001/jingle/notifier/base/proxy_...
jingle/notifier/base/proxy_resolving_client_socket.cc:182: if (status !=
net::OK)
On 2011/04/15 01:47:33, akalin wrote:
> combine with previous clause

Done.

http://codereview.chromium.org/6833031/diff/15001/jingle/notifier/base/proxy_...
jingle/notifier/base/proxy_resolving_client_socket.cc:187: // TODO(sanjeevr):
This has largely been copied from
On 2011/04/15 01:47:33, akalin wrote:
> Can you document the expected behavior of this fn?  It looks like error is
> expected to be != OK and != PENDING, and this function either returns the
error
> directly if there's nothing to be done, or kicks off another proxy request and
> returns IO_PENDING.

Done.

http://codereview.chromium.org/6833031/diff/15001/jingle/notifier/base/proxy_...
jingle/notifier/base/proxy_resolving_client_socket.cc:191:
DCHECK(!pac_request_);
On 2011/04/15 01:47:33, akalin wrote:
> add DCHECK error != OK, DCHECK error != PENDING

Done.

http://codereview.chromium.org/6833031/diff/15001/jingle/notifier/base/proxy_...
File jingle/notifier/base/proxy_resolving_client_socket.h (right):

http://codereview.chromium.org/6833031/diff/15001/jingle/notifier/base/proxy_...
jingle/notifier/base/proxy_resolving_client_socket.h:76: // The transport
socket.
On 2011/04/15 01:47:33, akalin wrote:
> Move transport_ to be below this comment and separate from the other vars

Done.

http://codereview.chromium.org/6833031/diff/15001/jingle/notifier/base/xmpp_c...
File jingle/notifier/base/xmpp_client_socket_factory.h (right):

http://codereview.chromium.org/6833031/diff/15001/jingle/notifier/base/xmpp_c...
jingle/notifier/base/xmpp_client_socket_factory.h:27: // resolve host names and
tunnel through proxies.
On 2011/04/15 01:47:33, akalin wrote:
> can you put this into its own file, and add a TODO to move it to net/?  (We
> don't want to move XmppClientSocketFactory)

Done.

http://codereview.chromium.org/6833031/diff/15001/jingle/notifier/base/xmpp_c...
File jingle/notifier/base/xmpp_connection.cc (left):

http://codereview.chromium.org/6833031/diff/15001/jingle/notifier/base/xmpp_c...
jingle/notifier/base/xmpp_connection.cc:38: net::NetLog* const net_log = NULL;
On 2011/04/15 01:47:33, akalin wrote:
> remove TODO, use netlog from URLRequestContext

Done.

http://codereview.chromium.org/6833031/diff/15001/jingle/notifier/base/xmpp_c...
File jingle/notifier/base/xmpp_connection_unittest.cc (right):

http://codereview.chromium.org/6833031/diff/15001/jingle/notifier/base/xmpp_c...
jingle/notifier/base/xmpp_connection_unittest.cc:56: }
On 2011/04/15 01:47:33, akalin wrote:
> }  // namespace

Done.

[akalin, 2011-04-15 17:41:05]

LGTM, after taking care of the below minor comments

Make sure to hunt down willchan to give the OK on the net/ changes.

Don't forget to run through linux_clang and mac_clang, as they might reveal more
warnings.

http://codereview.chromium.org/6833031/diff/35006/jingle/notifier/base/chrome...
File jingle/notifier/base/chrome_async_socket.cc (right):

http://codereview.chromium.org/6833031/diff/35006/jingle/notifier/base/chrome...
jingle/notifier/base/chrome_async_socket.cc:7: #if defined(OS_WIN)
remove this block since you moved it to the unittest file

http://codereview.chromium.org/6833031/diff/35006/jingle/notifier/base/proxy_...
File jingle/notifier/base/proxy_resolving_client_socket.cc (right):

http://codereview.chromium.org/6833031/diff/35006/jingle/notifier/base/proxy_...
jingle/notifier/base/proxy_resolving_client_socket.cc:67:
net::CompletionCallback* callback) {
indentation

http://codereview.chromium.org/6833031/diff/35006/jingle/notifier/base/proxy_...
jingle/notifier/base/proxy_resolving_client_socket.cc:94: GURL url =
GURL("http://" + dest_host_port_pair_.ToString());
GURL url("http://" + ...);

http://codereview.chromium.org/6833031/diff/35006/jingle/notifier/base/proxy_...
jingle/notifier/base/proxy_resolving_client_socket.cc:180: DCHECK(status !=
net::OK);
DCHECK_NE

http://codereview.chromium.org/6833031/diff/35006/jingle/notifier/base/proxy_...
jingle/notifier/base/proxy_resolving_client_socket.cc:184: } else {
no need for 'else' since you're returning

http://codereview.chromium.org/6833031/diff/35006/jingle/notifier/base/proxy_...
jingle/notifier/base/proxy_resolving_client_socket.cc:199: DCHECK((error !=
net::OK) && (error != net::ERR_IO_PENDING));
split into two DCHECK_NEs

http://codereview.chromium.org/6833031/diff/35006/jingle/notifier/base/proxy_...
jingle/notifier/base/proxy_resolving_client_socket.cc:240: GURL url =
GURL("http://" + dest_host_port_pair_.ToString());
GURL url("http://" + ...

http://codereview.chromium.org/6833031/diff/35006/jingle/notifier/base/proxy_...
File jingle/notifier/base/proxy_resolving_client_socket_unittest.cc (right):

http://codereview.chromium.org/6833031/diff/35006/jingle/notifier/base/proxy_...
jingle/notifier/base/proxy_resolving_client_socket_unittest.cc:15: namespace {
Add a TODO to move chrome/test/test_url_request_context_getter.h to net test
support and use that.

http://codereview.chromium.org/6833031/diff/35006/jingle/notifier/base/xmpp_c...
File jingle/notifier/base/xmpp_connection_unittest.cc (right):

http://codereview.chromium.org/6833031/diff/35006/jingle/notifier/base/xmpp_c...
jingle/notifier/base/xmpp_connection_unittest.cc:33: namespace {
Add a TODO to move chrome/test/test_url_request_context_getter.h to net test
support and use that.

[sanjeevr, 2011-04-15 18:04:33]

Done

http://codereview.chromium.org/6833031/diff/35006/jingle/notifier/base/chrome...
File jingle/notifier/base/chrome_async_socket.cc (right):

http://codereview.chromium.org/6833031/diff/35006/jingle/notifier/base/chrome...
jingle/notifier/base/chrome_async_socket.cc:7: #if defined(OS_WIN)
On 2011/04/15 17:41:05, akalin wrote:
> remove this block since you moved it to the unittest file

Done.

http://codereview.chromium.org/6833031/diff/35006/jingle/notifier/base/proxy_...
File jingle/notifier/base/proxy_resolving_client_socket.cc (right):

http://codereview.chromium.org/6833031/diff/35006/jingle/notifier/base/proxy_...
jingle/notifier/base/proxy_resolving_client_socket.cc:67:
net::CompletionCallback* callback) {
On 2011/04/15 17:41:05, akalin wrote:
> indentation

Done.

http://codereview.chromium.org/6833031/diff/35006/jingle/notifier/base/proxy_...
jingle/notifier/base/proxy_resolving_client_socket.cc:94: GURL url =
GURL("http://" + dest_host_port_pair_.ToString());
On 2011/04/15 17:41:05, akalin wrote:
> GURL url("http://" + ...);

Oops, Done.

http://codereview.chromium.org/6833031/diff/35006/jingle/notifier/base/proxy_...
jingle/notifier/base/proxy_resolving_client_socket.cc:180: DCHECK(status !=
net::OK);
On 2011/04/15 17:41:05, akalin wrote:
> DCHECK_NE

Done.

http://codereview.chromium.org/6833031/diff/35006/jingle/notifier/base/proxy_...
jingle/notifier/base/proxy_resolving_client_socket.cc:184: } else {
On 2011/04/15 17:41:05, akalin wrote:
> no need for 'else' since you're returning

Done.

http://codereview.chromium.org/6833031/diff/35006/jingle/notifier/base/proxy_...
jingle/notifier/base/proxy_resolving_client_socket.cc:199: DCHECK((error !=
net::OK) && (error != net::ERR_IO_PENDING));
On 2011/04/15 17:41:05, akalin wrote:
> split into two DCHECK_NEs

Done.

http://codereview.chromium.org/6833031/diff/35006/jingle/notifier/base/proxy_...
jingle/notifier/base/proxy_resolving_client_socket.cc:240: GURL url =
GURL("http://" + dest_host_port_pair_.ToString());
On 2011/04/15 17:41:05, akalin wrote:
> GURL url("http://" + ...

Done.

http://codereview.chromium.org/6833031/diff/35006/jingle/notifier/base/proxy_...
File jingle/notifier/base/proxy_resolving_client_socket_unittest.cc (right):

http://codereview.chromium.org/6833031/diff/35006/jingle/notifier/base/proxy_...
jingle/notifier/base/proxy_resolving_client_socket_unittest.cc:15: namespace {
On 2011/04/15 17:41:05, akalin wrote:
> Add a TODO to move chrome/test/test_url_request_context_getter.h to net test
> support and use that.

We may not be able to do that since
chrome/test/test_url_request_context_getter.h relies on BrowserThread. But we
can move this version to net test support

http://codereview.chromium.org/6833031/diff/35006/jingle/notifier/base/xmpp_c...
File jingle/notifier/base/xmpp_connection_unittest.cc (right):

http://codereview.chromium.org/6833031/diff/35006/jingle/notifier/base/xmpp_c...
jingle/notifier/base/xmpp_connection_unittest.cc:33: namespace {
On 2011/04/15 17:41:05, akalin wrote:
> Add a TODO to move chrome/test/test_url_request_context_getter.h to net test
> support and use that.

Same.

[willchan no longer on Chromium, 2011-04-15 18:07:08]

Sorry I didn't get around to this earlier. The net/ change LGTM. I wish I had
time to read the jingle/ change to see the usage, but I don't right now.

[Timur Iskhodzhanov, 2011-04-16 11:49:53]

Memory leak introduced by this CL : crbug.com/79651
Please consider running your tests under Valgrind before commiting