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..9dae9e645393cec26b5e02c84f64452d464a7238 100644
--- a/net/url_request/test_url_fetcher_factory.h
+++ b/net/url_request/test_url_fetcher_factory.h
@@ -10,13 +10,17 @@
 #include <string>
 #include <utility>
 
+

[akalin, 2013/02/12 23:10:10]

remove extra newline

[Noam Samuel (WRONG ACCOUNT), 2013/02/13 00:25:52]

Done.

+#include "base/callback.h"
 #include "base/compiler_specific.h"
+#include "base/memory/weak_ptr.h"
 #include "base/threading/non_thread_safe.h"
 #include "googleurl/src/gurl.h"
 #include "net/http/http_request_headers.h"
 #include "net/url_request/url_fetcher_factory.h"
 #include "net/url_request/url_request_status.h"
 
+

[akalin, 2013/02/12 23:10:10]

remove extra newline

[Noam Samuel (WRONG ACCOUNT), 2013/02/13 00:25:52]

Done.

 namespace net {
 
 // Changes URLFetcher's Factory for the lifetime of the object.
@@ -287,13 +291,27 @@ class TestURLFetcherFactory : public URLFetcherFactory,
 //  SomeService service;
 //  service.Run();  // Will eventually request these two URLs.
 
+class FakeURLFetcher;
 class FakeURLFetcherFactory : public URLFetcherFactory,
                               public ScopedURLFetcherFactory {
  public:
+  typedef base::Callback< FakeURLFetcher*(

[akalin, 2013/02/12 23:10:10]

no space after <

[akalin, 2013/02/12 23:10:10]

add comment describing what the parameters are (since they don't have names)

[akalin, 2013/02/12 23:10:10]

since the caller should take ownership of the returned FakeURLFetcher*, perhaps
have it return a scoped_ptr<FakeURLFetcher>?

[Noam Samuel (WRONG ACCOUNT), 2013/02/13 00:25:52]

Done.

[Noam Samuel (WRONG ACCOUNT), 2013/02/13 00:25:52]

Since this callback is only called by the factory itself, which should return a
URLFetcher* (per its interface), I do not think this is the case.

[Noam Samuel (WRONG ACCOUNT), 2013/02/13 00:25:52]

Done.

[akalin, 2013/02/13 20:44:01]

I'd argue that the factory method should also return a scoped_ptr<>.  That might
require touching too many files, though, so I'm okay with leaving the
scoped_ptr<> in the callback and calling release.

+      const GURL&,
+      URLFetcherDelegate*,
+      const std::string&,
+      bool) > FakeURLFetcherCreator;

[akalin, 2013/02/12 23:10:10]

no space before >

[Noam Samuel (WRONG ACCOUNT), 2013/02/13 00:25:52]

Done.

+
   FakeURLFetcherFactory();

[akalin, 2013/02/12 23:10:10]

see if you can get away with only having a single constructor.  There are only a
few places that uses FakeURLFetcherFactory so it shouldn't be too hard.  You may
need to expose a "MakeFakeURLFetcher()" function that simply calls new
FakeURLFetcher....

[Noam Samuel (WRONG ACCOUNT), 2013/02/13 00:25:52]

Since the callback has an obvious but nontrivial default 
value, wouldn't it make sense to leave the option of not 
passing it? The creation of this class may be confusing 
otherwise.

On 2013/02/12 23:10:10, akalin wrote:

[akalin, 2013/02/13 20:44:01]

I suppose. Although I would at least like to get rid of the default constructor
(and have callers pass in NULL).

[Noam Samuel (WRONG ACCOUNT), 2013/02/13 21:37:46]

Added to latest patch set. If it's too cumbersome as part of this CL, it can
also be factored into its own CL.

   // FakeURLFetcherFactory that will delegate creating URLFetcher for unknown
   // url to the given factory.
   explicit FakeURLFetcherFactory(URLFetcherFactory* default_factory);
+
+  // FakeURLFetcherFactory that will delegate creating URLFetcher for unknown

[akalin, 2013/02/12 23:10:10]

this comment isn't a complete sentence and is hard to understand.  Perhaps
something like:

// |creator| is a callback that ...
// |default_factory|, which can be NULL, is a ...

[Noam Samuel (WRONG ACCOUNT), 2013/02/13 00:25:52]

Done.

+  // url to the given factory (can be NULL) and will delegate creating for
+  // known url to given callback
+  FakeURLFetcherFactory(const FakeURLFetcherCreator& creator,
+                        URLFetcherFactory* default_factory);
+
   virtual ~FakeURLFetcherFactory();
 
   // If no fake response is set for the given URL this method will delegate the
@@ -313,18 +331,55 @@ class FakeURLFetcherFactory : public URLFetcherFactory,
                        const std::string& response_data,
                        bool success);
 
+

[akalin, 2013/02/12 23:10:10]

remove extra newline

[Noam Samuel (WRONG ACCOUNT), 2013/02/13 00:25:52]

Done.

   // Clear all the fake responses that were previously set via
   // SetFakeResponse().
   void ClearFakeResponses();
 
  private:
+  FakeURLFetcherCreator creator_;

[akalin, 2013/02/12 23:10:10]

make it const

[Noam Samuel (WRONG ACCOUNT), 2013/02/13 00:25:52]

Done.

   typedef std::map<GURL, std::pair<std::string, bool> > FakeResponseMap;
   FakeResponseMap fake_responses_;
   URLFetcherFactory* default_factory_;

[akalin, 2013/02/12 23:10:10]

make this const, like

URLFetcherFactory* const 

[Noam Samuel (WRONG ACCOUNT), 2013/02/13 00:25:52]

Done.

 
+
+  static FakeURLFetcher* DefaultFakeURLFetcherCreator(
+      const GURL& url,
+      URLFetcherDelegate* delegate,
+      const std::string& response,
+      bool success);
   DISALLOW_COPY_AND_ASSIGN(FakeURLFetcherFactory);
 };
 
+
+
+// This class is used by the FakeURLFetcherFactory above.

[akalin, 2013/02/12 23:10:10]

Try to describe what the FakeURLFetcher does without referring to
FakeURLFetcherFactory.

[Noam Samuel (WRONG ACCOUNT), 2013/02/13 00:25:52]

Done.

+class FakeURLFetcher : public TestURLFetcher {

[akalin, 2013/02/12 23:10:10]

you can move this declaration up above FakeURLFetcherFactory instead of just
forward-declaring.

[Noam Samuel (WRONG ACCOUNT), 2013/02/13 00:25:52]

Done.

+ public:
+  // Normal URL fetcher constructor but also takes in a pre-baked response.
+  FakeURLFetcher(const GURL& url,
+                 URLFetcherDelegate* d,
+                 const std::string& response_data, bool success);
+
+  // Start the request.  This will call the given delegate asynchronously
+  // with the pre-baked response as parameter.
+  virtual void Start() OVERRIDE;
+
+  virtual const GURL& GetURL() const OVERRIDE;
+
+ private:
+  virtual ~FakeURLFetcher();
+
+  // This is the method which actually calls the delegate that is passed in the
+  // constructor.
+  void RunDelegate();
+
+  base::WeakPtrFactory<FakeURLFetcher> weak_factory_;
+
+  DISALLOW_COPY_AND_ASSIGN(FakeURLFetcher);
+};
+
+
 // 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