Change SDCHDictionaryFetcher to use URLRequest instead of URLFetcher.

net/base/sdch_dictionary_fetcher.cc

Index: net/base/sdch_dictionary_fetcher.cc
diff --git a/net/base/sdch_dictionary_fetcher.cc b/net/base/sdch_dictionary_fetcher.cc
index f0a417e925954eebcd972793f9c72fccbe50d8bd..5974be9be7046ef6cf2c8c70408ef1884a90da38 100644
--- a/net/base/sdch_dictionary_fetcher.cc
+++ b/net/base/sdch_dictionary_fetcher.cc
@@ -4,25 +4,36 @@
 
 #include "net/base/sdch_dictionary_fetcher.h"
 
+#include <stdint.h>
+
+#include "base/auto_reset.h"
 #include "base/bind.h"
 #include "base/compiler_specific.h"
-#include "base/message_loop/message_loop.h"
+#include "base/thread_task_runner_handle.h"
 #include "net/base/load_flags.h"
-#include "net/url_request/url_fetcher.h"
-#include "net/url_request/url_request_context_getter.h"
+#include "net/url_request/url_request_context.h"
 #include "net/url_request/url_request_status.h"
+#include "net/url_request/url_request_throttler_manager.h"
+
+namespace {
+
+const int kBufferSize = 4096;
+
+}  // namespace
 
 namespace net {
 
 SdchDictionaryFetcher::SdchDictionaryFetcher(
     SdchManager* manager,
-    scoped_refptr<URLRequestContextGetter> context)
-    : manager_(manager),
-      weak_factory_(this),
-      task_is_pending_(false),
-      context_(context) {
+    URLRequestContext* context)
+    : next_state_(STATE_NONE),
+      in_loop_(false),
+      manager_(manager),
+      context_(context),
+      weak_factory_(this) {
   DCHECK(CalledOnValidThread());
   DCHECK(manager);
+  DCHECK(context);
 }
 
 SdchDictionaryFetcher::~SdchDictionaryFetcher() {
@@ -47,59 +58,164 @@ void SdchDictionaryFetcher::Schedule(const GURL& dictionary_url) {
   }
   attempted_load_.insert(dictionary_url);
   fetch_queue_.push(dictionary_url);
-  ScheduleDelayedRun();
+
+  next_state_ = STATE_IDLE;
+
+  // 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_fetch_.reset(NULL);
+  current_request_.reset(NULL);
+  buffer_ = NULL;
+  dictionary_.clear();
 }
 
-void SdchDictionaryFetcher::ScheduleDelayedRun() {
-  if (fetch_queue_.empty() || current_fetch_.get() || task_is_pending_)
-    return;
-  base::MessageLoop::current()->PostDelayedTask(FROM_HERE,
-      base::Bind(&SdchDictionaryFetcher::StartFetching,
-                 weak_factory_.GetWeakPtr()),
-      base::TimeDelta::FromMilliseconds(kMsDelayFromRequestTillDownload));
-  task_is_pending_ = true;
+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);
+        break;
+      case STATE_REQUEST_STARTED:
+        rv = DoRequestStarted(rv);
+        break;
+      case STATE_REQUEST_READING:
+        rv = DoRead(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;
 }
 
-void SdchDictionaryFetcher::StartFetching() {
+int SdchDictionaryFetcher::DoDispatchRequest(int rv) {
   DCHECK(CalledOnValidThread());
-  DCHECK(task_is_pending_);
-  task_is_pending_ = false;
 
-  // Handle losing the race against Cancel().
-  if (fetch_queue_.empty())
-    return;
+  // Ignoring rv, as we don't really care what happened to the last
+  // request on initial dispatch.

[Ryan Sleevi, 2014/09/04 01:14:21]

Avoid "we" in comments when possible (see
https://groups.google.com/a/chromium.org/d/topic/chromium-dev/NH-S6KCkr2M/dis...
)

"|rv| is ignored, as the result from the previous request doesn't affect the
next request."

or something

[Randy Smith (Not in Mondays), 2014/09/04 18:52:57]

Right, sorry, still training myself out of that habit and I sometimes slip.
Done.

+
+  if (fetch_queue_.empty() || current_request_.get()) {
+    next_state_ = STATE_NONE;
+    return OK;
+  }
 
-  DCHECK(context_.get());
-  current_fetch_.reset(URLFetcher::Create(
-      fetch_queue_.front(), URLFetcher::GET, this));
+  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();
-  current_fetch_->SetRequestContext(context_.get());
-  current_fetch_->SetLoadFlags(LOAD_DO_NOT_SEND_COOKIES |
-                               LOAD_DO_NOT_SAVE_COOKIES);
-  current_fetch_->Start();
+
+  next_state_ = STATE_REQUEST_STARTED;
+  current_request_->Start();
+
+  return OK;
+}
+
+int SdchDictionaryFetcher::DoRequestStarted(int rv) {
+  DCHECK(CalledOnValidThread());
+  DCHECK_EQ(rv, OK);  // Can only come straight from above function.
+
+  next_state_ = STATE_REQUEST_STARTED;
+
+  // We're waiting to be gotten back to through OnResponseStarted().

[Ryan Sleevi, 2014/09/04 01:14:21]

Avoid "we" in comments

// While this seems like it would cause an infinite loop in the
STATE_REQUEST_STARTED phase,
// it's the responsibility of OnResponseStarted() to advance to the next state.
This can either
// happen synchronously (when Start() is called in DoDispatchRequest), in which
case, this
// code will never be reached, or it may be called asynchronously. In the
asynchronous case,
// it will update next_state_ and the loop will be restarted, so return
ERR_IO_PENDING.


Or something that explains the subtlety we talked about here, since it's
non-obvious without that context.

[Randy Smith (Not in Mondays), 2014/09/04 18:52:57]

I did some text that made sense to me (same meaning as what you have).  

+  return ERR_IO_PENDING;
 }
 
-void SdchDictionaryFetcher::OnURLFetchComplete(
-    const URLFetcher* source) {
+int SdchDictionaryFetcher::DoRead(int rv) {
   DCHECK(CalledOnValidThread());
-  if ((200 == source->GetResponseCode()) &&
-      (source->GetStatus().status() == URLRequestStatus::SUCCESS)) {
-    std::string data;
-    source->GetResponseAsString(&data);
-    manager_->AddSdchDictionary(data, source->GetURL());
+
+  // If there's been an error, abort the current request.
+  if (rv != OK) {
+    current_request_.reset();
+    buffer_ = NULL;
+    next_state_ = STATE_IDLE;
+
+    return OK;
   }
-  current_fetch_.reset(NULL);
-  ScheduleDelayedRun();
+
+  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;
+
+    DCHECK_NE(current_request_->status().error(), OK);
+
+    return current_request_->status().error();
+  }
+
+  if (bytes_read != 0)
+    dictionary_.append(buffer_->data(), bytes_read);
+  else
+    next_state_ = STATE_REQUEST_COMPLETE;
+
+  return OK;
+}
+
+int SdchDictionaryFetcher::DoCompleteRequest(int rv) {
+  DCHECK(CalledOnValidThread());
+
+  // If we haven't had an error, add the dictionary.

[Ryan Sleevi, 2014/09/04 01:14:21]

// If the dictionary was successfully fetched, add it to the list of available
dictionaries.

[Randy Smith (Not in Mondays), 2014/09/04 18:52:57]

Done.

+  if (rv == OK)
+    manager_->AddSdchDictionary(dictionary_, current_request_->url());
+
+  current_request_.reset();
+  buffer_ = NULL;
+  dictionary_.clear();
+
+  next_state_ = STATE_IDLE;
+
+  return OK;
+}
+
+void SdchDictionaryFetcher::OnResponseStarted(URLRequest* request) {
+  DCHECK(CalledOnValidThread());
+  DCHECK_EQ(request, current_request_.get());
+  DCHECK_EQ(next_state_, STATE_REQUEST_STARTED);
+
+  next_state_ = STATE_REQUEST_READING;
+
+  if (in_loop_)

[Ryan Sleevi, 2014/09/04 01:14:21]

// If OnResponseStarted was synchronously called, then DoLoop is still running.
In this case,
// just advance to the next state.
// Otherwise, if it was called asynchronously by the URLRequest, then restart
the state machine loop.

[Randy Smith (Not in Mondays), 2014/09/04 18:52:57]

If you ask me again, I'll do this, but it feels like overkill to me.  The
concept of advancing states had better be pretty clear to anyone reading this
code; if it isn't, then I don't think a random comment here will help (I could
do a top of file comment explaining state machines?).  I'm happy to put in
something explaining the loop guard.  

[Ryan Sleevi, 2014/09/04 19:05:30]

I'm specifically trying to explain the subtlety of why you're short-circuiting,
and how it relates to the overall operation of the loop.

The goal is for both the comments here, DoRequestStarted, and OnReadCompleted to
point to each other and provide a complete narrative of the subtlety, of which
there is some, precisely because of URLRequest's sync/async weirdness.

+    return;
+
+  DoLoop(request->status().error());
+}
+
+void SdchDictionaryFetcher::OnReadCompleted(URLRequest* request,
+                                            int bytes_read) {
+  DCHECK(CalledOnValidThread());
+  DCHECK_EQ(request, current_request_.get());
+  DCHECK_EQ(next_state_, STATE_REQUEST_READING);
+
+  if (request->status().is_success())
+    dictionary_.append(buffer_->data(), bytes_read);
+
+  if (in_loop_)
+    return;

[Ryan Sleevi, 2014/09/04 01:14:21]

ditto comments above

Why is next_state_ not needed to be updated here?

[Randy Smith (Not in Mondays), 2014/09/04 18:52:57]

ditto response above.
I'm afraid that in digging to answer your question, I've come up with a question
for you.  A close reading of url_request.h says to me that a call to Start()
always results in an OnResponseStarted() callback, but a call to Read() only
results in an OnReadCompleted() callback if the read is asynchronous.  Does that
match your understanding?  It surprised me a little when I engaged with your
question.  

Having said that, I think the answer is the same either way.  All Do*() methods
are required to specify their next state, because of the next_state_ =
STATE_NONE; at the beginning of the loop.  Callbacks only need to specify states
when they're transitioning, and reads completing don't transition states.  

If OnReadCompleted() was guaranteed to be called, I might shift the checking for
EOF and transition to STATE_REQUEST_COMPLETE into it (invariant: state
transitions occur at the end of an operation, including if it's asynchronous)
but given that it isn't guaranteed to be called, I'm disinclined to duplicate
the EOF checking; let it happen on the next round around the guitar.

Of course, now I start engaging with what comments are necessary, and if there's
any confusion about the fact that the state transition is occurring in one
On*Completed() method but not in the other (which there obviously is, given your
question :-}).   I've tossed a note in OnReadCompleted() about it, but I'd also
be willing to refactor so that the transition *is* in OnReadCompleted() and I
call OnReadCompleted() from DoRead() in the sync case.  If I do that, the price
will be a snarky comment about the inconsistency of URLRequest's interface :-}.

[Ryan Sleevi, 2014/09/04 19:05:30]

I don't know. mmenke would/should.
Reasonable reply :)
Indeed, I was wanting to call attention to the inconsistency, since it grabbed
my attention, and encourage you to document if appropriate.

[mmenke, 2014/09/04 19:34:23]

Yes, Start doesn't have a return value, so always returns the response via a
callback.

I'm not sure if it's possible for OnResponseStarted to be called synchronously /
re-entrantly, during the call to Start().  It's not possible for
HttpNetworkTransactions, or GetAllCookiesForURLAsync, to return synchronously,
but I don't see code in URLRequest / URLRequestJob that ensures this.  So
non-HTTP/HTTPS requests, or requests that don't go through the cookie store
(Because of load flags, or lack of a cookie store) and are served out of the
in-memory cache may theoretically be able to do that.

+
+  DoLoop(request->status().error());
 }
 
 }  // namespace net