Make URLRequestJob::SetStatus private.

Make URLRequestJob::SetStatus private.

Recent CLs have already made it so URLRequestJob itself calls the
method itself whenever necessary, except when URLRequestJob::Start is
being called, where subclasses were still expected to invoke it.
Most of them were failing to do so, but since nothing cares about the
state after start, and little distinguished between ERR_IO_PENDING and
SUCCESS, everything was still working.  URLRequest now sets its status
itself before it starts a URLRequestJob.

This CL also makes the URLRequest consistently have a status of
ERR_IO_PENDING when it's doing something - there were a number of
paths where this wasn't previously the case.

The state machine changes also coincidentally fix issue 575213,
which regressed in https://codereview.chromium.org/1467603002, so this
CL includes a test for that fix.

In the future, I'd really like to get rid of URLRequestJob::SetStatus,
and have URLRequests manage their own status.

BUG=564250, 575213
TBR=jianli@chromium.org, phajdan.jr@chromium.org

Committed: https://crrev.com/0fdf6ebeb19c8dfbb73e398d72a00be61d2aa444
Cr-Commit-Position: refs/heads/master@{#371072}

[mmenke, 2016-01-05 23:10:09]

Description was changed from

==========
Make URLRequestJob::SetStatus private.

Recent CLs have already made it so URLRequestJob calls the method itself
whenever necessary, except when URLRequestJob::Start is being called,
where subclasses were still expected to invoke it.  Most of them were
failing to do so, but since nothing cares about the state after start,
everything was working.  URLRequest now sets its status itself before
it starts a URLRequestJob.

In the future, I'd really like to get rid of URLRequestJob::SetStatus,
and have URLRequests manage their own status.

BUG=564250
==========

to

==========
Make URLRequestJob::SetStatus private.

Recent CLs have already made it so URLRequestJob calls the method itself
whenever necessary, except when URLRequestJob::Start is being called,
where subclasses were still expected to invoke it.  Most of them were
failing to do so, but since nothing cares about the state after start,
everything was working.  URLRequest now sets its status itself before
it starts a URLRequestJob.

In the future, I'd really like to get rid of URLRequestJob::SetStatus,
and have URLRequests manage their own status.

BUG=564250
==========

[mmenke, 2016-01-05 23:10:09]

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

[mmenke, 2016-01-05 23:10:27]

mmenke@chromium.org changed reviewers:
- rdsmith@chromium.org

[mmenke, 2016-01-06 23:35:17]

Description was changed from

==========
Make URLRequestJob::SetStatus private.

Recent CLs have already made it so URLRequestJob calls the method itself
whenever necessary, except when URLRequestJob::Start is being called,
where subclasses were still expected to invoke it.  Most of them were
failing to do so, but since nothing cares about the state after start,
everything was working.  URLRequest now sets its status itself before
it starts a URLRequestJob.

In the future, I'd really like to get rid of URLRequestJob::SetStatus,
and have URLRequests manage their own status.

BUG=564250
==========

to

==========
Make URLRequestJob::SetStatus private.

Recent CLs have already made it so URLRequestJob itself calls the
method itself whenever necessary, except when URLRequestJob::Start is
being called, where subclasses were still expected to invoke it.
Most of them were failing to do so, but since nothing cares about the
state after start, and little distinguished between ERR_IO_PENDING and
SUCCESS, everything was still working.  URLRequest now sets its status
itself before it starts a URLRequestJob.

This CL also makes the URLRequest consistently have a status of
ERR_IO_PENDING when it's doing something - there were a number of
paths where this wasn't previously the case.

In the future, I'd really like to get rid of URLRequestJob::SetStatus,
and have URLRequests manage their own status.

BUG=564250
==========

[mmenke, 2016-01-07 05:22:50]

Patchset #9 (id:160001) has been deleted

[mmenke, 2016-01-07 05:23:06]

Patchset #8 (id:140001) has been deleted

[mmenke, 2016-01-07 05:26:32]

Patchset #8 (id:180001) has been deleted

[mmenke, 2016-01-07 05:28:04]

Patchset #8 (id:200001) has been deleted

[mmenke, 2016-01-07 05:29:40]

Patchset #8 (id:150018) has been deleted

[mmenke, 2016-01-07 15:54:54]

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

[mmenke, 2016-01-07 15:54:55]

Suggest reviewing url_request_job / url_request first, as everything else is
fallout from that.

https://codereview.chromium.org/1563633002/diff/230001/android_webview/browse...
File android_webview/browser/net/android_stream_reader_url_request_job.cc
(left):

https://codereview.chromium.org/1563633002/diff/230001/android_webview/browse...
android_webview/browser/net/android_stream_reader_url_request_job.cc:257:
DCHECK(!request_->status().is_io_pending());
This removal is needed because this used to partially reflect the
AndroidStreamReaderURLRequestJob's view of the request's state, but now it
solely reflects the URLRequest's/URLRequestJob's view of the request's state,
and this is called in HeadersComplete before the URLRequestJob has been notified
headers are available.

https://codereview.chromium.org/1563633002/diff/230001/content/browser/servic...
File
content/browser/service_worker/service_worker_read_from_cache_job_unittest.cc
(right):

https://codereview.chromium.org/1563633002/diff/230001/content/browser/servic...
content/browser/service_worker/service_worker_read_from_cache_job_unittest.cc:157:
request->Start();
A lot of these test-only changes are because the new DCHECKs in URLRequestJob
expect the state to be ERR_IO_PENDING, which is generally set by the URLRequest
itself.  Running the job directly is ugly, anyways.

https://codereview.chromium.org/1563633002/diff/230001/net/url_request/url_re...
File net/url_request/url_request.cc (left):

https://codereview.chromium.org/1563633002/diff/230001/net/url_request/url_re...
net/url_request/url_request.cc:664: // the caller does not get reentered.
This comment is rather outdated - NotifyDone always posts an async task, now.

https://codereview.chromium.org/1563633002/diff/230001/net/url_request/url_re...
File net/url_request/url_request_job.cc (right):

https://codereview.chromium.org/1563633002/diff/230001/net/url_request/url_re...
net/url_request/url_request_job.cc:793: void URLRequestJob::SetStatus(const
URLRequestStatus &status) {
This file is rather out of order - thinking I'll fix the order of everything in
a followup CL.

[mmenke, 2016-01-08 15:32:05]

Patchset #9 (id:250001) has been deleted

[Randy Smith (Not in Mondays), 2016-01-11 02:27:48]

First round of comments; only real concern is understanding the URLRequest state
machine based on entry points.

https://codereview.chromium.org/1563633002/diff/230001/android_webview/browse...
File android_webview/browser/net/android_stream_reader_url_request_job.cc
(left):

https://codereview.chromium.org/1563633002/diff/230001/android_webview/browse...
android_webview/browser/net/android_stream_reader_url_request_job.cc:257:
DCHECK(!request_->status().is_io_pending());
On 2016/01/07 15:54:55, mmenke wrote:
> This removal is needed because this used to partially reflect the
> AndroidStreamReaderURLRequestJob's view of the request's state, but now it
> solely reflects the URLRequest's/URLRequestJob's view of the request's state,
> and this is called in HeadersComplete before the URLRequestJob has been
notified
> headers are available.

Is it worth replacing the DCHECK with one based on just class-internal state?

https://codereview.chromium.org/1563633002/diff/230001/content/browser/servic...
File
content/browser/service_worker/service_worker_read_from_cache_job_unittest.cc
(right):

https://codereview.chromium.org/1563633002/diff/230001/content/browser/servic...
content/browser/service_worker/service_worker_read_from_cache_job_unittest.cc:157:
request->Start();
On 2016/01/07 15:54:55, mmenke wrote:
> A lot of these test-only changes are because the new DCHECKs in URLRequestJob
> expect the state to be ERR_IO_PENDING, which is generally set by the
URLRequest
> itself.  Running the job directly is ugly, anyways.

Acknowledged.

https://codereview.chromium.org/1563633002/diff/230001/content/browser/servic...
content/browser/service_worker/service_worker_read_from_cache_job_unittest.cc:178:
net::TestJobInterceptor* test_job_interceptor_;
Willing to have a comment indicating ownership here as well as (or in place of)
the comment above?  I feel uncomfortable about persistent raw pointers without
ownership explanation.

https://codereview.chromium.org/1563633002/diff/230001/content/browser/stream...
File content/browser/streams/stream_url_request_job.cc (right):

https://codereview.chromium.org/1563633002/diff/230001/content/browser/stream...
content/browser/streams/stream_url_request_job.cc:161: // We only support GET
requests.
nit: Worth fixing use of "we"?

https://codereview.chromium.org/1563633002/diff/230001/net/url_request/url_re...
File net/url_request/url_request.cc (left):

https://codereview.chromium.org/1563633002/diff/230001/net/url_request/url_re...
net/url_request/url_request.cc:664: // the caller does not get reentered.
On 2016/01/07 15:54:55, mmenke wrote:
> This comment is rather outdated - NotifyDone always posts an async task, now.

Acknowledged.

https://codereview.chromium.org/1563633002/diff/230001/net/url_request/url_re...
File net/url_request/url_request.cc (right):

https://codereview.chromium.org/1563633002/diff/230001/net/url_request/url_re...
net/url_request/url_request.cc:668: // Don't allow errors to be sent from within
Start().
Do you understand this comment?  It confuses me--I don't see any code here
that's relevant to errors coming from within start, other than the lack of a
return value, which suggests to me that the comment should be in the URLRJ
header file.  Maybe "Start isn't allowed to return errors synchronously" or
removing the comment?

https://codereview.chromium.org/1563633002/diff/230001/net/url_request/url_re...
net/url_request/url_request.cc:865: status_ =
URLRequestStatus::FromError(ERR_IO_PENDING);
I'm struggling with my conscience as to whether this review requires me to grok
the full URLRequest state machine to verify these assignments.  Can you give me
a sense as to the logic you used in choosing them so that I can decide how deep
I have to dig to validate them?  

(Obviously, I'd love to have the state machine actually documented somewhere,
and one really nice thing about this CL is that we're moving towards a place
where that might be done.  But I don't think we're quite there yet?)

https://codereview.chromium.org/1563633002/diff/230001/net/url_request/url_re...
File net/url_request/url_request_http_job_unittest.cc (right):

https://codereview.chromium.org/1563633002/diff/230001/net/url_request/url_re...
net/url_request/url_request_http_job_unittest.cc:104: TestJobInterceptor*
test_job_interceptor_;
Willing to put a comment about ownership here in addition to (or instead of) the
one above?  Maybe I'm cossetting future code readers, but I at least have a knee
jerk "better look closely at that" when I see this pattern :-}.

https://codereview.chromium.org/1563633002/diff/230001/net/url_request/url_re...
net/url_request/url_request_http_job_unittest.cc:752: #if !defined(OS_IOS)
Why did they start failing on this CL?

https://codereview.chromium.org/1563633002/diff/230001/net/url_request/url_re...
File net/url_request/url_request_job.cc (right):

https://codereview.chromium.org/1563633002/diff/230001/net/url_request/url_re...
net/url_request/url_request_job.cc:544: }
I have a vague memory that Elly tried to move this code to this location and ran
into a problem; do you have a clearer memory on that issue?

https://codereview.chromium.org/1563633002/diff/230001/net/url_request/url_re...
net/url_request/url_request_job.cc:793: void URLRequestJob::SetStatus(const
URLRequestStatus &status) {
On 2016/01/07 15:54:55, mmenke wrote:
> This file is rather out of order - thinking I'll fix the order of everything
in
> a followup CL.

SGTM.

https://codereview.chromium.org/1563633002/diff/230001/net/url_request/url_re...
net/url_request/url_request_job.cc:798: // resolved.
Worth switching this to a DCHECK as long as you're here?  (Conceptually; I know
you're not changing this particular piece of code.)

[mmenke, 2016-01-11 06:17:11]

https://codereview.chromium.org/1563633002/diff/230001/android_webview/browse...
File android_webview/browser/net/android_stream_reader_url_request_job.cc
(left):

https://codereview.chromium.org/1563633002/diff/230001/android_webview/browse...
android_webview/browser/net/android_stream_reader_url_request_job.cc:257:
DCHECK(!request_->status().is_io_pending());
On 2016/01/11 02:27:48, Randy Smith - Not in Fridays wrote:
> On 2016/01/07 15:54:55, mmenke wrote:
> > This removal is needed because this used to partially reflect the
> > AndroidStreamReaderURLRequestJob's view of the request's state, but now it
> > solely reflects the URLRequest's/URLRequestJob's view of the request's
state,
> > and this is called in HeadersComplete before the URLRequestJob has been
> notified
> > headers are available.
> 
> Is it worth replacing the DCHECK with one based on just class-internal state?

I've been trying to avoid learning this class, so I don't know.  Hoping the
owners will comment, when I add them, if they want some updated check.

https://codereview.chromium.org/1563633002/diff/230001/content/browser/servic...
File
content/browser/service_worker/service_worker_read_from_cache_job_unittest.cc
(right):

https://codereview.chromium.org/1563633002/diff/230001/content/browser/servic...
content/browser/service_worker/service_worker_read_from_cache_job_unittest.cc:178:
net::TestJobInterceptor* test_job_interceptor_;
On 2016/01/11 02:27:48, Randy Smith - Not in Fridays wrote:
> Willing to have a comment indicating ownership here as well as (or in place
of)
> the comment above?  I feel uncomfortable about persistent raw pointers without
> ownership explanation.

Done.  Also added it to the initializer list.

https://codereview.chromium.org/1563633002/diff/230001/content/browser/stream...
File content/browser/streams/stream_url_request_job.cc (right):

https://codereview.chromium.org/1563633002/diff/230001/content/browser/stream...
content/browser/streams/stream_url_request_job.cc:161: // We only support GET
requests.
On 2016/01/11 02:27:48, Randy Smith - Not in Fridays wrote:
> nit: Worth fixing use of "we"?

Done (I generally do fewer cleanups in files "we" don't own, in case other teams
have differing opinions on style).

https://codereview.chromium.org/1563633002/diff/230001/net/url_request/url_re...
File net/url_request/url_request.cc (right):

https://codereview.chromium.org/1563633002/diff/230001/net/url_request/url_re...
net/url_request/url_request.cc:668: // Don't allow errors to be sent from within
Start().
On 2016/01/11 02:27:48, Randy Smith - Not in Fridays wrote:
> Do you understand this comment?  It confuses me--I don't see any code here
> that's relevant to errors coming from within start, other than the lack of a
> return value, which suggests to me that the comment should be in the URLRJ
> header file.  Maybe "Start isn't allowed to return errors synchronously" or
> removing the comment?

I merged it with the above comment - yea, I think the point is that a result
isn't returned synchronously.  I left the comment alone originally just because
it's a behavior I want to change eventually, but agree it's good to have
reasonable comments in the meantime.

https://codereview.chromium.org/1563633002/diff/230001/net/url_request/url_re...
net/url_request/url_request.cc:865: status_ =
URLRequestStatus::FromError(ERR_IO_PENDING);
On 2016/01/11 02:27:48, Randy Smith - Not in Fridays wrote:
> I'm struggling with my conscience as to whether this review requires me to
grok
> the full URLRequest state machine to verify these assignments.  Can you give
me
> a sense as to the logic you used in choosing them so that I can decide how
deep
> I have to dig to validate them?  
> 
> (Obviously, I'd love to have the state machine actually documented somewhere,
> and one really nice thing about this CL is that we're moving towards a place
> where that might be done.  But I don't think we're quite there yet?)

So, we don't really distinguish well between what ERR_IO_PENDING means and what
URLRequestStatus::SUCCESS means.  In this CL, I've attempt to modify them so
that they're at least consistent:

* From request Start until URLRequestJob::NotifyHeadersComplete (Or
NotifyStartError), status is ERR_IO_PENDING.  We change it before we notify
anything (Including both delegates) of headers being received / a redirect. 
This is basically old behavior (It was just being set to OK before the methods
are invoked, instead of after).
* Any call that will result in NotifyHeadersComplete / NotifyStartError being
called again will set it to ERR_IO_PENDING again.  We weren't doing this before,
which was bizarre and made the state pretty unpredictable.
* URLRequest::Read sets it to ERR_IO_PENDING again, and it's only set to OK once
we have (post-filter) data to send to the URLRequest.

Long term, I'd really like to get rid of the distinction as a queriable state
(And get rid of URLRequestStatus as well).

https://codereview.chromium.org/1563633002/diff/230001/net/url_request/url_re...
File net/url_request/url_request_http_job_unittest.cc (right):

https://codereview.chromium.org/1563633002/diff/230001/net/url_request/url_re...
net/url_request/url_request_http_job_unittest.cc:104: TestJobInterceptor*
test_job_interceptor_;
On 2016/01/11 02:27:48, Randy Smith - Not in Fridays wrote:
> Willing to put a comment about ownership here in addition to (or instead of)
the
> one above?  Maybe I'm cossetting future code readers, but I at least have a
knee
> jerk "better look closely at that" when I see this pattern :-}.

Done.  Always happy to add more comments.

https://codereview.chromium.org/1563633002/diff/230001/net/url_request/url_re...
net/url_request/url_request_http_job_unittest.cc:752: #if !defined(OS_IOS)
On 2016/01/11 02:27:48, Randy Smith - Not in Fridays wrote:
> Why did they start failing on this CL?

Sorry, thought I commented on it, but turns out I didn't.  The logic that we
don't support WebSockets on iOS is at the URLRequestJobFactory layer
(URLRequestJobManager, actually), so since these tests just create the job
manually, they bypass that logic.

https://codereview.chromium.org/1563633002/diff/230001/net/url_request/url_re...
File net/url_request/url_request_job.cc (right):

https://codereview.chromium.org/1563633002/diff/230001/net/url_request/url_re...
net/url_request/url_request_job.cc:544: }
On 2016/01/11 02:27:48, Randy Smith - Not in Fridays wrote:
> I have a vague memory that Elly tried to move this code to this location and
ran
> into a problem; do you have a clearer memory on that issue?

I assume it's the issue I ran into - ReadFilteredData can return ERR_IO_PENDING
if the (pre-filter) bytes_read passed in to it is 0 due to the socket being
closed, unless NotifyDone has already been been called.  It should be returning
net::OK instead, seemed simplest to just bypass the call in that case (See
changes above).

This *also* fixed the issue the unit test checks for in the other CL, I was
reluctant to include the new test in this CL, because while this CL isn't huge,
it is complicated.

https://codereview.chromium.org/1563633002/diff/230001/net/url_request/url_re...
net/url_request/url_request_job.cc:798: // resolved.
On 2016/01/11 02:27:48, Randy Smith - Not in Fridays wrote:
> Worth switching this to a DCHECK as long as you're here?  (Conceptually; I
know
> you're not changing this particular piece of code.)

Done.

[mmenke, 2016-01-11 14:21:40]

https://codereview.chromium.org/1563633002/diff/270001/net/url_request/url_re...
File net/url_request/url_request_job_unittest.cc (right):

https://codereview.chromium.org/1563633002/diff/270001/net/url_request/url_re...
net/url_request/url_request_job_unittest.cc:129: const MockTransaction
kInvalidContentGZip_Transaction = {
Ahh...This test was supposed to be in the other CL!  Hence the confusion.  I
could land it as part of this one, or move it to the other CL.

[mmenke, 2016-01-11 14:22:15]

https://codereview.chromium.org/1563633002/diff/270001/net/url_request/url_re...
File net/url_request/url_request_job_unittest.cc (right):

https://codereview.chromium.org/1563633002/diff/270001/net/url_request/url_re...
net/url_request/url_request_job_unittest.cc:129: const MockTransaction
kInvalidContentGZip_Transaction = {
On 2016/01/11 14:21:40, mmenke wrote:
> Ahh...This test was supposed to be in the other CL!  Hence the confusion.  I
> could land it as part of this one, or move it to the other CL.

If I land it as part of this one, need to merge the other CL into this one.

[Randy Smith (Not in Mondays), 2016-01-11 15:51:59]

On 2016/01/11 14:22:15, mmenke wrote:
>
https://codereview.chromium.org/1563633002/diff/270001/net/url_request/url_re...
> File net/url_request/url_request_job_unittest.cc (right):
> 
>
https://codereview.chromium.org/1563633002/diff/270001/net/url_request/url_re...
> net/url_request/url_request_job_unittest.cc:129: const MockTransaction
> kInvalidContentGZip_Transaction = {
> On 2016/01/11 14:21:40, mmenke wrote:
> > Ahh...This test was supposed to be in the other CL!  Hence the confusion.  I
> > could land it as part of this one, or move it to the other CL.
> 
> If I land it as part of this one, need to merge the other CL into this one.

Ok, that makes more sense.  I'm good either way; it's about the same amount of
work for me.  I probably won't be coming back to these CLs for a day or so
(network bug triage :-}) so you can do whichever you'd like.  Just let me know.

[mmenke, 2016-01-11 16:33:46]

On 2016/01/11 15:51:59, Randy Smith - Not in Fridays wrote:
> On 2016/01/11 14:22:15, mmenke wrote:
> >
>
https://codereview.chromium.org/1563633002/diff/270001/net/url_request/url_re...
> > File net/url_request/url_request_job_unittest.cc (right):
> > 
> >
>
https://codereview.chromium.org/1563633002/diff/270001/net/url_request/url_re...
> > net/url_request/url_request_job_unittest.cc:129: const MockTransaction
> > kInvalidContentGZip_Transaction = {
> > On 2016/01/11 14:21:40, mmenke wrote:
> > > Ahh...This test was supposed to be in the other CL!  Hence the confusion. 
I
> > > could land it as part of this one, or move it to the other CL.
> > 
> > If I land it as part of this one, need to merge the other CL into this one.
> 
> Ok, that makes more sense.  I'm good either way; it's about the same amount of
> work for me.  I probably won't be coming back to these CLs for a day or so
> (network bug triage :-}) so you can do whichever you'd like.  Just let me
know.

I guess I'll merge them (Hrm...Not sure why git CL format mucked with the
formatting of one of the other transactions in the other CL, but not in this
one...).

Thanks for telling me you're on triage.  No rush.

[mmenke, 2016-01-11 17:39:40]

Description was changed from

==========
Make URLRequestJob::SetStatus private.

Recent CLs have already made it so URLRequestJob itself calls the
method itself whenever necessary, except when URLRequestJob::Start is
being called, where subclasses were still expected to invoke it.
Most of them were failing to do so, but since nothing cares about the
state after start, and little distinguished between ERR_IO_PENDING and
SUCCESS, everything was still working.  URLRequest now sets its status
itself before it starts a URLRequestJob.

This CL also makes the URLRequest consistently have a status of
ERR_IO_PENDING when it's doing something - there were a number of
paths where this wasn't previously the case.

In the future, I'd really like to get rid of URLRequestJob::SetStatus,
and have URLRequests manage their own status.

BUG=564250
==========

to

==========
Make URLRequestJob::SetStatus private.

Recent CLs have already made it so URLRequestJob itself calls the
method itself whenever necessary, except when URLRequestJob::Start is
being called, where subclasses were still expected to invoke it.
Most of them were failing to do so, but since nothing cares about the
state after start, and little distinguished between ERR_IO_PENDING and
SUCCESS, everything was still working.  URLRequest now sets its status
itself before it starts a URLRequestJob.

This CL also makes the URLRequest consistently have a status of
ERR_IO_PENDING when it's doing something - there were a number of
paths where this wasn't previously the case.

The state machine changes also coincidentally fix issue 575213,
which regressed in https://codereview.chromium.org/1467603002, so this
CL includes a test for that fix.

In the future, I'd really like to get rid of URLRequestJob::SetStatus,
and have URLRequests manage their own status.

BUG=564250,575213
==========

[mmenke, 2016-01-12 22:23:40]

Patchset #11 (id:310001) has been deleted

[mmenke, 2016-01-13 16:21:28]

Patchset #11 (id:330001) has been deleted

[Randy Smith (Not in Mondays), 2016-01-15 17:42:02]

Pretty much nits below.  You can do what you want with the comment change and
the symbolic error code suggestion, so I'll LGTM conditional on having
understood correctly that the triage doc changes weren't meant to be in this CL
and you'll be removing them.  If that's wrong, let's do another review round.

https://codereview.chromium.org/1563633002/diff/270001/net/url_request/url_re...
File net/url_request/url_request_job_unittest.cc (right):

https://codereview.chromium.org/1563633002/diff/270001/net/url_request/url_re...
net/url_request/url_request_job_unittest.cc:305: EXPECT_EQ(-330,
req->status().error());
Why not the symbolic error code?

https://codereview.chromium.org/1563633002/diff/290001/net/docs/bug-triage.md
File net/docs/bug-triage.md (right):

https://codereview.chromium.org/1563633002/diff/290001/net/docs/bug-triage.md...
net/docs/bug-triage.md:1: # Chrome Network Bug Triage
Should the changes in this file and the next one be in this CL?  I don't
understand why they're here.

https://codereview.chromium.org/1563633002/diff/290001/net/url_request/url_re...
File net/url_request/url_request_job.cc (right):

https://codereview.chromium.org/1563633002/diff/290001/net/url_request/url_re...
net/url_request/url_request_job.cc:503: // need to call DoneReading() in that
case.
nit, suggestion: I'm inclined to think this comment would be better stopping at
the ",".  As I read it, the second part of the comment is an explanation as to
why the old code was broken, and doesn't really illuminate this code.   I'm even
inclined to suggest changing the first part of the comment to indicate the
concern about the filter code returning ERR_IO_PENDING on a write of 0 bytes,
which as I understand it is is the reason for the conditional below.

I'm slightly torn about this code structure, because (given that we signal EOF
by writing zero bytes as a general pattern) the filter code is buggy if it
returns ERR_IO_PENDING based on an input of zero data; the interface
implementation is implicitly bad and we're working around it by avoiding calling
the interface in the bad way.  However, given that this is all going to go up in
a puff of refactoring very soon, and given that this fix is almost certainly
simpler than the fix to the filter code, I'm happy with this the way it is.

[mmenke, 2016-01-15 18:09:42]

Oops...Yea, triage doc changes are ourdated (Stilly playing with them), and
weren't meant to be in the CL, anyways.

On 2016/01/15 17:42:02, Randy Smith - Not in Fridays wrote:
> Pretty much nits below.  You can do what you want with the comment change and
> the symbolic error code suggestion, so I'll LGTM conditional on having
> understood correctly that the triage doc changes weren't meant to be in this
CL
> and you'll be removing them.  If that's wrong, let's do another review round.
> 
>
https://codereview.chromium.org/1563633002/diff/270001/net/url_request/url_re...
> File net/url_request/url_request_job_unittest.cc (right):
> 
>
https://codereview.chromium.org/1563633002/diff/270001/net/url_request/url_re...
> net/url_request/url_request_job_unittest.cc:305: EXPECT_EQ(-330,
> req->status().error());
> Why not the symbolic error code?
> 
> https://codereview.chromium.org/1563633002/diff/290001/net/docs/bug-triage.md
> File net/docs/bug-triage.md (right):
> 
>
https://codereview.chromium.org/1563633002/diff/290001/net/docs/bug-triage.md...
> net/docs/bug-triage.md:1: # Chrome Network Bug Triage
> Should the changes in this file and the next one be in this CL?  I don't
> understand why they're here.
> 
>
https://codereview.chromium.org/1563633002/diff/290001/net/url_request/url_re...
> File net/url_request/url_request_job.cc (right):
> 
>
https://codereview.chromium.org/1563633002/diff/290001/net/url_request/url_re...
> net/url_request/url_request_job.cc:503: // need to call DoneReading() in that
> case.
> nit, suggestion: I'm inclined to think this comment would be better stopping
at
> the ",".  As I read it, the second part of the comment is an explanation as to
> why the old code was broken, and doesn't really illuminate this code.   I'm
even
> inclined to suggest changing the first part of the comment to indicate the
> concern about the filter code returning ERR_IO_PENDING on a write of 0 bytes,
> which as I understand it is is the reason for the conditional below.
> 
> I'm slightly torn about this code structure, because (given that we signal EOF
> by writing zero bytes as a general pattern) the filter code is buggy if it
> returns ERR_IO_PENDING based on an input of zero data; the interface
> implementation is implicitly bad and we're working around it by avoiding
calling
> the interface in the bad way.  However, given that this is all going to go up
in
> a puff of refactoring very soon, and given that this fix is almost certainly
> simpler than the fix to the filter code, I'm happy with this the way it is.

[mmenke, 2016-01-15 18:31:04]

mmenke@chromium.org changed reviewers:
+ falken@chromium.org, jianli@chromium.org, mnaganov@chromium.org,
phajdan.jr@chromium.org, zork@chromium.org

[mmenke, 2016-01-15 18:31:05]

[+mnaganov]:  Please review android_webview
[+falken]:  Please review content/browser/service_worker
[+zork]:  Please review content/browser/streams
[+jianli]:  Please review storage/browser/blob
[+phajdan.jr]:  Please review
content/public/test/test_download_request_handler.cc (Wonder if that should
actually be in content/test/net).

https://codereview.chromium.org/1563633002/diff/270001/net/url_request/url_re...
File net/url_request/url_request_job_unittest.cc (right):

https://codereview.chromium.org/1563633002/diff/270001/net/url_request/url_re...
net/url_request/url_request_job_unittest.cc:305: EXPECT_EQ(-330,
req->status().error());
On 2016/01/15 17:42:02, Randy Smith - Not in Fridays wrote:
> Why not the symbolic error code?

Done.  Forgot to upload that fix after merging from the other CL.

https://codereview.chromium.org/1563633002/diff/290001/net/url_request/url_re...
File net/url_request/url_request_job.cc (right):

https://codereview.chromium.org/1563633002/diff/290001/net/url_request/url_re...
net/url_request/url_request_job.cc:503: // need to call DoneReading() in that
case.
On 2016/01/15 17:42:02, Randy Smith - Not in Fridays wrote:
> nit, suggestion: I'm inclined to think this comment would be better stopping
at
> the ",".  As I read it, the second part of the comment is an explanation as to
> why the old code was broken, and doesn't really illuminate this code.   I'm
even
> inclined to suggest changing the first part of the comment to indicate the
> concern about the filter code returning ERR_IO_PENDING on a write of 0 bytes,
> which as I understand it is is the reason for the conditional below.
> 
> I'm slightly torn about this code structure, because (given that we signal EOF
> by writing zero bytes as a general pattern) the filter code is buggy if it
> returns ERR_IO_PENDING based on an input of zero data; the interface
> implementation is implicitly bad and we're working around it by avoiding
calling
> the interface in the bad way.  However, given that this is all going to go up
in
> a puff of refactoring very soon, and given that this fix is almost certainly
> simpler than the fix to the filter code, I'm happy with this the way it is.  

I've updated the comment to explicitly mention the ERR_IO_PENDING.  I agree that
we want a better solution, but that it doesn't belong in this CL (And the filter
refactor should make it a moot point).

https://codereview.chromium.org/1563633002/diff/350001/android_webview/browse...
File android_webview/browser/net/android_stream_reader_url_request_job.cc
(right):

https://codereview.chromium.org/1563633002/diff/350001/android_webview/browse...
android_webview/browser/net/android_stream_reader_url_request_job.cc:251: return
delegate_->GetMimeType(
request_->status() is now managed by net, so it hasn't been updated in the call
to this method from HeadersComplete(), below.  Happy to add some other DCHECK
instead, if you want.

[mnaganov (inactive), 2016-01-15 19:04:58]

android_webview/ LGTM

https://codereview.chromium.org/1563633002/diff/350001/android_webview/browse...
File android_webview/browser/net/android_stream_reader_url_request_job.cc
(right):

https://codereview.chromium.org/1563633002/diff/350001/android_webview/browse...
android_webview/browser/net/android_stream_reader_url_request_job.cc:251: return
delegate_->GetMimeType(
On 2016/01/15 18:31:05, mmenke wrote:
> request_->status() is now managed by net, so it hasn't been updated in the
call
> to this method from HeadersComplete(), below.  Happy to add some other DCHECK
> instead, if you want.

I think it should be fine, thanks for letting us know!

[falken, 2016-01-18 00:55:57]

service_worker lgtm

[xunjieli, 2016-01-19 15:18:54]

xunjieli@chromium.org changed reviewers:
+ xunjieli@chromium.org

[mmenke, 2016-01-20 16:47:31]

[zork]:  Ping!  Please review content/browser/streams.

[jianli, phajdan.jr]:  I consider the parts of the review you own part of a
trivial API change, and thus TBRable.  I'll plan to TBR both of you, once I get
zork's signoff, if you haven't chimed in.

https://codereview.chromium.org/1563633002/diff/350001/content/browser/stream...
File content/browser/streams/stream_url_request_job.cc (right):

https://codereview.chromium.org/1563633002/diff/350001/content/browser/stream...
content/browser/streams/stream_url_request_job.cc:154:
NotifyFailure(net::ERR_METHOD_NOT_SUPPORTED);
Note that the old code called NotifyFailure in SetExtraRequestHeaders, which
isn't allowed (And a new DCHECK explicitly catches).

[Zachary Kuznia, 2016-01-22 18:02:43]

lgtm

[mmenke, 2016-01-22 21:43:19]

On 2016/01/22 18:02:43, Zachary Kuznia wrote:
> lgtm

Thanks!

[mmenke, 2016-01-22 21:43:57]

Description was changed from

==========
Make URLRequestJob::SetStatus private.

Recent CLs have already made it so URLRequestJob itself calls the
method itself whenever necessary, except when URLRequestJob::Start is
being called, where subclasses were still expected to invoke it.
Most of them were failing to do so, but since nothing cares about the
state after start, and little distinguished between ERR_IO_PENDING and
SUCCESS, everything was still working.  URLRequest now sets its status
itself before it starts a URLRequestJob.

This CL also makes the URLRequest consistently have a status of
ERR_IO_PENDING when it's doing something - there were a number of
paths where this wasn't previously the case.

The state machine changes also coincidentally fix issue 575213,
which regressed in https://codereview.chromium.org/1467603002, so this
CL includes a test for that fix.

In the future, I'd really like to get rid of URLRequestJob::SetStatus,
and have URLRequests manage their own status.

BUG=564250,575213
==========

to

==========
Make URLRequestJob::SetStatus private.

Recent CLs have already made it so URLRequestJob itself calls the
method itself whenever necessary, except when URLRequestJob::Start is
being called, where subclasses were still expected to invoke it.
Most of them were failing to do so, but since nothing cares about the
state after start, and little distinguished between ERR_IO_PENDING and
SUCCESS, everything was still working.  URLRequest now sets its status
itself before it starts a URLRequestJob.

This CL also makes the URLRequest consistently have a status of
ERR_IO_PENDING when it's doing something - there were a number of
paths where this wasn't previously the case.

The state machine changes also coincidentally fix issue 575213,
which regressed in https://codereview.chromium.org/1467603002, so this
CL includes a test for that fix.

In the future, I'd really like to get rid of URLRequestJob::SetStatus,
and have URLRequests manage their own status.

BUG=564250,575213
TBR=jianli@chromium.org, phajdan.jr@chromium.org
==========

[mmenke, 2016-01-22 21:44:10]

The CQ bit was checked by mmenke@chromium.org

[mmenke, 2016-01-22 21:44:10]

The patchset sent to the CQ was uploaded after l-g-t-m from rdsmith@chromium.org
Link to the patchset: https://codereview.chromium.org/1563633002/#ps350001
(title: "Response to comments")

[commit-bot: I haz the power, 2016-01-22 21:45:52]

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

[commit-bot: I haz the power, 2016-01-22 23:18:31]

Description was changed from

==========
Make URLRequestJob::SetStatus private.

Recent CLs have already made it so URLRequestJob itself calls the
method itself whenever necessary, except when URLRequestJob::Start is
being called, where subclasses were still expected to invoke it.
Most of them were failing to do so, but since nothing cares about the
state after start, and little distinguished between ERR_IO_PENDING and
SUCCESS, everything was still working.  URLRequest now sets its status
itself before it starts a URLRequestJob.

This CL also makes the URLRequest consistently have a status of
ERR_IO_PENDING when it's doing something - there were a number of
paths where this wasn't previously the case.

The state machine changes also coincidentally fix issue 575213,
which regressed in https://codereview.chromium.org/1467603002, so this
CL includes a test for that fix.

In the future, I'd really like to get rid of URLRequestJob::SetStatus,
and have URLRequests manage their own status.

BUG=564250,575213
TBR=jianli@chromium.org, phajdan.jr@chromium.org
==========

to

==========
Make URLRequestJob::SetStatus private.

Recent CLs have already made it so URLRequestJob itself calls the
method itself whenever necessary, except when URLRequestJob::Start is
being called, where subclasses were still expected to invoke it.
Most of them were failing to do so, but since nothing cares about the
state after start, and little distinguished between ERR_IO_PENDING and
SUCCESS, everything was still working.  URLRequest now sets its status
itself before it starts a URLRequestJob.

This CL also makes the URLRequest consistently have a status of
ERR_IO_PENDING when it's doing something - there were a number of
paths where this wasn't previously the case.

The state machine changes also coincidentally fix issue 575213,
which regressed in https://codereview.chromium.org/1467603002, so this
CL includes a test for that fix.

In the future, I'd really like to get rid of URLRequestJob::SetStatus,
and have URLRequests manage their own status.

BUG=564250,575213
TBR=jianli@chromium.org, phajdan.jr@chromium.org
==========

[commit-bot: I haz the power, 2016-01-22 23:18:32]

Committed patchset #11 (id:350001)

[commit-bot: I haz the power, 2016-01-22 23:19:32]

Description was changed from

==========
Make URLRequestJob::SetStatus private.

Recent CLs have already made it so URLRequestJob itself calls the
method itself whenever necessary, except when URLRequestJob::Start is
being called, where subclasses were still expected to invoke it.
Most of them were failing to do so, but since nothing cares about the
state after start, and little distinguished between ERR_IO_PENDING and
SUCCESS, everything was still working.  URLRequest now sets its status
itself before it starts a URLRequestJob.

This CL also makes the URLRequest consistently have a status of
ERR_IO_PENDING when it's doing something - there were a number of
paths where this wasn't previously the case.

The state machine changes also coincidentally fix issue 575213,
which regressed in https://codereview.chromium.org/1467603002, so this
CL includes a test for that fix.

In the future, I'd really like to get rid of URLRequestJob::SetStatus,
and have URLRequests manage their own status.

BUG=564250,575213
TBR=jianli@chromium.org, phajdan.jr@chromium.org
==========

to

==========
Make URLRequestJob::SetStatus private.

Recent CLs have already made it so URLRequestJob itself calls the
method itself whenever necessary, except when URLRequestJob::Start is
being called, where subclasses were still expected to invoke it.
Most of them were failing to do so, but since nothing cares about the
state after start, and little distinguished between ERR_IO_PENDING and
SUCCESS, everything was still working.  URLRequest now sets its status
itself before it starts a URLRequestJob.

This CL also makes the URLRequest consistently have a status of
ERR_IO_PENDING when it's doing something - there were a number of
paths where this wasn't previously the case.

The state machine changes also coincidentally fix issue 575213,
which regressed in https://codereview.chromium.org/1467603002, so this
CL includes a test for that fix.

In the future, I'd really like to get rid of URLRequestJob::SetStatus,
and have URLRequests manage their own status.

BUG=564250,575213
TBR=jianli@chromium.org, phajdan.jr@chromium.org

Committed: https://crrev.com/0fdf6ebeb19c8dfbb73e398d72a00be61d2aa444
Cr-Commit-Position: refs/heads/master@{#371072}
==========

[commit-bot: I haz the power, 2016-01-22 23:19:33]

Patchset 11 (id:??) landed as
https://crrev.com/0fdf6ebeb19c8dfbb73e398d72a00be61d2aa444
Cr-Commit-Position: refs/heads/master@{#371072}