Add request_id to HttpRequestInfo and pass it to the NetworkDelegate for events.

net/http/http_network_transaction.cc

Index: net/http/http_network_transaction.cc
diff --git a/net/http/http_network_transaction.cc b/net/http/http_network_transaction.cc
index 4bd867d1432d96545f9b820f8c0c34126681c809..c1196eba43610d28a82f602dc3948ec82a2d3278 100644
--- a/net/http/http_network_transaction.cc
+++ b/net/http/http_network_transaction.cc
@@ -99,6 +99,9 @@ HttpNetworkTransaction::HttpNetworkTransaction(HttpNetworkSession* session)
     : pending_auth_target_(HttpAuth::AUTH_NONE),
       ALLOW_THIS_IN_INITIALIZER_LIST(
           io_callback_(this, &HttpNetworkTransaction::OnIOComplete)),
+      ALLOW_THIS_IN_INITIALIZER_LIST(delegate_callback_(
+          new CancelableCompletionCallback<HttpNetworkTransaction>(
+              this, &HttpNetworkTransaction::OnIOComplete))),
       user_callback_(NULL),
       session_(session),
       request_(NULL),
@@ -146,6 +149,8 @@ HttpNetworkTransaction::~HttpNetworkTransaction() {
       }
     }
   }
+
+  delegate_callback_->Cancel();
 }
 
 int HttpNetworkTransaction::Start(const HttpRequestInfo* request_info,
@@ -514,10 +519,16 @@ int HttpNetworkTransaction::DoLoop(int result) {
       case STATE_GENERATE_SERVER_AUTH_TOKEN_COMPLETE:
         rv = DoGenerateServerAuthTokenComplete(rv);
         break;
-      case STATE_SEND_REQUEST:
+      case STATE_BUILD_REQUEST:
         DCHECK_EQ(OK, rv);
         net_log_.BeginEvent(NetLog::TYPE_HTTP_TRANSACTION_SEND_REQUEST, NULL);
-        rv = DoSendRequest();
+        rv = DoBuildRequest();
+        break;
+      case STATE_BUILD_REQUEST_COMPLETE:
+        rv = DoBuildRequestComplete(rv);
+        break;
+      case STATE_SEND_REQUEST:
+        rv = DoSendRequest(rv);
         break;
       case STATE_SEND_REQUEST_COMPLETE:
         rv = DoSendRequestComplete(rv);
@@ -661,38 +672,61 @@ int HttpNetworkTransaction::DoGenerateServerAuthToken() {
 int HttpNetworkTransaction::DoGenerateServerAuthTokenComplete(int rv) {
   DCHECK_NE(ERR_IO_PENDING, rv);
   if (rv == OK)
-    next_state_ = STATE_SEND_REQUEST;
+    next_state_ = STATE_BUILD_REQUEST;
   return rv;
 }
 
-int HttpNetworkTransaction::DoSendRequest() {
-  next_state_ = STATE_SEND_REQUEST_COMPLETE;
+int HttpNetworkTransaction::DoBuildRequest() {
+  next_state_ = STATE_BUILD_REQUEST_COMPLETE;
 
-  UploadDataStream* request_body = NULL;
+  request_body_.reset(NULL);
   if (request_->upload_data) {
     int error_code;
-    request_body = UploadDataStream::Create(request_->upload_data, &error_code);
-    if (!request_body)
+    request_body_.reset(
+        UploadDataStream::Create(request_->upload_data, &error_code));
+    if (!request_body_.get())
       return error_code;
   }
 
+  headers_valid_ = false;
+
   // This is constructed lazily (instead of within our Start method), so that
   // we have proxy info available.
   if (request_headers_.IsEmpty()) {
-    bool using_proxy = (proxy_info_.is_http()|| proxy_info_.is_https()) &&
+    bool using_proxy = (proxy_info_.is_http() || proxy_info_.is_https()) &&
                         !is_https_request();
-    HttpUtil::BuildRequestHeaders(request_, request_body, auth_controllers_,
+    HttpUtil::BuildRequestHeaders(request_, request_body_.get(),
+                                  auth_controllers_,
                                   ShouldApplyServerAuth(),
                                   ShouldApplyProxyAuth(), using_proxy,
                                   &request_headers_);
+  }
 
-    if (session_->network_delegate())
-      session_->network_delegate()->NotifySendHttpRequest(&request_headers_);
+  return OK;
+}
+
+int HttpNetworkTransaction::DoBuildRequestComplete(int result) {

[willchan no longer on Chromium, 2011/03/22 23:05:35]

Sorry for nitpicking like this, but to make it more like net code, the
parameters need to make sense too.

Please move this code into DoBuildRequest(). DoBuildRequestComplete() should be
simple. Something like:

DoBuildRequestComplete(int result) {
  if (result == OK)
    next_state_ = STATE_SEND_REQUEST
  return result;
}

DoSendRequest(void) shouldn't need a parameter, and thus shouldn't need the if
(result == OK)

[Matt Perry, 2011/03/24 00:11:25]

Done. Though now I notice that delegate_callback_ may never get to the Release()
in DoSendRequest() - I think. Should I  be concerned about that? I'm following
the pattern in HttpCache::Transaction, and it doesn't seem to handle that case,
so maybe I'm misinterpreting it.

[willchan no longer on Chromium, 2011/03/26 01:46:49]

I think maybe you want to only create the delegate_callback_ In BuildRequest().
AddRef() it at that point. Then in the destructor, if |delegate_callback_| is
non-NULL, Cancel() it. And in BuildRequestComplete(), NULL it out too, in
addition to the Release() call.

[Matt Perry, 2011/03/28 22:51:01]

Any reason to prefer doing it that way as opposed to always creating the
callback?

[willchan no longer on Chromium, 2011/03/29 20:46:27]

I don't know that this is possible today, but let's say we get to BuildRequest()
and get ERR_IO_PENDING. Now, let's say we restart the transaction (I think we
only restart the transaction in STATE_NONE right now, but I'm not sure how
strict of a convention that is).

At that point, we have pending callback that should be Cancel()'d. Now that
we've restarted, we need to use a new callback object, otherwise, we'll be
trying to use one that has already have Cancel() called on it.

The fact that we can restart transactions and Cancel() can't be reset (nor we
would want to), makes me feel like it's best not to keep the callback alive for
the whole transaction, just create and release it when we're done.

+  next_state_ = STATE_SEND_REQUEST;
+  delegate_callback_->AddRef();  // balanced in DoSendRequest
+
+  if (session_->network_delegate()) {
+    if (session_->network_delegate()->NotifyBeforeSendHeaders(
+            request_->request_id, &request_headers_, delegate_callback_))
+      return ERR_IO_PENDING;
   }
 
-  headers_valid_ = false;
-  return stream_->SendRequest(request_headers_, request_body, &response_,
-                              &io_callback_);
+  return OK;
+}
+
+int HttpNetworkTransaction::DoSendRequest(int result) {
+  next_state_ = STATE_SEND_REQUEST_COMPLETE;
+  delegate_callback_->Release();  // balanced in DoBuildRequestComplete
+
+  if (result == net::OK) {
+    return stream_->SendRequest(
+        request_headers_, request_body_.release(), &response_, &io_callback_);
+  }
+  return result;
 }
 
 int HttpNetworkTransaction::DoSendRequestComplete(int result) {
@@ -1238,6 +1272,8 @@ std::string HttpNetworkTransaction::DescribeState(State state) {
   switch (state) {
     STATE_CASE(STATE_CREATE_STREAM);
     STATE_CASE(STATE_CREATE_STREAM_COMPLETE);
+    STATE_CASE(STATE_BUILD_REQUEST);
+    STATE_CASE(STATE_BUILD_REQUEST_COMPLETE);
     STATE_CASE(STATE_SEND_REQUEST);
     STATE_CASE(STATE_SEND_REQUEST_COMPLETE);
     STATE_CASE(STATE_READ_HEADERS);