Change SDCHDictionaryFetcher to use URLRequest instead of URLFetcher.

net/base/sdch_dictionary_fetcher.h

 // 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.
 
-// 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_
 
 #include <queue>
 #include <set>
 #include <string>
 
 #include "base/memory/scoped_ptr.h"
 #include "base/memory/weak_ptr.h"
 #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

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

anal-retentive nit: once space, not two, between sentences.

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

Apologies for getting a-r back at you, but could you give me a style guide
reference?  The web indicates that there's more debate about this than I thought
(http://en.wikipedia.org/wiki/Sentence_spacing), but that's not how I was taught
to write sentences.

+// 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 |*manager| and |*context| outlive
+  // this object.
+  SdchDictionaryFetcher(SdchManager* manager, URLRequestContext* context);
   virtual ~SdchDictionaryFetcher();
 
   // Implementation of SdchFetcher class.
   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;
+  enum State {
+    STATE_NONE,
+    STATE_IDLE,
+    STATE_REQUEST_STARTED,
+    STATE_REQUEST_READING,
+    STATE_REQUEST_COMPLETE,
+  };
 
-  // Ensure the download after the above delay.
-  void ScheduleDelayedRun();
+  // State machine implementation.
+  int DoLoop(int rv);
+  int DoDispatchRequest(int rv);
+  int DoRequestStarted(int rv);
+  int DoRead(int rv);
+  int DoCompleteRequest(int rv);
 
-  // Make sure we're processing (or waiting for) the the arrival of the next URL
-  // in the  |fetch_queue_|.
-  void StartFetching();
+  // URLRequest::Delegate overrides
+  virtual void OnResponseStarted(URLRequest* request) OVERRIDE;
+  virtual void OnReadCompleted(URLRequest* request, int bytes_read) OVERRIDE;

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

I argue with pkasting a lot about this, whether or not it's acceptable to 'hide'
methods like this.

If you feel strongly about it, fine, but I tend to prefer to keep the same
visibility as the base class, since by virtue of public inheritance, these
methods are exposed.

That is, just because they're private, someone can always

URLRequestDelegate* fetcher = somesdchdictionaryfetcher;
fetcher->OnResponseStarted(...) stuff

(Yes, I know no one will, but that's the idea behind it)

If you really did want to keep it private, you have to double-indirect. Namely,
SdchDictionaryFetcher is NOT a URLRequest::Delegate, but it HAS a
URLRequest::Delegate subclass (that's private), that it's friended

private:
  class Inner;
  friend class Inner;

  void OnResponseStarted(...);
  void OnReadCompleted(...);  // no virtual OR override

class Fetcher::Inner : public URLRequest::Delegate {
  explicit Inner(Fetcher* fetcher);

  virtual void OnResponseStarted(URLRequest* request) OVERRIDE {
    fetcher_->OnResponseStarted(request);
  }
  virtual void OnReadCompleted(...) OVERRIDE {
    fetcher_->OnReadCompleted(...);
  }
};


The above pattern is good not just for breaking visibility issues, but it also
helps solve encapsulation issues (e.g. "what if" URLRequest::Delegate has some
method added that conflicts with another interface here? What if it becomes a
ref-counted type? etc)

The compiler is generally smart enough to optimize this into (effectively) the
same code (your vtable for your Inner-as-a-Delegate just pops the member var
after the vtable offset and passes it into the concrete Fetcher class as the
first arg / 'this' pointer, as opposed to having the Fetcher-as-a-Delegate,
which... pops the this pointer after the vtable offset and passes it to the
concrete Fetcher class as the first arg / 'this' pointer)

In order of preference:
1) Prefer the above
2) Make these two methods public, since it's a lie to call them private even if
you want them to be
3) Do nothing. I feel strongly about this pattern, but there's enough vocal
disagreement from some (but not all) that it's not a fight worth having if you
don't agree :)

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

Wow. Ok, will do.

(I personally prefer the style as written, and while I acknowledge the virtues
of the approach you sketch out above, I consider it too much extra boilerplate
to be worth it.  However, the feelings are weak in this one; so I'll do it your
way on this CL.)

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

Sorry, I realized this was ambiguous.  I've made the methods public.

 
-  // Implementation of URLFetcherDelegate. Called after transmission
-  // completes (either successfully or with failure).
-  virtual void OnURLFetchComplete(const URLFetcher* source) OVERRIDE;
+  State next_state_;
+  bool in_loop_;
 
   SdchManager* const manager_;
 
   // 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_;
 
-  // 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_;
-  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
   // 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
   // bandwidth trying repeatedly).
   // The following set lists all the dictionary URLs that we've tried to load,
   // so that we won't try to load from an URL more than once.
   // 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_;
+
+  base::WeakPtrFactory<SdchDictionaryFetcher> weak_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(SdchDictionaryFetcher);
 };
 
 }  // namespace net
 
 #endif  // NET_BASE_SDCH_DICTIONARY_FETCHER_H_