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

net/url_request/url_request_test_util.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "net/url_request/url_request_test_util.h"
 
 #include <utility>
 
 #include "base/compiler_specific.h"
 #include "base/location.h"
@@ skipping 175 matching lines @@
       allow_certificate_errors_(false),
       response_started_count_(0),
       received_bytes_count_(0),
       received_redirect_count_(0),
       received_data_before_response_(false),
       request_failed_(false),
       have_certificate_errors_(false),
       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() {}
 
 void TestDelegate::ClearFullRequestHeaders() {
   full_request_headers_.Clear();
   have_full_request_headers_ = false;
 }
 
 void TestDelegate::OnReceivedRedirect(URLRequest* request,
                                       const RedirectInfo& redirect_info,
@@ skipping 36 matching lines @@
   // cancel the request.
   have_certificate_errors_ = true;
   certificate_errors_are_fatal_ = fatal;
   if (allow_certificate_errors_)
     request->ContinueDespiteLastError();
   else
     request->Cancel();
 }
 
 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());
 
   have_full_request_headers_ =
       request->GetFullRequestHeaders(&full_request_headers_);
 
   response_started_count_++;
   if (cancel_in_rs_) {
     request->Cancel();
     OnResponseCompleted(request);
   } else if (!request->status().is_success()) {
     DCHECK(request->status().status() == URLRequestStatus::FAILED ||
            request->status().status() == URLRequestStatus::CANCELED);
     request_failed_ = true;
     OnResponseCompleted(request);
   } else {
     // Initiate the first read.
     int bytes_read = 0;
     if (request->Read(buf_.get(), kBufferSize, &bytes_read))
       OnReadCompleted(request, bytes_read);
     else if (!request->status().is_io_pending())
       OnResponseCompleted(request);
   }
 }
 
+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_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);
+  }
+}
+
 void TestDelegate::OnReadCompleted(URLRequest* request, int bytes_read) {
-  // It doesn't make sense for the request to have IO pending at this point.
-  DCHECK(!request->status().is_io_pending());
+  if (old_) {
+    // It doesn't make sense for the request to have IO pending at this point.
+    DCHECK(!request->status().is_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 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 (response_started_count_ == 0)
+      received_data_before_response_ = true;
 
-  if (cancel_in_rd_)
-    request->Cancel();
+    if (cancel_in_rd_)
+      request->Cancel();
 
-  if (bytes_read >= 0) {
-    // There is data to read.
-    received_bytes_count_ += bytes_read;
+    if (bytes_read >= 0) {
+      // There is data to read.
+      received_bytes_count_ += bytes_read;
 
-    // consume the data
-    data_received_.append(buf_->data(), 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 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) {
   if (quit_on_complete_)
     base::ThreadTaskRunnerHandle::Get()->PostTask(
         FROM_HERE, base::MessageLoop::QuitWhenIdleClosure());
 }
 
 TestNetworkDelegate::TestNetworkDelegate()
     : last_error_(0),
@@ skipping 169 matching lines @@
       kStageBeforeStartTransaction |  // Redirects from the network delegate do
                                       // not
                                       // trigger onBeforeURLRequest.
       kStageCompletedError;
 
   // A redirect can lead to a file or a data URL. In this case, we do not send
   // headers.
   next_states_[req_id] |= kStageResponseStarted;
 }
 
+// Deprecated.
 void TestNetworkDelegate::OnResponseStarted(URLRequest* request) {
   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 (request->status().status() == URLRequestStatus::FAILED) {
     error_count_++;
     last_error_ = request->status().error();
   }
 }
 
+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";
   total_network_bytes_received_ += bytes_received;
 }
 
 void TestNetworkDelegate::OnNetworkBytesSent(URLRequest* request,
                                              int64_t bytes_sent) {
   event_order_[request->identifier()] += "OnNetworkBytesSent\n";
   total_network_bytes_sent_ += bytes_sent;
 }
 
+// Deprecated.
 void TestNetworkDelegate::OnCompleted(URLRequest* request, bool started) {
   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(!request->status().is_success() ||
               (next_states_[req_id] & kStageCompletedSuccess)) <<
       event_order_[req_id];
   EXPECT_TRUE(request->status().is_success() ||
               (next_states_[req_id] & kStageCompletedError)) <<
       event_order_[req_id];
   next_states_[req_id] = kStageURLRequestDestroyed;
   completed_requests_++;
   if (request->status().status() == URLRequestStatus::FAILED) {
     error_count_++;
     last_error_ = request->status().error();
   } else if (request->status().status() == URLRequestStatus::CANCELED) {
     canceled_requests_++;
   } else {
     DCHECK_EQ(URLRequestStatus::SUCCESS, request->status().status());
   }
 }
 
+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);
   event_order_[req_id] += "OnURLRequestDestroyed\n";
   EXPECT_TRUE(next_states_[req_id] & kStageURLRequestDestroyed) <<
       event_order_[req_id];
   next_states_[req_id] = kStageDestruction;
   destroyed_requests_++;
 }
 
@@ skipping 86 matching lines @@
     NetworkDelegate* network_delegate) const {
   return main_intercept_job_.release();
 }
 
 void TestJobInterceptor::set_main_intercept_job(
     std::unique_ptr<URLRequestJob> job) {
   main_intercept_job_ = std::move(job);
 }
 
 }  // namespace net