diff options
author | willchan@chromium.org <willchan@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2010-10-14 18:03:09 +0000 |
---|---|---|
committer | willchan@chromium.org <willchan@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2010-10-14 18:03:09 +0000 |
commit | 2681688fc8449fe1cc6b2f66267fcf04f0327ea1 (patch) | |
tree | 424611ca258e9699de6b63349e1d0fba118144a3 /net/http | |
parent | 0f337f026ad2bca4d3eda9bb1cb8e5c9dc2dd7f3 (diff) | |
download | chromium_src-2681688fc8449fe1cc6b2f66267fcf04f0327ea1.zip chromium_src-2681688fc8449fe1cc6b2f66267fcf04f0327ea1.tar.gz chromium_src-2681688fc8449fe1cc6b2f66267fcf04f0327ea1.tar.bz2 |
Cleanup StreamFactory && StreamRequest APIs.
Stop refcounting StreamRequest. Establish a clear ownership of the StreamRequest. Deletion implies cancellation. Use ScopedRunnableMethodFactory::NewRunnableMethod() instead of NewRunnableMethod().
Remove Start() from StreamRequest. This is an implementation detail of HttpStreamRequest, so it only exists there now.
BUG=59103
TEST=existing, this is a pure refactor.
Review URL: http://codereview.chromium.org/3746002
git-svn-id: svn://svn.chromium.org/chrome/trunk/src@62581 0039d316-1c4b-4281-b951-d872f2087c98
Diffstat (limited to 'net/http')
-rw-r--r-- | net/http/http_network_transaction.cc | 32 | ||||
-rw-r--r-- | net/http/http_network_transaction.h | 6 | ||||
-rw-r--r-- | net/http/http_stream_factory.cc | 15 | ||||
-rw-r--r-- | net/http/http_stream_factory.h | 13 | ||||
-rw-r--r-- | net/http/http_stream_request.cc | 86 | ||||
-rw-r--r-- | net/http/http_stream_request.h | 32 | ||||
-rw-r--r-- | net/http/stream_factory.h | 116 |
7 files changed, 122 insertions, 178 deletions
diff --git a/net/http/http_network_transaction.cc b/net/http/http_network_transaction.cc index b5a5707..f4c12d0 100644 --- a/net/http/http_network_transaction.cc +++ b/net/http/http_network_transaction.cc @@ -198,11 +198,6 @@ HttpNetworkTransaction::~HttpNetworkTransaction() { } } } - - if (stream_request_.get()) { - stream_request_->Cancel(); - stream_request_ = NULL; - } } int HttpNetworkTransaction::Start(const HttpRequestInfo* request_info, @@ -598,13 +593,15 @@ int HttpNetworkTransaction::DoLoop(int result) { int HttpNetworkTransaction::DoCreateStream() { next_state_ = STATE_CREATE_STREAM_COMPLETE; - session_->http_stream_factory()->RequestStream(request_, - &ssl_config_, - &proxy_info_, - this, - net_log_, - session_, - &stream_request_); + stream_request_.reset( + session_->http_stream_factory()->RequestStream( + request_, + &ssl_config_, + &proxy_info_, + session_, + this, + net_log_)); + DCHECK(stream_request_.get()); return ERR_IO_PENDING; } @@ -615,7 +612,7 @@ int HttpNetworkTransaction::DoCreateStreamComplete(int result) { } // At this point we are done with the stream_request_. - stream_request_ = NULL; + stream_request_.reset(); return result; } @@ -1073,12 +1070,9 @@ int HttpNetworkTransaction::HandleCertificateRequest(int error) { stream_.reset(); } - if (stream_request_.get()) { - // The server is asking for a client certificate during the initial - // handshake. - stream_request_->Cancel(); - stream_request_ = NULL; - } + // The server is asking for a client certificate during the initial + // handshake. + stream_request_.reset(); // If the user selected one of the certificate in client_certs for this // server before, use it automatically. diff --git a/net/http/http_network_transaction.h b/net/http/http_network_transaction.h index 7ddee29..a8d371d 100644 --- a/net/http/http_network_transaction.h +++ b/net/http/http_network_transaction.h @@ -33,7 +33,7 @@ class SSLHostInfo; struct HttpRequestInfo; class HttpNetworkTransaction : public HttpTransaction, - public StreamFactory::StreamRequestDelegate { + public StreamRequest::Delegate { public: explicit HttpNetworkTransaction(HttpNetworkSession* session); @@ -58,7 +58,7 @@ class HttpNetworkTransaction : public HttpTransaction, virtual uint64 GetUploadProgress() const; virtual void SetSSLHostInfo(SSLHostInfo* host_info); - // StreamRequestDelegate methods: + // StreamRequest::Delegate methods: virtual void OnStreamReady(HttpStream* stream); virtual void OnStreamFailed(int status); virtual void OnCertificateError(int status, const SSLInfo& ssl_info); @@ -211,7 +211,7 @@ class HttpNetworkTransaction : public HttpTransaction, ProxyInfo proxy_info_; - scoped_refptr<StreamFactory::StreamRequestJob> stream_request_; + scoped_ptr<StreamRequest> stream_request_; scoped_ptr<HttpStream> stream_; // True if we've validated the headers that the stream parser has returned. diff --git a/net/http/http_stream_factory.cc b/net/http/http_stream_factory.cc index 7a26282..9342564 100644 --- a/net/http/http_stream_factory.cc +++ b/net/http/http_stream_factory.cc @@ -44,17 +44,16 @@ HttpStreamFactory::HttpStreamFactory() { HttpStreamFactory::~HttpStreamFactory() { } -void HttpStreamFactory::RequestStream( +StreamRequest* HttpStreamFactory::RequestStream( const HttpRequestInfo* request_info, SSLConfig* ssl_config, ProxyInfo* proxy_info, - StreamFactory::StreamRequestDelegate* delegate, - const BoundNetLog& net_log, - const scoped_refptr<HttpNetworkSession>& session, - scoped_refptr<StreamRequestJob>* stream) { - DCHECK(stream != NULL); - *stream = new HttpStreamRequest(this, session); - (*stream)->Start(request_info, ssl_config, proxy_info, delegate, net_log); + HttpNetworkSession* session, + StreamRequest::Delegate* delegate, + const BoundNetLog& net_log) { + HttpStreamRequest* stream = new HttpStreamRequest(this, session); + stream->Start(request_info, ssl_config, proxy_info, delegate, net_log); + return stream; } void HttpStreamFactory::AddTLSIntolerantServer(const GURL& url) { diff --git a/net/http/http_stream_factory.h b/net/http/http_stream_factory.h index 08da415..b9f5586 100644 --- a/net/http/http_stream_factory.h +++ b/net/http/http_stream_factory.h @@ -30,13 +30,12 @@ class HttpStreamFactory : public StreamFactory, virtual ~HttpStreamFactory(); // StreamFactory Interface - virtual void RequestStream(const HttpRequestInfo* info, - SSLConfig* ssl_config, - ProxyInfo* proxy_info, - StreamRequestDelegate* delegate, - const BoundNetLog& net_log, - const scoped_refptr<HttpNetworkSession>& session, - scoped_refptr<StreamRequestJob>* stream); + virtual StreamRequest* RequestStream(const HttpRequestInfo* info, + SSLConfig* ssl_config, + ProxyInfo* proxy_info, + HttpNetworkSession* session, + StreamRequest::Delegate* delegate, + const BoundNetLog& net_log); // TLS Intolerant Server API void AddTLSIntolerantServer(const GURL& url); diff --git a/net/http/http_stream_request.cc b/net/http/http_stream_request.cc index 13c30e6..f43037e 100644 --- a/net/http/http_stream_request.cc +++ b/net/http/http_stream_request.cc @@ -47,7 +47,7 @@ GURL UpgradeUrlToHttps(const GURL& original_url) { HttpStreamRequest::HttpStreamRequest( HttpStreamFactory* factory, - const scoped_refptr<HttpNetworkSession>& session) + HttpNetworkSession* session) : request_info_(NULL), proxy_info_(NULL), ssl_config_(NULL), @@ -67,7 +67,7 @@ HttpStreamRequest::HttpStreamRequest( establishing_tunnel_(false), was_alternate_protocol_available_(false), was_npn_negotiated_(false), - cancelled_(false) { + ALLOW_THIS_IN_INITIALIZER_LIST(method_factory_(this)) { if (factory->use_alternate_protocols()) alternate_protocol_mode_ = kUnspecified; else @@ -80,21 +80,24 @@ HttpStreamRequest::~HttpStreamRequest() { // this stream at all. if (next_state_ == STATE_WAITING_USER_ACTION) { connection_->socket()->Disconnect(); - connection_->Reset(); connection_.reset(); } if (pac_request_) session_->proxy_service()->CancelPacRequest(pac_request_); + + // The stream could be in a partial state. It is not reusable. + if (stream_.get() && next_state_ != STATE_DONE) { + stream_->Close(true /* not reusable */); + } } void HttpStreamRequest::Start(const HttpRequestInfo* request_info, SSLConfig* ssl_config, ProxyInfo* proxy_info, - StreamFactory::StreamRequestDelegate* delegate, + Delegate* delegate, const BoundNetLog& net_log) { CHECK_EQ(STATE_NONE, next_state_); - CHECK(!cancelled_); request_info_ = request_info; ssl_config_ = ssl_config; @@ -106,29 +109,6 @@ void HttpStreamRequest::Start(const HttpRequestInfo* request_info, DCHECK_EQ(ERR_IO_PENDING, rv); } -void HttpStreamRequest::Cancel() { - cancelled_ = true; - - // If we were waiting for the user to take action, then the connection - // is in a partially connected state. All we can do is close it at this - // point. - if (next_state_ == STATE_WAITING_USER_ACTION) { - connection_->socket()->Disconnect(); - connection_->Reset(); - connection_.reset(); - } - - // The stream could be in a partial state. It is not reusable. - if (stream_.get()) { - stream_->Close(true); - stream_.reset(); - } - - delegate_ = NULL; - - next_state_ = STATE_NONE; -} - int HttpStreamRequest::RestartWithCertificate(X509Certificate* client_cert) { ssl_config()->client_cert = client_cert; ssl_config()->send_client_cert = true; @@ -170,54 +150,39 @@ void HttpStreamRequest::GetSSLInfo() { } const HttpRequestInfo& HttpStreamRequest::request_info() const { - DCHECK(!cancelled_); // Can't access this after cancellation. return *request_info_; } ProxyInfo* HttpStreamRequest::proxy_info() const { - DCHECK(!cancelled_); // Can't access this after cancellation. return proxy_info_; } SSLConfig* HttpStreamRequest::ssl_config() const { - DCHECK(!cancelled_); // Can't access this after cancellation. return ssl_config_; } -void HttpStreamRequest::OnStreamReadyCallback(HttpStream* stream) { - if (cancelled_) { - // The delegate is gone. We need to cleanup the stream. - delete stream; - return; - } - delegate_->OnStreamReady(stream); +void HttpStreamRequest::OnStreamReadyCallback() { + DCHECK(stream_.get()); + delegate_->OnStreamReady(stream_.release()); } void HttpStreamRequest::OnStreamFailedCallback(int result) { - if (cancelled_) - return; delegate_->OnStreamFailed(result); } void HttpStreamRequest::OnCertificateErrorCallback(int result, const SSLInfo& ssl_info) { - if (cancelled_) - return; delegate_->OnCertificateError(result, ssl_info); } void HttpStreamRequest::OnNeedsProxyAuthCallback( const HttpResponseInfo& response, HttpAuthController* auth_controller) { - if (cancelled_) - return; delegate_->OnNeedsProxyAuth(response, auth_controller); } void HttpStreamRequest::OnNeedsClientAuthCallback( SSLCertRequestInfo* cert_info) { - if (cancelled_) - return; delegate_->OnNeedsClientAuth(cert_info); } @@ -226,9 +191,6 @@ void HttpStreamRequest::OnIOComplete(int result) { } int HttpStreamRequest::RunLoop(int result) { - if (cancelled_) - return ERR_ABORTED; - result = DoLoop(result); DCHECK(delegate_); @@ -240,9 +202,9 @@ int HttpStreamRequest::RunLoop(int result) { next_state_ = STATE_WAITING_USER_ACTION; MessageLoop::current()->PostTask( FROM_HERE, - NewRunnableMethod(this, - &HttpStreamRequest::OnCertificateErrorCallback, - result, ssl_info_)); + method_factory_.NewRunnableMethod( + &HttpStreamRequest::OnCertificateErrorCallback, + result, ssl_info_)); return ERR_IO_PENDING; } @@ -261,7 +223,7 @@ int HttpStreamRequest::RunLoop(int result) { next_state_ = STATE_WAITING_USER_ACTION; MessageLoop::current()->PostTask( FROM_HERE, - NewRunnableMethod(this, + method_factory_.NewRunnableMethod( &HttpStreamRequest::OnNeedsProxyAuthCallback, *tunnel_auth_response, http_proxy_socket->auth_controller())); @@ -271,28 +233,28 @@ int HttpStreamRequest::RunLoop(int result) { case ERR_SSL_CLIENT_AUTH_CERT_NEEDED: MessageLoop::current()->PostTask( FROM_HERE, - NewRunnableMethod(this, - &HttpStreamRequest::OnNeedsClientAuthCallback, - connection_->ssl_error_response_info().cert_request_info)); + method_factory_.NewRunnableMethod( + &HttpStreamRequest::OnNeedsClientAuthCallback, + connection_->ssl_error_response_info().cert_request_info)); return ERR_IO_PENDING; case ERR_IO_PENDING: break; case OK: + next_state_ = STATE_DONE; MessageLoop::current()->PostTask( FROM_HERE, - NewRunnableMethod(this, - &HttpStreamRequest::OnStreamReadyCallback, - stream_.release())); + method_factory_.NewRunnableMethod( + &HttpStreamRequest::OnStreamReadyCallback)); return ERR_IO_PENDING; default: MessageLoop::current()->PostTask( FROM_HERE, - NewRunnableMethod(this, - &HttpStreamRequest::OnStreamFailedCallback, - result)); + method_factory_.NewRunnableMethod( + &HttpStreamRequest::OnStreamFailedCallback, + result)); return ERR_IO_PENDING; } return result; diff --git a/net/http/http_stream_request.h b/net/http/http_stream_request.h index dcec385..0933272 100644 --- a/net/http/http_stream_request.h +++ b/net/http/http_stream_request.h @@ -6,6 +6,7 @@ #define NET_HTTP_HTTP_STREAM_REQUEST_H_ #include "base/scoped_ptr.h" +#include "base/task.h" #include "net/base/completion_callback.h" #include "net/base/host_mapping_rules.h" #include "net/base/ssl_config_service.h" @@ -32,19 +33,24 @@ struct HttpRequestInfo; // An HttpStreamRequest exists for each stream which is in progress of being // created for the StreamFactory. -class HttpStreamRequest : public StreamFactory::StreamRequestJob { +class HttpStreamRequest : public StreamRequest { public: HttpStreamRequest(HttpStreamFactory* factory, - const scoped_refptr<HttpNetworkSession>& session); + HttpNetworkSession* session); virtual ~HttpStreamRequest(); + // Start initiates the process of creating a new HttpStream. + // 3 parameters are passed in by reference. The caller asserts that the + // lifecycle of these parameters will remain valid until the stream is + // created, failed, or destroyed. In the first two cases, the delegate will + // be called to notify completion of the request. + void Start(const HttpRequestInfo* request_info, + SSLConfig* ssl_config, + ProxyInfo* proxy_info, + Delegate* delegate, + const BoundNetLog& net_log); + // StreamRequest interface - virtual void Start(const HttpRequestInfo* request_info, - SSLConfig* ssl_config, - ProxyInfo* proxy_info, - StreamFactory::StreamRequestDelegate* delegate, - const BoundNetLog& net_log); - virtual void Cancel(); virtual int RestartWithCertificate(X509Certificate* client_cert); virtual int RestartTunnelWithProxyAuth(const string16& username, const string16& password); @@ -75,6 +81,7 @@ class HttpStreamRequest : public StreamFactory::StreamRequestJob { STATE_CREATE_STREAM_COMPLETE, STATE_DRAIN_BODY_FOR_AUTH_RESTART, STATE_DRAIN_BODY_FOR_AUTH_RESTART_COMPLETE, + STATE_DONE, STATE_NONE }; @@ -83,7 +90,7 @@ class HttpStreamRequest : public StreamFactory::StreamRequestJob { SSLConfig* ssl_config() const; // Callbacks to the delegate. - void OnStreamReadyCallback(HttpStream* stream); + void OnStreamReadyCallback(); void OnStreamFailedCallback(int result); void OnCertificateErrorCallback(int result, const SSLInfo& ssl_info); void OnNeedsProxyAuthCallback(const HttpResponseInfo& response_info, @@ -163,7 +170,7 @@ class HttpStreamRequest : public StreamFactory::StreamRequestJob { CompletionCallbackImpl<HttpStreamRequest> io_callback_; scoped_ptr<ClientSocketHandle> connection_; scoped_refptr<HttpStreamFactory> factory_; - StreamFactory::StreamRequestDelegate* delegate_; + Delegate* delegate_; BoundNetLog net_log_; State next_state_; ProxyService::PacRequest* pac_request_; @@ -208,10 +215,7 @@ class HttpStreamRequest : public StreamFactory::StreamRequestJob { // True if we negotiated NPN. bool was_npn_negotiated_; - // Indicates that this StreamRequest has been cancelled. Note that once - // this has been cancelled, input parameters passed into the StreamRequest - // can no longer be touched (as they belong to the requestor). - bool cancelled_; + ScopedRunnableMethodFactory<HttpStreamRequest> method_factory_; DISALLOW_COPY_AND_ASSIGN(HttpStreamRequest); }; diff --git a/net/http/stream_factory.h b/net/http/stream_factory.h index 9ec85f1..592ce5a 100644 --- a/net/http/stream_factory.h +++ b/net/http/stream_factory.h @@ -26,15 +26,19 @@ class X509Certificate; struct HttpRequestInfo; struct SSLConfig; -// The StreamFactory defines an interface for creating usable HttpStreams. -class StreamFactory { +// The StreamRequest is the client's handle to the worker object which handles +// the creation of an HttpStream. While the HttpStream is being created, this +// object is the creator's handle for interacting with the HttpStream creation +// process. The request is cancelled by deleting it, after which no callbacks +// will be invoked. +class StreamRequest { public: // The StreamRequestDelegate is a set of callback methods for a // StreamRequestJob. Generally, only one of these methods will be // called as a result of a stream request. - class StreamRequestDelegate { + class Delegate { public: - virtual ~StreamRequestDelegate() {} + virtual ~Delegate() {} // This is the success case. // |stream| is now owned by the delegate. @@ -66,72 +70,54 @@ class StreamFactory { virtual void OnNeedsClientAuth(SSLCertRequestInfo* cert_info) = 0; }; - // The StreamRequestJob is the worker object which handles the creation - // of an HttpStream. While the HttpStream is being created, this job - // is the creator's handle for interacting with the HttpStream creation - // process. - class StreamRequestJob : public base::RefCounted<StreamRequestJob> { - public: - virtual ~StreamRequestJob() {} - - // Start initiates the process of creating a new HttpStream. - // 3 parameters are passed in by reference. The caller asserts that the - // lifecycle of these parameters will remain valid until the stream is - // created, failed, or until the caller calls Cancel() on the stream - // request. In all cases, the delegate will be called to notify - // completion of the request. - virtual void Start(const HttpRequestInfo* request_info, - SSLConfig* ssl_config, - ProxyInfo* proxy_info, - StreamRequestDelegate* delegate, - const BoundNetLog& net_log) = 0; - - // Cancel can be used to abort the HttpStream creation. Once cancelled, - // the delegates associated with the request will not be invoked. - virtual void Cancel() = 0; - - // When a HttpStream creation process requires a SSL Certificate, - // the delegate OnNeedsClientAuth handler will have been called. - // It now becomes the delegate's responsibility to collect the certificate - // (probably from the user), and then call this method to resume - // the HttpStream creation process. - // Ownership of |client_cert| remains with the StreamRequest. The - // delegate can take a reference if needed beyond the lifetime of this - // call. - virtual int RestartWithCertificate(X509Certificate* client_cert) = 0; - - // When a HttpStream creation process is stalled due to necessity - // of Proxy authentication credentials, the delegate OnNeedsProxyAuth - // will have been called. It now becomes the delegate's responsibility - // to collect the necessary credentials, and then call this method to - // resume the HttpStream creation process. - virtual int RestartTunnelWithProxyAuth(const string16& username, - const string16& password) = 0; - - // Returns the LoadState for the request. - virtual LoadState GetLoadState() const = 0; - - // Returns true if an AlternateProtocol for this request was available. - virtual bool was_alternate_protocol_available() const = 0; - - // Returns true if TLS/NPN was negotiated for this stream. - virtual bool was_npn_negotiated() const = 0; - - // Returns true if this stream is being fetched over SPDY. - virtual bool using_spdy() const = 0; - }; + virtual ~StreamRequest() {} + + // When a HttpStream creation process requires a SSL Certificate, + // the delegate OnNeedsClientAuth handler will have been called. + // It now becomes the delegate's responsibility to collect the certificate + // (probably from the user), and then call this method to resume + // the HttpStream creation process. + // Ownership of |client_cert| remains with the StreamRequest. The + // delegate can take a reference if needed beyond the lifetime of this + // call. + virtual int RestartWithCertificate(X509Certificate* client_cert) = 0; + + // When a HttpStream creation process is stalled due to necessity + // of Proxy authentication credentials, the delegate OnNeedsProxyAuth + // will have been called. It now becomes the delegate's responsibility + // to collect the necessary credentials, and then call this method to + // resume the HttpStream creation process. + virtual int RestartTunnelWithProxyAuth(const string16& username, + const string16& password) = 0; + + // Returns the LoadState for the request. + virtual LoadState GetLoadState() const = 0; + + // Returns true if an AlternateProtocol for this request was available. + virtual bool was_alternate_protocol_available() const = 0; + + // Returns true if TLS/NPN was negotiated for this stream. + virtual bool was_npn_negotiated() const = 0; + + // Returns true if this stream is being fetched over SPDY. + virtual bool using_spdy() const = 0; +}; +// The StreamFactory defines an interface for creating usable HttpStreams. +class StreamFactory { + public: virtual ~StreamFactory() {} // Request a stream. // Will callback to the StreamRequestDelegate upon completion. - virtual void RequestStream(const HttpRequestInfo* info, - SSLConfig* ssl_config, - ProxyInfo* proxy_info, - StreamRequestDelegate* delegate, - const BoundNetLog& net_log, - const scoped_refptr<HttpNetworkSession>& session, - scoped_refptr<StreamRequestJob>* stream) = 0; + // |info|, |ssl_config|, and |proxy_info| must been kept alive until + // |delegate| is called. + virtual StreamRequest* RequestStream(const HttpRequestInfo* info, + SSLConfig* ssl_config, + ProxyInfo* proxy_info, + HttpNetworkSession* session, + StreamRequest::Delegate* delegate, + const BoundNetLog& net_log) = 0; // TLS Intolerant Server API virtual void AddTLSIntolerantServer(const GURL& url) = 0; |