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..0fe3fd8017df55a344e63639f74f7e6ac3e64253 100644
--- a/net/base/sdch_dictionary_fetcher.cc
+++ b/net/base/sdch_dictionary_fetcher.cc
@@ -4,25 +4,34 @@
 
 #include "net/base/sdch_dictionary_fetcher.h"
 
+#include <stdint.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),
+      manager_(manager),
+      context_(context),
+      weak_factory_(this) {
   DCHECK(CalledOnValidThread());
   DCHECK(manager);
+  DCHECK(context);
 }
 
 SdchDictionaryFetcher::~SdchDictionaryFetcher() {
@@ -47,59 +56,136 @@ void SdchDictionaryFetcher::Schedule(const GURL& dictionary_url) {
   }
   attempted_load_.insert(dictionary_url);
   fetch_queue_.push(dictionary_url);
-  ScheduleDelayedRun();
+
+  next_state_ = STATE_IDLE;
+  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) {
+  do {
+    if (rv != OK) {
+      HandleResult(rv);
+      rv = OK;
+    }

[Ryan Sleevi, 2014/09/02 20:04:18]

This doesn't seem right. Normally we pass the previous return value into the
call (DoDispatch/DoRead/DoComplete) and it can decide how to handle the result
for the previous state. This swallows all errors before the state transitions
are allowed to handle them.

[Randy Smith (Not in Mondays), 2014/09/02 23:34:32]

Done.

+
+    State state = next_state_;
+    next_state_ = STATE_NONE;
+    switch (state) {
+      case STATE_IDLE:
+        rv = DoDispatchRequest();
+        break;
+      case STATE_REQUEST_READING:
+        rv = DoRead();
+        break;
+      case STATE_REQUEST_COMPLETE:
+        rv = DoCompleteRequest();
+        break;
+      case STATE_NONE:
+        NOTREACHED();
+    }
+  } while (rv != ERR_IO_PENDING && next_state_ != STATE_NONE);
+
+  return rv;
 }
 
-void SdchDictionaryFetcher::StartFetching() {
+int SdchDictionaryFetcher::DoDispatchRequest() {
   DCHECK(CalledOnValidThread());
-  DCHECK(task_is_pending_);
-  task_is_pending_ = false;
 
-  // Handle losing the race against Cancel().
-  if (fetch_queue_.empty())
-    return;
+  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_READING;
+
+  current_request_->Start();
+
+  return ERR_IO_PENDING;                // TODO(rdsmith): Is this correct?

[Ryan Sleevi, 2014/09/02 20:04:18]

I guess that's the question for URLRequest::Delegate / URLFetcher::Delegate,
whether it's guaranteed to asynchronously call the Delegate. This was the issue
I was alluding to in the previous remark - that your safety here is contingent
upon how the delegate methods get invoked.

[Randy Smith (Not in Mondays), 2014/09/02 23:34:32]

I'm afraid I don't follow.  At least in the future, it seems like we should be
prepared at this point to handle either a synchronous or an asynchronous
starting.  If OnRequestStarted() was called synchronously i response to the
Start() call, I want to return from this routine with whatever status that call
was passed.  If it was called asynchronously, I want to return with
ERR_IO_PENDING.  I've made the assumption in the code I've modified this too
that current_request_->status().error() will be correct in either the
synchronous or the asynchronous case; is that correct?  If not, how should I
figure out whether or not the call was made synchronously or asynchronously?

 }
 
-void SdchDictionaryFetcher::OnURLFetchComplete(
-    const URLFetcher* source) {
+int SdchDictionaryFetcher::DoRead() {
   DCHECK(CalledOnValidThread());
-  if ((200 == source->GetResponseCode()) &&
-      (source->GetStatus().status() == URLRequestStatus::SUCCESS)) {
-    std::string data;
-    source->GetResponseAsString(&data);
-    manager_->AddSdchDictionary(data, source->GetURL());
+
+  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();
   }
-  current_fetch_.reset(NULL);
-  ScheduleDelayedRun();
+
+  if (bytes_read != 0)
+    dictionary_.append(buffer_->data(), bytes_read);
+  else
+    next_state_ = STATE_REQUEST_COMPLETE;
+
+  return OK;
+}
+
+int SdchDictionaryFetcher::DoCompleteRequest() {
+  DCHECK(CalledOnValidThread());
+
+  manager_->AddSdchDictionary(dictionary_, current_request_->url());
+  current_request_.reset();
+  buffer_ = NULL;
+  dictionary_.clear();
+
+  next_state_ = STATE_IDLE;
+
+  return OK;
+}
+
+void SdchDictionaryFetcher::HandleResult(int rv) {
+  DCHECK(CalledOnValidThread());
+
+  current_request_.reset();
+  buffer_ = NULL;
+  next_state_ = STATE_IDLE;
+  DoLoop(OK);
+}
+
+void SdchDictionaryFetcher::OnResponseStarted(URLRequest* request) {
+  DCHECK(CalledOnValidThread());
+  DCHECK_EQ(request, current_request_.get());
+
+  // TODO(rdsmith): Break potential infinite recursion.
+  DoLoop(OK);

[Ryan Sleevi, 2014/09/02 20:04:18]

Right, this has the same issue, in that it's contingent upon how the
URLRequest::Delegate invokes things.

[Randy Smith (Not in Mondays), 2014/09/02 23:34:32]

Yep, but again, I really don't think I should be coding assumptions about that
into this code if I can avoid it.

+}
+
+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);
+
+  DoLoop(request->status().error());
 }
 
 }  // namespace net