Make URLRequest::Read to return net errors or bytes read instead of a bool

net/url_request/url_request_test_util.cc

Index: net/url_request/url_request_test_util.cc
diff --git a/net/url_request/url_request_test_util.cc b/net/url_request/url_request_test_util.cc
index 767c25295704da1046611983ee77362a24a7ab84..9068ddb5888855a556c21c66d8580bacfc87f190 100644
--- a/net/url_request/url_request_test_util.cc
+++ b/net/url_request/url_request_test_util.cc
@@ -193,8 +193,8 @@ TestDelegate::TestDelegate()
       certificate_errors_are_fatal_(false),
       auth_required_(false),
       have_full_request_headers_(false),
-      buf_(new IOBuffer(kBufferSize)) {
-}
+      request_status_(1),
+      buf_(new IOBuffer(kBufferSize)) {}
 
 TestDelegate::~TestDelegate() {}
 
@@ -251,6 +251,9 @@ void TestDelegate::OnSSLCertificateError(URLRequest* request,
 }
 
 void TestDelegate::OnResponseStarted(URLRequest* request) {
+  // Temporarly used to distinguish old and new code for OnReadCompleted().
+  old_ = true;

[mmenke, 2016/08/26 19:36:48]

Rather than having two paths, can we just make the old OnResponseStarted call
into the new one?  Same for OnReadCompleted.

[maksims (do not use this acc), 2016/08/29 12:30:36]

Done.

+
   // It doesn't make sense for the request to have IO pending at this point.
   DCHECK(!request->status().is_io_pending());
   EXPECT_FALSE(request->is_redirecting());
@@ -277,45 +280,113 @@ void TestDelegate::OnResponseStarted(URLRequest* request) {
   }
 }
 
-void TestDelegate::OnReadCompleted(URLRequest* request, int bytes_read) {
+void TestDelegate::OnResponseStarted(URLRequest* request, int net_error) {
+  // Temporarly used to distinguish old and new code for OnReadCompleted().
+  old_ = false;
+
   // It doesn't make sense for the request to have IO pending at this point.
-  DCHECK(!request->status().is_io_pending());
+  DCHECK_NE(ERR_IO_PENDING, net_error);
+  EXPECT_FALSE(request->is_redirecting());
+
+  have_full_request_headers_ =
+      request->GetFullRequestHeaders(&full_request_headers_);
+
+  response_started_count_++;
+  request_status_ = net_error;
+  if (cancel_in_rs_) {
+    request_status_ = request->Cancel();
+    OnResponseCompleted(request);
+  } else if (net_error != OK) {
+    request_failed_ = true;
+    OnResponseCompleted(request);
+  } else {
+    // Initiate the first read.
+    int bytes_read = request->Read(buf_.get(), kBufferSize);
+    if (bytes_read >= 0)
+      OnReadCompleted(request, bytes_read);
+    else if (bytes_read != ERR_IO_PENDING)
+      OnResponseCompleted(request);
+  }
+}
 
-  // If the request was cancelled in a redirect, it should not signal
-  // OnReadCompleted. Note that |cancel_in_rs_| may be true due to
-  // https://crbug.com/564848.
-  EXPECT_FALSE(cancel_in_rr_);
+void TestDelegate::OnReadCompleted(URLRequest* request, int bytes_read) {
+  if (old_) {
+    // It doesn't make sense for the request to have IO pending at this point.
+    DCHECK(!request->status().is_io_pending());
 
-  if (response_started_count_ == 0)
-    received_data_before_response_ = true;
+    // If the request was cancelled in a redirect, it should not signal
+    // OnReadCompleted. Note that |cancel_in_rs_| may be true due to
+    // https://crbug.com/564848.
+    EXPECT_FALSE(cancel_in_rr_);
 
-  if (cancel_in_rd_)
-    request->Cancel();
+    if (response_started_count_ == 0)
+      received_data_before_response_ = true;
 
-  if (bytes_read >= 0) {
-    // There is data to read.
-    received_bytes_count_ += bytes_read;
+    if (cancel_in_rd_)
+      request->Cancel();
 
-    // consume the data
-    data_received_.append(buf_->data(), bytes_read);
-  }
+    if (bytes_read >= 0) {
+      // There is data to read.
+      received_bytes_count_ += bytes_read;
 
-  // If it was not end of stream, request to read more.
-  if (request->status().is_success() && bytes_read > 0) {
-    bytes_read = 0;
-    while (request->Read(buf_.get(), kBufferSize, &bytes_read)) {
+      // consume the data
+      data_received_.append(buf_->data(), bytes_read);
+    }
+
+    // If it was not end of stream, request to read more.
+    if (request->status().is_success() && bytes_read > 0) {
+      bytes_read = 0;
+      while (request->Read(buf_.get(), kBufferSize, &bytes_read)) {
+        if (bytes_read > 0) {
+          data_received_.append(buf_->data(), bytes_read);
+          received_bytes_count_ += bytes_read;
+        } else {
+          break;
+        }
+      }
+    }
+    if (!request->status().is_io_pending())
+      OnResponseCompleted(request);
+    else if (cancel_in_rd_pending_)
+      request->Cancel();
+  } else {
+    // It doesn't make sense for the request to have IO pending at this point.
+    DCHECK_NE(bytes_read, ERR_IO_PENDING);
+
+    // If the request was cancelled in a redirect, it should not signal
+    // OnReadCompleted. Note that |cancel_in_rs_| may be true due to
+    // https://crbug.com/564848.
+    EXPECT_FALSE(cancel_in_rr_);
+
+    if (response_started_count_ == 0)
+      received_data_before_response_ = true;
+
+    if (cancel_in_rd_)
+      request_status_ = request->Cancel();
+
+    if (bytes_read >= 0) {
+      // There is data to read.
+      received_bytes_count_ += bytes_read;
+
+      // consume the data
+      data_received_.append(buf_->data(), bytes_read);
+    }
+
+    // If it was not end of stream, request to read more.
+    while (bytes_read > 0) {
+      bytes_read = request->Read(buf_.get(), kBufferSize);
       if (bytes_read > 0) {
         data_received_.append(buf_->data(), bytes_read);
         received_bytes_count_ += bytes_read;
-      } else {
-        break;
       }
     }
+
+    request_status_ = bytes_read;
+    if (request_status_ != ERR_IO_PENDING)
+      OnResponseCompleted(request);
+    else if (cancel_in_rd_pending_)
+      request_status_ = request->Cancel();
   }
-  if (!request->status().is_io_pending())
-    OnResponseCompleted(request);
-  else if (cancel_in_rd_pending_)
-    request->Cancel();
 }
 
 void TestDelegate::OnResponseCompleted(URLRequest* request) {
@@ -505,6 +576,7 @@ void TestNetworkDelegate::OnBeforeRedirect(URLRequest* request,
   next_states_[req_id] |= kStageResponseStarted;
 }
 
+// Deprecated.
 void TestNetworkDelegate::OnResponseStarted(URLRequest* request) {
   LoadTimingInfo load_timing_info;
   request->GetLoadTimingInfo(&load_timing_info);
@@ -523,6 +595,30 @@ void TestNetworkDelegate::OnResponseStarted(URLRequest* request) {
   }
 }
 
+void TestNetworkDelegate::OnResponseStarted(URLRequest* request,
+                                            int net_error) {
+  DCHECK_NE(ERR_IO_PENDING, net_error);
+
+  LoadTimingInfo load_timing_info;
+  request->GetLoadTimingInfo(&load_timing_info);
+  EXPECT_FALSE(load_timing_info.request_start_time.is_null());
+  EXPECT_FALSE(load_timing_info.request_start.is_null());
+
+  int req_id = request->identifier();
+  InitRequestStatesIfNew(req_id);
+  event_order_[req_id] += "OnResponseStarted\n";
+  EXPECT_TRUE(next_states_[req_id] & kStageResponseStarted)
+      << event_order_[req_id];
+  next_states_[req_id] = kStageCompletedSuccess | kStageCompletedError;
+  if (net_error == ERR_ABORTED)
+    return;
+
+  if (net_error != OK) {
+    error_count_++;
+    last_error_ = net_error;
+  }
+}
+
 void TestNetworkDelegate::OnNetworkBytesReceived(URLRequest* request,
                                                  int64_t bytes_received) {
   event_order_[request->identifier()] += "OnNetworkBytesReceived\n";
@@ -535,6 +631,7 @@ void TestNetworkDelegate::OnNetworkBytesSent(URLRequest* request,
   total_network_bytes_sent_ += bytes_sent;
 }
 
+// Deprecated.
 void TestNetworkDelegate::OnCompleted(URLRequest* request, bool started) {
   int req_id = request->identifier();
   InitRequestStatesIfNew(req_id);
@@ -560,6 +657,34 @@ void TestNetworkDelegate::OnCompleted(URLRequest* request, bool started) {
   }
 }
 
+void TestNetworkDelegate::OnCompleted(URLRequest* request,
+                                      bool started,
+                                      int net_error) {
+  DCHECK_NE(net_error, net::ERR_IO_PENDING);
+
+  int req_id = request->identifier();
+  InitRequestStatesIfNew(req_id);
+  event_order_[req_id] += "OnCompleted\n";
+  // Expect "Success -> (next_states_ & kStageCompletedSuccess)"
+  // is logically identical to
+  // Expect "!(Success) || (next_states_ & kStageCompletedSuccess)"
+  EXPECT_TRUE(net_error != OK ||
+              (next_states_[req_id] & kStageCompletedSuccess))
+      << event_order_[req_id];
+  EXPECT_TRUE(net_error == OK || (next_states_[req_id] & kStageCompletedError))
+      << event_order_[req_id];
+  next_states_[req_id] = kStageURLRequestDestroyed;
+  completed_requests_++;
+  if (net_error == ERR_ABORTED) {
+    canceled_requests_++;
+  } else if (net_error != OK) {
+    error_count_++;
+    last_error_ = net_error;
+  } else {
+    DCHECK_EQ(net_error, OK);
+  }
+}
+
 void TestNetworkDelegate::OnURLRequestDestroyed(URLRequest* request) {
   int req_id = request->identifier();
   InitRequestStatesIfNew(req_id);