URLRequestJob: change ReadRawData contract

content/browser/service_worker/service_worker_write_to_cache_job.cc

Index: content/browser/service_worker/service_worker_write_to_cache_job.cc
diff --git a/content/browser/service_worker/service_worker_write_to_cache_job.cc b/content/browser/service_worker/service_worker_write_to_cache_job.cc
index 95d607411899759794b7d7af6340d19b51f67d82..089b3732620760261f1690351278cdb71f66ff66 100644
--- a/content/browser/service_worker/service_worker_write_to_cache_job.cc
+++ b/content/browser/service_worker/service_worker_write_to_cache_job.cc
@@ -157,40 +157,19 @@ void ServiceWorkerWriteToCacheJob::SetExtraRequestHeaders(
   net_request_->SetExtraRequestHeaders(headers);
 }
 
-bool ServiceWorkerWriteToCacheJob::ReadRawData(net::IOBuffer* buf,
-                                               int buf_size,
-                                               int* bytes_read) {
-  net::URLRequestStatus status = ReadNetData(buf, buf_size, bytes_read);
-  SetStatus(status);
+int ServiceWorkerWriteToCacheJob::ReadRawData(net::IOBuffer* buf,
+                                              int buf_size) {
+  int bytes_read = 0;
+  net::URLRequestStatus status = ReadNetData(buf, buf_size, &bytes_read);
   if (status.is_io_pending())
-    return false;
-
-  if (!status.is_success()) {
-    NotifyDoneHelper(status, kFetchScriptError);
-    return false;
-  }
-
-  HandleNetData(*bytes_read);
-  status = GetStatus();
-
-  // Synchronous EOFs that do not replace the incumbent entry are considered
-  // failures. Since normally the URLRequestJob's status would be set by
-  // ReadNetData or HandleNetData, this code has to manually fix up the status
-  // to match the failure this function is about to return.
-  if (status.status() == net::URLRequestStatus::SUCCESS &&
-      *bytes_read == 0 && !cache_writer_->did_replace()) {
-    status = net::URLRequestStatus::FromError(kIdenticalScriptError);
-  }
+    return net::ERR_IO_PENDING;
 
   if (!status.is_success()) {
-    NotifyDoneHelper(status, "");
-    return false;
+    NotifyFinishedCaching(status, kFetchScriptError);
+    return status.error();
   }
 
-  // Since URLRequestStatus::is_success() means "SUCCESS or IO_PENDING", but the
-  // contract of this function is "return true for synchronous successes only",
-  // it is important to test against SUCCESS explicitly here.
-  return status.status() == net::URLRequestStatus::SUCCESS;
+  return HandleNetData(bytes_read);
 }
 
 const net::HttpResponseInfo* ServiceWorkerWriteToCacheJob::http_info() const {
@@ -245,9 +224,9 @@ void ServiceWorkerWriteToCacheJob::OnReceivedRedirect(
   TRACE_EVENT0("ServiceWorker",
                "ServiceWorkerWriteToCacheJob::OnReceivedRedirect");
   // Script resources can't redirect.
-  NotifyDoneHelper(net::URLRequestStatus(net::URLRequestStatus::FAILED,
-                                         net::ERR_UNSAFE_REDIRECT),
-                   kRedirectError);
+  NotifyStartErrorHelper(net::URLRequestStatus(net::URLRequestStatus::FAILED,
+                                               net::ERR_UNSAFE_REDIRECT),
+                         kRedirectError);
 }
 
 void ServiceWorkerWriteToCacheJob::OnAuthRequired(
@@ -257,7 +236,7 @@ void ServiceWorkerWriteToCacheJob::OnAuthRequired(
   TRACE_EVENT0("ServiceWorker",
                "ServiceWorkerWriteToCacheJob::OnAuthRequired");
   // TODO(michaeln): Pass this thru to our jobs client.
-  NotifyDoneHelper(
+  NotifyStartErrorHelper(
       net::URLRequestStatus(net::URLRequestStatus::FAILED, net::ERR_FAILED),
       kClientAuthenticationError);
 }
@@ -270,7 +249,7 @@ void ServiceWorkerWriteToCacheJob::OnCertificateRequested(
                "ServiceWorkerWriteToCacheJob::OnCertificateRequested");
   // TODO(michaeln): Pass this thru to our jobs client.
   // see NotifyCertificateRequested.
-  NotifyDoneHelper(
+  NotifyStartErrorHelper(
       net::URLRequestStatus(net::URLRequestStatus::FAILED, net::ERR_FAILED),
       kClientAuthenticationError);
 }
@@ -284,9 +263,9 @@ void ServiceWorkerWriteToCacheJob::OnSSLCertificateError(
                "ServiceWorkerWriteToCacheJob::OnSSLCertificateError");
   // TODO(michaeln): Pass this thru to our jobs client,
   // see NotifySSLCertificateError.
-  NotifyDoneHelper(net::URLRequestStatus(net::URLRequestStatus::FAILED,
-                                         net::ERR_INSECURE_RESPONSE),
-                   kSSLError);
+  NotifyStartErrorHelper(net::URLRequestStatus(net::URLRequestStatus::FAILED,
+                                               net::ERR_INSECURE_RESPONSE),
+                         kSSLError);
 }
 
 void ServiceWorkerWriteToCacheJob::OnBeforeNetworkStart(
@@ -302,15 +281,15 @@ void ServiceWorkerWriteToCacheJob::OnResponseStarted(
     net::URLRequest* request) {
   DCHECK_EQ(net_request_, request);
   if (!request->status().is_success()) {
-    NotifyDoneHelper(request->status(), kFetchScriptError);
+    NotifyStartErrorHelper(request->status(), kFetchScriptError);
     return;
   }
   if (request->GetResponseCode() / 100 != 2) {
     std::string error_message =
         base::StringPrintf(kBadHTTPResponseError, request->GetResponseCode());
-    NotifyDoneHelper(net::URLRequestStatus(net::URLRequestStatus::FAILED,
-                                           net::ERR_INVALID_RESPONSE),
-                     error_message);
+    NotifyStartErrorHelper(net::URLRequestStatus(net::URLRequestStatus::FAILED,
+                                                 net::ERR_INVALID_RESPONSE),
+                           error_message);
     // TODO(michaeln): Instead of error'ing immediately, send the net
     // response to our consumer, just don't cache it?
     return;
@@ -321,9 +300,10 @@ void ServiceWorkerWriteToCacheJob::OnResponseStarted(
     const net::HttpNetworkSession::Params* session_params =
         request->context()->GetNetworkSessionParams();
     if (!session_params || !session_params->ignore_certificate_errors) {
-      NotifyDoneHelper(net::URLRequestStatus(net::URLRequestStatus::FAILED,
-                                             net::ERR_INSECURE_RESPONSE),
-                       kSSLError);
+      NotifyStartErrorHelper(
+          net::URLRequestStatus(net::URLRequestStatus::FAILED,
+                                net::ERR_INSECURE_RESPONSE),
+          kSSLError);
       return;
     }
   }
@@ -338,9 +318,10 @@ void ServiceWorkerWriteToCacheJob::OnResponseStarted(
           mime_type.empty()
               ? kNoMIMEError
               : base::StringPrintf(kBadMIMEError, mime_type.c_str());
-      NotifyDoneHelper(net::URLRequestStatus(net::URLRequestStatus::FAILED,
-                                             net::ERR_INSECURE_RESPONSE),
-                       error_message);
+      NotifyStartErrorHelper(
+          net::URLRequestStatus(net::URLRequestStatus::FAILED,
+                                net::ERR_INSECURE_RESPONSE),
+          error_message);
       return;
     }
 
@@ -376,53 +357,47 @@ void ServiceWorkerWriteToCacheJob::OnWriteHeadersComplete(net::Error error) {
   NotifyHeadersComplete();
 }
 
-void ServiceWorkerWriteToCacheJob::HandleNetData(int bytes_read) {
+int ServiceWorkerWriteToCacheJob::HandleNetData(int bytes_read) {
   io_buffer_bytes_ = bytes_read;
   net::Error error = cache_writer_->MaybeWriteData(
       io_buffer_.get(), bytes_read,
       base::Bind(&ServiceWorkerWriteToCacheJob::OnWriteDataComplete,
                  weak_factory_.GetWeakPtr()));
-  SetStatus(net::URLRequestStatus::FromError(error));
 
   // In case of ERR_IO_PENDING, this logic is done in OnWriteDataComplete.
   if (error != net::ERR_IO_PENDING && bytes_read == 0) {
     NotifyFinishedCaching(net::URLRequestStatus::FromError(error),
                           std::string());
   }
+  return error == net::OK ? bytes_read : error;
 }
 
 void ServiceWorkerWriteToCacheJob::OnWriteDataComplete(net::Error error) {
   SetStatus(net::URLRequestStatus::FromError(error));
   DCHECK_NE(net::ERR_IO_PENDING, error);
-  if (io_buffer_bytes_ == 0) {
-    NotifyDoneHelper(net::URLRequestStatus::FromError(error), std::string());
-  }
-  NotifyReadComplete(error == net::OK ? io_buffer_bytes_ : error);
+  if (io_buffer_bytes_ == 0)
+    NotifyFinishedCaching(net::URLRequestStatus::FromError(error), "");
+  ReadRawDataComplete(error == net::OK ? io_buffer_bytes_ : error);
 }
 
-void ServiceWorkerWriteToCacheJob::OnReadCompleted(
-    net::URLRequest* request,
-    int bytes_read) {
+void ServiceWorkerWriteToCacheJob::OnReadCompleted(net::URLRequest* request,
+                                                   int result) {
   DCHECK_EQ(net_request_, request);
-  if (bytes_read < 0) {
-    DCHECK(!request->status().is_success());
-    NotifyDoneHelper(request->status(), kFetchScriptError);
+  DCHECK_NE(result, net::ERR_IO_PENDING);
+
+  if (result >= 0)
+    result = HandleNetData(result);
+
+  // ReadRawDataComplete will be called in OnWriteDataComplete, so return early.
+  if (result == net::ERR_IO_PENDING)
     return;
+
+  if (result < 0) {
+    DCHECK_NE(net::ERR_IO_PENDING, result);

[Randy Smith (Not in Mondays), 2015/10/22 20:38:45]

nit: This feels like overkill, since it's basically checking "If the if
statement four lines above this one failed to execute correctly for some
reason".

[xunjieli, 2015/10/23 13:43:08]

Done.

+    DCHECK(!request->status().is_success());
+    NotifyFinishedCaching(request->status(), kFetchScriptError);
   }
-  HandleNetData(bytes_read);
-  // HandleNetData can cause status of this job to change. If the status changes
-  // to IO_PENDING, that means HandleNetData has pending IO, and
-  // NotifyReadComplete will be called later by the appropriate callback.
-  if (!GetStatus().is_io_pending()) {
-    int result = GetStatus().status() == net::URLRequestStatus::SUCCESS
-                     ? bytes_read
-                     : GetStatus().error();
-    // If bytes_read is 0, HandleNetData synchronously completed and this job is
-    // at EOF.
-    if (bytes_read == 0)
-      NotifyDoneHelper(GetStatus(), std::string());
-    NotifyReadComplete(result);
-  }
+  ReadRawDataComplete(result);
 }
 
 bool ServiceWorkerWriteToCacheJob::CheckPathRestriction(
@@ -436,15 +411,15 @@ bool ServiceWorkerWriteToCacheJob::CheckPathRestriction(
   if (!ServiceWorkerUtils::IsPathRestrictionSatisfied(
           version_->scope(), url_,
           has_header ? &service_worker_allowed : nullptr, &error_message)) {
-    NotifyDoneHelper(net::URLRequestStatus(net::URLRequestStatus::FAILED,
-                                           net::ERR_INSECURE_RESPONSE),
-                     error_message);
+    NotifyStartErrorHelper(net::URLRequestStatus(net::URLRequestStatus::FAILED,
+                                                 net::ERR_INSECURE_RESPONSE),
+                           error_message);
     return false;
   }
   return true;
 }
 
-void ServiceWorkerWriteToCacheJob::NotifyDoneHelper(
+void ServiceWorkerWriteToCacheJob::NotifyStartErrorHelper(
     const net::URLRequestStatus& status,
     const std::string& status_message) {
   DCHECK(!status.is_io_pending());
@@ -465,7 +440,7 @@ void ServiceWorkerWriteToCacheJob::NotifyDoneHelper(
   }
 
   SetStatus(reported_status);
-  NotifyDone(reported_status);
+  NotifyStartError(reported_status);
 }
 
 void ServiceWorkerWriteToCacheJob::NotifyFinishedCaching(