Naming and refactoring changes to SdchDictionaryFetcher.

net/url_request/sdch_dictionary_fetcher.cc

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "net/url_request/sdch_dictionary_fetcher.h"
 
 #include <stdint.h>
 
 #include "base/auto_reset.h"
 #include "base/bind.h"
@@ skipping 42 matching lines @@
     // TODO(rdsmith): log this error to the net log of the URLRequest
     // initiating this fetch, once URLRequest will be passed here.
     SdchManager::SdchErrorRecovery(
         SDCH_DICTIONARY_PREVIOUSLY_SCHEDULED_TO_DOWNLOAD);
     return;
   }
 
   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()
   // does not require an |if (in_loop_) return;| guard.
   DoLoop(OK);
 }
 
 void SdchDictionaryFetcher::Cancel() {
   DCHECK(CalledOnValidThread());
 
   next_state_ = STATE_NONE;
 
   while (!fetch_queue_.empty())
     fetch_queue_.pop();
   attempted_load_.clear();
   weak_factory_.InvalidateWeakPtrs();
   current_request_.reset(NULL);
   buffer_ = NULL;
   dictionary_.clear();
 }
 
 void SdchDictionaryFetcher::OnResponseStarted(URLRequest* request) {
   // TODO(vadimt): Remove ScopedTracker below once crbug.com/423948 is fixed.
   tracked_objects::ScopedTracker tracking_profile(
       FROM_HERE_WITH_EXPLICIT_FUNCTION(
           "423948 SdchDictionaryFetcher::OnResponseStarted"));
 
   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());
 }
 
 void SdchDictionaryFetcher::OnReadCompleted(URLRequest* request,
                                             int bytes_read) {
   // TODO(vadimt): Remove ScopedTracker below once crbug.com/423948 is fixed.
   tracked_objects::ScopedTracker tracking_profile(
       FROM_HERE_WITH_EXPLICIT_FUNCTION(
           "423948 SdchDictionaryFetcher::OnReadCompleted"));
 
   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) {
   DCHECK(!in_loop_);
   base::AutoReset<bool> auto_reset_in_loop(&in_loop_, true);
 
   do {
     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_REQUEST_STARTED:
-        rv = DoRequestStarted(rv);
+      case STATE_SEND_REQUEST_COMPLETE:
+        rv = DoSendRequestComplete(rv);
         break;
-      case STATE_REQUEST_READING:
-        rv = DoRead(rv);
+      case STATE_READ_BODY:
+        rv = DoReadBody(rv);
+        break;
+      case STATE_READ_BODY_COMPLETE:
+        rv = DoReadBodyComplete(rv);
         break;
       case STATE_REQUEST_COMPLETE:
         rv = DoCompleteRequest(rv);
         break;
       case STATE_NONE:
         NOTREACHED();
     }
   } while (rv != ERR_IO_PENDING && next_state_ != STATE_NONE);
 
   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
   // affect the next request.
 
   if (fetch_queue_.empty() || current_request_.get()) {
     next_state_ = STATE_NONE;
     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 |
                                  LOAD_DO_NOT_SAVE_COOKIES);
   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;
-}
-
-int SdchDictionaryFetcher::DoRequestStarted(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;
 }
 
-int SdchDictionaryFetcher::DoRead(int rv) {
+int SdchDictionaryFetcher::DoSendRequestComplete(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;
+  return OK;
+}
+
+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_SEND_REQUEST;
+
+    return OK;
+  }
+
+  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;
 }
 
 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;
 }
 
 }  // namespace net