Use modified URLRequest::Read() and delegate methods in /appcache/

content/browser/appcache/appcache_update_job.cc

Index: content/browser/appcache/appcache_update_job.cc
diff --git a/content/browser/appcache/appcache_update_job.cc b/content/browser/appcache/appcache_update_job.cc
index d612dc0add91cc4b9157917a755428cd6034da66..080bbeecabb97869834f6ae6842859ab94dd30e5 100644
--- a/content/browser/appcache/appcache_update_job.cc
+++ b/content/browser/appcache/appcache_update_job.cc
@@ -184,16 +184,18 @@ void AppCacheUpdateJob::URLFetcher::OnReceivedRedirect(
   // Redirect is not allowed by the update process.
   job_->MadeProgress();
   redirect_response_code_ = request->GetResponseCode();
-  request->Cancel();
+  int result = request->Cancel();
   result_ = REDIRECT_ERROR;
-  OnResponseCompleted();
+  OnResponseCompleted(result);

[michaeln, 2016/09/07 20:49:39]

I think i'd prefer to see net::ERR_ABORTED explicitly here, i just spent a
couple minutes verifying that OK was not an expected return value.

[maksims (do not use this acc), 2016/09/12 12:45:58]

I can use net::ERR_ABORTED here, but if there were any other errors set for some
reason, net::ERR_ABORTED would not be set as an error. I don't know how often it
might happen, but there is a probability of this. 

[michaeln, 2016/09/12 20:14:02]

Thnx

This class is explicitly choosing to abort the request for this non network
error condition. We're processing some kind of redirect response, so i don't
think the request can be in an error state, otherwise, we'd be getting an
OnResponseCompleted callback with the error instead. Even if it is, this class
is explicitly choosing to cancel for other reasons that have nothing todo with
any existing error.

 }
 
-void AppCacheUpdateJob::URLFetcher::OnResponseStarted(
-    net::URLRequest *request) {
+void AppCacheUpdateJob::URLFetcher::OnResponseStarted(net::URLRequest* request,
+                                                      int net_error) {
   DCHECK_EQ(request_.get(), request);
+  DCHECK_NE(net::ERR_IO_PENDING, net_error);
+
   int response_code = -1;
-  if (request->status().is_success()) {
+  if (net_error == net::OK) {
     response_code = request->GetResponseCode();
     job_->MadeProgress();
   }
@@ -203,7 +205,7 @@ void AppCacheUpdateJob::URLFetcher::OnResponseStarted(
       result_ = SERVER_ERROR;
     else
       result_ = NETWORK_ERROR;
-    OnResponseCompleted();
+    OnResponseCompleted(net_error);
     return;
   }
 
@@ -226,9 +228,9 @@ void AppCacheUpdateJob::URLFetcher::OnResponseStarted(
             request->response_headers()->
                 HasHeaderValue("cache-control", "no-store"))) {
       DCHECK_EQ(-1, redirect_response_code_);
-      request->Cancel();
+      int result = request->Cancel();
       result_ = SECURITY_ERROR;
-      OnResponseCompleted();
+      OnResponseCompleted(result);

[michaeln, 2016/09/07 20:49:39]

ditto preference for net::ERR_ABORTED

[maksims (do not use this acc), 2016/09/12 12:45:58]

Done.

       return;
     }
   }
@@ -252,25 +254,25 @@ void AppCacheUpdateJob::URLFetcher::OnReadCompleted(
     net::URLRequest* request, int bytes_read) {

[michaeln, 2016/09/07 20:49:39]

maybe DCHECK_NE(net::ERR_IO_PENDING, bytes_read) for more consistency with other
overrides of this method in other classes (we'd have to change the callsite too)

[maksims (do not use this acc), 2016/09/12 12:45:58]

Done.

   DCHECK_EQ(request_.get(), request);
   bool data_consumed = true;
-  if (request->status().is_success() && bytes_read > 0) {
+  if (bytes_read > 0) {
     job_->MadeProgress();
     data_consumed = ConsumeResponseData(bytes_read);
     if (data_consumed) {
       bytes_read = 0;
-      while (request->Read(buffer_.get(), kBufferSize, &bytes_read)) {
+      while (bytes_read > 0) {

[michaeln, 2016/09/07 20:49:39]

somethings off here, the body of the while loop when never be entered. did you
mean to delete line 261? or maybe just make this while (true) break more
explicitly on the two conditions

while (true) {
  bytes_read = requst->Read();
  if (bytes_read <= 0)
    break;
  data_consumed = ConsumeResponseData();
  if (!data_consumed)
    break;
}

[maksims (do not use this acc), 2016/09/12 12:45:58]

Yes, I've forgotten to remove that line. Thanks!

+        bytes_read = request->Read(buffer_.get(), kBufferSize);
         if (bytes_read > 0) {
           data_consumed = ConsumeResponseData(bytes_read);
           if (!data_consumed)
             break;  // wait for async data processing, then read more
-        } else {
-          break;
         }
       }
     }
   }
-  if (data_consumed && !request->status().is_io_pending()) {
+
+  if (data_consumed && bytes_read != net::ERR_IO_PENDING) {
     DCHECK_EQ(UPDATE_OK, result_);
-    OnResponseCompleted();
+    OnResponseCompleted(bytes_read);
   }
 }
 
@@ -303,9 +305,9 @@ void AppCacheUpdateJob::URLFetcher::AddConditionalHeaders(
 
 void  AppCacheUpdateJob::URLFetcher::OnWriteComplete(int result) {
   if (result < 0) {
-    request_->Cancel();
+    int result = request_->Cancel();
     result_ = DISKCACHE_ERROR;
-    OnResponseCompleted();
+    OnResponseCompleted(result);

[michaeln, 2016/09/07 20:49:39]

ditto pref for net::ERR_ABORTED

[maksims (do not use this acc), 2016/09/12 12:45:58]

Done.

     return;
   }
   ReadResponseData();
@@ -315,8 +317,7 @@ void AppCacheUpdateJob::URLFetcher::ReadResponseData() {
   InternalUpdateState state = job_->internal_state_;
   if (state == CACHE_FAILURE || state == CANCELLED || state == COMPLETED)
     return;
-  int bytes_read = 0;
-  request_->Read(buffer_.get(), kBufferSize, &bytes_read);
+  int bytes_read = request_->Read(buffer_.get(), kBufferSize);
   OnReadCompleted(request_.get(), bytes_read);

[michaeln, 2016/09/07 20:49:39]

maybe avoid calling this if byte_read == IO_PENDING, it'd be more in keeping
wither other impls of this method to be able to make that assumption/assertion

[maksims (do not use this acc), 2016/09/12 12:45:58]

Yes, sure. You're right. But as I noticed not all of the other impls have this
kind of logic. They don't call OnReadCompleted by themselves! But good point.
I'll take it into consideration. Thanks!

 }
 
@@ -344,29 +345,28 @@ bool AppCacheUpdateJob::URLFetcher::ConsumeResponseData(int bytes_read) {
   return true;
 }
 
-void AppCacheUpdateJob::URLFetcher::OnResponseCompleted() {
-  if (request_->status().is_success())
+void AppCacheUpdateJob::URLFetcher::OnResponseCompleted(int net_error) {
+  if (net_error == net::OK)
     job_->MadeProgress();
 
   // Retry for 503s where retry-after is 0.
-  if (request_->status().is_success() &&
-      request_->GetResponseCode() == 503 &&
+  if (net_error == net::OK && request_->GetResponseCode() == 503 &&
       MaybeRetryRequest()) {
     return;
   }
 
   switch (fetch_type_) {
     case MANIFEST_FETCH:
-      job_->HandleManifestFetchCompleted(this);
+      job_->HandleManifestFetchCompleted(this, net_error);
       break;
     case URL_FETCH:
-      job_->HandleUrlFetchCompleted(this);
+      job_->HandleUrlFetchCompleted(this, net_error);
       break;
     case MASTER_ENTRY_FETCH:
-      job_->HandleMasterEntryFetchCompleted(this);
+      job_->HandleMasterEntryFetchCompleted(this, net_error);
       break;
     case MANIFEST_REFETCH:
-      job_->HandleManifestRefetchCompleted(this);
+      job_->HandleManifestRefetchCompleted(this, net_error);
       break;
     default:
       NOTREACHED();
@@ -579,17 +579,17 @@ void AppCacheUpdateJob::FetchManifest(bool is_first_fetch) {
   manifest_fetcher_->Start();
 }
 
-
-void AppCacheUpdateJob::HandleManifestFetchCompleted(
-    URLFetcher* fetcher) {
+void AppCacheUpdateJob::HandleManifestFetchCompleted(URLFetcher* fetcher,
+                                                     int net_error) {
   DCHECK_EQ(internal_state_, FETCH_MANIFEST);
   DCHECK_EQ(manifest_fetcher_, fetcher);
+
   manifest_fetcher_ = NULL;
 
   net::URLRequest* request = fetcher->request();
   int response_code = -1;
   bool is_valid_response_code = false;
-  if (request->status().is_success()) {
+  if (net_error == net::OK) {
     response_code = request->GetResponseCode();
     is_valid_response_code = (response_code / 100 == 2);
 
@@ -717,7 +717,8 @@ void AppCacheUpdateJob::ContinueHandleManifestFetchCompleted(bool changed) {
   MaybeCompleteUpdate();  // if not done, continues when async fetches complete
 }
 
-void AppCacheUpdateJob::HandleUrlFetchCompleted(URLFetcher* fetcher) {
+void AppCacheUpdateJob::HandleUrlFetchCompleted(URLFetcher* fetcher,
+                                                int net_error) {
   DCHECK(internal_state_ == DOWNLOADING);
 
   net::URLRequest* request = fetcher->request();
@@ -726,9 +727,8 @@ void AppCacheUpdateJob::HandleUrlFetchCompleted(URLFetcher* fetcher) {
   NotifyAllProgress(url);
   ++url_fetches_completed_;
 
-  int response_code = request->status().is_success()
-                          ? request->GetResponseCode()
-                          : fetcher->redirect_response_code();
+  int response_code = net_error == net::OK ? request->GetResponseCode()
+                                           : fetcher->redirect_response_code();
 
   AppCacheEntry& entry = url_file_list_.find(url)->second;
 
@@ -751,8 +751,7 @@ void AppCacheUpdateJob::HandleUrlFetchCompleted(URLFetcher* fetcher) {
     // whose value doesn't match the manifest url of the application cache
     // being processed, mark the entry as being foreign.
   } else {
-    VLOG(1) << "Request status: " << request->status().status()
-            << " error: " << request->status().error()
+    VLOG(1) << "Request error: " << net_error
             << " response code: " << response_code;
     if (entry.IsExplicit() || entry.IsFallback() || entry.IsIntercept()) {
       if (response_code == 304 && fetcher->existing_entry().has_response_id()) {
@@ -814,8 +813,8 @@ void AppCacheUpdateJob::HandleUrlFetchCompleted(URLFetcher* fetcher) {
   MaybeCompleteUpdate();
 }
 
-void AppCacheUpdateJob::HandleMasterEntryFetchCompleted(
-    URLFetcher* fetcher) {
+void AppCacheUpdateJob::HandleMasterEntryFetchCompleted(URLFetcher* fetcher,
+                                                        int net_error) {
   DCHECK(internal_state_ == NO_UPDATE || internal_state_ == DOWNLOADING);
 
   // TODO(jennb): Handle downloads completing during cache failure when update
@@ -828,8 +827,7 @@ void AppCacheUpdateJob::HandleMasterEntryFetchCompleted(
   master_entry_fetches_.erase(url);
   ++master_entries_completed_;
 
-  int response_code = request->status().is_success()
-      ? request->GetResponseCode() : -1;
+  int response_code = net_error == net::OK ? request->GetResponseCode() : -1;
 
   PendingMasters::iterator found = pending_master_entries_.find(url);
   DCHECK(found != pending_master_entries_.end());
@@ -911,15 +909,14 @@ void AppCacheUpdateJob::HandleMasterEntryFetchCompleted(
   MaybeCompleteUpdate();
 }
 
-void AppCacheUpdateJob::HandleManifestRefetchCompleted(
-    URLFetcher* fetcher) {
+void AppCacheUpdateJob::HandleManifestRefetchCompleted(URLFetcher* fetcher,
+                                                       int net_error) {
   DCHECK(internal_state_ == REFETCH_MANIFEST);
   DCHECK(manifest_fetcher_ == fetcher);
   manifest_fetcher_ = NULL;
 
-  net::URLRequest* request = fetcher->request();
-  int response_code = request->status().is_success()
-      ? request->GetResponseCode() : -1;
+  int response_code =
+      net_error == net::OK ? fetcher->request()->GetResponseCode() : -1;
   if (response_code == 304 || manifest_data_ == fetcher->manifest_data()) {
     // Only need to store response in storage if manifest is not already
     // an entry in the cache.
@@ -937,8 +934,7 @@ void AppCacheUpdateJob::HandleManifestRefetchCompleted(
                      base::Unretained(this)));
     }
   } else {
-    VLOG(1) << "Request status: " << request->status().status()
-            << " error: " << request->status().error()
+    VLOG(1) << "Request error: " << net_error
             << " response code: " << response_code;
     ScheduleUpdateRetry(kRerunDelayMs);
     if (response_code == 200) {