Naming and refactoring changes to SdchDictionaryFetcher.

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 450a60822353be756024a88b720b36ddd3ecf05a..1361de8d48433481e2841ee70418a90f8a801456 100644
--- a/net/url_request/sdch_dictionary_fetcher.cc
+++ b/net/url_request/sdch_dictionary_fetcher.cc
@@ -60,7 +60,7 @@ void SdchDictionaryFetcher::Schedule(const GURL& dictionary_url) {
   attempted_load_.insert(dictionary_url);
   fetch_queue_.push(dictionary_url);
 
-  next_state_ = STATE_IDLE;
+  next_state_ = STATE_SEND_REQUEST;
 
   // There are no callbacks to user code from the dictionary fetcher,
   // and Schedule() is only called from user code, so this call to DoLoop()
@@ -90,16 +90,8 @@ void SdchDictionaryFetcher::OnResponseStarted(URLRequest* request) {
 
   DCHECK(CalledOnValidThread());
   DCHECK_EQ(request, current_request_.get());
-  DCHECK_EQ(next_state_, STATE_REQUEST_STARTED);
-
-  // The response has started, so the stream can be read from.
-  next_state_ = STATE_REQUEST_READING;
-
-  // If this function was synchronously called, the containing
-  // state machine loop will handle the state transition. Otherwise,
-  // restart the state machine loop.
-  if (in_loop_)
-    return;
+  DCHECK_EQ(next_state_, STATE_SEND_REQUEST_COMPLETE);
+  DCHECK(!in_loop_);
 
   DoLoop(request->status().error());
 }
@@ -113,21 +105,26 @@ void SdchDictionaryFetcher::OnReadCompleted(URLRequest* request,
 
   DCHECK(CalledOnValidThread());
   DCHECK_EQ(request, current_request_.get());
-  DCHECK_EQ(next_state_, STATE_REQUEST_READING);
+  DCHECK_EQ(next_state_, STATE_READ_BODY_COMPLETE);
+  DCHECK(!in_loop_);
 
-  // No state transition is required in this function; the
-  // completion of the request is detected in DoRead().
+  DoLoop(GetReadResult(bytes_read));
+}
 
-  if (request->status().is_success())
-    dictionary_.append(buffer_->data(), bytes_read);
+int SdchDictionaryFetcher::GetReadResult(int bytes_read) {

[mmenke, 2015/01/26 20:14:25]

optional:  May be clearer as a static method that takes the request as an
argument (Seems kinda weird to max and match a local with an argument).

If you prefer it as-is, that's fine, just make it const.

[Randy Smith (Not in Mondays), 2015/01/26 22:39:54]

Done.

+  int error = current_request_->status().error();
+  if (current_request_->status().is_success() && bytes_read < 0) {
+    error = current_request_->status().status() == URLRequestStatus::CANCELED
+                ? ERR_ABORTED
+                : ERR_FAILED;

[mmenke, 2015/01/26 20:14:25]

This code is strange.  If current_request_->status().is_success(), the status
cannot be URLRequestStatus::CANCELED (Look at URLRequestStatus::is_success()). 
If this ever happens, should always be using ERR_FAILED...Or bytes_read,
actually, since if it's negative, it's presumably a net error, I guess?

[mmenke, 2015/01/26 20:25:44]

Oh...you're right...It can actually do that, with a sufficiently bizarre
UrlRequestJob.  If we're not using a filter, an a UrlRequestJob calls
URLRequestJob::NotifyReadSuccess(<negative number>), then we'll tell the
NetworkDelegate the request completed, but then pass the negative value on to
the URLRequest::Delegate.

I think the solution is to have a DCHECK for that, rather than to create new
NetLog events for that one particular case.

[Randy Smith (Not in Mondays), 2015/01/26 22:39:54]

Acknowledged.

[Randy Smith (Not in Mondays), 2015/01/26 22:39:54]

So if you want me to skip the netlog event I'm happy to do so (breadcrumbs from
previous debugging, but do ask if you want it gone), but we know that the
is_success() && bytes_read < 0 happens in the field (see notes on
http://crbug.com/416639) so if that's what you want to put in a DCHECK for, that
won't work.  If what you're asking for the DCHECK for is for status() !=
CANCELED, having looked at the implementation of URLRequestStatus::is_success,
I'm happy to just remove that (and have done so).

I'll note with some slight amusement that the check was originally put in at
your request (see https://codereview.chromium.org/723133003/#msg4), or, at
least, your pointer to AsyncResourceHandler faked me into thinking you were
requesting it :-}.

[mmenke, 2015/01/30 16:20:43]

I thought the issue was URLRequest::Read returning false in weird cases, not
status being SUCCESS with negative read sizes?
In my defense, neither of my suggestions used status.is_success() - I was
suggesting replacing the block entirely, and checking the cancelled and failed
statuses directly.  I also hadn't realized this was turning success cases into
errors, rather than turning failure cases that may have no error code set into
errors.

+    current_request_->net_log().AddEventWithNetErrorCode(
+        NetLog::TYPE_SDCH_DICTIONARY_FETCH_IMPLIED_ERROR, error);

[mmenke, 2015/01/26 20:14:25]

Shouldn't we be logging all errors, not just ones where the status is success? 
Note:  I still don't think this happens.

Also, seems like the logging should also be moved into the "rv < 0" case in
DoReadBodyComplete.

[Randy Smith (Not in Mondays), 2015/01/26 22:39:54]

This logging was breadcrumbs from trying to figure out http://crbug.com/416639,
and I'm happy to remove it if you'd like.  I think I'll still wait for a second
request, as I have the impression you don't have 416639 at top of mind--this
wasn't just any error, this was a weird, how the heck can this be happening
error that showed up fairly rarely in the field and not at all locally.

+  }
 
-  // If this function was synchronously called, the containing
-  // state machine loop will handle the state transition. Otherwise,
-  // restart the state machine loop.
-  if (in_loop_)
-    return;
+  if (error == OK)
+    error = bytes_read;

[mmenke, 2015/01/26 20:14:25]

"error = bytes_read" seems a little weird.  Maybe just "return bytes_read"?  Or
rename "error" to "result" or "rv".

[Randy Smith (Not in Mondays), 2015/01/26 22:39:54]

Done.

 
-  DoLoop(request->status().error());
+  return error;
 }
 
 int SdchDictionaryFetcher::DoLoop(int rv) {
@@ -138,14 +135,17 @@ int SdchDictionaryFetcher::DoLoop(int rv) {
     State state = next_state_;
     next_state_ = STATE_NONE;
     switch (state) {
-      case STATE_IDLE:
-        rv = DoDispatchRequest(rv);
+      case STATE_SEND_REQUEST:
+        rv = DoSendRequest(rv);
+        break;
+      case STATE_SEND_REQUEST_COMPLETE:
+        rv = DoSendRequestComplete(rv);
         break;
-      case STATE_REQUEST_STARTED:
-        rv = DoRequestStarted(rv);
+      case STATE_READ_BODY:
+        rv = DoReadBody(rv);
         break;
-      case STATE_REQUEST_READING:
-        rv = DoRead(rv);
+      case STATE_READ_BODY_COMPLETE:
+        rv = DoReadBodyComplete(rv);
         break;
       case STATE_REQUEST_COMPLETE:
         rv = DoCompleteRequest(rv);
@@ -158,7 +158,7 @@ int SdchDictionaryFetcher::DoLoop(int rv) {
   return rv;
 }
 
-int SdchDictionaryFetcher::DoDispatchRequest(int rv) {
+int SdchDictionaryFetcher::DoSendRequest(int rv) {
   DCHECK(CalledOnValidThread());
 
   // |rv| is ignored, as the result from the previous request doesn't
@@ -169,6 +169,8 @@ int SdchDictionaryFetcher::DoDispatchRequest(int rv) {
     return OK;
   }
 
+  next_state_ = STATE_SEND_REQUEST_COMPLETE;
+
   current_request_ =
       context_->CreateRequest(fetch_queue_.front(), IDLE, this, NULL);
   current_request_->SetLoadFlags(LOAD_DO_NOT_SEND_COOKIES |
@@ -176,64 +178,69 @@ int SdchDictionaryFetcher::DoDispatchRequest(int rv) {
   buffer_ = new IOBuffer(kBufferSize);
   fetch_queue_.pop();
 
-  next_state_ = STATE_REQUEST_STARTED;
   current_request_->Start();
   current_request_->net_log().AddEvent(NetLog::TYPE_SDCH_DICTIONARY_FETCH);
 
-  return OK;
+  return ERR_IO_PENDING;
 }
 
-int SdchDictionaryFetcher::DoRequestStarted(int rv) {
+int SdchDictionaryFetcher::DoSendRequestComplete(int rv) {
   DCHECK(CalledOnValidThread());
-  DCHECK_EQ(rv, OK);  // Can only come straight from above function.
-
-  // The transition to STATE_REQUEST_READING occurs in the
-  // OnResponseStarted() callback triggered by URLRequest::Start()
-  // (called in DoDispatchRequest(), above). If that callback did not
-  // occur synchronously, this routine is executed; it returns ERR_IO_PENDING,
-  // indicating to the controlling loop that no further work should be done
-  // until the callback occurs (which will re-invoke DoLoop()).
-  next_state_ = STATE_REQUEST_STARTED;
-  return ERR_IO_PENDING;
+
+  // If there's been an error, abort the current request.
+  if (rv != OK) {
+    current_request_.reset();
+    buffer_ = NULL;
+    next_state_ = STATE_SEND_REQUEST;
+
+    return OK;
+  }
+
+  next_state_ = STATE_READ_BODY;
+  return OK;
 }
 
-int SdchDictionaryFetcher::DoRead(int rv) {
+int SdchDictionaryFetcher::DoReadBody(int rv) {
   DCHECK(CalledOnValidThread());
 
   // If there's been an error, abort the current request.
   if (rv != OK) {
     current_request_.reset();
     buffer_ = NULL;
-    next_state_ = STATE_IDLE;
+    next_state_ = STATE_SEND_REQUEST;
 
     return OK;
   }
 
-  next_state_ = STATE_REQUEST_READING;
+  next_state_ = STATE_READ_BODY_COMPLETE;
   int bytes_read = 0;
   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()) {
-    if (current_request_->status().error() != OK)
-      return current_request_->status().error();
+  return GetReadResult(bytes_read);
+}
 
-    // An error with request status of OK should not happen,
-    // but there's enough machinery underneath URLRequest::Read()
-    // that this routine checks for that case.
-    net::Error error =
-        current_request_->status().status() == URLRequestStatus::CANCELED ?
-        ERR_ABORTED : ERR_FAILED;
-    current_request_->net_log().AddEventWithNetErrorCode(
-        NetLog::TYPE_SDCH_DICTIONARY_FETCH_IMPLIED_ERROR, error);
-    return error;
+int SdchDictionaryFetcher::DoReadBodyComplete(int rv) {
+  DCHECK(CalledOnValidThread());
+
+  // If there's been an error, abort the current request.
+  if (rv < 0) {
+    current_request_.reset();
+    buffer_ = NULL;
+    next_state_ = STATE_SEND_REQUEST;
+
+    return OK;
   }
 
-  if (bytes_read == 0)
+  DCHECK(current_request_->status().is_success());
+
+  if (rv > 0) {
+    dictionary_.append(buffer_->data(), rv);
+    next_state_ = STATE_READ_BODY;
+  } else if (rv == 0) {

[mmenke, 2015/01/26 20:14:25]

Should that be a DCHECK_EQ instead?  Up to you, but if we reach the else clause
and it's not 0, something's gone badly wrong.

[Randy Smith (Not in Mondays), 2015/01/26 22:39:54]

Looking at the logic, it was a bit too obvious to me that it had to be 0 (I'm
willing to trust that the union of the sets <0, ==0, and >0 encompasses all
possible integers), so I didn't bother with a DCHECK.  I did rewrite the logic
to use early returns and keep the state transitions as linear in the code as
possible.

     next_state_ = STATE_REQUEST_COMPLETE;
-  else
-    dictionary_.append(buffer_->data(), bytes_read);
+  }
 
   return OK;
 }
@@ -241,17 +248,17 @@ int SdchDictionaryFetcher::DoRead(int rv) {
 int SdchDictionaryFetcher::DoCompleteRequest(int rv) {
   DCHECK(CalledOnValidThread());
 
-  // If the dictionary was successfully fetched, add it to the manager.
-  if (rv == OK) {
-    dictionary_fetched_callback_.Run(dictionary_, current_request_->url(),
-                                     current_request_->net_log());
-  }
+  // DoReadBodyComplete() only transitions to this state
+  // on success.
+  DCHECK_EQ(OK, rv);
 
+  dictionary_fetched_callback_.Run(dictionary_, current_request_->url(),
+                                   current_request_->net_log());
   current_request_.reset();
   buffer_ = NULL;
   dictionary_.clear();
 
-  next_state_ = STATE_IDLE;
+  next_state_ = STATE_SEND_REQUEST;
 
   return OK;
 }