Replace onRequestSent with onSendHeaders in webrequest API

This CL replaces the onRequestSent signal of the webrequest API with a new onSendHeader signal. The sequence is now: onBeforeRequest -> onBeforeSendHeaders -> onSendHeaders.

This change allows us to completely remove the webrequest API from the network transaction layer and stay in the URL request layer. That solve the problem that the network layer may convert one URLRequest to multiple HTTP requests which should be transparent to the webrequest API. It also solves the problem that requests answered from the cache did not trigger an onRequestSent event.

Given the choice of removing onRequestSent completely and replacing it with onSendHeaders, I chose the latter so that extensions can see the headers sent to the network after other extensions had a chance to modify them.

BUG=no
TEST=no

Review URL: http://codereview.chromium.org/7353021

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@92584 0039d316-1c4b-4281-b951-d872f2087c98

11 files changed, 33 insertions, 64 deletions

diff --git a/net/base/network_delegate.cc b/net/base/network_delegate.cc
index ecb9a01..fc4ff57 100644
--- a/net/base/network_delegate.cc
+++ b/net/base/network_delegate.cc
@@ -26,12 +26,10 @@ int NetworkDelegate::NotifyBeforeSendHeaders(URLRequest* request,
   return OnBeforeSendHeaders(request, callback, headers);
 }
 
-void NetworkDelegate::NotifyRequestSent(
-    uint64 request_id,
-    const HostPortPair& socket_address,
-    const HttpRequestHeaders& headers) {
+void NetworkDelegate::NotifySendHeaders(URLRequest* request,
+                                        const HttpRequestHeaders& headers) {
   DCHECK(CalledOnValidThread());
-  OnRequestSent(request_id, socket_address, headers);
+  OnSendHeaders(request, headers);
 }
 
 void NetworkDelegate::NotifyResponseStarted(URLRequest* request) {
@@ -65,11 +63,6 @@ void NetworkDelegate::NotifyURLRequestDestroyed(URLRequest* request) {
   OnURLRequestDestroyed(request);
 }
 
-void NetworkDelegate::NotifyHttpTransactionDestroyed(uint64 request_id) {
-  DCHECK(CalledOnValidThread());
-  OnHttpTransactionDestroyed(request_id);
-}
-
 void NetworkDelegate::NotifyPACScriptError(int line_number,
                                            const string16& error) {
   DCHECK(CalledOnValidThread());
diff --git a/net/base/network_delegate.h b/net/base/network_delegate.h
index df9fef8..0c0c556 100644
--- a/net/base/network_delegate.h
+++ b/net/base/network_delegate.h
@@ -43,8 +43,7 @@ class NetworkDelegate : public base::NonThreadSafe {
   int NotifyBeforeSendHeaders(URLRequest* request,
                               CompletionCallback* callback,
                               HttpRequestHeaders* headers);
-  void NotifyRequestSent(uint64 request_id,
-                         const HostPortPair& socket_address,
+  void NotifySendHeaders(URLRequest* request,
                          const HttpRequestHeaders& headers);
   void NotifyBeforeRedirect(URLRequest* request,
                             const GURL& new_location);
@@ -52,7 +51,6 @@ class NetworkDelegate : public base::NonThreadSafe {
   void NotifyRawBytesRead(const URLRequest& request, int bytes_read);
   void NotifyCompleted(URLRequest* request);
   void NotifyURLRequestDestroyed(URLRequest* request);
-  void NotifyHttpTransactionDestroyed(uint64 request_id);
   void NotifyPACScriptError(int line_number, const string16& error);
 
  private:
@@ -77,10 +75,8 @@ class NetworkDelegate : public base::NonThreadSafe {
                                   CompletionCallback* callback,
                                   HttpRequestHeaders* headers) = 0;
 
-  // Called right after the HTTP headers have been sent and notifies where
-  // the request has actually been sent to.
-  virtual void OnRequestSent(uint64 request_id,
-                             const HostPortPair& socket_address,
+  // Called right before the HTTP request(s) are being sent to the network.
+  virtual void OnSendHeaders(URLRequest* request,
                              const HttpRequestHeaders& headers) = 0;
 
   // Called right after a redirect response code was received.
@@ -101,10 +97,6 @@ class NetworkDelegate : public base::NonThreadSafe {
   // a virtual method call.
   virtual void OnURLRequestDestroyed(URLRequest* request) = 0;
 
-  // Called when the HttpTransaction for the request with the given ID is
-  // destroyed.
-  virtual void OnHttpTransactionDestroyed(uint64 request_id) = 0;
-
   // Corresponds to ProxyResolverJSBindings::OnError.
   virtual void OnPACScriptError(int line_number, const string16& error) = 0;
 };
diff --git a/net/http/http_cache_transaction.cc b/net/http/http_cache_transaction.cc
index ac3e373..b849dad 100644
--- a/net/http/http_cache_transaction.cc
+++ b/net/http/http_cache_transaction.cc
@@ -23,7 +23,6 @@
 #include "net/base/load_flags.h"
 #include "net/base/net_errors.h"
 #include "net/base/net_log.h"
-#include "net/base/network_delegate.h"
 #include "net/base/ssl_cert_request_info.h"
 #include "net/base/ssl_config_service.h"
 #include "net/disk_cache/disk_cache.h"
@@ -139,12 +138,6 @@ HttpCache::Transaction::~Transaction() {
   // after this point.
   callback_ = NULL;
 
-  if (request_ && cache_ && cache_->GetSession() &&
-      cache_->GetSession()->network_delegate()) {
-    cache_->GetSession()->network_delegate()->NotifyHttpTransactionDestroyed(
-        request_->request_id);
-  }
-
   if (cache_) {
     if (entry_) {
       bool cancel_request = reading_;
diff --git a/net/http/http_network_transaction.cc b/net/http/http_network_transaction.cc
index bbd19b3..d856d02 100644
--- a/net/http/http_network_transaction.cc
+++ b/net/http/http_network_transaction.cc
@@ -25,7 +25,6 @@
 #include "net/base/load_flags.h"
 #include "net/base/net_errors.h"
 #include "net/base/net_util.h"
-#include "net/base/network_delegate.h"
 #include "net/base/ssl_cert_request_info.h"
 #include "net/base/ssl_connection_status_flags.h"
 #include "net/base/upload_data_stream.h"
@@ -114,11 +113,6 @@ HttpNetworkTransaction::HttpNetworkTransaction(HttpNetworkSession* session)
 }
 
 HttpNetworkTransaction::~HttpNetworkTransaction() {
-  if (request_ && session_->network_delegate()) {
-    session_->network_delegate()->NotifyHttpTransactionDestroyed(
-        request_->request_id);
-  }
-
   if (stream_.get()) {
     HttpResponseHeaders* headers = GetResponseHeaders();
     // TODO(mbelshe): The stream_ should be able to compute whether or not the
@@ -770,11 +764,6 @@ int HttpNetworkTransaction::DoSendRequest() {
 }
 
 int HttpNetworkTransaction::DoSendRequestComplete(int result) {
-  if (session_->network_delegate()) {
-    session_->network_delegate()->NotifyRequestSent(
-        request_->request_id, response_.socket_address, request_headers_);
-  }
-
   if (result < 0)
     return HandleIOError(result);
   next_state_ = STATE_READ_HEADERS;
diff --git a/net/proxy/network_delegate_error_observer_unittest.cc b/net/proxy/network_delegate_error_observer_unittest.cc
index ca81258..5979909 100644
--- a/net/proxy/network_delegate_error_observer_unittest.cc
+++ b/net/proxy/network_delegate_error_observer_unittest.cc
@@ -35,8 +35,7 @@ class TestNetworkDelegate : public net::NetworkDelegate {
                                   HttpRequestHeaders* headers) {
     return net::OK;
   }
-  virtual void OnRequestSent(uint64 request_id,
-                             const HostPortPair& socket_address,
+  virtual void OnSendHeaders(URLRequest* request,
                              const HttpRequestHeaders& headers) {}
   virtual void OnBeforeRedirect(URLRequest* request,
                                 const GURL& new_location) {}
diff --git a/net/url_request/url_request.cc b/net/url_request/url_request.cc
index f058c2f..6187463 100644
--- a/net/url_request/url_request.cc
+++ b/net/url_request/url_request.cc
@@ -46,6 +46,7 @@ void StripPostSpecificHeaders(HttpRequestHeaders* headers) {
   headers->RemoveHeader(HttpRequestHeaders::kOrigin);
 }
 
+// TODO(battre): Delete this, see http://crbug.com/89321:
 // This counter keeps track of the identifiers used for URL requests so far.
 // 0 is reserved to represent an invalid ID.
 uint64 g_next_url_request_identifier = 1;
@@ -730,7 +731,7 @@ void URLRequest::NotifyAuthRequired(AuthChallengeInfo* auth_info) {
   //}
   // This fixes URLRequestTestHTTP.BasicAuth but not
   // URLRequestTestHTTP.BasicAuthWithCookies. In both cases we observe a
-  // call sequence of OnBeforeSendHeaders -> OnRequestSent ->
+  // call sequence of OnBeforeSendHeaders -> OnSendHeaders ->
   // OnBeforeSendHeaders.
 
   if (delegate_)
diff --git a/net/url_request/url_request.h b/net/url_request/url_request.h
index de4ca27..def281f 100644
--- a/net/url_request/url_request.h
+++ b/net/url_request/url_request.h
@@ -682,6 +682,12 @@ class NET_API URLRequest : NON_EXPORTED_BASE(public base::NonThreadSafe) {
   // this to determine which URLRequest to allocate sockets to first.
   RequestPriority priority_;
 
+  // TODO(battre): The only consumer of the identifier_ is currently the
+  // web request API. We need to match identifiers of requests between the
+  // web request API and the web navigation API. As the URLRequest does not
+  // exist when the web navigation API is triggered, the tracking probably
+  // needs to be done outside of the URLRequest anyway. Therefore, this
+  // identifier should be deleted here. http://crbug.com/89321
   // A globally unique identifier for this request.
   const uint64 identifier_;
 
diff --git a/net/url_request/url_request_http_job.cc b/net/url_request/url_request_http_job.cc
index 3256c04..7ff619d 100644
--- a/net/url_request/url_request_http_job.cc
+++ b/net/url_request/url_request_http_job.cc
@@ -318,7 +318,7 @@ void URLRequestHttpJob::NotifyHeadersComplete() {
     DCHECK(!response_info_->auth_challenge.get());
     // TODO(battre): This breaks the webrequest API for
     // URLRequestTestHTTP.BasicAuthWithCookies
-    // where OnBeforeSendHeaders -> OnRequestSent -> OnBeforeSendHeaders
+    // where OnBeforeSendHeaders -> OnSendHeaders -> OnBeforeSendHeaders
     // occurs.
     RestartTransactionWithAuth(string16(), string16());
     return;
@@ -380,6 +380,11 @@ void URLRequestHttpJob::StartTransactionInternal() {
 
   int rv;
 
+  if (request_->context() && request_->context()->network_delegate()) {
+    request_->context()->network_delegate()->NotifySendHeaders(
+        request_, request_info_.extra_headers);
+  }
+
   if (transaction_.get()) {
     rv = transaction_->RestartWithAuth(username_, password_, &start_callback_);
     username_.clear();
diff --git a/net/url_request/url_request_test_util.cc b/net/url_request/url_request_test_util.cc
index cc6f1b9..59d00f0 100644
--- a/net/url_request/url_request_test_util.cc
+++ b/net/url_request/url_request_test_util.cc
@@ -20,7 +20,7 @@ namespace {
 // events in the wrong order.
 const int kStageBeforeURLRequest = 1 << 0;
 const int kStageBeforeSendHeaders = 1 << 1;
-const int kStageRequestSent = 1 << 2;
+const int kStageSendHeaders = 1 << 2;
 const int kStageBeforeRedirect = 1 << 3;
 const int kStageResponseStarted = 1 << 4;
 const int kStageCompletedSuccess = 1 << 5;
@@ -359,31 +359,28 @@ int TestNetworkDelegate::OnBeforeSendHeaders(
   EXPECT_TRUE(next_states_[req_id] & kStageBeforeSendHeaders) <<
       event_order_[req_id];
   next_states_[req_id] =
-      kStageRequestSent |
+      kStageSendHeaders |
       kStageCompletedError;  // request canceled by delegate
 
-  // In case we can answer a request from the cache we can respond directly.
-  next_states_[req_id] |= kStageResponseStarted;
-
   return net::OK;
 }
 
-void TestNetworkDelegate::OnRequestSent(
-    uint64 request_id,
-    const net::HostPortPair& socket_address,
+void TestNetworkDelegate::OnSendHeaders(
+    net::URLRequest* request,
     const net::HttpRequestHeaders& headers) {
-  event_order_[request_id] += "OnRequestSent\n";
-  InitRequestStatesIfNew(request_id);
-  EXPECT_TRUE(next_states_[request_id] & kStageRequestSent) <<
-      event_order_[request_id];
-  next_states_[request_id] =
+  int req_id = request->identifier();
+  event_order_[req_id] += "OnSendHeaders\n";
+  InitRequestStatesIfNew(req_id);
+  EXPECT_TRUE(next_states_[req_id] & kStageSendHeaders) <<
+      event_order_[req_id];
+  next_states_[req_id] =
       kStageBeforeRedirect |
       kStageResponseStarted |
       kStageCompletedError;  // e.g. proxy resolution problem
 
   // Basic authentication sends a second request from the URLRequestHttpJob
   // layer before the URLRequest reports that a response has started.
-  next_states_[request_id] |= kStageBeforeSendHeaders;
+  next_states_[req_id] |= kStageBeforeSendHeaders;
 }
 
 void TestNetworkDelegate::OnBeforeRedirect(net::URLRequest* request,
diff --git a/net/url_request/url_request_test_util.h b/net/url_request/url_request_test_util.h
index 14cee1c..3bd1292 100644
--- a/net/url_request/url_request_test_util.h
+++ b/net/url_request/url_request_test_util.h
@@ -202,8 +202,7 @@ class TestNetworkDelegate : public net::NetworkDelegate {
   virtual int OnBeforeSendHeaders(net::URLRequest* request,
                                   net::CompletionCallback* callback,
                                   net::HttpRequestHeaders* headers);
-  virtual void OnRequestSent(uint64 request_id,
-                             const net::HostPortPair& socket_address,
+  virtual void OnSendHeaders(net::URLRequest* request,
                              const net::HttpRequestHeaders& headers);
   virtual void OnBeforeRedirect(net::URLRequest* request,
                                 const GURL& new_location);
diff --git a/net/url_request/url_request_unittest.cc b/net/url_request/url_request_unittest.cc
index 9e5092d..d929155 100644
--- a/net/url_request/url_request_unittest.cc
+++ b/net/url_request/url_request_unittest.cc
@@ -1497,11 +1497,6 @@ TEST_F(URLRequestTestHTTP, VaryHeader) {
   }
 
   // expect a cache hit
-
-  // TODO(battre): We have a sequence OnBeforeSendHeaders -> OnResponseStarted
-  // here because the cache hit does not trigger a OnRequestSent event.
-  // Do we need it?
-
   {
     TestDelegate d;
     URLRequest req(test_server_.GetURL("echoheadercache?foo"), &d);