Refactored FakeURLFetcher to make it more flexible

net/url_request/test_url_fetcher_factory.h

Index: net/url_request/test_url_fetcher_factory.h
diff --git a/net/url_request/test_url_fetcher_factory.h b/net/url_request/test_url_fetcher_factory.h
index d7f0d305481c7e5588c96f8ffa907ea1b2a1ffab..8af5c4b9b29cd34865b8c24697b96bc8113b7c84 100644
--- a/net/url_request/test_url_fetcher_factory.h
+++ b/net/url_request/test_url_fetcher_factory.h
@@ -317,6 +317,13 @@ class FakeURLFetcherFactory : public URLFetcherFactory,
   // SetFakeResponse().
   void ClearFakeResponses();
 
+ protected:
+  // For use when subclassing FakeURLFetcherFactory.
+  // If no fake response is set, return NULL.
+  virtual TestURLFetcher* CreateFakeURLFetcher(const GURL& URL,

[akalin, 2013/02/08 07:47:35]

I'm confused by the need for this.  FakeURLFetcherFactory already has the
parameter default_factory, so you can implement URLFetcherFactory directly and
have it do whatever you want, passing it into FakeURLFetcherFactory.  Is there a
reason why this won't work for you?

Also, in general, I prefer passing in parameters over subclassing for
customization.

[Noam Samuel (WRONG ACCOUNT), 2013/02/08 16:27:10]

The main scenario I'm trying to support is the ability to simulate HTTP headers
using FakeURLFetcher. Without this modification, the cleanest way to do that
would be to re-implement FakeURLFetcherFactory using TestURLFetcher. Another way
to address this would be to add an optional "headers" parameter to
SetFakeResponse, but I suspected that adding "just one more" HTTP feature to
simulate is a slippery slope to feature creep.

On 2013/02/08 07:47:35, akalin wrote:

[noelutz, 2013/02/08 18:21:40]

I don't have a strong opinion here but I would also lean towards avoiding
sub-classing.  Have you considered exposing the FakeUrlFetcher class in the
header file and then adding a more generic function to the factory class which
sets fake fetchers?  E.g., void SetFakeFetcher(FakeURLFetcher* fetcher);  That
would give you all the flexibility you need, wouldn't it?

On 2013/02/08 16:27:10, noamsml wrote:

[Noam Samuel (WRONG ACCOUNT), 2013/02/08 18:58:44]

Not sure how that would work. The FakeURLFetcherFactory returns a new object
each time, and the user of the URL fetcher is responsible for deleting it. It
wouldn't really make sense in this context to set a static FakeURLFetcher that
is returned, even per URL (since the tested code may instantiate multiple
fetchers for the same URL). 

One option that avoids subclassing would be to surface a delegate interface that
will be invoked on URL Fetcher creation and passed the URL and a pointer to the
fetcher (as a TestURLFetcher*, allowing the test code to set additional
properties). Would this be more elegant?

On 2013/02/08 18:21:40, noelutz wrote:

+                                       URLFetcher::RequestType request_type,
+                                       URLFetcherDelegate* d);
+
  private:
   typedef std::map<GURL, std::pair<std::string, bool> > FakeResponseMap;
   FakeResponseMap fake_responses_;