Adjust callers and networking delegates in net/ to modified APIs

net/cert_net/cert_net_fetcher_impl.cc

 // Copyright 2015 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/cert_net/cert_net_fetcher_impl.h"
 
 #include <tuple>
 #include <utility>
 
 #include "base/callback_helpers.h"
@@ skipping 160 matching lines @@
   void StartURLRequest(URLRequestContext* context);
 
  private:
   // The pointers in RequestList are not owned by the Job.
   using RequestList = base::LinkedList<RequestImpl>;
 
   // Implementation of URLRequest::Delegate
   void OnReceivedRedirect(URLRequest* request,
                           const RedirectInfo& redirect_info,
                           bool* defer_redirect) override;
-  void OnResponseStarted(URLRequest* request) override;
+  void OnResponseStarted(URLRequest* request, int net_error) override;
   void OnReadCompleted(URLRequest* request, int bytes_read) override;
 
   // Clears the URLRequest and timer. Helper for doing work common to
   // cancellation and job completion.
   void Stop();
 
   // Reads as much data as available from |request|.
   void ReadBody(URLRequest* request);
 
   // Helper to copy the partial bytes read from the read IOBuffer to an
   // aggregated buffer.
   bool ConsumeBytesRead(URLRequest* request, int num_bytes);
 
   // Called once the job has exceeded its deadline.
   void OnTimeout();
 
   // Called when the URLRequest has completed (either success or failure).
-  void OnUrlRequestCompleted(URLRequest* request);
+  void OnUrlRequestCompleted(URLRequest* request, int net_error);
 
   // Called when the Job has completed. The job may finish in response to a
   // timeout, an invalid URL, or the URLRequest completing. By the time this
   // method is called, the response variables have been assigned
   // (result_net_error_ and response_body_).
   void OnJobCompleted();
 
   // The requests attached to this job.
   RequestList requests_;
 
@@ skipping 94 matching lines @@
 
 void CertNetFetcherImpl::Job::OnReceivedRedirect(
     URLRequest* request,
     const RedirectInfo& redirect_info,
     bool* defer_redirect) {
   DCHECK_EQ(url_request_.get(), request);
 
   // Ensure that the new URL matches the policy.
   Error error = CanFetchUrl(redirect_info.new_url);
   if (error != OK) {
-    request->CancelWithError(error);
-    OnUrlRequestCompleted(request);
+    int status = request->CancelWithError(error);

[mmenke, 2016/08/30 22:13:22]

Can we call these results, instead?  net:Errors are generally thought of more as
the result of an operation, rather than an ongoing status.  (There's a comment
below about replacing these two lines with a function call, but I mean in
general)

[maksims (do not use this acc), 2016/09/01 12:22:29]

Done.

+    OnUrlRequestCompleted(request, status);
     return;
   }
 }
 
-void CertNetFetcherImpl::Job::OnResponseStarted(URLRequest* request) {
+void CertNetFetcherImpl::Job::OnResponseStarted(URLRequest* request,
+                                                int net_error) {
   DCHECK_EQ(url_request_.get(), request);
+  DCHECK_NE(ERR_IO_PENDING, net_error);
 
-  if (!request->status().is_success()) {
-    OnUrlRequestCompleted(request);
+  if (net_error != OK) {
+    OnUrlRequestCompleted(request, net_error);
     return;
   }
 
   if (request->GetResponseCode() != 200) {
     // TODO(eroman): Use a more specific error code.
-    request->CancelWithError(ERR_FAILED);
-    OnUrlRequestCompleted(request);
+    int status = request->CancelWithError(ERR_FAILED);
+    OnUrlRequestCompleted(request, status);
     return;
   }
 
   ReadBody(request);
 }
 
 void CertNetFetcherImpl::Job::OnReadCompleted(URLRequest* request,
                                               int bytes_read) {
   DCHECK_EQ(url_request_.get(), request);
+  DCHECK_NE(ERR_IO_PENDING, bytes_read);
 
   // Keep reading the response body.
   if (ConsumeBytesRead(request, bytes_read))
     ReadBody(request);
 }
 
 void CertNetFetcherImpl::Job::Stop() {
   timer_.Stop();
   url_request_.reset();
 }
 
 void CertNetFetcherImpl::Job::ReadBody(URLRequest* request) {
   // Read as many bytes as are available synchronously.
-  int num_bytes;
-  while (
-      request->Read(read_buffer_.get(), kReadBufferSizeInBytes, &num_bytes)) {
+  int num_bytes = 0;
+  while (num_bytes >= 0) {
+    num_bytes = request->Read(read_buffer_.get(), kReadBufferSizeInBytes);
     if (!ConsumeBytesRead(request, num_bytes))
       return;
   }
 
   // Check whether the read failed synchronously.
-  if (!request->status().is_io_pending())
-    OnUrlRequestCompleted(request);
+  if (num_bytes != ERR_IO_PENDING)
+    OnUrlRequestCompleted(request, num_bytes);
   return;
 }
 
 bool CertNetFetcherImpl::Job::ConsumeBytesRead(URLRequest* request,
                                                int num_bytes) {
   if (num_bytes <= 0) {
     // Error while reading, or EOF.
-    OnUrlRequestCompleted(request);
+    OnUrlRequestCompleted(request, num_bytes);
     return false;
   }
 
   // Enforce maximum size bound.
   if (num_bytes + response_body_.size() > request_params_->max_response_bytes) {
-    request->CancelWithError(ERR_FILE_TOO_BIG);
-    OnUrlRequestCompleted(request);
+    int status = request->CancelWithError(ERR_FILE_TOO_BIG);
+    OnUrlRequestCompleted(request, status);
     return false;
   }
 
   // Append the data to |response_body_|.
   response_body_.reserve(num_bytes);
   response_body_.insert(response_body_.end(), read_buffer_->data(),
                         read_buffer_->data() + num_bytes);
   return true;
 }
 
 void CertNetFetcherImpl::Job::OnTimeout() {

[mmenke, 2016/08/30 22:13:22]

Suggestion:  Make this FailRequest(net::Error error).  Then have this call
CancelWithError / OnJobCompleted.

Then have everything that does CancelWithError and then calls
OnUrlRequestCompleted just call this instead.  The one caller of OnTimeout can
use base::Bind(Job::OnTimeout&, this, ERR_TIMED_OUT) instead.  That makes all
the cancellation code go through the same cancel/complete calls.

[maksims (do not use this acc), 2016/09/01 12:22:29]

Done.

   result_net_error_ = ERR_TIMED_OUT;
   url_request_->CancelWithError(result_net_error_);
   OnJobCompleted();
 }
 
-void CertNetFetcherImpl::Job::OnUrlRequestCompleted(URLRequest* request) {
+void CertNetFetcherImpl::Job::OnUrlRequestCompleted(URLRequest* request,
+                                                    int net_error) {
   DCHECK_EQ(request, url_request_.get());
 
-  if (request->status().is_success())
+  if (net_error == OK)
     result_net_error_ = OK;
   else
-    result_net_error_ = static_cast<Error>(request->status().error());
+    result_net_error_ = static_cast<Error>(net_error);

[mmenke, 2016/08/30 22:13:22]

This whole thing is just:

result_net_error_ = static_cast<Error>(net_error);

[maksims (do not use this acc), 2016/09/01 12:22:29]

Done.

 
   OnJobCompleted();
 }
 
 void CertNetFetcherImpl::Job::OnJobCompleted() {

[mmenke, 2016/08/30 22:13:22]

Suggestion:  Make this OnJobCompleted(net::Error), and set result_net_error_
here (And only here).  That ensures all paths set it, and gets rid of redundant
code.  OR better, I think you can just get rid of result_net_error_ entirely, if
you do that?  It's only used in this method, I believe.

[maksims (do not use this acc), 2016/09/01 12:22:29]

Yes, you're right. Done!

   // Stop the timer and clear the URLRequest.
   Stop();
 
   // Invoking the callbacks is subtle as state may be mutated while iterating
   // through the callbacks:
   //
   //   * The parent CertNetFetcherImpl may be deleted
   //   * Requests in this job may be cancelled
 
   std::unique_ptr<Job> delete_this = parent_->RemoveJob(this);
@@ skipping 130 matching lines @@
 void CertNetFetcherImpl::ClearCurrentlyCompletingJob(Job* job) {
   DCHECK_EQ(currently_completing_job_, job);
   currently_completing_job_ = nullptr;
 }
 
 bool CertNetFetcherImpl::IsCurrentlyCompletingJob(Job* job) {
   return job == currently_completing_job_;
 }
 
 }  // namespace net