Change SDCHDictionaryFetcher to use URLRequest instead of URLFetcher.

net/base/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/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_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) {
+      context_(context),
+      weak_factory_(this) {
   DCHECK(CalledOnValidThread());
   DCHECK(manager);
 }
 
 SdchDictionaryFetcher::~SdchDictionaryFetcher() {
   DCHECK(CalledOnValidThread());
 }
 
 void SdchDictionaryFetcher::Schedule(const GURL& dictionary_url) {
   DCHECK(CalledOnValidThread());
 
   // Avoid pushing duplicate copy onto queue.  We may fetch this url again later
   // and get a different dictionary, but there is no reason to have it in the
   // queue twice at one time.
   if (!fetch_queue_.empty() && fetch_queue_.back() == dictionary_url) {
     SdchManager::SdchErrorRecovery(
         SdchManager::DICTIONARY_ALREADY_SCHEDULED_TO_DOWNLOAD);
     return;
   }
   if (attempted_load_.find(dictionary_url) != attempted_load_.end()) {
     SdchManager::SdchErrorRecovery(
         SdchManager::DICTIONARY_ALREADY_TRIED_TO_DOWNLOAD);
     return;
   }
   attempted_load_.insert(dictionary_url);
   fetch_queue_.push(dictionary_url);
-  ScheduleDelayedRun();
+  DispatchRun();
 }
 
 void SdchDictionaryFetcher::Cancel() {
   DCHECK(CalledOnValidThread());
 
   while (!fetch_queue_.empty())
     fetch_queue_.pop();
   attempted_load_.clear();
   weak_factory_.InvalidateWeakPtrs();
-  current_fetch_.reset(NULL);
+  current_request_.reset(NULL);
+  buffer_ = NULL;
+  dictionary_ = "";

[Ryan Sleevi, 2014/08/25 20:00:00]

s/""/std::string()

or just dictionary_.clear()

[Randy Smith (Not in Mondays), 2014/08/26 15:34:17]

Done.

 }
 
-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;
+void SdchDictionaryFetcher::OnResponseStarted(URLRequest* request) {
+  DCHECK(CalledOnValidThread());
+  DCHECK_EQ(request, current_request_.get());
+
+  int bytes_read = 0;
+  request->Read(buffer_.get(), kBufferSize, &bytes_read);

[Ryan Sleevi, 2014/08/25 20:00:00]

Read() returns a bool. Please check it before assuming bytes_read is valid.

We should be distinguishing an error from an EOF.

[Randy Smith (Not in Mondays), 2014/08/26 15:34:17]

Done, but I'd like a bit more of a consult here.  Both
URLFetcherCore::ReadResponse() (which I don't completely trust) and
ResourceLoader::ReadMore() (which I'm more inclined to take as the canonical
"way it should be done") ignore the result of request_->Read().  ResourceLoader
has a comment 

  // No need to check the return value here as we'll detect errors by
  // inspecting the URLRequest's status.

I think the same logic applies here.  If EOF, the status will be success, and
bytes_read will be set (to zero) and we can trust it.  If error, the status will
indicate that, and we won't look at bytes_read.  So I think the code was correct
as written, and if you disagree I want to understand why because "If you don't
understand it, it's dangerous."

Having said that, if I was going to keep the code as written, I'd want to define
the "OnReadCompleted()" interface to guarantee that it ignores bytes_read if the
request is in error.   That's not the guarantee in the url_request.h header
file, so I've reset bytes_read to -1 in the case of an error, which fits that
API interface.  If that was what you were originally getting at with your
comment: SG, Done :-}.

(Also done in url_fetcher_core.cc.)

+  OnReadCompleted(request, bytes_read);
 }
 
-void SdchDictionaryFetcher::StartFetching() {
+void SdchDictionaryFetcher::OnReadCompleted(URLRequest* request,
+                                            int bytes_read) {
   DCHECK(CalledOnValidThread());
-  DCHECK(task_is_pending_);
-  task_is_pending_ = false;
+  DCHECK_EQ(request, current_request_.get());
 
-  // Handle losing the race against Cancel().
-  if (fetch_queue_.empty())
+  URLRequestThrottlerManager* throttler_manager =
+      request->context()->throttler_manager();
+  if (throttler_manager) {
+    url_throttler_entry_ =
+        throttler_manager->RegisterRequestUrl(request->url());
+  }
+
+  do {
+    if (!current_request_->status().is_success() || bytes_read <= 0)
+      break;
+
+    dictionary_ += std::string(buffer_->data(), bytes_read);

[Ryan Sleevi, 2014/08/25 20:00:00]

dictionary_.append(), and avoid an unnecessary allocation+copy

[Randy Smith (Not in Mondays), 2014/08/26 15:34:17]

Done.

+  } while (current_request_->Read(buffer_.get(), kBufferSize, &bytes_read));
+
+  const URLRequestStatus status = current_request_->status();
+
+  if (!status.is_io_pending()) {
+    if (status.is_success())
+      manager_->AddSdchDictionary(dictionary_, current_request_->url());
+
+    current_request_.reset();
+    buffer_ = NULL;
+    dictionary_ = "";

[Ryan Sleevi, 2014/08/25 20:00:00]

.clear()

[Randy Smith (Not in Mondays), 2014/08/26 15:34:17]

Done.

+
+    DispatchRun();
+  }
+}
+
+void SdchDictionaryFetcher::DispatchRun() {
+  DCHECK(CalledOnValidThread());
+  DCHECK(context_.get());
+
+  if (fetch_queue_.empty() || current_request_.get())
     return;
 
-  DCHECK(context_.get());
-  current_fetch_.reset(URLFetcher::Create(
-      fetch_queue_.front(), URLFetcher::GET, this));
-  fetch_queue_.pop();
-  current_fetch_->SetRequestContext(context_.get());
-  current_fetch_->SetLoadFlags(LOAD_DO_NOT_SEND_COOKIES |
-                               LOAD_DO_NOT_SAVE_COOKIES);
-  current_fetch_->Start();
+  int64 delay = INT64_C(0);
+  if (!original_url_throttler_entry_.get()) {
+    URLRequestThrottlerManager* manager =
+        context_->GetURLRequestContext()->throttler_manager();
+    if (manager) {
+      original_url_throttler_entry_ =
+          manager->RegisterRequestUrl(fetch_queue_.front());
+    }
+  }
+  if (original_url_throttler_entry_.get()) {
+    delay = original_url_throttler_entry_->ReserveSendingTimeForNextRequest(
+        GetBackoffReleaseTime());
+  }
+
+  if (delay != INT64_C(0)) {
+    base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
+        FROM_HERE, base::Bind(&SdchDictionaryFetcher::StartURLRequest,
+                              weak_factory_.GetWeakPtr()),
+        base::TimeDelta::FromMilliseconds(delay));
+    return;
+  }
+
+  StartURLRequest();

[Ryan Sleevi, 2014/08/25 20:00:01]

DANGER: This potentially leads to large recursion.

Line 158 -> Line 78 -> line 110 -> line 143.

I would prefer to see this worked into an explicit state machine using the //net
idioms, so that we can have a single looping iteration of pending URLs until
exhaustion, error, or asynchronicity, aka DoLoop w/ an entry point.

(I realize that in practice 158 -> 78 will typically be asynchronous, but that
isn't a hard/documented guarantee of URLRequest's API)

[Randy Smith (Not in Mondays), 2014/09/02 19:35:03]

See top level comments; we need to hash a bit here.

 }
 
-void SdchDictionaryFetcher::OnURLFetchComplete(
-    const URLFetcher* source) {
+void SdchDictionaryFetcher::StartURLRequest() {
   DCHECK(CalledOnValidThread());
-  if ((200 == source->GetResponseCode()) &&
-      (source->GetStatus().status() == URLRequestStatus::SUCCESS)) {
-    std::string data;
-    source->GetResponseAsString(&data);
-    manager_->AddSdchDictionary(data, source->GetURL());
+  DCHECK(context_.get());
+  DCHECK(!current_request_.get());
+
+  current_request_ = context_->GetURLRequestContext()->CreateRequest(
+      fetch_queue_.front(), IDLE, this, NULL);

[Ryan Sleevi, 2014/08/25 20:00:01]

Can this fail?

[Randy Smith (Not in Mondays), 2014/08/26 15:34:17]

The header is silent on the topic.  As I read the code, only on an OOM error (it
translates directly to wrapping a "new URLRequest" in a scoped_ptr). 
ResourceDispatcherHostImpl::BeginRequest() (which I tend to think of as the
canonical request creation point in chrome) assumes it can't fail.  So I think
if I'm doing it wrong, I have good company :-}.  Let me know what you'd like me
to do.

+  current_request_->SetLoadFlags(LOAD_DO_NOT_SEND_COOKIES |
+                                 LOAD_DO_NOT_SAVE_COOKIES);
+  buffer_ = new IOBuffer(kBufferSize);
+  fetch_queue_.pop();
+
+  current_request_->Start();
+}
+
+base::TimeTicks SdchDictionaryFetcher::GetBackoffReleaseTime() {
+  DCHECK(CalledOnValidThread());
+
+  if (!original_url_throttler_entry_.get())
+    return base::TimeTicks();
+
+  base::TimeTicks original_url_backoff =
+      original_url_throttler_entry_->GetExponentialBackoffReleaseTime();
+  base::TimeTicks destination_url_backoff;
+  if (url_throttler_entry_.get() &&
+      original_url_throttler_entry_.get() != url_throttler_entry_.get()) {
+    destination_url_backoff =
+        url_throttler_entry_->GetExponentialBackoffReleaseTime();
   }
-  current_fetch_.reset(NULL);
-  ScheduleDelayedRun();
+
+  return original_url_backoff > destination_url_backoff ?
+      original_url_backoff : destination_url_backoff;
 }
 
 }  // namespace net