Change SDCHDictionaryFetcher to use URLRequest instead of URLFetcher.

Also shifts mechanism for minimizing interference with top-level traffic
from delaying dispatch to using the IDLE request priority.

BUG=387890
BUG=383404
R=rsleevi@chromium.org

Review URL: https://codereview.chromium.org/495523003

Cr-Commit-Position: refs/heads/master@{#294002}

chrome/browser/profiles/off_the_record_profile_io_data.cc

@@ -276,16 +276,9 @@ void OffTheRecordProfileIOData::InitializeInternal(
 
   // Setup the SDCHManager for this profile.
   sdch_manager_.reset(new net::SdchManager);
-
-  scoped_refptr<net::URLRequestContextGetter> getter(
-      new net::TrivialURLRequestContextGetter(
-          main_context,
-          content::BrowserThread::GetMessageLoopProxyForThread(
-              content::BrowserThread::IO)));
-
   sdch_manager_->set_sdch_fetcher(scoped_ptr<net::SdchFetcher>(
-      new net::SdchDictionaryFetcher(sdch_manager_.get(), getter)).Pass());
-
+      new net::SdchDictionaryFetcher(sdch_manager_.get(),
+                                     main_context)).Pass());
   main_context->set_sdch_manager(sdch_manager_.get());
 
 #if defined(ENABLE_EXTENSIONS)

chrome/browser/profiles/profile_impl_io_data.cc

@@ -585,16 +585,9 @@ void ProfileImplIOData::InitializeInternal(
 
   // Setup the SDCHManager for this profile.
   sdch_manager_.reset(new net::SdchManager);
-
-  scoped_refptr<net::URLRequestContextGetter> getter(
-      new net::TrivialURLRequestContextGetter(
-          main_context,
-          content::BrowserThread::GetMessageLoopProxyForThread(
-              content::BrowserThread::IO)));
-
   sdch_manager_->set_sdch_fetcher(scoped_ptr<net::SdchFetcher>(
-      new net::SdchDictionaryFetcher(sdch_manager_.get(), getter)).Pass());
-
+      new net::SdchDictionaryFetcher(sdch_manager_.get(),
+                                     main_context)).Pass());
   main_context->set_sdch_manager(sdch_manager_.get());
 
   // Create a media request context based on the main context, but using a

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),
-      task_is_pending_(false),
+    SdchFetcher::Delegate* consumer,
+    URLRequestContext* context)
+    : next_state_(STATE_NONE),
+      in_loop_(false),
+      consumer_(consumer),
       context_(context),
       weak_factory_(this) {
   DCHECK(CalledOnValidThread());
-  DCHECK(manager);
+  DCHECK(consumer);
+  DCHECK(context);
 }
 
 SdchDictionaryFetcher::~SdchDictionaryFetcher() {
@@ -32,7 +43,7 @@ SdchDictionaryFetcher::~SdchDictionaryFetcher() {
 void SdchDictionaryFetcher::Schedule(const GURL& dictionary_url) {
   DCHECK(CalledOnValidThread());
 
-  // Avoid pushing duplicate copy onto queue.  We may fetch this url again later
+  // 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) {
@@ -47,59 +58,178 @@ 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_)
+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;
-  base::MessageLoop::current()->PostDelayedTask(FROM_HERE,
-      base::Bind(&SdchDictionaryFetcher::StartFetching,
-                 weak_factory_.GetWeakPtr()),
-      base::TimeDelta::FromMilliseconds(kMsDelayFromRequestTillDownload));
-  task_is_pending_ = true;
+
+  DoLoop(request->status().error());
 }
 
-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());
+  DCHECK_EQ(next_state_, STATE_REQUEST_READING);
+
+  // No state transition is required in this function; the
+  // completion of the request is detected in DoRead().
 
-  // Handle losing the race against Cancel().
-  if (fetch_queue_.empty())
+  if (request->status().is_success())
+    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;
 
-  DCHECK(context_.get());
-  current_fetch_.reset(URLFetcher::Create(
-      fetch_queue_.front(), URLFetcher::GET, this));
+  DoLoop(request->status().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);
+        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;
+}
+
+int SdchDictionaryFetcher::DoDispatchRequest(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;
+  }
+
+  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;
 }
 
-void SdchDictionaryFetcher::OnURLFetchComplete(
-    const URLFetcher* source) {
+int SdchDictionaryFetcher::DoRequestStarted(int rv) {
   DCHECK(CalledOnValidThread());
-  if ((200 == source->GetResponseCode()) &&
-      (source->GetStatus().status() == URLRequestStatus::SUCCESS)) {
-    std::string data;
-    source->GetResponseAsString(&data);
-    manager_->AddSdchDictionary(data, source->GetURL());
+  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) {
+  DCHECK(CalledOnValidThread());
+
+  // If there's been an error, abort the current request.
+  if (rv != OK) {
+    current_request_.reset();
+    buffer_ = NULL;
+    next_state_ = STATE_IDLE;
+
+    return OK;
+  }
+
+  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(int rv) {
+  DCHECK(CalledOnValidThread());
+
+  // If the dictionary was successfully fetched, add it to the manager.
+  if (rv == OK)
+    consumer_->AddSdchDictionary(dictionary_, current_request_->url());
+
+  current_request_.reset();
+  buffer_ = NULL;
+  dictionary_.clear();
+
+  next_state_ = STATE_IDLE;
+
+  return OK;
 }
 
 }  // namespace net

net/base/sdch_dictionary_fetcher.h

@@ -2,9 +2,11 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-// Support modularity by calling to load a new SDCH filter dictionary.
-// Note that this sort of calling can't be done in the /net directory, as it has
-// no concept of the HTTP cache (which is only visible at the browser level).
+// TODO(rdsmith): This class needs to be moved out to the net/ embedder and
+// hooked into whatever mechanisms the embedder uses for authentication.
+// Specifically, this class needs methods overriding
+// URLRequest::Delegate::{OnAuthRequired,OnCertificateRequested} and can't
+// implement them at the net/ layer.
 
 #ifndef NET_BASE_SDCH_DICTIONARY_FETCHER_H_
 #define NET_BASE_SDCH_DICTIONARY_FETCHER_H_
@@ -18,62 +20,75 @@
 #include "base/threading/non_thread_safe.h"
 #include "net/base/sdch_manager.h"
 #include "net/url_request/url_fetcher_delegate.h"
+#include "net/url_request/url_request.h"
 
 namespace net {
 
-class URLFetcher;
-class URLRequestContextGetter;
+class URLRequest;
+class URLRequestThrottlerEntryInterface;
 
+// This class implements the SdchFetcher interface. It queues requests
+// for dictionaries and dispatches them serially, implementing
+// the URLRequest::Delegate interface to handle callbacks (but see above
+// TODO). It tracks all requests, only attempting to fetch each dictionary
+// once.
 class NET_EXPORT SdchDictionaryFetcher
-    : public URLFetcherDelegate,
-      public SdchFetcher,
+    : public SdchFetcher,
+      public URLRequest::Delegate,
       public base::NonThreadSafe {
  public:
-  // The consumer must guarantee that |*manager| outlives
-  // this object.  The current implementation guarantees this by
-  // the SdchManager owning this object.
-  SdchDictionaryFetcher(SdchManager* manager,
-                        scoped_refptr<URLRequestContextGetter> context);
+  // The consumer must guarantee that |*consumer| and |*context| outlive
+  // this object.
+  SdchDictionaryFetcher(SdchFetcher::Delegate* consumer,
+                        URLRequestContext* context);
   virtual ~SdchDictionaryFetcher();
 
-  // Implementation of SdchFetcher class.
+  // Implementation of SdchFetcher methods.
   virtual void Schedule(const GURL& dictionary_url) OVERRIDE;
   virtual void Cancel() OVERRIDE;
 
- private:
-  // Delay in ms between Schedule and actual download.
-  // This leaves the URL in a queue, which is de-duped, so that there is less
-  // chance we'll try to load the same URL multiple times when a pile of
-  // page subresources (or tabs opened in parallel) all suggest the dictionary.
-  static const int kMsDelayFromRequestTillDownload = 100;
-
-  // Ensure the download after the above delay.
-  void ScheduleDelayedRun();
-
-  // Make sure we're processing (or waiting for) the the arrival of the next URL
-  // in the  |fetch_queue_|.
-  void StartFetching();
+  // Implementation of URLRequest::Delegate methods.
+  virtual void OnResponseStarted(URLRequest* request) OVERRIDE;
+  virtual void OnReadCompleted(URLRequest* request, int bytes_read) OVERRIDE;
 
-  // Implementation of URLFetcherDelegate. Called after transmission
-  // completes (either successfully or with failure).
-  virtual void OnURLFetchComplete(const URLFetcher* source) OVERRIDE;
-
-  SdchManager* const manager_;
+ private:
+  enum State {
+    STATE_NONE,
+    STATE_IDLE,
+    STATE_REQUEST_STARTED,
+    STATE_REQUEST_READING,
+    STATE_REQUEST_COMPLETE,
+  };
+
+  // State machine implementation.
+  int DoLoop(int rv);
+  int DoDispatchRequest(int rv);
+  int DoRequestStarted(int rv);
+  int DoRead(int rv);
+  int DoCompleteRequest(int rv);
+
+  State next_state_;
+  bool in_loop_;
+
+  SdchFetcher::Delegate* const consumer_;
 
   // A queue of URLs that are being used to download dictionaries.
   std::queue<GURL> fetch_queue_;
-  // The currently outstanding URL fetch of a dicitonary.
-  // If this is null, then there is no outstanding request.
-  scoped_ptr<URLFetcher> current_fetch_;
 
-  bool task_is_pending_;
+  // The request and buffer used for getting the current dictionary
+  // Both are null when a fetch is not in progress.
+  scoped_ptr<URLRequest> current_request_;
+  scoped_refptr<IOBuffer> buffer_;
+
+  // The currently accumulating dictionary.
+  std::string dictionary_;
 
   // Althought the SDCH spec does not preclude a server from using a single URL
   // to load several distinct dictionaries (by telling a client to load a
   // dictionary from an URL several times), current implementations seem to have
   // that 1-1 relationship (i.e., each URL points at a single dictionary, and
   // the dictionary content does not change over time, and hence is not worth
-  // trying to load more than once).  In addition, some dictionaries prove
+  // trying to load more than once). In addition, some dictionaries prove
   // unloadable only after downloading them (because they are too large?  ...or
   // malformed?). As a protective element, Chromium will *only* load a
   // dictionary at most once from a given URL (so that it doesn't waste
@@ -83,13 +98,10 @@ class NET_EXPORT SdchDictionaryFetcher
   // TODO(jar): Try to augment the SDCH proposal to include this restiction.
   std::set<GURL> attempted_load_;
 
-  // Store the system_url_request_context_getter to use it when we start
-  // fetching.
-  scoped_refptr<URLRequestContextGetter> context_;
+  // Store the URLRequestContext associated with the owning SdchManager for
+  // use while fetching.
+  URLRequestContext* context_;
 
-  // Always spread out the dictionary fetches, so that they don't steal
-  // bandwidth from the actual page load.  Create delayed tasks to spread out
-  // the download.
   base::WeakPtrFactory<SdchDictionaryFetcher> weak_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(SdchDictionaryFetcher);

net/base/sdch_dictionary_fetcher_unittest.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 <string>
+
+#include "base/run_loop.h"
+#include "base/strings/stringprintf.h"
+#include "base/thread_task_runner_handle.h"
+#include "net/base/sdch_manager.h"
+#include "net/url_request/url_request_data_job.h"
+#include "net/url_request/url_request_filter.h"
+#include "net/url_request/url_request_interceptor.h"
+#include "net/url_request/url_request_test_util.h"
+#include "testing/gtest/include/gtest/gtest.h"
+#include "url/gurl.h"
+
+namespace net {
+
+static const char* kSampleBufferContext = "This is a sample buffer.";
+static const char* kTestDomain = "top.domain.test";
+
+class URLRequestSpecifiedResponseJob : public URLRequestSimpleJob {
+ public:
+  URLRequestSpecifiedResponseJob(URLRequest* request,
+                                 NetworkDelegate* network_delegate)
+      : URLRequestSimpleJob(request, network_delegate) {}
+
+  static void AddUrlHandler() {
+    net::URLRequestFilter* filter = net::URLRequestFilter::GetInstance();
+    jobs_requested_ = 0;
+    filter->AddHostnameHandler("http", kTestDomain,
+                               &URLRequestSpecifiedResponseJob::Factory);
+  }
+
+  static void RemoveUrlHandler() {
+    net::URLRequestFilter* filter = net::URLRequestFilter::GetInstance();
+    filter->RemoveHostnameHandler("http", kTestDomain);
+    jobs_requested_ = 0;
+  }
+
+  static URLRequestJob* Factory(
+      URLRequest* request,
+      net::NetworkDelegate* network_delegate,
+      const std::string& scheme) {
+    ++jobs_requested_;
+    return new URLRequestSpecifiedResponseJob(request, network_delegate);
+  }
+
+  static std::string ExpectedResponseForURL(const GURL& url) {
+    return base::StringPrintf("Response for %s\n%s\nEnd Response for %s\n",
+                              url.spec().c_str(), kSampleBufferContext,
+                              url.spec().c_str());
+  }
+
+  static int jobs_requested() { return jobs_requested_; }
+
+ private:
+  virtual ~URLRequestSpecifiedResponseJob() {};
+  virtual int GetData(std::string* mime_type,
+                      std::string* charset,
+                      std::string* data,
+                      const CompletionCallback& callback) const OVERRIDE {
+    GURL url(request_->url());
+    *data = ExpectedResponseForURL(url);
+    return OK;
+  }
+
+  static int jobs_requested_;
+};
+
+int URLRequestSpecifiedResponseJob::jobs_requested_(0);
+
+class SdchTestDelegate : public SdchFetcher::Delegate {
+ public:
+  struct DictionaryAdditions {
+    DictionaryAdditions(const std::string& dictionary_text,
+                        const GURL& dictionary_url)
+        : dictionary_text(dictionary_text),
+          dictionary_url(dictionary_url) {}
+
+
+    std::string dictionary_text;
+    GURL dictionary_url;
+  };
+
+  virtual void AddSdchDictionary(const std::string& dictionary_text,
+                                 const GURL& dictionary_url) OVERRIDE {
+    dictionary_additions.push_back(
+        DictionaryAdditions(dictionary_text, dictionary_url));
+  }
+
+  void GetDictionaryAdditions(std::vector<DictionaryAdditions>* out) {
+    out->swap(dictionary_additions);
+    dictionary_additions.clear();
+  }
+
+ private:
+  std::vector<DictionaryAdditions> dictionary_additions;
+};
+
+class SdchDictionaryFetcherTest : public ::testing::Test {
+ public:
+  SdchDictionaryFetcherTest() {}
+
+  virtual void SetUp() OVERRIDE {
+    DCHECK(!fetcher_.get());
+
+    URLRequestSpecifiedResponseJob::AddUrlHandler();
+    fetcher_delegate_.reset(new SdchTestDelegate);
+    context_.reset(new TestURLRequestContext);
+    fetcher_.reset(new SdchDictionaryFetcher(
+        fetcher_delegate_.get(), context_.get()));
+  }
+
+  virtual void TearDown() OVERRIDE {
+    URLRequestSpecifiedResponseJob::RemoveUrlHandler();
+    fetcher_.reset();
+    context_.reset();
+    fetcher_delegate_.reset();
+  }
+
+  SdchDictionaryFetcher* fetcher() { return fetcher_.get(); }
+  SdchTestDelegate* manager() { return fetcher_delegate_.get(); }
+
+  // May not be called outside the SetUp()/TearDown() interval.
+  int JobsRequested() {
+    return URLRequestSpecifiedResponseJob::jobs_requested();
+  }
+
+  GURL PathToGurl(const char* path) {
+    std::string gurl_string("http://");
+    gurl_string += kTestDomain;
+    gurl_string += "/";
+    gurl_string += path;
+    return GURL(gurl_string);
+  }
+
+ private:
+  scoped_ptr<SdchTestDelegate> fetcher_delegate_;
+  scoped_ptr<TestURLRequestContext> context_;
+  scoped_ptr<SdchDictionaryFetcher> fetcher_;
+};
+
+// Schedule a fetch and make sure it happens.
+TEST_F(SdchDictionaryFetcherTest, Basic) {
+  GURL dictionary_url(PathToGurl("dictionary"));
+  fetcher()->Schedule(dictionary_url);
+
+  base::RunLoop().RunUntilIdle();
+  EXPECT_EQ(1, JobsRequested());
+  std::vector<SdchTestDelegate::DictionaryAdditions> additions;
+  manager()->GetDictionaryAdditions(&additions);
+  ASSERT_EQ(1u, additions.size());
+  EXPECT_EQ(URLRequestSpecifiedResponseJob::ExpectedResponseForURL(
+      dictionary_url), additions[0].dictionary_text);
+}
+
+// Multiple fetches of the same URL should result in only one request.
+TEST_F(SdchDictionaryFetcherTest, Multiple) {
+  GURL dictionary_url(PathToGurl("dictionary"));
+  fetcher()->Schedule(dictionary_url);
+  fetcher()->Schedule(dictionary_url);
+  fetcher()->Schedule(dictionary_url);
+  base::RunLoop().RunUntilIdle();
+
+  EXPECT_EQ(1, JobsRequested());
+  std::vector<SdchTestDelegate::DictionaryAdditions> additions;
+  manager()->GetDictionaryAdditions(&additions);
+  ASSERT_EQ(1u, additions.size());
+  EXPECT_EQ(URLRequestSpecifiedResponseJob::ExpectedResponseForURL(
+      dictionary_url), additions[0].dictionary_text);
+}
+
+// A cancel should result in no actual requests being generated.
+TEST_F(SdchDictionaryFetcherTest, Cancel) {
+  GURL dictionary_url_1(PathToGurl("dictionary_1"));
+  GURL dictionary_url_2(PathToGurl("dictionary_2"));
+  GURL dictionary_url_3(PathToGurl("dictionary_3"));
+
+  fetcher()->Schedule(dictionary_url_1);
+  fetcher()->Schedule(dictionary_url_2);
+  fetcher()->Schedule(dictionary_url_3);
+  fetcher()->Cancel();
+  base::RunLoop().RunUntilIdle();
+
+  // Synchronous execution may have resulted in a single job being scheduled.
+  EXPECT_GE(1, JobsRequested());
+}
+
+}

net/base/sdch_manager.cc

@@ -400,7 +400,85 @@ bool SdchManager::CanFetchDictionary(const GURL& referring_url,
   return true;
 }
 
-bool SdchManager::AddSdchDictionary(const std::string& dictionary_text,
+void SdchManager::GetVcdiffDictionary(
+    const std::string& server_hash,
+    const GURL& referring_url,
+    scoped_refptr<Dictionary>* dictionary) {
+  DCHECK(CalledOnValidThread());
+  *dictionary = NULL;
+  DictionaryMap::iterator it = dictionaries_.find(server_hash);
+  if (it == dictionaries_.end()) {
+    return;
+  }
+  scoped_refptr<Dictionary> matching_dictionary = it->second;
+  if (!IsInSupportedDomain(referring_url))
+    return;
+  if (!matching_dictionary->CanUse(referring_url))
+    return;
+  *dictionary = matching_dictionary;
+}
+
+// TODO(jar): If we have evictions from the dictionaries_, then we need to
+// change this interface to return a list of reference counted Dictionary
+// instances that can be used if/when a server specifies one.
+void SdchManager::GetAvailDictionaryList(const GURL& target_url,
+                                         std::string* list) {
+  DCHECK(CalledOnValidThread());
+  int count = 0;
+  for (DictionaryMap::iterator it = dictionaries_.begin();
+       it != dictionaries_.end(); ++it) {
+    if (!IsInSupportedDomain(target_url))
+      continue;
+    if (!it->second->CanAdvertise(target_url))
+      continue;
+    ++count;
+    if (!list->empty())
+      list->append(",");
+    list->append(it->second->client_hash());
+  }
+  // Watch to see if we have corrupt or numerous dictionaries.
+  if (count > 0)
+    UMA_HISTOGRAM_COUNTS("Sdch3.Advertisement_Count", count);
+}
+
+// static
+void SdchManager::GenerateHash(const std::string& dictionary_text,
+    std::string* client_hash, std::string* server_hash) {
+  char binary_hash[32];
+  crypto::SHA256HashString(dictionary_text, binary_hash, sizeof(binary_hash));
+
+  std::string first_48_bits(&binary_hash[0], 6);
+  std::string second_48_bits(&binary_hash[6], 6);
+  UrlSafeBase64Encode(first_48_bits, client_hash);
+  UrlSafeBase64Encode(second_48_bits, server_hash);
+
+  DCHECK_EQ(server_hash->length(), 8u);
+  DCHECK_EQ(client_hash->length(), 8u);
+}
+
+//------------------------------------------------------------------------------
+// Methods for supporting latency experiments.
+
+bool SdchManager::AllowLatencyExperiment(const GURL& url) const {
+  DCHECK(CalledOnValidThread());
+  return allow_latency_experiment_.end() !=
+      allow_latency_experiment_.find(url.host());
+}
+
+void SdchManager::SetAllowLatencyExperiment(const GURL& url, bool enable) {
+  DCHECK(CalledOnValidThread());
+  if (enable) {
+    allow_latency_experiment_.insert(url.host());
+    return;
+  }
+  ExperimentSet::iterator it = allow_latency_experiment_.find(url.host());
+  if (allow_latency_experiment_.end() == it)
+    return;  // It was already erased, or never allowed.
+  SdchErrorRecovery(LATENCY_TEST_DISALLOWED);
+  allow_latency_experiment_.erase(it);
+}
+
+void SdchManager::AddSdchDictionary(const std::string& dictionary_text,
     const GURL& dictionary_url) {
   DCHECK(CalledOnValidThread());
   std::string client_hash;
@@ -408,7 +486,7 @@ bool SdchManager::AddSdchDictionary(const std::string& dictionary_text,
   GenerateHash(dictionary_text, &client_hash, &server_hash);
   if (dictionaries_.find(server_hash) != dictionaries_.end()) {
     SdchErrorRecovery(DICTIONARY_ALREADY_LOADED);
-    return false;  // Already loaded.
+    return;                             // Already loaded.
   }
 
   std::string domain, path;
@@ -417,13 +495,13 @@ bool SdchManager::AddSdchDictionary(const std::string& dictionary_text,
 
   if (dictionary_text.empty()) {
     SdchErrorRecovery(DICTIONARY_HAS_NO_TEXT);
-    return false;  // Missing header.
+    return;                             // Missing header.
   }
 
   size_t header_end = dictionary_text.find("\n\n");
   if (std::string::npos == header_end) {
     SdchErrorRecovery(DICTIONARY_HAS_NO_HEADER);
-    return false;  // Missing header.
+    return;                             // Missing header.
   }
   size_t line_start = 0;  // Start of line being parsed.
   while (1) {
@@ -434,7 +512,7 @@ bool SdchManager::AddSdchDictionary(const std::string& dictionary_text,
     size_t colon_index = dictionary_text.find(':', line_start);
     if (std::string::npos == colon_index) {
       SdchErrorRecovery(DICTIONARY_HEADER_LINE_MISSING_COLON);
-      return false;  // Illegal line missing a colon.
+      return;                         // Illegal line missing a colon.
     }
 
     if (colon_index > line_end)
@@ -454,7 +532,7 @@ bool SdchManager::AddSdchDictionary(const std::string& dictionary_text,
         path = value;
       } else if (name == "format-version") {
         if (value != "1.0")
-          return false;
+          return;
       } else if (name == "max-age") {
         int64 seconds;
         base::StringToInt64(value, &seconds);
@@ -473,10 +551,10 @@ bool SdchManager::AddSdchDictionary(const std::string& dictionary_text,
   }
 
   if (!IsInSupportedDomain(dictionary_url))
-    return false;
+    return;
 
   if (!Dictionary::CanSet(domain, path, ports, dictionary_url))
-    return false;
+    return;
 
   // TODO(jar): Remove these hacks to preclude a DOS attack involving piles of
   // useless dictionaries.  We should probably have a cache eviction plan,
@@ -484,11 +562,11 @@ bool SdchManager::AddSdchDictionary(const std::string& dictionary_text,
   // is probably not worth doing eviction handling.
   if (kMaxDictionarySize < dictionary_text.size()) {
     SdchErrorRecovery(DICTIONARY_IS_TOO_LARGE);
-    return false;
+    return;
   }
   if (kMaxDictionaryCount <= dictionaries_.size()) {
     SdchErrorRecovery(DICTIONARY_COUNT_EXCEEDED);
-    return false;
+    return;
   }
 
   UMA_HISTOGRAM_COUNTS("Sdch3.Dictionary size loaded", dictionary_text.size());
@@ -498,85 +576,7 @@ bool SdchManager::AddSdchDictionary(const std::string& dictionary_text,
       new Dictionary(dictionary_text, header_end + 2, client_hash,
                      dictionary_url, domain, path, expiration, ports);
   dictionaries_[server_hash] = dictionary;
-  return true;
-}
-
-void SdchManager::GetVcdiffDictionary(
-    const std::string& server_hash,
-    const GURL& referring_url,
-    scoped_refptr<Dictionary>* dictionary) {
-  DCHECK(CalledOnValidThread());
-  *dictionary = NULL;
-  DictionaryMap::iterator it = dictionaries_.find(server_hash);
-  if (it == dictionaries_.end()) {
-    return;
-  }
-  scoped_refptr<Dictionary> matching_dictionary = it->second;
-  if (!IsInSupportedDomain(referring_url))
-    return;
-  if (!matching_dictionary->CanUse(referring_url))
-    return;
-  *dictionary = matching_dictionary;
-}
-
-// TODO(jar): If we have evictions from the dictionaries_, then we need to
-// change this interface to return a list of reference counted Dictionary
-// instances that can be used if/when a server specifies one.
-void SdchManager::GetAvailDictionaryList(const GURL& target_url,
-                                         std::string* list) {
-  DCHECK(CalledOnValidThread());
-  int count = 0;
-  for (DictionaryMap::iterator it = dictionaries_.begin();
-       it != dictionaries_.end(); ++it) {
-    if (!IsInSupportedDomain(target_url))
-      continue;
-    if (!it->second->CanAdvertise(target_url))
-      continue;
-    ++count;
-    if (!list->empty())
-      list->append(",");
-    list->append(it->second->client_hash());
-  }
-  // Watch to see if we have corrupt or numerous dictionaries.
-  if (count > 0)
-    UMA_HISTOGRAM_COUNTS("Sdch3.Advertisement_Count", count);
-}
-
-// static
-void SdchManager::GenerateHash(const std::string& dictionary_text,
-    std::string* client_hash, std::string* server_hash) {
-  char binary_hash[32];
-  crypto::SHA256HashString(dictionary_text, binary_hash, sizeof(binary_hash));
-
-  std::string first_48_bits(&binary_hash[0], 6);
-  std::string second_48_bits(&binary_hash[6], 6);
-  UrlSafeBase64Encode(first_48_bits, client_hash);
-  UrlSafeBase64Encode(second_48_bits, server_hash);
-
-  DCHECK_EQ(server_hash->length(), 8u);
-  DCHECK_EQ(client_hash->length(), 8u);
-}
-
-//------------------------------------------------------------------------------
-// Methods for supporting latency experiments.
-
-bool SdchManager::AllowLatencyExperiment(const GURL& url) const {
-  DCHECK(CalledOnValidThread());
-  return allow_latency_experiment_.end() !=
-      allow_latency_experiment_.find(url.host());
-}
-
-void SdchManager::SetAllowLatencyExperiment(const GURL& url, bool enable) {
-  DCHECK(CalledOnValidThread());
-  if (enable) {
-    allow_latency_experiment_.insert(url.host());
-    return;
-  }
-  ExperimentSet::iterator it = allow_latency_experiment_.find(url.host());
-  if (allow_latency_experiment_.end() == it)
-    return;  // It was already erased, or never allowed.
-  SdchErrorRecovery(LATENCY_TEST_DISALLOWED);
-  allow_latency_experiment_.erase(it);
+  return;
 }
 
 // static

net/base/sdch_manager.h

@@ -8,7 +8,7 @@
 // Exactly one instance of SdchManager is built, and all references are made
 // into that collection.
 //
-// The SdchManager maintains a collection of memory resident dictionaries.  It
+// The SdchManager maintains a collection of memory resident dictionaries. It
 // can find a dictionary (based on a server specification of a hash), store a
 // dictionary, and make judgements about what URLs can use, set, etc. a
 // dictionary.
@@ -38,16 +38,27 @@ namespace net {
 // Create a public interface to help us load SDCH dictionaries.
 // The SdchManager class allows registration to support this interface.
 // A browser may register a fetcher that is used by the dictionary managers to
-// get data from a specified URL.  This allows us to use very high level browser
-// functionality in this base (when the functionaity can be provided).
+// get data from a specified URL. This allows us to use very high level browser
+// functionality in this base (when the functionality can be provided).
 class NET_EXPORT SdchFetcher {
  public:
+  class NET_EXPORT Delegate {
+   public:
+    virtual ~Delegate() {}
+
+    // Called whenever the SdchFetcher has successfully retrieved a
+    // dictionary.  |dictionary_text| contains the body of the dictionary
+    // retrieved from |dictionary_url|.
+    virtual void AddSdchDictionary(const std::string& dictionary_text,
+                                   const GURL& dictionary_url) = 0;
+  };
+
   SdchFetcher() {}
   virtual ~SdchFetcher() {}
 
   // The Schedule() method is called when there is a need to get a dictionary
-  // from a server.  The callee is responsible for getting that dictionary_text,
-  // and then calling back to AddSdchDictionary() to the SdchManager instance.
+  // from a server. The callee is responsible for getting that dictionary_text,
+  // and then calling back to AddSdchDictionary() in the Delegate instance.
   virtual void Schedule(const GURL& dictionary_url) = 0;
 
   // The Cancel() method is called to cancel all pending dictionary fetches.
@@ -60,7 +71,9 @@ class NET_EXPORT SdchFetcher {
 
 //------------------------------------------------------------------------------
 
-class NET_EXPORT SdchManager : public NON_EXPORTED_BASE(base::NonThreadSafe) {
+class NET_EXPORT SdchManager
+    : public SdchFetcher::Delegate,
+      public NON_EXPORTED_BASE(base::NonThreadSafe) {
  public:
   // A list of errors that appeared and were either resolved, or used to turn
   // off sdch encoding.
@@ -178,7 +191,7 @@ class NET_EXPORT SdchManager : public NON_EXPORTED_BASE(base::NonThreadSafe) {
     FRIEND_TEST_ALL_PREFIXES(SdchManagerTest, PathMatch);
 
     // Construct a vc-diff usable dictionary from the dictionary_text starting
-    // at the given offset.  The supplied client_hash should be used to
+    // at the given offset. The supplied client_hash should be used to
     // advertise the dictionary's availability relative to the suppplied URL.
     Dictionary(const std::string& dictionary_text,
                size_t offset,
@@ -188,7 +201,7 @@ class NET_EXPORT SdchManager : public NON_EXPORTED_BASE(base::NonThreadSafe) {
                const std::string& path,
                const base::Time& expiration,
                const std::set<int>& ports);
-    ~Dictionary();
+    virtual ~Dictionary();
 
     const GURL& url() const { return url_; }
     const std::string& client_hash() const { return client_hash_; }
@@ -227,7 +240,7 @@ class NET_EXPORT SdchManager : public NON_EXPORTED_BASE(base::NonThreadSafe) {
 
     // Metadate "headers" in before dictionary text contained the following:
     // Each dictionary payload consists of several headers, followed by the text
-    // of the dictionary.  The following are the known headers.
+    // of the dictionary. The following are the known headers.
     const std::string domain_;
     const std::string path_;
     const base::Time expiration_;  // Implied by max-age.
@@ -237,7 +250,7 @@ class NET_EXPORT SdchManager : public NON_EXPORTED_BASE(base::NonThreadSafe) {
   };
 
   SdchManager();
-  ~SdchManager();
+  virtual ~SdchManager();
 
   // Clear data (for browser data removal).
   void ClearData();
@@ -260,7 +273,7 @@ class NET_EXPORT SdchManager : public NON_EXPORTED_BASE(base::NonThreadSafe) {
 
   // Briefly prevent further advertising of SDCH on this domain (if SDCH is
   // enabled). After enough calls to IsInSupportedDomain() the blacklisting
-  // will be removed.  Additional blacklists take exponentially more calls
+  // will be removed. Additional blacklists take exponentially more calls
   // to IsInSupportedDomain() before the blacklisting is undone.
   // Used when filter errors are found from a given domain, but it is plausible
   // that the cause is temporary (such as application startup, where cached
@@ -286,7 +299,7 @@ class NET_EXPORT SdchManager : public NON_EXPORTED_BASE(base::NonThreadSafe) {
 
   // Check to see if SDCH is enabled (globally), and the given URL is in a
   // supported domain (i.e., not blacklisted, and either the specific supported
-  // domain, or all domains were assumed supported).  If it is blacklist, reduce
+  // domain, or all domains were assumed supported). If it is blacklist, reduce
   // by 1 the number of times it will be reported as blacklisted.
   bool IsInSupportedDomain(const GURL& url);
 
@@ -301,15 +314,9 @@ class NET_EXPORT SdchManager : public NON_EXPORTED_BASE(base::NonThreadSafe) {
   bool CanFetchDictionary(const GURL& referring_url,
                           const GURL& dictionary_url) const;
 
-  // Add an SDCH dictionary to our list of availible dictionaries. This addition
-  // will fail (return false) if addition is illegal (data in the dictionary is
-  // not acceptable from the dictionary_url; dictionary already added, etc.).
-  bool AddSdchDictionary(const std::string& dictionary_text,
-                         const GURL& dictionary_url);
-
   // Find the vcdiff dictionary (the body of the sdch dictionary that appears
   // after the meta-data headers like Domain:...) with the given |server_hash|
-  // to use to decompreses data that arrived as SDCH encoded content.  Check to
+  // to use to decompreses data that arrived as SDCH encoded content. Check to
   // be sure the returned |dictionary| can be used for decoding content supplied
   // in response to a request for |referring_url|.
   // Return null in |dictionary| if there is no matching legal dictionary.
@@ -318,18 +325,18 @@ class NET_EXPORT SdchManager : public NON_EXPORTED_BASE(base::NonThreadSafe) {
                            scoped_refptr<Dictionary>* dictionary);
 
   // Get list of available (pre-cached) dictionaries that we have already loaded
-  // into memory.  The list is a comma separated list of (client) hashes per
+  // into memory. The list is a comma separated list of (client) hashes per
   // the SDCH spec.
   void GetAvailDictionaryList(const GURL& target_url, std::string* list);
 
   // Construct the pair of hashes for client and server to identify an SDCH
-  // dictionary.  This is only made public to facilitate unit testing, but is
+  // dictionary. This is only made public to facilitate unit testing, but is
   // otherwise private
   static void GenerateHash(const std::string& dictionary_text,
                            std::string* client_hash, std::string* server_hash);
 
   // For Latency testing only, we need to know if we've succeeded in doing a
-  // round trip before starting our comparative tests.  If ever we encounter
+  // round trip before starting our comparative tests. If ever we encounter
   // problems with SDCH, we opt-out of the test unless/until we perform a
   // complete SDCH decoding.
   bool AllowLatencyExperiment(const GURL& url) const;
@@ -340,6 +347,15 @@ class NET_EXPORT SdchManager : public NON_EXPORTED_BASE(base::NonThreadSafe) {
     return fetches_count_for_testing_;
   }
 
+  // Implementation of SdchFetcher::Delegate.
+
+  // Add an SDCH dictionary to our list of availible
+  // dictionaries. This addition will fail if addition is illegal
+  // (data in the dictionary is not acceptable from the
+  // dictionary_url; dictionary already added, etc.).
+  virtual void AddSdchDictionary(const std::string& dictionary_text,
+                                 const GURL& dictionary_url) OVERRIDE;
+
  private:
   struct BlacklistInfo {
     BlacklistInfo()

net/base/sdch_manager_unittest.cc

@@ -35,6 +35,20 @@ class SdchManagerTest : public testing::Test {
     SdchManager::EnableSecureSchemeSupport(false);
   }
 
+  // Attempt to add a dictionary to the manager and probe for success or
+  // failure.
+  bool AddSdchDictionary(const std::string& dictionary_text,
+                         const GURL& gurl) {
+    std::string list;
+    sdch_manager_->GetAvailDictionaryList(gurl, &list);
+    sdch_manager_->AddSdchDictionary(dictionary_text, gurl);
+    std::string list2;
+    sdch_manager_->GetAvailDictionaryList(gurl, &list2);
+
+    // The list of hashes should change iff the addition succeeds.
+    return (list != list2);
+  }
+
  private:
   scoped_ptr<SdchManager> sdch_manager_;
 };
@@ -140,16 +154,16 @@ TEST_F(SdchManagerTest, CanSetExactMatchDictionary) {
   std::string dictionary_text(NewSdchDictionary(dictionary_domain));
 
   // Perfect match should work.
-  EXPECT_TRUE(sdch_manager()->AddSdchDictionary(dictionary_text,
-              GURL("http://" + dictionary_domain)));
+  EXPECT_TRUE(AddSdchDictionary(dictionary_text,
+                                GURL("http://" + dictionary_domain)));
 }
 
 TEST_F(SdchManagerTest, CanAdvertiseDictionaryOverHTTP) {
   std::string dictionary_domain("x.y.z.google.com");
   std::string dictionary_text(NewSdchDictionary(dictionary_domain));
 
-  EXPECT_TRUE(sdch_manager()->AddSdchDictionary(dictionary_text,
-              GURL("http://" + dictionary_domain)));
+  EXPECT_TRUE(AddSdchDictionary(dictionary_text,
+                                GURL("http://" + dictionary_domain)));
 
   std::string dictionary_list;
   // HTTP target URL can advertise dictionary.
@@ -163,8 +177,8 @@ TEST_F(SdchManagerTest, CanNotAdvertiseDictionaryOverHTTPS) {
   std::string dictionary_domain("x.y.z.google.com");
   std::string dictionary_text(NewSdchDictionary(dictionary_domain));
 
-  EXPECT_TRUE(sdch_manager()->AddSdchDictionary(dictionary_text,
-              GURL("http://" + dictionary_domain)));
+  EXPECT_TRUE(AddSdchDictionary(dictionary_text,
+                                GURL("http://" + dictionary_domain)));
 
   std::string dictionary_list;
   // HTTPS target URL should NOT advertise dictionary.
@@ -178,11 +192,11 @@ TEST_F(SdchManagerTest, CanUseHTTPSDictionaryOverHTTPSIfEnabled) {
   std::string dictionary_domain("x.y.z.google.com");
   std::string dictionary_text(NewSdchDictionary(dictionary_domain));
 
-  EXPECT_FALSE(sdch_manager()->AddSdchDictionary(
-      dictionary_text, GURL("https://" + dictionary_domain)));
+  EXPECT_FALSE(AddSdchDictionary(dictionary_text,
+                                 GURL("https://" + dictionary_domain)));
   SdchManager::EnableSecureSchemeSupport(true);
-  EXPECT_TRUE(sdch_manager()->AddSdchDictionary(
-      dictionary_text, GURL("https://" + dictionary_domain)));
+  EXPECT_TRUE(AddSdchDictionary(dictionary_text,
+                                GURL("https://" + dictionary_domain)));
 
   GURL target_url("https://" + dictionary_domain + "/test");
   std::string dictionary_list;
@@ -204,8 +218,8 @@ TEST_F(SdchManagerTest, CanNotUseHTTPDictionaryOverHTTPS) {
   std::string dictionary_domain("x.y.z.google.com");
   std::string dictionary_text(NewSdchDictionary(dictionary_domain));
 
-  EXPECT_TRUE(sdch_manager()->AddSdchDictionary(dictionary_text,
-              GURL("http://" + dictionary_domain)));
+  EXPECT_TRUE(AddSdchDictionary(dictionary_text,
+                                GURL("http://" + dictionary_domain)));
 
   GURL target_url("https://" + dictionary_domain + "/test");
   std::string dictionary_list;
@@ -228,8 +242,8 @@ TEST_F(SdchManagerTest, CanNotUseHTTPSDictionaryOverHTTP) {
   std::string dictionary_text(NewSdchDictionary(dictionary_domain));
 
   SdchManager::EnableSecureSchemeSupport(true);
-  EXPECT_TRUE(sdch_manager()->AddSdchDictionary(dictionary_text,
-              GURL("https://" + dictionary_domain)));
+  EXPECT_TRUE(AddSdchDictionary(dictionary_text,
+                                GURL("https://" + dictionary_domain)));
 
   GURL target_url("http://" + dictionary_domain + "/test");
   std::string dictionary_list;
@@ -251,8 +265,8 @@ TEST_F(SdchManagerTest, FailToSetDomainMismatchDictionary) {
   std::string dictionary_text(NewSdchDictionary(dictionary_domain));
 
   // Fail the "domain match" requirement.
-  EXPECT_FALSE(sdch_manager()->AddSdchDictionary(dictionary_text,
-               GURL("http://y.z.google.com")));
+  EXPECT_FALSE(AddSdchDictionary(dictionary_text,
+                                 GURL("http://y.z.google.com")));
 }
 
 TEST_F(SdchManagerTest, FailToSetDotHostPrefixDomainDictionary) {
@@ -260,8 +274,8 @@ TEST_F(SdchManagerTest, FailToSetDotHostPrefixDomainDictionary) {
   std::string dictionary_text(NewSdchDictionary(dictionary_domain));
 
   // Fail the HD with D being the domain and H having a dot requirement.
-  EXPECT_FALSE(sdch_manager()->AddSdchDictionary(dictionary_text,
-               GURL("http://w.x.y.z.google.com")));
+  EXPECT_FALSE(AddSdchDictionary(dictionary_text,
+                                 GURL("http://w.x.y.z.google.com")));
 }
 
 TEST_F(SdchManagerTest, FailToSetRepeatPrefixWithDotDictionary) {
@@ -271,8 +285,8 @@ TEST_F(SdchManagerTest, FailToSetRepeatPrefixWithDotDictionary) {
   std::string dictionary_text(NewSdchDictionary(dictionary_domain));
 
   // Fail the HD with D being the domain and H having a dot requirement.
-  EXPECT_FALSE(sdch_manager()->AddSdchDictionary(dictionary_text,
-               GURL("http://www.google.com.www.google.com")));
+  EXPECT_FALSE(AddSdchDictionary(dictionary_text,
+                                 GURL("http://www.google.com.www.google.com")));
 }
 
 TEST_F(SdchManagerTest, CanSetLeadingDotDomainDictionary) {
@@ -283,8 +297,7 @@ TEST_F(SdchManagerTest, CanSetLeadingDotDomainDictionary) {
 
   // Verify that a leading dot in the domain is acceptable, as long as the host
   // name does not contain any dots preceding the matched domain name.
-  EXPECT_TRUE(sdch_manager()->AddSdchDictionary(dictionary_text,
-               GURL("http://www.google.com")));
+  EXPECT_TRUE(AddSdchDictionary(dictionary_text, GURL("http://www.google.com")));
 }
 
 // Make sure the order of the tests is not helping us or confusing things.
@@ -294,8 +307,8 @@ TEST_F(SdchManagerTest, CanStillSetExactMatchDictionary) {
   std::string dictionary_text(NewSdchDictionary(dictionary_domain));
 
   // Perfect match should *STILL* work.
-  EXPECT_TRUE(sdch_manager()->AddSdchDictionary(dictionary_text,
-              GURL("http://" + dictionary_domain)));
+  EXPECT_TRUE(AddSdchDictionary(dictionary_text,
+                                GURL("http://" + dictionary_domain)));
 }
 
 // Make sure the DOS protection precludes the addition of too many dictionaries.
@@ -303,16 +316,13 @@ TEST_F(SdchManagerTest, TooManyDictionaries) {
   std::string dictionary_domain(".google.com");
   std::string dictionary_text(NewSdchDictionary(dictionary_domain));
 
-  size_t count = 0;
-  while (count <= SdchManager::kMaxDictionaryCount + 1) {
-    if (!sdch_manager()->AddSdchDictionary(dictionary_text,
-                                          GURL("http://www.google.com")))
-      break;
-
+  for (size_t count = 0; count < SdchManager::kMaxDictionaryCount; ++count) {
+    EXPECT_TRUE(AddSdchDictionary(dictionary_text,
+                                  GURL("http://www.google.com")));
     dictionary_text += " ";  // Create dictionary with different SHA signature.
-    ++count;
   }
-  EXPECT_EQ(SdchManager::kMaxDictionaryCount, count);
+  EXPECT_FALSE(
+      AddSdchDictionary(dictionary_text, GURL("http://www.google.com")));
 }
 
 TEST_F(SdchManagerTest, DictionaryNotTooLarge) {
@@ -321,8 +331,8 @@ TEST_F(SdchManagerTest, DictionaryNotTooLarge) {
 
   dictionary_text.append(
       SdchManager::kMaxDictionarySize  - dictionary_text.size(), ' ');
-  EXPECT_TRUE(sdch_manager()->AddSdchDictionary(dictionary_text,
-              GURL("http://" + dictionary_domain)));
+  EXPECT_TRUE(AddSdchDictionary(dictionary_text,
+                                GURL("http://" + dictionary_domain)));
 }
 
 TEST_F(SdchManagerTest, DictionaryTooLarge) {
@@ -331,8 +341,8 @@ TEST_F(SdchManagerTest, DictionaryTooLarge) {
 
   dictionary_text.append(
       SdchManager::kMaxDictionarySize + 1 - dictionary_text.size(), ' ');
-  EXPECT_FALSE(sdch_manager()->AddSdchDictionary(dictionary_text,
-              GURL("http://" + dictionary_domain)));
+  EXPECT_FALSE(AddSdchDictionary(dictionary_text,
+                                 GURL("http://" + dictionary_domain)));
 }
 
 TEST_F(SdchManagerTest, PathMatch) {
@@ -422,8 +432,8 @@ TEST_F(SdchManagerTest, CanUseMultipleManagers) {
 
   // Confirm that if you add directories to one manager, you
   // can't get them from the other.
-  EXPECT_TRUE(sdch_manager()->AddSdchDictionary(
-      dictionary_text_1, GURL("http://" + dictionary_domain_1)));
+  EXPECT_TRUE(AddSdchDictionary(dictionary_text_1,
+                                GURL("http://" + dictionary_domain_1)));
   scoped_refptr<SdchManager::Dictionary> dictionary;
   sdch_manager()->GetVcdiffDictionary(
       server_hash_1,
@@ -431,8 +441,8 @@ TEST_F(SdchManagerTest, CanUseMultipleManagers) {
       &dictionary);
   EXPECT_TRUE(dictionary.get());
 
-  EXPECT_TRUE(second_manager.AddSdchDictionary(
-      dictionary_text_2, GURL("http://" + dictionary_domain_2)));
+  second_manager.AddSdchDictionary(
+      dictionary_text_2, GURL("http://" + dictionary_domain_2));
   second_manager.GetVcdiffDictionary(
       server_hash_2,
       GURL("http://" + dictionary_domain_2 + "/random_url"),
@@ -474,8 +484,8 @@ TEST_F(SdchManagerTest, ClearDictionaryData) {
 
   SdchManager::GenerateHash(dictionary_text, &tmp_hash, &server_hash);
 
-  EXPECT_TRUE(sdch_manager()->AddSdchDictionary(
-      dictionary_text, GURL("http://" + dictionary_domain)));
+  EXPECT_TRUE(AddSdchDictionary(dictionary_text,
+                                GURL("http://" + dictionary_domain)));
   scoped_refptr<SdchManager::Dictionary> dictionary;
   sdch_manager()->GetVcdiffDictionary(
       server_hash,

net/filter/sdch_filter_unittest.cc

@@ -63,9 +63,33 @@ class SdchFilterTest : public testing::Test {
     url_request_context->set_sdch_manager(sdch_manager_.get());
   }
 
+  // Attempt to add a dictionary to the manager; returns whether or not
+  // the attempt succeeded.
+  bool AddSdchDictionary(const std::string& dictionary_text,
+                         const GURL& gurl) {
+    std::string list;
+    sdch_manager_->GetAvailDictionaryList(gurl, &list);
+    sdch_manager_->AddSdchDictionary(dictionary_text, gurl);
+    std::string list2;
+    sdch_manager_->GetAvailDictionaryList(gurl, &list2);
+
+    // The list of hashes should change iff the addition succeeds.
+    return (list != list2);
+  }
+
   MockFilterContext* filter_context() { return filter_context_.get(); }
 
-  std::string NewSdchCompressedData(const std::string dictionary);
+  std::string NewSdchCompressedData(const std::string dictionary) {
+    std::string client_hash;
+    std::string server_hash;
+    SdchManager::GenerateHash(dictionary, &client_hash, &server_hash);
+
+    // Build compressed data that refers to our dictionary.
+    std::string compressed(server_hash);
+    compressed.append("\0", 1);
+    compressed.append(vcdiff_compressed_data_);
+    return compressed;
+  }
 
   const std::string test_vcdiff_dictionary_;
   const std::string vcdiff_compressed_data_;
@@ -75,19 +99,6 @@ class SdchFilterTest : public testing::Test {
   scoped_ptr<MockFilterContext> filter_context_;
 };
 
-std::string SdchFilterTest::NewSdchCompressedData(
-    const std::string dictionary) {
-  std::string client_hash;
-  std::string server_hash;
-  SdchManager::GenerateHash(dictionary, &client_hash, &server_hash);
-
-  // Build compressed data that refers to our dictionary.
-  std::string compressed(server_hash);
-  compressed.append("\0", 1);
-  compressed.append(vcdiff_compressed_data_);
-  return compressed;
-}
-
 //------------------------------------------------------------------------------
 
 
@@ -413,10 +424,10 @@ TEST_F(SdchFilterTest, DictionaryAddOnce) {
 
   std::string url_string = "http://" + kSampleDomain;
   GURL url(url_string);
-  EXPECT_TRUE(sdch_manager_->AddSdchDictionary(dictionary, url));
+  EXPECT_TRUE(AddSdchDictionary(dictionary, url));
 
   // Check we can't add it twice.
-  EXPECT_FALSE(sdch_manager_->AddSdchDictionary(dictionary, url));
+  EXPECT_FALSE(AddSdchDictionary(dictionary, url));
 
   const std::string kSampleDomain2 = "sdchtest2.com";
 
@@ -427,7 +438,7 @@ TEST_F(SdchFilterTest, DictionaryAddOnce) {
 
     std::string url_string2 = "http://" + kSampleDomain2;
     GURL url2(url_string2);
-    EXPECT_TRUE(sdch_manager_->AddSdchDictionary(dictionary2, url2));
+    EXPECT_TRUE(AddSdchDictionary(dictionary2, url2));
   }
 }
 
@@ -439,7 +450,7 @@ TEST_F(SdchFilterTest, BasicDictionary) {
   std::string url_string = "http://" + kSampleDomain;
 
   GURL url(url_string);
-  EXPECT_TRUE(sdch_manager_->AddSdchDictionary(dictionary, url));
+  EXPECT_TRUE(AddSdchDictionary(dictionary, url));
 
   std::string compressed(NewSdchCompressedData(dictionary));
 
@@ -476,7 +487,7 @@ TEST_F(SdchFilterTest, NoDecodeHttps) {
   std::string url_string = "http://" + kSampleDomain;
 
   GURL url(url_string);
-  EXPECT_TRUE(sdch_manager_->AddSdchDictionary(dictionary, url));
+  EXPECT_TRUE(AddSdchDictionary(dictionary, url));
 
   std::string compressed(NewSdchCompressedData(dictionary));
 
@@ -506,7 +517,7 @@ TEST_F(SdchFilterTest, NoDecodeFtp) {
   std::string url_string = "http://" + kSampleDomain;
 
   GURL url(url_string);
-  EXPECT_TRUE(sdch_manager_->AddSdchDictionary(dictionary, url));
+  EXPECT_TRUE(AddSdchDictionary(dictionary, url));
 
   std::string compressed(NewSdchCompressedData(dictionary));
 
@@ -532,7 +543,7 @@ TEST_F(SdchFilterTest, NoDecodeFileColon) {
   std::string url_string = "http://" + kSampleDomain;
 
   GURL url(url_string);
-  EXPECT_TRUE(sdch_manager_->AddSdchDictionary(dictionary, url));
+  EXPECT_TRUE(AddSdchDictionary(dictionary, url));
 
   std::string compressed(NewSdchCompressedData(dictionary));
 
@@ -558,7 +569,7 @@ TEST_F(SdchFilterTest, NoDecodeAboutColon) {
   std::string url_string = "http://" + kSampleDomain;
 
   GURL url(url_string);
-  EXPECT_TRUE(sdch_manager_->AddSdchDictionary(dictionary, url));
+  EXPECT_TRUE(AddSdchDictionary(dictionary, url));
 
   std::string compressed(NewSdchCompressedData(dictionary));
 
@@ -584,7 +595,7 @@ TEST_F(SdchFilterTest, NoDecodeJavaScript) {
   std::string url_string = "http://" + kSampleDomain;
 
   GURL url(url_string);
-  EXPECT_TRUE(sdch_manager_->AddSdchDictionary(dictionary, url));
+  EXPECT_TRUE(AddSdchDictionary(dictionary, url));
 
   std::string compressed(NewSdchCompressedData(dictionary));
 
@@ -610,7 +621,7 @@ TEST_F(SdchFilterTest, CanStillDecodeHttp) {
   std::string url_string = "http://" + kSampleDomain;
 
   GURL url(url_string);
-  EXPECT_TRUE(sdch_manager_->AddSdchDictionary(dictionary, url));
+  EXPECT_TRUE(AddSdchDictionary(dictionary, url));
 
   std::string compressed(NewSdchCompressedData(dictionary));
 
@@ -636,7 +647,7 @@ TEST_F(SdchFilterTest, CrossDomainDictionaryUse) {
   std::string url_string = "http://" + kSampleDomain;
 
   GURL url(url_string);
-  EXPECT_TRUE(sdch_manager_->AddSdchDictionary(dictionary, url));
+  EXPECT_TRUE(AddSdchDictionary(dictionary, url));
 
   std::string compressed(NewSdchCompressedData(dictionary));
 
@@ -675,13 +686,14 @@ TEST_F(SdchFilterTest, DictionaryPathValidation) {
   std::string url_string = "http://" + kSampleDomain;
 
   GURL url(url_string);
-  EXPECT_TRUE(sdch_manager_->AddSdchDictionary(dictionary, url));
+  EXPECT_TRUE(AddSdchDictionary(dictionary, url));
 
   // Create a dictionary with a path restriction, by prefixing dictionary.
   const std::string path("/special_path/bin");
   std::string dictionary_with_path("Path: " + path + "\n");
   dictionary_with_path.append(dictionary);
-  EXPECT_TRUE(sdch_manager_->AddSdchDictionary(dictionary_with_path, url));
+  GURL url2(url_string + path);
+  EXPECT_TRUE(AddSdchDictionary(dictionary_with_path, url2));
 
   std::string compressed_for_path(NewSdchCompressedData(dictionary_with_path));
 
@@ -729,16 +741,15 @@ TEST_F(SdchFilterTest, DictionaryPortValidation) {
   std::string url_string = "http://" + kSampleDomain;
 
   GURL url(url_string);
-  EXPECT_TRUE(sdch_manager_->AddSdchDictionary(dictionary, url));
-
+  EXPECT_TRUE(AddSdchDictionary(dictionary, url));
 
   // Create a dictionary with a port restriction, by prefixing old dictionary.
   const std::string port("502");
   std::string dictionary_with_port("Port: " + port + "\n");
   dictionary_with_port.append("Port: 80\n");  // Add default port.
   dictionary_with_port.append(dictionary);
-  EXPECT_TRUE(sdch_manager_->AddSdchDictionary(dictionary_with_port,
-                                            GURL(url_string + ":" + port)));
+  EXPECT_TRUE(AddSdchDictionary(dictionary_with_port,
+                                GURL(url_string + ":" + port)));
 
   std::string compressed_for_port(NewSdchCompressedData(dictionary_with_port));
 
@@ -859,7 +870,7 @@ TEST_F(SdchFilterTest, FilterChaining) {
   std::string url_string = "http://" + kSampleDomain;
 
   GURL url(url_string);
-  EXPECT_TRUE(sdch_manager_->AddSdchDictionary(dictionary, url));
+  EXPECT_TRUE(AddSdchDictionary(dictionary, url));
 
   std::string sdch_compressed(NewSdchCompressedData(dictionary));
 
@@ -941,7 +952,7 @@ TEST_F(SdchFilterTest, DefaultGzipIfSdch) {
   std::string url_string = "http://" + kSampleDomain;
 
   GURL url(url_string);
-  EXPECT_TRUE(sdch_manager_->AddSdchDictionary(dictionary, url));
+  EXPECT_TRUE(AddSdchDictionary(dictionary, url));
 
   std::string sdch_compressed(NewSdchCompressedData(dictionary));
 
@@ -997,7 +1008,7 @@ TEST_F(SdchFilterTest, AcceptGzipSdchIfGzip) {
   std::string url_string = "http://" + kSampleDomain;
 
   GURL url(url_string);
-  EXPECT_TRUE(sdch_manager_->AddSdchDictionary(dictionary, url));
+  EXPECT_TRUE(AddSdchDictionary(dictionary, url));
 
   std::string sdch_compressed(NewSdchCompressedData(dictionary));
 
@@ -1056,7 +1067,7 @@ TEST_F(SdchFilterTest, DefaultSdchGzipIfEmpty) {
   std::string url_string = "http://" + kSampleDomain;
 
   GURL url(url_string);
-  EXPECT_TRUE(sdch_manager_->AddSdchDictionary(dictionary, url));
+  EXPECT_TRUE(AddSdchDictionary(dictionary, url));
 
   std::string sdch_compressed(NewSdchCompressedData(dictionary));
 
@@ -1112,7 +1123,7 @@ TEST_F(SdchFilterTest, AcceptGzipGzipSdchIfGzip) {
   std::string url_string = "http://" + kSampleDomain;
 
   GURL url(url_string);
-  EXPECT_TRUE(sdch_manager_->AddSdchDictionary(dictionary, url));
+  EXPECT_TRUE(AddSdchDictionary(dictionary, url));
 
   std::string sdch_compressed(NewSdchCompressedData(dictionary));
 

net/net.gypi

@@ -1274,6 +1274,7 @@
       'base/prioritized_dispatcher_unittest.cc',
       'base/priority_queue_unittest.cc',
       'base/registry_controlled_domains/registry_controlled_domain_unittest.cc',
+      'base/sdch_dictionary_fetcher_unittest.cc',
       'base/sdch_manager_unittest.cc',
       'base/static_cookie_policy_unittest.cc',
       'base/test_completion_callback_unittest.cc',

net/url_request/url_fetcher_core.cc

@@ -4,6 +4,8 @@
 
 #include "net/url_request/url_fetcher_core.h"
 
+#include <stdint.h>
+
 #include "base/bind.h"
 #include "base/logging.h"
 #include "base/metrics/histogram.h"
@@ -476,7 +478,7 @@ void URLFetcherCore::SetIgnoreCertificateRequests(bool ignored) {
 }
 
 URLFetcherCore::~URLFetcherCore() {
-  // |request_| should be NULL.  If not, it's unsafe to delete it here since we
+  // |request_| should be NULL. If not, it's unsafe to delete it here since we
   // may not be on the IO thread.
   DCHECK(!request_.get());
 }
@@ -608,8 +610,8 @@ void URLFetcherCore::StartURLRequestWhenAppropriate() {
 
   DCHECK(request_context_getter_.get());
 
-  int64 delay = 0LL;
-  if (original_url_throttler_entry_.get() == NULL) {
+  int64 delay = INT64_C(0);
+  if (!original_url_throttler_entry_.get()) {
     URLRequestThrottlerManager* manager =
         request_context_getter_->GetURLRequestContext()->throttler_manager();
     if (manager) {
@@ -617,12 +619,12 @@ void URLFetcherCore::StartURLRequestWhenAppropriate() {
           manager->RegisterRequestUrl(original_url_);
     }
   }
-  if (original_url_throttler_entry_.get() != NULL) {
+  if (original_url_throttler_entry_.get()) {
     delay = original_url_throttler_entry_->ReserveSendingTimeForNextRequest(
         GetBackoffReleaseTime());
   }
 
-  if (delay == 0) {
+  if (delay == INT64_C(0)) {
     StartURLRequest();
   } else {
     base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
@@ -678,7 +680,7 @@ void URLFetcherCore::InformDelegateFetchIsComplete() {
 
 void URLFetcherCore::NotifyMalformedContent() {
   DCHECK(network_task_runner_->BelongsToCurrentThread());
-  if (url_throttler_entry_.get() != NULL) {
+  if (url_throttler_entry_.get()) {
     int status_code = response_code_;
     if (status_code == URLFetcher::RESPONSE_CODE_INVALID) {
       // The status code will generally be known by the time clients
@@ -768,21 +770,20 @@ void URLFetcherCore::ReleaseRequest() {
 base::TimeTicks URLFetcherCore::GetBackoffReleaseTime() {
   DCHECK(network_task_runner_->BelongsToCurrentThread());
 
-  if (original_url_throttler_entry_.get()) {
-    base::TimeTicks original_url_backoff =
-        original_url_throttler_entry_->GetExponentialBackoffReleaseTime();
-    base::TimeTicks destination_url_backoff;
-    if (url_throttler_entry_.get() != NULL &&
-        original_url_throttler_entry_.get() != url_throttler_entry_.get()) {
-      destination_url_backoff =
-          url_throttler_entry_->GetExponentialBackoffReleaseTime();
-    }
-
-    return original_url_backoff > destination_url_backoff ?
-        original_url_backoff : destination_url_backoff;
-  } else {
+  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();
   }
+
+  return original_url_backoff > destination_url_backoff ?
+      original_url_backoff : destination_url_backoff;
 }
 
 void URLFetcherCore::CompleteAddingUploadDataChunk(
@@ -840,14 +841,16 @@ void URLFetcherCore::DidWriteBuffer(scoped_refptr<DrainableIOBuffer> data,
 }
 
 void URLFetcherCore::ReadResponse() {
-  // Some servers may treat HEAD requests as GET requests.  To free up the
+  // Some servers may treat HEAD requests as GET requests. To free up the
   // network connection as soon as possible, signal that the request has
   // completed immediately, without trying to read any data back (all we care
   // about is the response code and headers, which we already have).
   int bytes_read = 0;
   if (request_->status().is_success() &&
-      (request_type_ != URLFetcher::HEAD))
-    request_->Read(buffer_.get(), kBufferSize, &bytes_read);
+      (request_type_ != URLFetcher::HEAD)) {
+    if (!request_->Read(buffer_.get(), kBufferSize, &bytes_read))
+      bytes_read = -1;  // Match OnReadCompleted() interface contract.
+  }
   OnReadCompleted(request_.get(), bytes_read);
 }
 
@@ -861,7 +864,7 @@ void URLFetcherCore::InformDelegateUploadProgress() {
       if (!is_chunked_upload_) {
         total = static_cast<int64>(request_->GetUploadProgress().size());
         // Total may be zero if the UploadDataStream::Init has not been called
-        // yet.  Don't send the upload progress until the size is initialized.
+        // yet. Don't send the upload progress until the size is initialized.
         if (!total)
           return;
       }

net/url_request/url_fetcher_core.h

@@ -45,15 +45,15 @@ class URLFetcherCore
                  URLFetcher::RequestType request_type,
                  URLFetcherDelegate* d);
 
-  // Starts the load.  It's important that this not happen in the constructor
+  // Starts the load. It's important that this not happen in the constructor
   // because it causes the IO thread to begin AddRef()ing and Release()ing
-  // us.  If our caller hasn't had time to fully construct us and take a
+  // us. If our caller hasn't had time to fully construct us and take a
   // reference, the IO thread could interrupt things, run a task, Release()
   // us, and destroy us, leaving the caller with an already-destroyed object
   // when construction finishes.
   void Start();
 
-  // Stops any in-progress load and ensures no callback will happen.  It is
+  // Stops any in-progress load and ensures no callback will happen. It is
   // safe to call this multiple times.
   void Stop();
 
@@ -111,9 +111,9 @@ class URLFetcherCore
   int GetResponseCode() const;
   const ResponseCookies& GetCookies() const;
   // Reports that the received content was malformed (i.e. failed parsing
-  // or validation).  This makes the throttling logic that does exponential
+  // or validation). This makes the throttling logic that does exponential
   // back-off when servers are having problems treat the current request as
-  // a failure.  Your call to this method will be ignored if your request is
+  // a failure. Your call to this method will be ignored if your request is
   // already considered a failure based on the HTTP response code or response
   // headers.
   void ReceivedContentWasMalformed();
@@ -251,13 +251,13 @@ class URLFetcherCore
   //
   // Both of them can only be accessed on the IO thread.
   //
-  // We need not only the throttler entry for |original_URL|, but also
-  // the one for |url|. For example, consider the case that URL A
-  // redirects to URL B, for which the server returns a 500
-  // response. In this case, the exponential back-off release time of
-  // URL A won't increase. If we retry without considering the
-  // back-off constraint of URL B, we may send out too many requests
-  // for URL A in a short period of time.
+  // To determine the proper backoff timing, throttler entries for
+  // both |original_URL| and |url| are needed. For example, consider
+  // the case that URL A redirects to URL B, for which the server
+  // returns a 500 response. In this case, the exponential back-off
+  // release time of URL A won't increase. If only the backoff
+  // constraints for URL A are considered, too many requests for URL A
+  // may be sent in a short period of time.
   //
   // Both of these will be NULL if
   // URLRequestContext::throttler_manager() is NULL.
@@ -271,11 +271,11 @@ class URLFetcherCore
   // Writer object to write response to the destination like file and string.
   scoped_ptr<URLFetcherResponseWriter> response_writer_;
 
-  // By default any server-initiated redirects are automatically followed.  If
+  // By default any server-initiated redirects are automatically followed. If
   // this flag is set to true, however, a redirect will halt the fetch and call
   // back to to the delegate immediately.
   bool stop_on_redirect_;
-  // True when we're actually stopped due to a redirect halted by the above.  We
+  // True when we're actually stopped due to a redirect halted by the above. We
   // use this to ensure that |url_| is set to the redirect destination rather
   // than the originally-fetched URL.
   bool stopped_on_redirect_;
@@ -286,7 +286,7 @@ class URLFetcherCore
   // true by default.
   bool automatically_retry_on_5xx_;
   // |num_retries_on_5xx_| indicates how many times we've failed to successfully
-  // fetch this URL due to 5xx responses.  Once this value exceeds the maximum
+  // fetch this URL due to 5xx responses. Once this value exceeds the maximum
   // number of retries specified by the owner URLFetcher instance,
   // we'll give up.
   int num_retries_on_5xx_;