Fix all failed and canceled URLRequestStatuses without errors.

Fix all failed and canceled URLRequestStatuses without errors.

As a step towards removing URLRequestStatus::Status, stop creating FAILED
and CANCELED URLRequestStatuses without supplying an error.

IO_PENDING is left for later as that'll want a bit more careful auditing of
error() callers.

BUG=490311
Committed: https://crrev.com/5c411d94add205780f6e6c701aecc9b297d885b0
Cr-Commit-Position: refs/heads/master@{#341913}

[davidben, 2015-07-16 21:28:46]

davidben@chromium.org changed reviewers:
+ mmenke@chromium.org

[davidben, 2015-07-16 21:28:46]

I'm thinking after this we *should* be able to assume that, unless you have
IO_PENDING, status.error() will have something sensible in there.

There was just the one instance outside of test code.

Next step is probably to convert the FakeURLFetcher constructor parameter to a
net::Error, which should do away with a lot of URLRequestStatus::Status
consumers.

[mmenke, 2015-07-16 21:46:32]

On 2015/07/16 21:28:46, David Benjamin wrote:
> I'm thinking after this we *should* be able to assume that, unless you have
> IO_PENDING, status.error() will have something sensible in there.
> 
> There was just the one instance outside of test code.
> 
> Next step is probably to convert the FakeURLFetcher constructor parameter to a
> net::Error, which should do away with a lot of URLRequestStatus::Status
> consumers.

Should we do a DCHECK in URLRequestStatus::FromError that
ErrorToShortString(error) is not "<unknown>" or add a debug-only switch
statement to DCHECK on non-net error?  Or just make it take a net::Error, and
add casts to the places that need it?

Suppose given the relatively low number of files this touches, regressions
aren't too likely, but I like DCHECKs.  :)

[mmenke, 2015-07-16 21:48:08]

On 2015/07/16 21:46:32, mmenke wrote:
> On 2015/07/16 21:28:46, David Benjamin wrote:
> > I'm thinking after this we *should* be able to assume that, unless you have
> > IO_PENDING, status.error() will have something sensible in there.
> > 
> > There was just the one instance outside of test code.
> > 
> > Next step is probably to convert the FakeURLFetcher constructor parameter to
a
> > net::Error, which should do away with a lot of URLRequestStatus::Status
> > consumers.
> 
> Should we do a DCHECK in URLRequestStatus::FromError that
> ErrorToShortString(error) is not "<unknown>" or add a debug-only switch
> statement to DCHECK on non-net error?  Or just make it take a net::Error, and
> add casts to the places that need it?
> 
> Suppose given the relatively low number of files this touches, regressions
> aren't too likely, but I like DCHECKs.  :)

Also, do we still need to expose URLRequestStatus's two argument constructor?

[davidben, 2015-07-16 21:52:46]

On 2015/07/16 21:48:08, mmenke wrote:
> On 2015/07/16 21:46:32, mmenke wrote:
> > On 2015/07/16 21:28:46, David Benjamin wrote:
> > > I'm thinking after this we *should* be able to assume that, unless you
have
> > > IO_PENDING, status.error() will have something sensible in there.
> > > 
> > > There was just the one instance outside of test code.
> > > 
> > > Next step is probably to convert the FakeURLFetcher constructor parameter
to
> a
> > > net::Error, which should do away with a lot of URLRequestStatus::Status
> > > consumers.
> > 
> > Should we do a DCHECK in URLRequestStatus::FromError that
> > ErrorToShortString(error) is not "<unknown>" or add a debug-only switch
> > statement to DCHECK on non-net error?  Or just make it take a net::Error,
and
> > add casts to the places that need it?
> > 
> > Suppose given the relatively low number of files this touches, regressions
> > aren't too likely, but I like DCHECKs.  :)
> 
> Also, do we still need to expose URLRequestStatus's two argument constructor?

There's a lot of legal FAILED, ERR_FOO => FromError(ERR_FOO) transforms that I
haven't bothered with. My thinking was that we're going to need to change them
again when killing URLRequestStatus, so it might not be worth bothering yet.
Other things that need to change:

URLRequestStatus(SUCCESS, 0) and URLRequestStatus(SUCCESS, OK) are common. So
that's a bit more turbulence.

But the two messier ones are:
- URLRequestStatus(IO_PENDING, 0) is *everywhere*. I'm not sure I'm happy
switching those without auditing some callers.
- We have a few URLRequestStatus(CANCELED, ERR_FOO) that will need to change in
behavior, but that would need to go with another audit.

So I decided to just carve off the "FAILED/CANCELED + useless error() is
possible" problem.


(I'll toy with adding the DCHECK and friends.)

[mmenke, 2015-07-16 21:58:01]

On 2015/07/16 21:52:46, David Benjamin wrote:
> On 2015/07/16 21:48:08, mmenke wrote:
> > On 2015/07/16 21:46:32, mmenke wrote:
> > > On 2015/07/16 21:28:46, David Benjamin wrote:
> > > > I'm thinking after this we *should* be able to assume that, unless you
> have
> > > > IO_PENDING, status.error() will have something sensible in there.
> > > > 
> > > > There was just the one instance outside of test code.
> > > > 
> > > > Next step is probably to convert the FakeURLFetcher constructor
parameter
> to
> > a
> > > > net::Error, which should do away with a lot of URLRequestStatus::Status
> > > > consumers.
> > > 
> > > Should we do a DCHECK in URLRequestStatus::FromError that
> > > ErrorToShortString(error) is not "<unknown>" or add a debug-only switch
> > > statement to DCHECK on non-net error?  Or just make it take a net::Error,
> and
> > > add casts to the places that need it?
> > > 
> > > Suppose given the relatively low number of files this touches, regressions
> > > aren't too likely, but I like DCHECKs.  :)
> > 
> > Also, do we still need to expose URLRequestStatus's two argument
constructor?
> 
> There's a lot of legal FAILED, ERR_FOO => FromError(ERR_FOO) transforms that I
> haven't bothered with. My thinking was that we're going to need to change them
> again when killing URLRequestStatus, so it might not be worth bothering yet.
> Other things that need to change:
> 
> URLRequestStatus(SUCCESS, 0) and URLRequestStatus(SUCCESS, OK) are common. So
> that's a bit more turbulence.
> 
> But the two messier ones are:
> - URLRequestStatus(IO_PENDING, 0) is *everywhere*. I'm not sure I'm happy
> switching those without auditing some callers.
> - We have a few URLRequestStatus(CANCELED, ERR_FOO) that will need to change
in
> behavior, but that would need to go with another audit.
> 
> So I decided to just carve off the "FAILED/CANCELED + useless error() is
> possible" problem.
> 
> 
> (I'll toy with adding the DCHECK and friends.)

Makes sense.  Feel free to worry about that in another CL, if you want.  This
one LGTM (Modulo the compile failure, of course).

We may want to try and get rid of all the URLREquestStatus constructor calls in
URLRequestJob subclasses.  One of the things Elly's refactor is going to do is
to get rid of subclasses calling NotifyBlah() and setting statuses on Read
calls, which is definitely a step in the right direction.

[davidben, 2015-07-16 22:06:15]

Added the DCHECK, so you probably want to give that a second look.

[mmenke, 2015-07-16 22:14:54]

Still LGTM

https://codereview.chromium.org/1239993004/diff/20001/net/url_request/url_req...
File net/url_request/url_request_status.cc (right):

https://codereview.chromium.org/1239993004/diff/20001/net/url_request/url_req...
net/url_request/url_request_status.cc:34: }
Are compilers smart enough to compile this out, or should we wrap it around
#ifdef DCHECKS_ARE_ENABLED or whatever it is?

[davidben, 2015-07-17 16:20:02]

davidben@chromium.org changed reviewers:
+ bengr@chromium.org, mnissler@chromium.org, xiyuan@chromium.org,
zelidrag@chromium.org

[davidben, 2015-07-17 16:20:02]

+mnissler for chrome/browser/chromeos/attestation/, chrome/browser/policy/, and
components/policy/
+bengr for components/data_reduction_proxy/
+zelidrag for google_apis/gaia/ and chromeos/
+xiyuan for components/proximity_auth/

https://codereview.chromium.org/1239993004/diff/20001/net/url_request/url_req...
File net/url_request/url_request_status.cc (right):

https://codereview.chromium.org/1239993004/diff/20001/net/url_request/url_req...
net/url_request/url_request_status.cc:34: }
On 2015/07/16 22:14:54, mmenke wrote:
> Are compilers smart enough to compile this out, or should we wrap it around
> #ifdef DCHECKS_ARE_ENABLED or whatever it is?

I would hope so. It's an empty switch-case.

[xiyuan, 2015-07-17 16:23:33]

proximity_auth LGTM

[Mattias Nissler (ping if slow), 2015-07-17 16:28:59]

policy and attestation LGTM

I suggest you updated the commit message though to call out that this is a step
towards getting rid of URLRequestStatus::Status, which I first had to figure out
to make sense of this CL.

[davidben, 2015-07-17 16:43:44]

On 2015/07/17 16:28:59, Mattias Nissler wrote:
> I suggest you updated the commit message though to call out that this is a
step
> towards getting rid of URLRequestStatus::Status, which I first had to figure
out
> to make sense of this CL.

Done.

[bengr, 2015-07-21 00:08:42]

data_reduction_proxy LGTM

[mmenke, 2015-07-29 19:48:28]

What's the status of this?  Still need one more signoff?

[davidben, 2015-07-30 15:53:43]

On 2015/07/29 19:48:28, mmenke wrote:
> What's the status of this?  Still need one more signoff?

Yeah.

zelidrag: Friendly ping.

[davidben, 2015-08-03 14:46:58]

davidben@chromium.org changed reviewers:
+ dpolukhin@chromium.org, rogerta@chromium.org

[davidben, 2015-08-03 14:46:58]

+rogerta for google_apis
+dpolukhin for chromeos

[Dmitry Polukhin, 2015-08-03 14:50:25]

LGTM chromeos

[Roger Tawa OOO till Jul 10th, 2015-08-04 15:46:05]

lgtm google_apis

[davidben, 2015-08-04 15:56:33]

The CQ bit was checked by davidben@chromium.org

[davidben, 2015-08-04 15:56:33]

The patchset sent to the CQ was uploaded after l-g-t-m from mmenke@chromium.org
Link to the patchset: https://codereview.chromium.org/1239993004/#ps40001
(title: "fixes")

[commit-bot: I haz the power, 2015-08-04 15:56:57]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1239993004/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1239993004/40001

[commit-bot: I haz the power, 2015-08-04 15:59:42]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-08-04 15:59:42]

Try jobs failed on following builders:
  linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[davidben, 2015-08-04 17:36:00]

davidben@chromium.org changed reviewers:
+ sky@chromium.org, treib@chromium.org

[davidben, 2015-08-04 17:36:00]

On rebase, it seems a new one crept in.

+sky for mojo/ OWNERS. I'm guessing this'll need some cross-repo cleverness?

Also +treib for chrome/browser/superverised_user/. It seems I missed that on
first pass.

[sky, 2015-08-04 19:47:29]

src/mojo is no longer pulled from the mojo repo. So, you should be good. LGTM

[Marc Treib, 2015-08-05 07:37:08]

supervised_user LGTM

[davidben, 2015-08-05 15:15:10]

The CQ bit was checked by davidben@chromium.org

[davidben, 2015-08-05 15:15:10]

The patchset sent to the CQ was uploaded after l-g-t-m from
rogerta@chromium.org, mnissler@chromium.org, dpolukhin@chromium.org,
xiyuan@chromium.org, bengr@chromium.org, mmenke@chromium.org, sky@chromium.org
Link to the patchset: https://codereview.chromium.org/1239993004/#ps100001
(title: "fix more failures")

[commit-bot: I haz the power, 2015-08-05 15:16:16]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1239993004/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1239993004/100001

[commit-bot: I haz the power, 2015-08-05 16:35:22]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-08-05 16:35:59]

Patchset 6 (id:??) landed as
https://crrev.com/5c411d94add205780f6e6c701aecc9b297d885b0
Cr-Commit-Position: refs/heads/master@{#341913}