[sync] Allow FakeURLFetcher to return an arbitrary URLRequestStatus

[sync] Allow FakeURLFetcher to return an arbitrary URLRequestStatus

In r232724, FakeURLFetcher went from being able to return only HTTP/200
or HTTP/500, to being able to return any arbitrary HttpResponseCode.
However, the URLRequestStatus returned was hard coded to FAILURE for
HTTP/5xx and SUCCESS for all other codes.

This patch further modifies FakeURLFetcher to be able to return
arbitrary URLRequestStatus values in addition to an HttpResponseCode.
We no longer hard code the URLRequestStatus based on the
HttpResponseCode being returned. It also updates all call sites that
currently use FakeURLFetcher.

R=achuith@chromium.org, ajwong@chromium.org, akalin@chromium.org, mattm@chromium.org, mmenke@chromium.org, nyquist@chromium.org, sky@chromium.org, tim@chromium.org, vitalybuka@chromium.org
TBR=bengr, sergeyu
BUG=313905
TEST=All existing tests pass trybots and waterfall

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=233304

[Raghu Simha, 2013-11-05 21:43:27]

Matt / Fred / Tim, please review.

This CL fixes the concerns raised in https://codereview.chromium.org/48713008.

Again, the meat of this CL is in
net/url_request/test_url_fetcher_factory.{h|cc}.

Thanks.

[akalin, 2013-11-05 22:03:09]

https://codereview.chromium.org/60923002/diff/540001/net/url_request/test_url...
File net/url_request/test_url_fetcher_factory.cc (right):

https://codereview.chromium.org/60923002/diff/540001/net/url_request/test_url...
net/url_request/test_url_fetcher_factory.cc:367: response_data,
URLRequestStatus(status, response_code));
is the 'error' field in URLRequestStatus supposed to hold a response code? From
the comments it seems like it's supposed to hold a net::Error. I'd rather just
have the FakeURLFetcher take in and store the response_code and status
separately, or update the comments to URLRequestStatus to indicate the error
field == response code (and/or update the name of that field, too).

[Raghu Simha, 2013-11-05 22:09:06]

https://codereview.chromium.org/60923002/diff/540001/net/url_request/test_url...
File net/url_request/test_url_fetcher_factory.cc (right):

https://codereview.chromium.org/60923002/diff/540001/net/url_request/test_url...
net/url_request/test_url_fetcher_factory.cc:367: response_data,
URLRequestStatus(status, response_code));
On 2013/11/05 22:03:09, akalin wrote:
> is the 'error' field in URLRequestStatus supposed to hold a response code?
From
> the comments it seems like it's supposed to hold a net::Error. I'd rather just
> have the FakeURLFetcher take in and store the response_code and status
> separately, or update the comments to URLRequestStatus to indicate the error
> field == response code (and/or update the name of that field, too).

In that case, it probably makes sense to store the response_code and status
separately. This means we'll have to change FakeResponseMap to be a mapping
between a GURL and a struct containing response text, response code and status.

Updated CL coming up.

[Raghu Simha, 2013-11-05 23:00:12]

Fred, PTAL.

[Raghu Simha, 2013-11-05 23:00:43]

Adding more reviewers for OWNERS approval:

ajwong@: chrome/service
nyquist@: components/dom_distiller
bengr@: components/precache
sergeyu@: remoting
achuith@: chrome/browser/chromeos
vitalybuka@: chrome/browser/local_discovery
mmenke@: chrome/browser/net
stevenjb@: chrome/browser/notifications
battre@: chrome/browser/profile_resetter
mattm@: chrome/browser/safe_browsing
tim@: chrome/browser/{signin|sync}
sky@: chrome/browser/ui

[Vitaly Buka (NO REVIEWS), 2013-11-05 23:10:14]

lgtm
chrome/browser/local_discovery
chrome/service/cloud_print/

[sky, 2013-11-05 23:46:50]

c/b/ui LGTM

[achuithb, 2013-11-05 23:51:08]

On 2013/11/05 23:46:50, sky wrote:
> c/b/ui LGTM

c/b/cros lgtm

[akalin, 2013-11-06 00:11:02]

almost there

https://codereview.chromium.org/60923002/diff/900001/components/precache/core...
File components/precache/core/precache_fetcher_unittest.cc (right):

https://codereview.chromium.org/60923002/diff/900001/components/precache/core...
components/precache/core/precache_fetcher_unittest.cc:39: if
(net::HttpStatusCode(request_status.error()) == net::HTTP_OK) {
is this still correct? revert?

https://codereview.chromium.org/60923002/diff/900001/net/url_request/test_url...
File net/url_request/test_url_fetcher_factory.h (right):

https://codereview.chromium.org/60923002/diff/900001/net/url_request/test_url...
net/url_request/test_url_fetcher_factory.h:414: typedef struct {
no need for typedef, just do:

struct FakeURLResponse { ... }

[mattm, 2013-11-06 00:17:03]

safe_browsing lgtm

[nyquist, 2013-11-06 06:50:53]

components/dom_distiller/ lgtm

[tim (not reviewing), 2013-11-06 15:25:42]

Only reviewed sync/, which LGTM

[Raghu Simha, 2013-11-06 15:27:59]

Thanks for the review, everyone.

Fred: Feedback addressed. PTAL.

https://codereview.chromium.org/60923002/diff/900001/components/precache/core...
File components/precache/core/precache_fetcher_unittest.cc (right):

https://codereview.chromium.org/60923002/diff/900001/components/precache/core...
components/precache/core/precache_fetcher_unittest.cc:39: if
(net::HttpStatusCode(request_status.error()) == net::HTTP_OK) {
On 2013/11/06 00:11:02, akalin wrote:
> is this still correct? revert?

Not needed any more. Reverted.

https://codereview.chromium.org/60923002/diff/900001/net/url_request/test_url...
File net/url_request/test_url_fetcher_factory.h (right):

https://codereview.chromium.org/60923002/diff/900001/net/url_request/test_url...
net/url_request/test_url_fetcher_factory.h:414: typedef struct {
On 2013/11/06 00:11:02, akalin wrote:
> no need for typedef, just do:
> 
> struct FakeURLResponse { ... }

Done.

[mmenke, 2013-11-06 15:40:11]

https://codereview.chromium.org/60923002/diff/1110001/net/url_request/test_ur...
File net/url_request/test_url_fetcher_factory.cc (right):

https://codereview.chromium.org/60923002/diff/1110001/net/url_request/test_ur...
net/url_request/test_url_fetcher_factory.cc:290:
set_status(URLRequestStatus(status, 0));
I think the second parameter should be OK when status is SUCCESS, ERR_FAILED
when status is FAILED, and ERR_ABORTED when status is CANCELED.

https://codereview.chromium.org/60923002/diff/1110001/net/url_request/test_ur...
File net/url_request/test_url_fetcher_factory.h (right):

https://codereview.chromium.org/60923002/diff/1110001/net/url_request/test_ur...
net/url_request/test_url_fetcher_factory.h:337: //                         
URLRequestStatus::FAILED);
This really should be URLRequestStatus::SUCCESS.

One option would be to have two or three different functions:

SetFakeSuccessfulResponse(url, body, response code);

And

SetFakeFailedResponse(url)

Could also have a cancelled version, or make SetFakeFailedResponse take in a
value indicating if it was cancelled or failed, but I'm not sure any consumers
really need to differentiate the two.  Or could just fix the example.  I am
rather worried about incorrect assumptions, however.

[Raghu Simha, 2013-11-06 16:58:58]

Matt, PTAL.

https://codereview.chromium.org/60923002/diff/1110001/net/url_request/test_ur...
File net/url_request/test_url_fetcher_factory.cc (right):

https://codereview.chromium.org/60923002/diff/1110001/net/url_request/test_ur...
net/url_request/test_url_fetcher_factory.cc:290:
set_status(URLRequestStatus(status, 0));
On 2013/11/06 15:40:12, mmenke wrote:
> I think the second parameter should be OK when status is SUCCESS, ERR_FAILED
> when status is FAILED, and ERR_ABORTED when status is CANCELED.

Done.

https://codereview.chromium.org/60923002/diff/1110001/net/url_request/test_ur...
File net/url_request/test_url_fetcher_factory.h (right):

https://codereview.chromium.org/60923002/diff/1110001/net/url_request/test_ur...
net/url_request/test_url_fetcher_factory.h:337: //                         
URLRequestStatus::FAILED);
On 2013/11/06 15:40:12, mmenke wrote:
> This really should be URLRequestStatus::SUCCESS.
> 
> One option would be to have two or three different functions:
> 
> SetFakeSuccessfulResponse(url, body, response code);
> 
> And
> 
> SetFakeFailedResponse(url)
> 
> Could also have a cancelled version, or make SetFakeFailedResponse take in a
> value indicating if it was cancelled or failed, but I'm not sure any consumers
> really need to differentiate the two.  Or could just fix the example.  I am
> rather worried about incorrect assumptions, however.

I've fixed the example section to include both the URLRequestStatus::SUCCESS and
URLRequestStatus::FAILED cases. Let me know what you think.

[mmenke, 2013-11-06 17:07:21]

LGTM, just a couple tiny suggestions.

https://codereview.chromium.org/60923002/diff/1310001/net/url_request/test_ur...
File net/url_request/test_url_fetcher_factory.cc (right):

https://codereview.chromium.org/60923002/diff/1310001/net/url_request/test_ur...
net/url_request/test_url_fetcher_factory.cc:295: error = net::OK;
nit:  net:: not needed for any of these, since we're in the net namespace.

https://codereview.chromium.org/60923002/diff/1310001/net/url_request/test_ur...
net/url_request/test_url_fetcher_factory.cc:295: error = net::OK;
Should theoretically use "error = ERR_IO_PENDING" in the IO_PENDING case.

https://codereview.chromium.org/60923002/diff/1310001/net/url_request/test_ur...
net/url_request/test_url_fetcher_factory.cc:305: break;
If you remove the default case, there will be a compiler error if a new status
is added, instead, which I think is slightly preferable.

Alternatively, you could make it a function in a private namespace, and use both
a NOTREACHED() and get the compiler error (return in each case, NOTREACHED()
outside the switch).

[Raghu Simha, 2013-11-06 17:31:40]

Nits fixed. Thanks for the review.

https://codereview.chromium.org/60923002/diff/1310001/net/url_request/test_ur...
File net/url_request/test_url_fetcher_factory.cc (right):

https://codereview.chromium.org/60923002/diff/1310001/net/url_request/test_ur...
net/url_request/test_url_fetcher_factory.cc:295: error = net::OK;
On 2013/11/06 17:07:21, mmenke wrote:
> nit:  net:: not needed for any of these, since we're in the net namespace.

Done.

https://codereview.chromium.org/60923002/diff/1310001/net/url_request/test_ur...
net/url_request/test_url_fetcher_factory.cc:295: error = net::OK;
On 2013/11/06 17:07:21, mmenke wrote:
> Should theoretically use "error = ERR_IO_PENDING" in the IO_PENDING case.

Done.

https://codereview.chromium.org/60923002/diff/1310001/net/url_request/test_ur...
net/url_request/test_url_fetcher_factory.cc:305: break;
On 2013/11/06 17:07:21, mmenke wrote:
> If you remove the default case, there will be a compiler error if a new status
> is added, instead, which I think is slightly preferable.
> 
> Alternatively, you could make it a function in a private namespace, and use
both
> a NOTREACHED() and get the compiler error (return in each case, NOTREACHED()
> outside the switch).

Default case removed. Done.

[Raghu Simha, 2013-11-06 18:03:10]

I'll hold off on landing this for a while to give Fred a chance to take another
look.

[awong, 2013-11-06 18:05:18]

service LGTM

[akalin, 2013-11-06 18:10:29]

lgtm

[Raghu Simha, 2013-11-06 18:37:19]

Committed patchset #7 manually as r233304 (presubmit successful).