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..ef055f90dc551a8e647d8b5865420d2942f10999 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,29 @@ 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(HandleReadCompletion(bytes_read));
+}
 
-  if (request->status().is_success())
+int SdchDictionaryFetcher::HandleReadCompletion(int bytes_read) {

[mmenke, 2015/01/26 16:40:12]

I suggest moving the append and state transition into DoReadBodyComplete, and
instead just make this map a bytes_read and status() to a single result code.

Something like:

// static
GetReadResult(bytes_read, request) {
  if (!request->status().is_success()) {
    if (request->status().status() == URLReuqestStatus::CANCELED)
      return ERR_ABORTED;
    if (request->status().status() == URLReuqestStatus::FAILED)
      return request->status().error();
    // Can't currently happen.
    return ERR_FAILED;
  }
  return bytes_read;
}

That puts most of the logic in DoReadComplete.  One day, I'd really like to make
it so that Read() returns the equivalent of net error code, rather than having 3
places one has to check to reassemble the original net error code...And then we
can completely get rid of the method.

[Randy Smith (Not in Mondays), 2015/01/26 19:52:21]

Done.

+  int error = current_request_->status().error();
+  if (current_request_->status().is_success() && bytes_read < 0) {

[mmenke, 2015/01/26 16:40:12]

Can this even happen?  

[Randy Smith (Not in Mondays), 2015/01/26 19:52:23]

That's a fascinating question.  I thought the answer was "yes", but when I went
over my notes the actual answer was "The behavior of URLRequest depends on the
URLRequestJob implementation (of which there are many), this is the way that
ResourceLoader is written, and I'm reluctant to deviate from that as we don't
have any guarantee that behavior other than what ResourceLoader expects is
actually held to by all URLRequestJob implementations."  I'll also note that
this is what the old code did; it's not new in this CL.

https://code.google.com/p/chromium/issues/detail?id=423484 in case you want a
pointer to the URLRequest::Read interface specification and regularization bug
(ah, good intentions :-{).

+    error = current_request_->status().status() == URLRequestStatus::CANCELED
+                ? ERR_ABORTED
+                : ERR_FAILED;
+    current_request_->net_log().AddEventWithNetErrorCode(
+        NetLog::TYPE_SDCH_DICTIONARY_FETCH_IMPLIED_ERROR, error);
+  }
+
+  if (error == OK && bytes_read > 0)
     dictionary_.append(buffer_->data(), bytes_read);
 
-  // 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 && bytes_read == 0)
+    next_state_ = STATE_REQUEST_COMPLETE;
 
-  DoLoop(request->status().error());
+  return error;
 }
 
 int SdchDictionaryFetcher::DoLoop(int rv) {
@@ -138,14 +138,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 +161,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 +172,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 +181,64 @@ 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 HandleReadCompletion(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 != OK) {
+    current_request_.reset();
+    buffer_ = NULL;
+    next_state_ = STATE_SEND_REQUEST;
+
+    return OK;
   }
 
-  if (bytes_read == 0)
-    next_state_ = STATE_REQUEST_COMPLETE;
-  else
-    dictionary_.append(buffer_->data(), bytes_read);
+  DCHECK(current_request_->status().is_success());
+
+  next_state_ = STATE_READ_BODY;
 
   return OK;
 }
@@ -241,17 +246,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());
-  }
+  // HandleReadCompletion() 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;
 }