Refactored FakeURLFetcher to make it more flexible

net/url_request/test_url_fetcher_factory.h

 // Copyright (c) 2012 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.
 
 #ifndef NET_URL_REQUEST_TEST_URL_FETCHER_FACTORY_H_
 #define NET_URL_REQUEST_TEST_URL_FETCHER_FACTORY_H_
 
 #include <list>
 #include <map>
 #include <string>
@@ skipping 299 matching lines @@
   // Sets the fake response for a given URL.  If success is true we will serve
   // an HTTP/200 and an HTTP/500 otherwise.  The |response_data| may be empty.
   void SetFakeResponse(const std::string& url,
                        const std::string& response_data,
                        bool success);
 
   // Clear all the fake responses that were previously set via
   // 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_;
   URLFetcherFactory* default_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(FakeURLFetcherFactory);
 };
 
 // This is an implementation of URLFetcherFactory that will create a
 // URLFetcherImpl. It can be use in conjunction with a FakeURLFetcherFactory in
 // integration tests to control the behavior of some requests but execute
 // all the other ones.
 class URLFetcherImplFactory : public URLFetcherFactory {
  public:
   URLFetcherImplFactory();
   virtual ~URLFetcherImplFactory();
 
   // This method will create a real URLFetcher.
   virtual URLFetcher* CreateURLFetcher(
       int id,
       const GURL& url,
       URLFetcher::RequestType request_type,
       URLFetcherDelegate* d) OVERRIDE;
 
 };
 
 }  // namespace net
 
 #endif  // NET_URL_REQUEST_TEST_URL_FETCHER_FACTORY_H_