Shift URLRequest::Read API contract used by fetcher to ResourceLoader's.

net/url_request/sdch_dictionary_fetcher.cc

Index: net/url_request/sdch_dictionary_fetcher.cc
diff --git a/net/url_request/sdch_dictionary_fetcher.cc b/net/url_request/sdch_dictionary_fetcher.cc
index 580619312c8ef646ef9d469b4fccdb1691c60dd6..1c86b8ffb852ddb9987a3b627052a1a4343ae7b2 100644
--- a/net/url_request/sdch_dictionary_fetcher.cc
+++ b/net/url_request/sdch_dictionary_fetcher.cc
@@ -198,25 +198,14 @@ int SdchDictionaryFetcher::DoRead(int rv) {
 
   next_state_ = STATE_REQUEST_READING;
   int bytes_read = 0;
-  if (!current_request_->Read(buffer_.get(), kBufferSize, &bytes_read)) {
-    if (current_request_->status().is_io_pending())
-      return ERR_IO_PENDING;
-
-    if (current_request_->status().error() == OK) {
-      // This "should never happen", but if it does the result will be
-      // an infinite loop.  It's not clear how to handle a read failure
-      // without a promise to invoke the callback at some point in the future,
-      // so the request is failed.
-      SdchManager::SdchErrorRecovery(SdchManager::DICTIONARY_FETCH_READ_FAILED);
-      DLOG(FATAL)
-          << "URLRequest::Read() returned false without IO pending or error!";
-      return ERR_FAILED;
-    }
+  current_request_->Read(buffer_.get(), kBufferSize, &bytes_read);
+  if (current_request_->status().is_io_pending())
+    return ERR_IO_PENDING;
 
+  if (bytes_read < 0 || !current_request_->status().is_success())
     return current_request_->status().error();

[mmenke, 2014/11/13 21:55:05]

Hrm...  if it's possible for is_success() is true, but bytes_read  to be less
than 0, I don't think error() will have a meaningful value.

[mmenke, 2014/11/13 21:55:05]

Should have a comment by the old enum that it's obsolete, or comment it out with
a comment, or something.

[Randy Smith (Not in Mondays), 2014/11/13 22:05:21]

Whoops, sorry, updated histograms.xml, but not the enum.  

[Randy Smith (Not in Mondays), 2014/11/13 22:05:22]

It's a fair concern, but ResourceLoader has the same problem:

void ResourceLoader::OnReadCompleted(net::URLRequest* unused, int bytes_read) {
  DCHECK_EQ(request_.get(), unused);
  VLOG(1) << "OnReadCompleted: \"" << request_->url().spec() << "\""
          << " bytes_read = " << bytes_read;

  // bytes_read == -1 always implies an error.
  if (bytes_read == -1 || !request_->status().is_success()) {
    ResponseCompleted();
    return;
  }

I'm not sure how to fix it, either.  Set bytes_read to zero if is_success() is
true?

[mmenke, 2014/11/13 22:23:43]

I suggest we channel AsyncResourceHandler here
(https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/lo...)

  int error_code = status.error();
  if (status.status() == net::URLRequestStatus::CANCELED &&
      error_code == net::OK) {
    error_code = net::ERR_ABORTED;
  } else if (status.status() == net::URLRequestStatus::FAILED &&
             error_code == net::OK) {
    error_code = net::ERR_FAILED;
  }

Or perhaps just:

  if (status.error() == net::OK) {
    if (status.status() == net::URLRequestStatus::CANCELED)
      return ERR_ABORTED;
    return ERR_URL_REQUEST_IS_WEIRD;  // Also known is ERR_FAILED
  }
  return status.error();

[Randy Smith (Not in Mondays), 2014/11/15 21:25:30]

Sounds good.  I've tried to duplicate that logic's effect in a somewhat clearer
flow for this routine; let me know what you think.

-  }
 
-  if (bytes_read != 0)
+  if (bytes_read > 0)
     dictionary_.append(buffer_->data(), bytes_read);
   else
     next_state_ = STATE_REQUEST_COMPLETE;