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),

[mmenke, 2016/08/30 22:09:12]

Suggest ERR_IO_PENDING (It's the one status we should never see)

[maksims (do not use this acc), 2016/08/31 11:17:00]

Done.

+      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 35 matching lines @@
   // independent of any possible errors, or whether it wants SSL errors to
   // cancel the request.
   have_certificate_errors_ = true;
   certificate_errors_are_fatal_ = fatal;
   if (allow_certificate_errors_)
     request->ContinueDespiteLastError();
   else
     request->Cancel();
 }
 
+// Deprecated.
 void TestDelegate::OnResponseStarted(URLRequest* request) {
+  int net_error = request->status().error();
+  OnResponseStarted(request, net_error);
+}

[mmenke, 2016/08/30 22:09:12]

This method is no longer needed (Be sure to remove it from the header, too)

[maksims (do not use this acc), 2016/08/31 11:16:59]

No, it's needed. Callers are not modified yet. For example, embedder calls
TestDelegate::OnReponseStarted(request);

[mmenke, 2016/08/31 21:35:47]

So...You're right about that, but I don't see how those tests are passing. 
Those tests all subclass TestDelegate, and TestDelegate overrides the new
OnResponseStarted, which normally calls the old one, but TestDelegate's version
of it does not.  So how are any of those methods that call
TestDelegate::OnReponseStarted being invoked?  I'm confused.

[mmenke, 2016/08/31 21:39:27]

Wait, aren't you updating all those callers, for just that reason?  Are there
any callers of TestDelegate::OnResponseStarted that you aren't already updating
in this CL to use the new method?

[maksims (do not use this acc), 2016/09/01 07:13:17]

Yes, I am. I'll add the embedded_test_server_unittest to this CL and remove old
OnResponseStarted from Test(Network)Delegate (right now it's in another CL that
is dependent on this one. That's why compilation fails here, but runs fine in
the CL with net callers).

+
+void TestDelegate::OnResponseStarted(URLRequest* request, int net_error) {
   // 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->Cancel();
+    request_status_ = request->Cancel();
     OnResponseCompleted(request);
-  } else if (!request->status().is_success()) {
-    DCHECK(request->status().status() == URLRequestStatus::FAILED ||
-           request->status().status() == URLRequestStatus::CANCELED);
+  } else if (net_error != OK) {
     request_failed_ = true;
     OnResponseCompleted(request);
   } else {
     // Initiate the first read.
-    int bytes_read = 0;
-    if (request->Read(buf_.get(), kBufferSize, &bytes_read))
+    int bytes_read = request->Read(buf_.get(), kBufferSize);
+    if (bytes_read >= 0)
       OnReadCompleted(request, bytes_read);
-    else if (!request->status().is_io_pending())
+    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());
+  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->Cancel();
+    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.
-  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;
-      }
+  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;
     }
   }
-  if (!request->status().is_io_pending())
+
+  request_status_ = bytes_read;
+  if (request_status_ != ERR_IO_PENDING)
     OnResponseCompleted(request);
   else if (cancel_in_rd_pending_)
-    request->Cancel();
+    request_status_ = 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) {
+  int net_error = request->status().error();
+  OnResponseStarted(request, net_error);
+}

[mmenke, 2016/08/30 22:09:12]

No longer needed.

[maksims (do not use this acc), 2016/09/01 07:13:17]

Done.

+
+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];
+  EXPECT_TRUE(next_states_[req_id] & kStageResponseStarted)
+      << event_order_[req_id];
   next_states_[req_id] = kStageCompletedSuccess | kStageCompletedError;
-  if (request->status().status() == URLRequestStatus::FAILED) {
+  if (net_error == ERR_ABORTED)
+    return;
+
+  if (net_error != OK) {
     error_count_++;
-    last_error_ = request->status().error();
+    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 net_error = request->status().error();
+  OnCompleted(request, started, net_error);
+}
+
+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(!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];
+  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 (request->status().status() == URLRequestStatus::FAILED) {
+  if (net_error == ERR_ABORTED) {
+    canceled_requests_++;
+  } else if (net_error != OK) {
     error_count_++;
-    last_error_ = request->status().error();
-  } else if (request->status().status() == URLRequestStatus::CANCELED) {
-    canceled_requests_++;
+    last_error_ = net_error;
   } else {
-    DCHECK_EQ(URLRequestStatus::SUCCESS, request->status().status());
+    DCHECK_EQ(net_error, OK);

[mmenke, 2016/08/30 22:09:12]

Flip order - expected should be first value.

[maksims (do not use this acc), 2016/08/31 11:17:00]

Done.

   }
 }
 
 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;
@@ skipping 89 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