net: clean up SSLHostInfo construction.

net: clean up SSLHostInfo construction.

Create an SSLHostInfoFactory interface and plumb it from the HttpCache
to the SSLConnectJob. Also, move the SSLHostInfo reference from the
ssl_config to being passed to the SSLClientSocket.

BUG=none
TEST=net_unittests

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

[agl, 2010-10-13 23:17:06]

I'm sure the try bot will tell me that I've missed something in Chrome Frame. So
just imagine that I've already fixed that.

[willchan no longer on Chromium, 2010-10-13 23:50:53]

http://codereview.chromium.org/3747003/diff/1/5
File net/base/ssl_host_info.h (right):

http://codereview.chromium.org/3747003/diff/1/5#newcode59
net/base/ssl_host_info.h:59: virtual void GetForHost(scoped_refptr<SSLHostInfo>*
out,
Style guide says output params come after input params.  But why not just return
a SSLHostInfo?

http://codereview.chromium.org/3747003/diff/1/8
File net/http/http_cache.h (right):

http://codereview.chromium.org/3747003/diff/1/8#newcode188
net/http/http_cache.h:188: // SSLHostInfoFactory implementation
I think it's unfortunate to contaminate the HttpCache interface like this.  No
consumer of the HttpCache really needs to know about it.  I think you should
compose a private factory implementation class that keeps a pointer to |this|. 
That class that inherit from SSLHostInfoFactory.  I know, a bit more work and
all, but I'm somewhat anal about keeping APIs clean.

http://codereview.chromium.org/3747003/diff/1/28
File net/socket/ssl_client_socket_pool.cc (right):

http://codereview.chromium.org/3747003/diff/1/28#newcode286
net/socket/ssl_client_socket_pool.cc:286:
ssl_socket_->set_ssl_host_info(ssl_host_info_.get());
Correct me if my understanding of SSLHostInfo ownership semantics is incorrect. 
SSLConnectJob uses the factory to get a SSLHostInfo object.  During this time
period, it owns the SSLHostInfo, but only for the purpose of transferring the
ownership to the SSLClientSocket if it gets created.  Once it gets created here,
we call set_ssl_host_info().

If my understanding is correct, then I think we should stop refcounting
SSLHostInfo.  We should use a scoped_ptr<SSLHostInfo> here to establish
ownership of it, and then call .release() to release it to the ssl_socket_, in
the same way we release the transport socket to ssl_socket_.

Also, are you using set_ssl_host_info() rather than the constructor in order to
avoid updating all the CreateSSLClientSocket callsites?  If so, I'd prefer just
to use the constructor.

[agl, 2010-10-14 18:06:08]

This CL is getting huge. I haven't made any changes other than the ones
requested in the latest update.

http://codereview.chromium.org/3747003/diff/1/5
File net/base/ssl_host_info.h (right):

http://codereview.chromium.org/3747003/diff/1/5#newcode59
net/base/ssl_host_info.h:59: virtual void GetForHost(scoped_refptr<SSLHostInfo>*
out,
On 2010/10/13 23:50:53, willchan wrote:
> Style guide says output params come after input params.  But why not just
return
> a SSLHostInfo?

I don't like returning pointers to things which are reference counted: the
caller might miss the reference counting. But since I'm going to kill the
reference counting here, I can change this.

http://codereview.chromium.org/3747003/diff/1/8
File net/http/http_cache.h (right):

http://codereview.chromium.org/3747003/diff/1/8#newcode188
net/http/http_cache.h:188: // SSLHostInfoFactory implementation
On 2010/10/13 23:50:53, willchan wrote:
> I think it's unfortunate to contaminate the HttpCache interface like this.  No
> consumer of the HttpCache really needs to know about it.  I think you should
> compose a private factory implementation class that keeps a pointer to |this|.

> That class that inherit from SSLHostInfoFactory.  I know, a bit more work and
> all, but I'm somewhat anal about keeping APIs clean.

Done.

http://codereview.chromium.org/3747003/diff/1/28
File net/socket/ssl_client_socket_pool.cc (right):

http://codereview.chromium.org/3747003/diff/1/28#newcode286
net/socket/ssl_client_socket_pool.cc:286:
ssl_socket_->set_ssl_host_info(ssl_host_info_.get());
On 2010/10/13 23:50:53, willchan wrote:
> If my understanding is correct, then I think we should stop refcounting
> SSLHostInfo.  We should use a scoped_ptr<SSLHostInfo> here to establish
> ownership of it, and then call .release() to release it to the ssl_socket_, in
> the same way we release the transport socket to ssl_socket_.

Yea, you're right. SSLHostInfo has been changed around so much now it no longer
even needs to be refcounted. Fixed.

> Also, are you using set_ssl_host_info() rather than the constructor in order
to
> avoid updating all the CreateSSLClientSocket callsites?  If so, I'd prefer
just
> to use the constructor.

I wasn't doing it for that reason, although had I know how much updating would
be needed then that wound have been the reason! Done, none the less. See
net/socket/client_socket_factory.cc:32 where it gets deleted in the case that
the socket isn't NSS.

[willchan no longer on Chromium, 2010-10-14 18:18:16]

Great, LGTM.

http://codereview.chromium.org/3747003/diff/1/5
File net/base/ssl_host_info.h (right):

http://codereview.chromium.org/3747003/diff/1/5#newcode59
net/base/ssl_host_info.h:59: virtual void GetForHost(scoped_refptr<SSLHostInfo>*
out,
On 2010/10/14 18:06:08, agl wrote:
> On 2010/10/13 23:50:53, willchan wrote:
> > Style guide says output params come after input params.  But why not just
> return
> > a SSLHostInfo?
> 
> I don't like returning pointers to things which are reference counted: the

That makes sense.  An alternative would be to return scoped_refptr<Foo> to force
a ref.  NRVO should hopefully prevent the unnecessary AddRef()/Release() pair.

> caller might miss the reference counting. But since I'm going to kill the
> reference counting here, I can change this.
> 
>