[sync] Allow FakeURLFetcher to return arbitrary HTTP response codes

net/url_request/test_url_fetcher_factory.cc

Index: net/url_request/test_url_fetcher_factory.cc
diff --git a/net/url_request/test_url_fetcher_factory.cc b/net/url_request/test_url_fetcher_factory.cc
index e4a47d7728080adf5fbcfb14ffb6bd4e0d76b21c..d5acbb9df8a273bc7bd71241f707e75f247bdca5 100644
--- a/net/url_request/test_url_fetcher_factory.cc
+++ b/net/url_request/test_url_fetcher_factory.cc
@@ -283,13 +283,14 @@ void TestURLFetcherFactory::SetDelegateForTests(
 FakeURLFetcher::FakeURLFetcher(const GURL& url,
                                URLFetcherDelegate* d,
                                const std::string& response_data,
-                               bool success)
+                               HttpStatusCode response_code)
     : TestURLFetcher(0, url, d),
       weak_factory_(this) {
   set_status(URLRequestStatus(
-      success ? URLRequestStatus::SUCCESS : URLRequestStatus::FAILED,
+      response_code == HTTP_OK ? URLRequestStatus::SUCCESS :
+                                 URLRequestStatus::FAILED,

[mmenke, 2013/11/01 21:05:02]

I don't believe this is correct.  I think we actually report SUCCESS on most
HTTP status codes - or at least I can't find code that only reports success on
2xx responses.  This is mostly used for network errors.

My suggestion is just to take a response code and a network status code.  Set
URLRequestStatus to SUCCESS for net::OK, CANCELLED for net::ERR_ABORTED, and
FAILED for everything else.

[Raghu Simha, 2013/11/01 21:55:16]

Good point. The logic I was using worked because the only two codes being
returned were HTTP_OK and HTTP_INTERNAL_SERVER_ERROR.
Given that this class is used only for testing, and returns HTTP_OK a vast
majority of the time, I'm hesitant to add an extra parameter due to the number
of call sites that need to be updated.

I searched through the code base for an existing mapping between HttpStatusCode
(net/http/http_status_code_list.h) and URLRequestStatus::Status
(net/url_request/url_request_status.h), but couldn't find any. I've changed the
logic to return FAILED for all HTTP/5xx server errors, and SUCCESS for all
HTTP/2xx, 3xx, and 4xx errors. Does this seem all right?

As of today, there's no test code in the code base that uses anything other than
HTTP_OK or HTTP_INTERNAL_SERVER_ERROR, so the new logic I suggest above will
continue to work as before. In future, I plan on adding tests that return
HTTP_UNAUTHORIZED, and will make sure URLRequestStatus::SUCCESS works in that
case.

Let me know what you think.

[akalin, 2013/11/01 22:36:59]

This sounds reasonable, but you probably should document this in a comment.

[mmenke, 2013/11/01 23:10:56]

I think the point about updating callsites is valid, and complexity for those
who don't have to care about it is a valid concern as well.

However, I worry about having people test bogus behaviors here.  If consumers
expect to get failures on 5xx errors, they could end up misusing the class,
resulting in bugs.  Not sure about a reasonable solution, I'll think about it.

       0));
-  set_response_code(success ? 200 : 500);
+  set_response_code(response_code);
   SetResponseString(response_data);
 }
 
@@ -327,10 +328,12 @@ FakeURLFetcherFactory::FakeURLFetcherFactory(
 scoped_ptr<FakeURLFetcher> FakeURLFetcherFactory::DefaultFakeURLFetcherCreator(
       const GURL& url,
       URLFetcherDelegate* delegate,
-      const std::string& response,
-      bool success) {
-  return scoped_ptr<FakeURLFetcher>(new FakeURLFetcher(url, delegate,
-                                                       response, success));
+      const std::string& response_data,
+      HttpStatusCode response_code) {
+  return scoped_ptr<FakeURLFetcher>(new FakeURLFetcher(url,
+                                                       delegate,
+                                                       response_data,
+                                                       response_code));
 }
 
 FakeURLFetcherFactory::~FakeURLFetcherFactory() {}
@@ -360,9 +363,9 @@ URLFetcher* FakeURLFetcherFactory::CreateURLFetcher(
 void FakeURLFetcherFactory::SetFakeResponse(
     const GURL& url,
     const std::string& response_data,
-    bool success) {
+    HttpStatusCode response_code) {
   // Overwrite existing URL if it already exists.
-  fake_responses_[url] = std::make_pair(response_data, success);
+  fake_responses_[url] = std::make_pair(response_data, response_code);
 }
 
 void FakeURLFetcherFactory::ClearFakeResponses() {