URLRequestJob: change ReadRawData contract

URLRequestJob: change ReadRawData contract

This CL is patched from ellyjones@ CL at crrev.com/1227893004.

Previously, the interface for URLRequestJob::ReadRawData was as follows:

  bool ReadRawData(IOBuffer*, int, int*)

Subclasses were expected to signal completion of the ReadRawData call by
calling NotifyDone, SetStatus, or maybe one of the other Notify* functions
on URLRequestJob, most of which do internal housekeeping and also drive the
URLRequest's state machine. This made it difficult to reason about the
URLRequestJob's state machine and needlessly complicated most of URLRequestJob.

The new interface is as follows:

  int ReadRawData(IOBuffer*, int)

Subclasses are required to either:

a) Return ERR_IO_PENDING, and call ReadRawDataComplete when the read completes in any way, or
b) Return a count of bytes read >= 0, indicating synchronous success, or
c) Return another error code < 0, indicating synchronous failure.

This substantially narrows the interface between URLRequestJob and its
subclasses and moves the logic for the URLRequest state machine largely into
URLRequestJob.

Also, the signature of URLRequestJob::ReadFilteredData and some other internal
URLRequestJob helpers changes to propagate detailed error codes instead of
coercing all errors to FAILED.

BUG=474859
BUG=329902
Committed: https://crrev.com/a505cbb45ec92d2617fe80490acf7d01ef4d3225
Cr-Commit-Position: refs/heads/master@{#358327}

[xunjieli, 2015-10-20 17:16:48]

Description was changed from

==========
URLRequestJob: change ReadRawData contract

This CL is patched from ellyjones@ CL at crrev.com/1227893004.

Previously, the interface for URLRequestJob::ReadRawData was as follows:

  int ReadRawData(IOBuffer*, int, int*)

Subclasses were expected to signal completion of the ReadRawData call by
calling NotifyDone, SetStatus, or maybe one of the other Notify* functions
on URLRequestJob, most of which do internal housekeeping and also drive the
URLRequest's state machine. This made it difficult to reason about the
URLRequestJob's state machine and needlessly complicated most of URLRequestJob.

The new interface is as follows:

  int ReadRawData(IOBuffer*, int)

Subclasses are required to either:

a) Return ERR_IO_PENDING, and call ReadRawDataComplete when the read completes
in any way, or
b) Return a count of bytes read >= 0, indicating synchronous success, or
c) Return another error code < 0, indicating synchronous failure.

This substantially narrows the interface between URLRequestJob and its
subclasses and moves the logic for the URLRequest state machine largely into
URLRequestJob.

Also, the signature of URLRequestJob::ReadFilteredData and some other internal
URLRequestJob helpers changes to propagate detailed error codes instead of
coercing all errors to FAILED.

BUG=474859
==========

to

==========
URLRequestJob: change ReadRawData contract

This CL is patched from ellyjones@ CL at crrev.com/1227893004.

Previously, the interface for URLRequestJob::ReadRawData was as follows:

  bool ReadRawData(IOBuffer*, int, int*)

Subclasses were expected to signal completion of the ReadRawData call by
calling NotifyDone, SetStatus, or maybe one of the other Notify* functions
on URLRequestJob, most of which do internal housekeeping and also drive the
URLRequest's state machine. This made it difficult to reason about the
URLRequestJob's state machine and needlessly complicated most of URLRequestJob.

The new interface is as follows:

  int ReadRawData(IOBuffer*, int)

Subclasses are required to either:

a) Return ERR_IO_PENDING, and call ReadRawDataComplete when the read completes
in any way, or
b) Return a count of bytes read >= 0, indicating synchronous success, or
c) Return another error code < 0, indicating synchronous failure.

This substantially narrows the interface between URLRequestJob and its
subclasses and moves the logic for the URLRequest state machine largely into
URLRequestJob.

Also, the signature of URLRequestJob::ReadFilteredData and some other internal
URLRequestJob helpers changes to propagate detailed error codes instead of
coercing all errors to FAILED.

BUG=474859
==========

[xunjieli, 2015-10-20 19:24:12]

Patchset #3 (id:40001) has been deleted

[xunjieli, 2015-10-21 21:15:02]

Patchset #5 (id:100001) has been deleted

[xunjieli, 2015-10-22 15:10:03]

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

[xunjieli, 2015-10-22 15:10:04]

Picking up where Elly has left off with crrev.com/1227893004. Bots should be
happy-ish now. PTAL.

[mmenke, 2015-10-22 15:45:30]

On 2015/10/22 15:10:04, xunjieli wrote:
> Picking up where Elly has left off with crrev.com/1227893004. Bots should be
> happy-ish now. PTAL.

Thanks so much for picking this up!  Know cleanups can seem rather unexciting,
but badness here has lead to a fair number of issues.

[mmenke, 2015-10-22 18:58:11]

Some comments, mostly about comments.  On triage duty next two workdays, and
I'll need to spend some time refreshing my memory about some of the changes in
URLRequestJob::ReadRawDataComplete, so may not have time to do a more thorough
pass until Tuesday.

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

https://codereview.chromium.org/1410643007/diff/180001/net/url_request/url_re...
net/url_request/url_request_job.cc:130: DoneReading();
Randy:  Did we ever figure out why this isn't called (Here or in
ReadRawDataComplete) when there's no filter?

https://codereview.chromium.org/1410643007/diff/180001/net/url_request/url_re...
net/url_request/url_request_job.cc:520: // Tell the filter that it has more data
nit:  +.

https://codereview.chromium.org/1410643007/diff/180001/net/url_request/url_re...
net/url_request/url_request_job.cc:528: notify = true;
Keep comment from before?  ("Don't notify if we had an error.")

https://codereview.chromium.org/1410643007/diff/180001/net/url_request/url_re...
net/url_request/url_request_job.cc:567: }
Maybe:

if (error == OK && bytes_read > 0) {
  SetStatus(URLRequetsStatus());
} else {
  NotifyDone(URLRequestStatus::FromError(error));
}

https://codereview.chromium.org/1410643007/diff/180001/net/url_request/url_re...
File net/url_request/url_request_job.h (right):

https://codereview.chromium.org/1410643007/diff/180001/net/url_request/url_re...
net/url_request/url_request_job.h:299: //         read.
Maybe add "0 means eof"?

https://codereview.chromium.org/1410643007/diff/180001/net/url_request/url_re...
net/url_request/url_request_job.h:303: //   Any other ERR_: Read failed
synchronously.
Maybe:  Something like:  "Any other negative number: Read failed syncrhonously.
Return value is a network error code."

https://codereview.chromium.org/1410643007/diff/180001/net/url_request/url_re...
net/url_request/url_request_job.h:356: // Turns an integer result code as from
|ReadRawData| into an Error and a
Suggest "Turns an integer result code as from" -> "Turns an integer result code,
such as one from |ReadRawData|" (Could even remove the reference to ReadRawData)

https://codereview.chromium.org/1410643007/diff/180001/net/url_request/url_re...
net/url_request/url_request_job.h:374: // an error occurred (or we are waiting
for IO to complete).
This comment is no longer accurate

https://codereview.chromium.org/1410643007/diff/180001/net/url_request/url_re...
net/url_request/url_request_job.h:392: // |bytes_read| is < 0, an error occurred
and no bytes were read.
This comment needs to be updated.

https://codereview.chromium.org/1410643007/diff/180001/net/url_request/url_re...
net/url_request/url_request_job.h:406: // finished doing all IO.
nit:  --"we"

https://codereview.chromium.org/1410643007/diff/180001/net/url_request/url_re...
net/url_request/url_request_job.h:409: // Some work performed by NotifyDone must
be completed on a separate task so
suggest "on a separate task" -> "asynchronously"

https://codereview.chromium.org/1410643007/diff/180001/net/url_request/url_re...
net/url_request/url_request_job.h:411: // perform that work.
"exists to perform" -> "performs"

[Randy Smith (Not in Mondays), 2015-10-22 20:38:45]

Thanks Helen!  

My comments are mostly nits--this is pretty close to ready from my perspective. 
Anything marked as "suggestion" is up to you what you want to do with it.

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

https://codereview.chromium.org/1410643007/diff/180001/android_webview/browse...
android_webview/browser/net/android_stream_reader_url_request_job.cc:380: //
because we need to do multipart encoding here.
I don't understand why the comment is duplicated (here and below)?

https://codereview.chromium.org/1410643007/diff/180001/content/browser/androi...
File content/browser/android/url_request_content_job.cc (right):

https://codereview.chromium.org/1410643007/diff/180001/content/browser/androi...
content/browser/android/url_request_content_job.cc:81: }
nit, suggestion: I think that this code would be clearer without the nested ifs
and early return:

if (rv == net::ERR_IO_PENDING)
    io_pending_ = true;
if (rv > 0)
    remaining_bytes -= rv;
DCHECK...
return rv;

https://codereview.chromium.org/1410643007/diff/180001/content/browser/servic...
File content/browser/service_worker/service_worker_write_to_cache_job.cc
(right):

https://codereview.chromium.org/1410643007/diff/180001/content/browser/servic...
content/browser/service_worker/service_worker_write_to_cache_job.cc:396:
DCHECK_NE(net::ERR_IO_PENDING, result);
nit: This feels like overkill, since it's basically checking "If the if
statement four lines above this one failed to execute correctly for some
reason".

https://codereview.chromium.org/1410643007/diff/180001/content/browser/servic...
File content/browser/service_worker/service_worker_write_to_cache_job.h (right):

https://codereview.chromium.org/1410643007/diff/180001/content/browser/servic...
content/browser/service_worker/service_worker_write_to_cache_job.h:75: void
NotifyReadComplete(int bytes_read);
I can't find this function anywhere in the .cc file; maybe a leftover bit of
pre-merge cruft?  (If it is used, I'm probably going to want a different name,
but I'm hoping that's moot.)

https://codereview.chromium.org/1410643007/diff/180001/net/url_request/url_re...
File net/url_request/url_request_file_job.cc (right):

https://codereview.chromium.org/1410643007/diff/180001/net/url_request/url_re...
net/url_request/url_request_file_job.cc:169: // not safe to call (it may reenter
the delegate).
This comment is useful, but there are several other jobs in the same position
(store range error in SetExtraRequestHeaders, report it later) which could use
the same comment; would you be willing to copy it to those other jobs?

https://codereview.chromium.org/1410643007/diff/180001/net/url_request/url_re...
net/url_request/url_request_file_job.cc:175: // later use in DidRead(), which is
responsible for some of the range
Should this refer to DidOpen() rather than DidRead()?

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

https://codereview.chromium.org/1410643007/diff/180001/net/url_request/url_re...
net/url_request/url_request_job.cc:130: DoneReading();
On 2015/10/22 18:58:10, mmenke wrote:
> Randy:  Did we ever figure out why this isn't called (Here or in
> ReadRawDataComplete) when there's no filter?

So the comment in url_request_job.h says:

  // Called to tell the job that a filter has successfully reached the end of
  // the stream.

So this usage is in accordance with that comment.  Having said that,
URLRequestHttpJob uses it to decide when to call transaction_->DoneReading(),
which seems like something that you want to do whether or not you have a filter
chain.  I think URLRequestHttpJob's use of it is the only use of it, so if we
don't need it (which seems likely--I presume URLRHJ is implicitly relying on
destruction for tearing down the transaction in most cases), we should probably
just remove it.  WDYT?

https://codereview.chromium.org/1410643007/diff/180001/net/url_request/url_re...
net/url_request/url_request_job.cc:890: return;
It looks to me (you should verify) like this will result in a leak of the
reference taken in ReadRawDataHelper above where we assign buf to
raw_read_buffer_; in the old code, OnRawReadComplete was always called on
synchronous return, and the entire function traversed (resulting in the rrb_ =
NULL always being executed) but now that only happens when there isn't an error.
 This isn't a big deal (the reference will be released when URLRequestJob is
destroyed) but it's a minor glitch.

https://codereview.chromium.org/1410643007/diff/180001/net/url_request/url_re...
File net/url_request/url_request_job.h (right):

https://codereview.chromium.org/1410643007/diff/180001/net/url_request/url_re...
net/url_request/url_request_job.h:305: // cancelled using the refcount on |buf|.
nit, suggestion: I find this sentence a bit confusing as written (because it
runs the behavior description into the mechanism).  How about: "This method
might hold onto a reference to |buf| (by incrementing the refcount) until the
method completes or is cancelled."

https://codereview.chromium.org/1410643007/diff/180001/net/url_request/url_re...
net/url_request/url_request_job.h:377: // Informs the filter that data has been
read into its buffer.
nit, suggestion: "Inform the filter chain" or "Inform the first filter".

https://codereview.chromium.org/1410643007/diff/180001/storage/browser/blob/b...
File storage/browser/blob/blob_url_request_job.cc (right):

https://codereview.chromium.org/1410643007/diff/180001/storage/browser/blob/b...
storage/browser/blob/blob_url_request_job.cc:221: if (result < 0) {
I don't think this conditional is needed anymore; you can just call
ReadRawDataComplete(result); unilaterally, right?

https://codereview.chromium.org/1410643007/diff/180001/storage/browser/blob/b...
storage/browser/blob/blob_url_request_job.cc:225: // Clear the IO_PENDING status
nit: I think this comment is out of date now?

[mmenke, 2015-10-22 20:42:47]

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

https://codereview.chromium.org/1410643007/diff/180001/net/url_request/url_re...
net/url_request/url_request_job.cc:130: DoneReading();
On 2015/10/22 20:38:45, rdsmith wrote:
> On 2015/10/22 18:58:10, mmenke wrote:
> > Randy:  Did we ever figure out why this isn't called (Here or in
> > ReadRawDataComplete) when there's no filter?
> 
> So the comment in url_request_job.h says:
> 
>   // Called to tell the job that a filter has successfully reached the end of
>   // the stream.
> 
> So this usage is in accordance with that comment.  Having said that,
> URLRequestHttpJob uses it to decide when to call transaction_->DoneReading(),
> which seems like something that you want to do whether or not you have a
filter
> chain.  I think URLRequestHttpJob's use of it is the only use of it, so if we
> don't need it (which seems likely--I presume URLRHJ is implicitly relying on
> destruction for tearing down the transaction in most cases), we should
probably
> just remove it.  WDYT?

I thought transaction_->DoneReading() was used to tell the cache that the entire
response was successfully received?  Maybe this is so that we successfully cache
responses in the case content-length is actually the uncompressed content
length?  IF so, I'm fine with not caching those responses.  I don't think that
would affect caching under any other circumstances.  Should probably talk to
Gavin before removing this, but if we can reasonably remove it, I'd like to do
so.  This state machine has too must stuff.  :(

[xunjieli, 2015-10-23 13:43:08]

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

https://codereview.chromium.org/1410643007/diff/180001/android_webview/browse...
android_webview/browser/net/android_stream_reader_url_request_job.cc:380: //
because we need to do multipart encoding here.
On 2015/10/22 20:38:45, rdsmith wrote:
> I don't understand why the comment is duplicated (here and below)?  

Done. I reverted some of elly's change, and ended up duplicating the comment.
Thanks for catching this!

https://codereview.chromium.org/1410643007/diff/180001/content/browser/androi...
File content/browser/android/url_request_content_job.cc (right):

https://codereview.chromium.org/1410643007/diff/180001/content/browser/androi...
content/browser/android/url_request_content_job.cc:81: }
On 2015/10/22 20:38:45, rdsmith wrote:
> nit, suggestion: I think that this code would be clearer without the nested
ifs
> and early return:
> 
> if (rv == net::ERR_IO_PENDING)
>     io_pending_ = true;
> if (rv > 0)
>     remaining_bytes -= rv;
> DCHECK...
> return rv;

Done.

https://codereview.chromium.org/1410643007/diff/180001/content/browser/servic...
File content/browser/service_worker/service_worker_write_to_cache_job.cc
(right):

https://codereview.chromium.org/1410643007/diff/180001/content/browser/servic...
content/browser/service_worker/service_worker_write_to_cache_job.cc:396:
DCHECK_NE(net::ERR_IO_PENDING, result);
On 2015/10/22 20:38:45, rdsmith wrote:
> nit: This feels like overkill, since it's basically checking "If the if
> statement four lines above this one failed to execute correctly for some
> reason".

Done.

https://codereview.chromium.org/1410643007/diff/180001/content/browser/servic...
File content/browser/service_worker/service_worker_write_to_cache_job.h (right):

https://codereview.chromium.org/1410643007/diff/180001/content/browser/servic...
content/browser/service_worker/service_worker_write_to_cache_job.h:75: void
NotifyReadComplete(int bytes_read);
On 2015/10/22 20:38:45, rdsmith wrote:
> I can't find this function anywhere in the .cc file; maybe a leftover bit of
> pre-merge cruft?  (If it is used, I'm probably going to want a different name,
> but I'm hoping that's moot.)

Done.

https://codereview.chromium.org/1410643007/diff/180001/net/url_request/url_re...
File net/url_request/url_request_file_job.cc (right):

https://codereview.chromium.org/1410643007/diff/180001/net/url_request/url_re...
net/url_request/url_request_file_job.cc:169: // not safe to call (it may reenter
the delegate).
On 2015/10/22 20:38:45, rdsmith wrote:
> This comment is useful, but there are several other jobs in the same position
> (store range error in SetExtraRequestHeaders, report it later) which could use
> the same comment; would you be willing to copy it to those other jobs?

Done.

I have a question though. Since SetExtraRequestHeaders is called before Start.
Why do we have a URLRequest::Delegate call on the stack?
My understanding is that NotifyStartError should not be called here, because the
request has not started.

https://codereview.chromium.org/1410643007/diff/180001/net/url_request/url_re...
net/url_request/url_request_file_job.cc:175: // later use in DidRead(), which is
responsible for some of the range
On 2015/10/22 20:38:45, rdsmith wrote:
> Should this refer to DidOpen() rather than DidRead()?

Done.

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

https://codereview.chromium.org/1410643007/diff/180001/net/url_request/url_re...
net/url_request/url_request_job.cc:520: // Tell the filter that it has more data
On 2015/10/22 18:58:10, mmenke wrote:
> nit:  +.

Done.

https://codereview.chromium.org/1410643007/diff/180001/net/url_request/url_re...
net/url_request/url_request_job.cc:528: notify = true;
On 2015/10/22 18:58:11, mmenke wrote:
> Keep comment from before?  ("Don't notify if we had an error.")

Done.

https://codereview.chromium.org/1410643007/diff/180001/net/url_request/url_re...
net/url_request/url_request_job.cc:567: }
On 2015/10/22 18:58:10, mmenke wrote:
> Maybe:
> 
> if (error == OK && bytes_read > 0) {
>   SetStatus(URLRequetsStatus());
> } else {
>   NotifyDone(URLRequestStatus::FromError(error));
> }

Done.

https://codereview.chromium.org/1410643007/diff/180001/net/url_request/url_re...
net/url_request/url_request_job.cc:890: return;
On 2015/10/22 20:38:45, rdsmith wrote:
> It looks to me (you should verify) like this will result in a leak of the
> reference taken in ReadRawDataHelper above where we assign buf to
> raw_read_buffer_; in the old code, OnRawReadComplete was always called on
> synchronous return, and the entire function traversed (resulting in the rrb_ =
> NULL always being executed) but now that only happens when there isn't an
error.
>  This isn't a big deal (the reference will be released when URLRequestJob is
> destroyed) but it's a minor glitch.

Done. I believe you are right.

https://codereview.chromium.org/1410643007/diff/180001/net/url_request/url_re...
File net/url_request/url_request_job.h (right):

https://codereview.chromium.org/1410643007/diff/180001/net/url_request/url_re...
net/url_request/url_request_job.h:299: //         read.
On 2015/10/22 18:58:11, mmenke wrote:
> Maybe add "0 means eof"?

Done.

https://codereview.chromium.org/1410643007/diff/180001/net/url_request/url_re...
net/url_request/url_request_job.h:303: //   Any other ERR_: Read failed
synchronously.
On 2015/10/22 18:58:11, mmenke wrote:
> Maybe:  Something like:  "Any other negative number: Read failed
syncrhonously.
> Return value is a network error code."

Done.

https://codereview.chromium.org/1410643007/diff/180001/net/url_request/url_re...
net/url_request/url_request_job.h:305: // cancelled using the refcount on |buf|.
On 2015/10/22 20:38:45, rdsmith wrote:
> nit, suggestion: I find this sentence a bit confusing as written (because it
> runs the behavior description into the mechanism).  How about: "This method
> might hold onto a reference to |buf| (by incrementing the refcount) until the
> method completes or is cancelled."

Done.

https://codereview.chromium.org/1410643007/diff/180001/net/url_request/url_re...
net/url_request/url_request_job.h:356: // Turns an integer result code as from
|ReadRawData| into an Error and a
On 2015/10/22 18:58:11, mmenke wrote:
> Suggest "Turns an integer result code as from" -> "Turns an integer result
code,
> such as one from |ReadRawData|" (Could even remove the reference to
ReadRawData)

Done.

https://codereview.chromium.org/1410643007/diff/180001/net/url_request/url_re...
net/url_request/url_request_job.h:374: // an error occurred (or we are waiting
for IO to complete).
On 2015/10/22 18:58:11, mmenke wrote:
> This comment is no longer accurate

Done.

https://codereview.chromium.org/1410643007/diff/180001/net/url_request/url_re...
net/url_request/url_request_job.h:377: // Informs the filter that data has been
read into its buffer.
On 2015/10/22 20:38:45, rdsmith wrote:
> nit, suggestion: "Inform the filter chain" or "Inform the first filter".

Done.

https://codereview.chromium.org/1410643007/diff/180001/net/url_request/url_re...
net/url_request/url_request_job.h:392: // |bytes_read| is < 0, an error occurred
and no bytes were read.
On 2015/10/22 18:58:11, mmenke wrote:
> This comment needs to be updated.

Done.

https://codereview.chromium.org/1410643007/diff/180001/net/url_request/url_re...
net/url_request/url_request_job.h:406: // finished doing all IO.
On 2015/10/22 18:58:11, mmenke wrote:
> nit:  --"we"

Done.

https://codereview.chromium.org/1410643007/diff/180001/net/url_request/url_re...
net/url_request/url_request_job.h:409: // Some work performed by NotifyDone must
be completed on a separate task so
On 2015/10/22 18:58:11, mmenke wrote:
> suggest "on a separate task" -> "asynchronously"

Done.

https://codereview.chromium.org/1410643007/diff/180001/net/url_request/url_re...
net/url_request/url_request_job.h:411: // perform that work.
On 2015/10/22 18:58:11, mmenke wrote:
> "exists to perform" -> "performs"

Done.

https://codereview.chromium.org/1410643007/diff/180001/storage/browser/blob/b...
File storage/browser/blob/blob_url_request_job.cc (right):

https://codereview.chromium.org/1410643007/diff/180001/storage/browser/blob/b...
storage/browser/blob/blob_url_request_job.cc:221: if (result < 0) {
On 2015/10/22 20:38:45, rdsmith wrote:
> I don't think this conditional is needed anymore; you can just call
> ReadRawDataComplete(result); unilaterally, right?

Done.

https://codereview.chromium.org/1410643007/diff/180001/storage/browser/blob/b...
storage/browser/blob/blob_url_request_job.cc:225: // Clear the IO_PENDING status
On 2015/10/22 20:38:45, rdsmith wrote:
> nit: I think this comment is out of date now?

Done.

[Randy Smith (Not in Mondays), 2015-10-26 21:38:03]

Looking good; only holding off stamp for the checkin with Elly around the
NotifyStartError() comment, and any further hashing that Matt & I need to do
around the right next step with DoneReading().

https://codereview.chromium.org/1410643007/diff/180001/net/url_request/url_re...
File net/url_request/url_request_file_job.cc (right):

https://codereview.chromium.org/1410643007/diff/180001/net/url_request/url_re...
net/url_request/url_request_file_job.cc:169: // not safe to call (it may reenter
the delegate).
On 2015/10/23 13:43:08, xunjieli wrote:
> On 2015/10/22 20:38:45, rdsmith wrote:
> > This comment is useful, but there are several other jobs in the same
position
> > (store range error in SetExtraRequestHeaders, report it later) which could
use
> > the same comment; would you be willing to copy it to those other jobs?
> 
> Done.
> 
> I have a question though. Since SetExtraRequestHeaders is called before Start.
> Why do we have a URLRequest::Delegate call on the stack?
> My understanding is that NotifyStartError should not be called here, because
the
> request has not started.

Hmmm.  Good point; I think you're right.  Specifically, it seems really wrong
that URLRequest::NotifyResponseStarted() can be called before
URLRequest::Start() is called.  I'd suggest checking with Elly (in case there's
something I'm missing) and then changing the comment over to indicating that
NotifyStartError() isn't legal to call before Start() has been called (might be
worthwhile making that explicit in url_request_job.h as well).

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

https://codereview.chromium.org/1410643007/diff/180001/net/url_request/url_re...
net/url_request/url_request_job.cc:130: DoneReading();
On 2015/10/22 20:42:46, mmenke wrote:
> On 2015/10/22 20:38:45, rdsmith wrote:
> > On 2015/10/22 18:58:10, mmenke wrote:
> > > Randy:  Did we ever figure out why this isn't called (Here or in
> > > ReadRawDataComplete) when there's no filter?
> > 
> > So the comment in url_request_job.h says:
> > 
> >   // Called to tell the job that a filter has successfully reached the end
of
> >   // the stream.
> > 
> > So this usage is in accordance with that comment.  Having said that,
> > URLRequestHttpJob uses it to decide when to call
transaction_->DoneReading(),
> > which seems like something that you want to do whether or not you have a
> filter
> > chain.  I think URLRequestHttpJob's use of it is the only use of it, so if
we
> > don't need it (which seems likely--I presume URLRHJ is implicitly relying on
> > destruction for tearing down the transaction in most cases), we should
> probably
> > just remove it.  WDYT?
> 
> I thought transaction_->DoneReading() was used to tell the cache that the
entire
> response was successfully received?  Maybe this is so that we successfully
cache
> responses in the case content-length is actually the uncompressed content
> length?  IF so, I'm fine with not caching those responses.  I don't think that
> would affect caching under any other circumstances.  Should probably talk to
> Gavin before removing this, but if we can reasonably remove it, I'd like to do
> so.  This state machine has too must stuff.  :(

So the comment in HttpTransaction suggests why we can get away with not calling
it in the filter case:

  // Called to tell the transaction that we have successfully reached the end
  // of the stream. This is equivalent to performing an extra Read() at the end
  // that should return 0 bytes. 

My presumption is that if we don't have a filter, we always do the final read,
since that's how we figure out that we're at EOF.  So that means we do need to
keep this in the filter case (IIUC).  I wouldn't at all mind making it happen
for both filter and non-filter cases though, for consistency so that some future
reviewers don't have to have this conversation again.

https://codereview.chromium.org/1410643007/diff/180001/net/url_request/url_re...
net/url_request/url_request_job.cc:890: return;
On 2015/10/23 13:43:08, xunjieli wrote:
> On 2015/10/22 20:38:45, rdsmith wrote:
> > It looks to me (you should verify) like this will result in a leak of the
> > reference taken in ReadRawDataHelper above where we assign buf to
> > raw_read_buffer_; in the old code, OnRawReadComplete was always called on
> > synchronous return, and the entire function traversed (resulting in the rrb_
=
> > NULL always being executed) but now that only happens when there isn't an
> error.
> >  This isn't a big deal (the reference will be released when URLRequestJob is
> > destroyed) but it's a minor glitch.
> 
> Done. I believe you are right.

nit, suggestion: I think the code would be a bit clearer (even if one line
longer) with the original "return on error/main code not indented" structure + a
duplication of the "read_raw_buffer_ = NULL;" line.  Up to you, though.

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

https://codereview.chromium.org/1410643007/diff/200001/net/url_request/url_re...
net/url_request/url_request_job.cc:538: // Don't notify if we had an error.
nit: Given that the sense of the code is the same ("notify = true;" is executed
if the error code is OK) I'd make the comments parallel as well; otherwise you
risk the reader wondering "What's the significance of the difference?".  I don't
care which of the two forms is used, though.

[xunjieli, 2015-10-27 14:17:21]

Thanks for the review! PTAL

https://codereview.chromium.org/1410643007/diff/180001/net/url_request/url_re...
File net/url_request/url_request_file_job.cc (right):

https://codereview.chromium.org/1410643007/diff/180001/net/url_request/url_re...
net/url_request/url_request_file_job.cc:169: // not safe to call (it may reenter
the delegate).
On 2015/10/26 21:38:03, rdsmith wrote:
> On 2015/10/23 13:43:08, xunjieli wrote:
> > On 2015/10/22 20:38:45, rdsmith wrote:
> > > This comment is useful, but there are several other jobs in the same
> position
> > > (store range error in SetExtraRequestHeaders, report it later) which could
> use
> > > the same comment; would you be willing to copy it to those other jobs?
> > 
> > Done.
> > 
> > I have a question though. Since SetExtraRequestHeaders is called before
Start.
> > Why do we have a URLRequest::Delegate call on the stack?
> > My understanding is that NotifyStartError should not be called here, because
> the
> > request has not started.
> 
> Hmmm.  Good point; I think you're right.  Specifically, it seems really wrong
> that URLRequest::NotifyResponseStarted() can be called before
> URLRequest::Start() is called.  I'd suggest checking with Elly (in case
there's
> something I'm missing) and then changing the comment over to indicating that
> NotifyStartError() isn't legal to call before Start() has been called (might
be
> worthwhile making that explicit in url_request_job.h as well).

Done. I've checked with Elly too.

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

https://codereview.chromium.org/1410643007/diff/180001/net/url_request/url_re...
net/url_request/url_request_job.cc:890: return;
On 2015/10/26 21:38:03, rdsmith wrote:
> On 2015/10/23 13:43:08, xunjieli wrote:
> > On 2015/10/22 20:38:45, rdsmith wrote:
> > > It looks to me (you should verify) like this will result in a leak of the
> > > reference taken in ReadRawDataHelper above where we assign buf to
> > > raw_read_buffer_; in the old code, OnRawReadComplete was always called on
> > > synchronous return, and the entire function traversed (resulting in the
rrb_
> =
> > > NULL always being executed) but now that only happens when there isn't an
> > error.
> > >  This isn't a big deal (the reference will be released when URLRequestJob
is
> > > destroyed) but it's a minor glitch.
> > 
> > Done. I believe you are right.
> 
> nit, suggestion: I think the code would be a bit clearer (even if one line
> longer) with the original "return on error/main code not indented" structure +
a
> duplication of the "read_raw_buffer_ = NULL;" line.  Up to you, though.

Done.

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

https://codereview.chromium.org/1410643007/diff/200001/net/url_request/url_re...
net/url_request/url_request_job.cc:538: // Don't notify if we had an error.
On 2015/10/26 21:38:03, rdsmith wrote:
> nit: Given that the sense of the code is the same ("notify = true;" is
executed
> if the error code is OK) I'd make the comments parallel as well; otherwise you
> risk the reader wondering "What's the significance of the difference?".  I
don't
> care which of the two forms is used, though.
> 

Done.

[Randy Smith (Not in Mondays), 2015-10-27 16:04:58]

LGTM, but obvious wait for Matt's stamp too.  My current belief is that the
DoneReading() issue doesn't affect this CL (we aren't changing behavior) but if
Matt disagrees we may need to do something about it.

[mmenke, 2015-10-27 17:53:51]

On 2015/10/27 16:04:58, rdsmith wrote:
> LGTM, but obvious wait for Matt's stamp too.  My current belief is that the
> DoneReading() issue doesn't affect this CL (we aren't changing behavior) but
if
> Matt disagrees we may need to do something about it.

I'll try to get to this today, but may not have time until tomorrow afternoon. 
Sorry for slowness.  :(

[mmenke, 2015-10-27 19:41:08]

Haven't review everything yet - started at net/ and then started to wrap around.

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

https://codereview.chromium.org/1410643007/diff/220001/android_webview/browse...
android_webview/browser/net/android_stream_reader_url_request_job.cc:292:
range_parse_result_));
It's not really safe to call NotifyStartError synchronously from Start(),
although we don't currently have any protection against it.  Suggest just
posting a task to run DoStart on construction, using a weak pointer, instead of
calling DoStart synchronously.

https://codereview.chromium.org/1410643007/diff/220001/android_webview/browse...
android_webview/browser/net/android_stream_reader_url_request_job.cc:374: // the
job has not started.
See comment in other file (*file_dir_job.cc?)

https://codereview.chromium.org/1410643007/diff/220001/android_webview/browse...
File android_webview/browser/net/android_stream_reader_url_request_job.h
(right):

https://codereview.chromium.org/1410643007/diff/220001/android_webview/browse...
android_webview/browser/net/android_stream_reader_url_request_job.h:134:
net::Error range_parse_result_;
include net/base/net_errors.h (And remove header from cc file - not going to
mention the second part every time I call this out).

https://codereview.chromium.org/1410643007/diff/220001/chrome/browser/chromeo...
File chrome/browser/chromeos/fileapi/external_file_url_request_job.cc (right):

https://codereview.chromium.org/1410643007/diff/220001/chrome/browser/chromeo...
chrome/browser/chromeos/fileapi/external_file_url_request_job.cc:173: // job has
not started.
See comment about this in another file.

https://codereview.chromium.org/1410643007/diff/220001/chrome/browser/chromeo...
chrome/browser/chromeos/fileapi/external_file_url_request_job.cc:198:
range_parse_result_));
Not really safe to call NotifyStartError synchronously on start (NotifyDone was
safe).  :(

https://codereview.chromium.org/1410643007/diff/220001/chrome/browser/chromeo...
File chrome/browser/chromeos/fileapi/external_file_url_request_job.h (right):

https://codereview.chromium.org/1410643007/diff/220001/chrome/browser/chromeo...
chrome/browser/chromeos/fileapi/external_file_url_request_job.h:94: net::Error
range_parse_result_;
include header

https://codereview.chromium.org/1410643007/diff/220001/content/browser/androi...
File content/browser/android/url_request_content_job.h (right):

https://codereview.chromium.org/1410643007/diff/220001/content/browser/androi...
content/browser/android/url_request_content_job.h:92: net::Error
range_parse_result_;
Include header

https://codereview.chromium.org/1410643007/diff/220001/net/url_request/url_re...
File net/url_request/url_request_file_dir_job.h (right):

https://codereview.chromium.org/1410643007/diff/220001/net/url_request/url_re...
net/url_request/url_request_file_dir_job.h:48: void CompleteRead(Error error);
include net/base/net_errors.h

https://codereview.chromium.org/1410643007/diff/220001/net/url_request/url_re...
File net/url_request/url_request_file_job.cc (right):

https://codereview.chromium.org/1410643007/diff/220001/net/url_request/url_re...
net/url_request/url_request_file_job.cc:66: range_parse_result_(net::OK),
net:: not needed.

https://codereview.chromium.org/1410643007/diff/220001/net/url_request/url_re...
net/url_request/url_request_file_job.cc:168: // the job has not started.
Method level documentation about what a method does generally goes in the
header, docs about how it does it generally goes just above the relevant code
inside the method body.

Suggest merging the part of the comment about NotifyStartError with the comment
below the GetHeader line, and the comment that it only cares about range headers
just above that line.

https://codereview.chromium.org/1410643007/diff/220001/net/url_request/url_re...
File net/url_request/url_request_file_job.h (right):

https://codereview.chromium.org/1410643007/diff/220001/net/url_request/url_re...
net/url_request/url_request_file_job.h:104: net::Error range_parse_result_;
include net/base/net_errors.h

https://codereview.chromium.org/1410643007/diff/220001/net/url_request/url_re...
net/url_request/url_request_file_job.h:104: net::Error range_parse_result_;
net:: not needed.

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

https://codereview.chromium.org/1410643007/diff/220001/net/url_request/url_re...
net/url_request/url_request_job.cc:540: notify = true;
I think this can be moved out of this if statement? The other branch
unconditionally sets notify to try, but is only run when error starts out as OK,
too.

https://codereview.chromium.org/1410643007/diff/220001/net/url_request/url_re...
net/url_request/url_request_job.cc:571: // there isn't a filter error? How do
filter errors get notified?
Need to fix this comment:  The issue is that we *do* call it on filter errors,
but not on read errors.  I actually suspect we should call it on no errors (Note
that NotifyDone calls CompleteNotifyDone which calls NotifyReadCompleted with -1
- this looks like a real bug to me, as we're calling NotifyReadCompleted twice)

https://codereview.chromium.org/1410643007/diff/220001/net/url_request/url_re...
File net/url_request/url_request_simple_job.cc (right):

https://codereview.chromium.org/1410643007/diff/220001/net/url_request/url_re...
net/url_request/url_request_simple_job.cc:86: }
Get rid of this method, and just have the posted task call ReadRawDataComplete
directly?

https://codereview.chromium.org/1410643007/diff/220001/net/url_request/url_re...
File net/url_request/url_request_status.cc (right):

https://codereview.chromium.org/1410643007/diff/220001/net/url_request/url_re...
net/url_request/url_request_status.cc:61: break;
Suggest removing this default case - that will result in a compile error if
another value is added, and we already have a NOTREACHED in the fallthrough
code, anyways.

https://codereview.chromium.org/1410643007/diff/220001/net/url_request/url_re...
File net/url_request/url_request_status.h (right):

https://codereview.chromium.org/1410643007/diff/220001/net/url_request/url_re...
net/url_request/url_request_status.h:50: net::Error ToNetError() const;
Remove net::

https://codereview.chromium.org/1410643007/diff/220001/storage/browser/blob/b...
File storage/browser/blob/blob_url_request_job.cc (right):

https://codereview.chromium.org/1410643007/diff/220001/storage/browser/blob/b...
storage/browser/blob/blob_url_request_job.cc:99: return 0;
BUG:  return blob_reader_->net_error()

https://codereview.chromium.org/1410643007/diff/220001/storage/browser/fileap...
File storage/browser/fileapi/file_system_url_request_job.cc (right):

https://codereview.chromium.org/1410643007/diff/220001/storage/browser/fileap...
storage/browser/fileapi/file_system_url_request_job.cc:119: // since the job has
not started.
See my other comment about a similar comment.

https://codereview.chromium.org/1410643007/diff/220001/storage/browser/fileap...
File storage/browser/fileapi/file_system_url_request_job.h (right):

https://codereview.chromium.org/1410643007/diff/220001/storage/browser/fileap...
storage/browser/fileapi/file_system_url_request_job.h:72: net::Error
range_parse_result_;
include net/base/net_errors.h

[xunjieli, 2015-10-27 21:19:30]

Patchset #11 (id:240001) has been deleted

[xunjieli, 2015-10-27 21:24:07]

Thanks for the review!

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

https://codereview.chromium.org/1410643007/diff/220001/android_webview/browse...
android_webview/browser/net/android_stream_reader_url_request_job.cc:292:
range_parse_result_));
On 2015/10/27 19:41:07, mmenke wrote:
> It's not really safe to call NotifyStartError synchronously from Start(),
> although we don't currently have any protection against it.  Suggest just
> posting a task to run DoStart on construction, using a weak pointer, instead
of
> calling DoStart synchronously.

Done. Good to know. Thanks!

https://codereview.chromium.org/1410643007/diff/220001/android_webview/browse...
android_webview/browser/net/android_stream_reader_url_request_job.cc:374: // the
job has not started.
On 2015/10/27 19:41:07, mmenke wrote:
> See comment in other file (*file_dir_job.cc?)

Done.

https://codereview.chromium.org/1410643007/diff/220001/android_webview/browse...
File android_webview/browser/net/android_stream_reader_url_request_job.h
(right):

https://codereview.chromium.org/1410643007/diff/220001/android_webview/browse...
android_webview/browser/net/android_stream_reader_url_request_job.h:134:
net::Error range_parse_result_;
On 2015/10/27 19:41:07, mmenke wrote:
> include net/base/net_errors.h (And remove header from cc file - not going to
> mention the second part every time I call this out).

Done.

https://codereview.chromium.org/1410643007/diff/220001/chrome/browser/chromeo...
File chrome/browser/chromeos/fileapi/external_file_url_request_job.cc (right):

https://codereview.chromium.org/1410643007/diff/220001/chrome/browser/chromeo...
chrome/browser/chromeos/fileapi/external_file_url_request_job.cc:173: // job has
not started.
On 2015/10/27 19:41:07, mmenke wrote:
> See comment about this in another file.

Done.

https://codereview.chromium.org/1410643007/diff/220001/chrome/browser/chromeo...
chrome/browser/chromeos/fileapi/external_file_url_request_job.cc:198:
range_parse_result_));
On 2015/10/27 19:41:07, mmenke wrote:
> Not really safe to call NotifyStartError synchronously on start (NotifyDone
was
> safe).  :(

Done. I created a helper method to be invoked asynchronously. Does it look okay?

https://codereview.chromium.org/1410643007/diff/220001/chrome/browser/chromeo...
File chrome/browser/chromeos/fileapi/external_file_url_request_job.h (right):

https://codereview.chromium.org/1410643007/diff/220001/chrome/browser/chromeo...
chrome/browser/chromeos/fileapi/external_file_url_request_job.h:94: net::Error
range_parse_result_;
On 2015/10/27 19:41:07, mmenke wrote:
> include header

Done.

https://codereview.chromium.org/1410643007/diff/220001/content/browser/androi...
File content/browser/android/url_request_content_job.h (right):

https://codereview.chromium.org/1410643007/diff/220001/content/browser/androi...
content/browser/android/url_request_content_job.h:92: net::Error
range_parse_result_;
On 2015/10/27 19:41:07, mmenke wrote:
> Include header

Done.

https://codereview.chromium.org/1410643007/diff/220001/net/url_request/url_re...
File net/url_request/url_request_file_dir_job.h (right):

https://codereview.chromium.org/1410643007/diff/220001/net/url_request/url_re...
net/url_request/url_request_file_dir_job.h:48: void CompleteRead(Error error);
On 2015/10/27 19:41:07, mmenke wrote:
> include net/base/net_errors.h

Done.

https://codereview.chromium.org/1410643007/diff/220001/net/url_request/url_re...
File net/url_request/url_request_file_job.cc (right):

https://codereview.chromium.org/1410643007/diff/220001/net/url_request/url_re...
net/url_request/url_request_file_job.cc:66: range_parse_result_(net::OK),
On 2015/10/27 19:41:07, mmenke wrote:
> net:: not needed.

Done.

https://codereview.chromium.org/1410643007/diff/220001/net/url_request/url_re...
net/url_request/url_request_file_job.cc:168: // the job has not started.
On 2015/10/27 19:41:08, mmenke wrote:
> Method level documentation about what a method does generally goes in the
> header, docs about how it does it generally goes just above the relevant code
> inside the method body.
> 
> Suggest merging the part of the comment about NotifyStartError with the
comment
> below the GetHeader line, and the comment that it only cares about range
headers
> just above that line.

Done.

https://codereview.chromium.org/1410643007/diff/220001/net/url_request/url_re...
File net/url_request/url_request_file_job.h (right):

https://codereview.chromium.org/1410643007/diff/220001/net/url_request/url_re...
net/url_request/url_request_file_job.h:104: net::Error range_parse_result_;
On 2015/10/27 19:41:08, mmenke wrote:
> net:: not needed.

Done.

https://codereview.chromium.org/1410643007/diff/220001/net/url_request/url_re...
net/url_request/url_request_file_job.h:104: net::Error range_parse_result_;
On 2015/10/27 19:41:08, mmenke wrote:
> include net/base/net_errors.h

Done.

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

https://codereview.chromium.org/1410643007/diff/220001/net/url_request/url_re...
net/url_request/url_request_job.cc:540: notify = true;
On 2015/10/27 19:41:08, mmenke wrote:
> I think this can be moved out of this if statement? The other branch
> unconditionally sets notify to try, but is only run when error starts out as
OK,
> too.

Done.

https://codereview.chromium.org/1410643007/diff/220001/net/url_request/url_re...
net/url_request/url_request_job.cc:571: // there isn't a filter error? How do
filter errors get notified?
On 2015/10/27 19:41:08, mmenke wrote:
> Need to fix this comment:  The issue is that we *do* call it on filter errors,
> but not on read errors.  I actually suspect we should call it on no errors
(Note
> that NotifyDone calls CompleteNotifyDone which calls NotifyReadCompleted with
-1
> - this looks like a real bug to me, as we're calling NotifyReadCompleted
twice)

If that's the case, can I get rid of |notify| and move
request_->NotifyReadCompleted inside the if statement on line 565?

https://codereview.chromium.org/1410643007/diff/220001/net/url_request/url_re...
File net/url_request/url_request_simple_job.cc (right):

https://codereview.chromium.org/1410643007/diff/220001/net/url_request/url_re...
net/url_request/url_request_simple_job.cc:86: }
On 2015/10/27 19:41:08, mmenke wrote:
> Get rid of this method, and just have the posted task call ReadRawDataComplete
> directly?

Done.

https://codereview.chromium.org/1410643007/diff/220001/net/url_request/url_re...
File net/url_request/url_request_status.cc (right):

https://codereview.chromium.org/1410643007/diff/220001/net/url_request/url_re...
net/url_request/url_request_status.cc:61: break;
On 2015/10/27 19:41:08, mmenke wrote:
> Suggest removing this default case - that will result in a compile error if
> another value is added, and we already have a NOTREACHED in the fallthrough
> code, anyways.

Done.

https://codereview.chromium.org/1410643007/diff/220001/net/url_request/url_re...
File net/url_request/url_request_status.h (right):

https://codereview.chromium.org/1410643007/diff/220001/net/url_request/url_re...
net/url_request/url_request_status.h:50: net::Error ToNetError() const;
On 2015/10/27 19:41:08, mmenke wrote:
> Remove net::

Done.

https://codereview.chromium.org/1410643007/diff/220001/storage/browser/blob/b...
File storage/browser/blob/blob_url_request_job.cc (right):

https://codereview.chromium.org/1410643007/diff/220001/storage/browser/blob/b...
storage/browser/blob/blob_url_request_job.cc:99: return 0;
On 2015/10/27 19:41:08, mmenke wrote:
> BUG:  return blob_reader_->net_error()

Done. Good catch! thanks

https://codereview.chromium.org/1410643007/diff/220001/storage/browser/fileap...
File storage/browser/fileapi/file_system_url_request_job.cc (right):

https://codereview.chromium.org/1410643007/diff/220001/storage/browser/fileap...
storage/browser/fileapi/file_system_url_request_job.cc:119: // since the job has
not started.
On 2015/10/27 19:41:08, mmenke wrote:
> See my other comment about a similar comment.

Done.

https://codereview.chromium.org/1410643007/diff/220001/storage/browser/fileap...
File storage/browser/fileapi/file_system_url_request_job.h (right):

https://codereview.chromium.org/1410643007/diff/220001/storage/browser/fileap...
storage/browser/fileapi/file_system_url_request_job.h:72: net::Error
range_parse_result_;
On 2015/10/27 19:41:08, mmenke wrote:
> include net/base/net_errors.h

Done.

[mmenke, 2015-10-27 21:30:51]

One quick response.

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

https://codereview.chromium.org/1410643007/diff/220001/net/url_request/url_re...
net/url_request/url_request_job.cc:571: // there isn't a filter error? How do
filter errors get notified?
On 2015/10/27 21:24:07, xunjieli wrote:
> On 2015/10/27 19:41:08, mmenke wrote:
> > Need to fix this comment:  The issue is that we *do* call it on filter
errors,
> > but not on read errors.  I actually suspect we should call it on no errors
> (Note
> > that NotifyDone calls CompleteNotifyDone which calls NotifyReadCompleted
with
> -1
> > - this looks like a real bug to me, as we're calling NotifyReadCompleted
> twice)
> 
> If that's the case, can I get rid of |notify| and move
> request_->NotifyReadCompleted inside the if statement on line 565?

We can....But I'm paranoid about making more behavior changes than strictly
necessary in an already huge CL.

[xunjieli, 2015-10-27 21:44:22]

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

https://codereview.chromium.org/1410643007/diff/220001/net/url_request/url_re...
net/url_request/url_request_job.cc:571: // there isn't a filter error? How do
filter errors get notified?
On 2015/10/27 21:30:50, mmenke wrote:
> On 2015/10/27 21:24:07, xunjieli wrote:
> > On 2015/10/27 19:41:08, mmenke wrote:
> > > Need to fix this comment:  The issue is that we *do* call it on filter
> errors,
> > > but not on read errors.  I actually suspect we should call it on no errors
> > (Note
> > > that NotifyDone calls CompleteNotifyDone which calls NotifyReadCompleted
> with
> > -1
> > > - this looks like a real bug to me, as we're calling NotifyReadCompleted
> > twice)
> > 
> > If that's the case, can I get rid of |notify| and move
> > request_->NotifyReadCompleted inside the if statement on line 565?
> 
> We can....But I'm paranoid about making more behavior changes than strictly
> necessary in an already huge CL.

Done. Thanks for explaining. I have moved NotifyReadCompleted and conditioned it
on |error| == OK.

[mmenke, 2015-10-28 16:40:42]

https://codereview.chromium.org/1410643007/diff/280001/content/browser/androi...
File content/browser/android/url_request_content_job.cc (right):

https://codereview.chromium.org/1410643007/diff/280001/content/browser/androi...
content/browser/android/url_request_content_job.cc:101: // the job has not
started.
nit:  See other comments about these.

https://codereview.chromium.org/1410643007/diff/280001/content/browser/fileap...
File content/browser/fileapi/mock_url_request_delegate.cc (right):

https://codereview.chromium.org/1410643007/diff/280001/content/browser/fileap...
content/browser/fileapi/mock_url_request_delegate.cc:71: FROM_HERE,
base::MessageLoop::QuitWhenIdleClosure());
Why is this change part of this CL?

https://codereview.chromium.org/1410643007/diff/280001/content/browser/servic...
File content/browser/service_worker/service_worker_read_from_cache_job.cc
(right):

https://codereview.chromium.org/1410643007/diff/280001/content/browser/servic...
content/browser/service_worker/service_worker_read_from_cache_job.cc:154:
Done(net::URLRequestStatus(net::URLRequestStatus::FAILED, result));
BUG:  Need to call NotifyStartError after the Done call.

https://codereview.chromium.org/1410643007/diff/280001/content/browser/servic...
File content/browser/service_worker/service_worker_write_to_cache_job.cc
(right):

https://codereview.chromium.org/1410643007/diff/280001/content/browser/servic...
content/browser/service_worker/service_worker_write_to_cache_job.cc:370:
std::string());
If NotifyFinishedCaching sets the error to kIdenticalScriptError, shouldn't we
pass on that error to ReadRawDataComplete, too?  Same goes for other calls to
the method as well.

https://codereview.chromium.org/1410643007/diff/280001/content/browser/servic...
content/browser/service_worker/service_worker_write_to_cache_job.cc:396:
DCHECK(!request->status().is_success());
This DCHECK seems wrong - shouldn't it be above the HandleNetData call?

https://codereview.chromium.org/1410643007/diff/280001/content/browser/servic...
content/browser/service_worker/service_worker_write_to_cache_job.cc:397:
NotifyFinishedCaching(request->status(), kFetchScriptError);
Seems like moving this above the HandleNetData call as well would also ensure
NotifyFinishedCaching is called only once.

https://codereview.chromium.org/1410643007/diff/280001/content/browser/servic...
content/browser/service_worker/service_worker_write_to_cache_job.cc:437:
reported_status = net::URLRequestStatus::FromError(kIdenticalScriptError);
I don't think this ever triggers on NotifyStartError?  We have to read through
the entire body before this is relevant.  This should be moved to the same logic
in NotifyFinishedCaching, I believe.

https://codereview.chromium.org/1410643007/diff/280001/mojo/services/network/...
File mojo/services/network/url_loader_impl_apptest.cc (right):

https://codereview.chromium.org/1410643007/diff/280001/mojo/services/network/...
mojo/services/network/url_loader_impl_apptest.cc:61: status_ = result == 0 ?
COMPLETED : STARTED;
BUG:  result <= 0

https://codereview.chromium.org/1410643007/diff/280001/net/test/url_request/u...
File net/test/url_request/url_request_failed_job.cc (right):

https://codereview.chromium.org/1410643007/diff/280001/net/test/url_request/u...
net/test/url_request/url_request_failed_job.cc:118: DCHECK_NE(ERR_IO_PENDING,
net_error_);
nit:  Can remove these - they were for documentation purposes, but now they just
match the previous line exactly.

https://codereview.chromium.org/1410643007/diff/280001/net/test/url_request/u...
File net/test/url_request/url_request_slow_download_job.cc (right):

https://codereview.chromium.org/1410643007/diff/280001/net/test/url_request/u...
net/test/url_request/url_request_slow_download_job.cc:207: return bytes_read;
Can just merge this with the BUFFER_FILLED case

[xunjieli, 2015-10-28 21:00:21]

Patchset #13 (id:300001) has been deleted

[xunjieli, 2015-10-28 21:11:52]

Thanks for the review! PTAL.

https://codereview.chromium.org/1410643007/diff/280001/content/browser/androi...
File content/browser/android/url_request_content_job.cc (right):

https://codereview.chromium.org/1410643007/diff/280001/content/browser/androi...
content/browser/android/url_request_content_job.cc:101: // the job has not
started.
On 2015/10/28 16:40:42, mmenke wrote:
> nit:  See other comments about these.

Done. Sorry, missed this one.

https://codereview.chromium.org/1410643007/diff/280001/content/browser/fileap...
File content/browser/fileapi/mock_url_request_delegate.cc (right):

https://codereview.chromium.org/1410643007/diff/280001/content/browser/fileap...
content/browser/fileapi/mock_url_request_delegate.cc:71: FROM_HERE,
base::MessageLoop::QuitWhenIdleClosure());
On 2015/10/28 16:40:42, mmenke wrote:
> Why is this change part of this CL?

Done.

https://codereview.chromium.org/1410643007/diff/280001/content/browser/servic...
File content/browser/service_worker/service_worker_read_from_cache_job.cc
(right):

https://codereview.chromium.org/1410643007/diff/280001/content/browser/servic...
content/browser/service_worker/service_worker_read_from_cache_job.cc:154:
Done(net::URLRequestStatus(net::URLRequestStatus::FAILED, result));
On 2015/10/28 16:40:42, mmenke wrote:
> BUG:  Need to call NotifyStartError after the Done call.

Done.

https://codereview.chromium.org/1410643007/diff/280001/content/browser/servic...
File content/browser/service_worker/service_worker_write_to_cache_job.cc
(right):

https://codereview.chromium.org/1410643007/diff/280001/content/browser/servic...
content/browser/service_worker/service_worker_write_to_cache_job.cc:370:
std::string());
On 2015/10/28 16:40:42, mmenke wrote:
> If NotifyFinishedCaching sets the error to kIdenticalScriptError, shouldn't we
> pass on that error to ReadRawDataComplete, too?  Same goes for other calls to
> the method as well.

Done.

https://codereview.chromium.org/1410643007/diff/280001/content/browser/servic...
content/browser/service_worker/service_worker_write_to_cache_job.cc:396:
DCHECK(!request->status().is_success());
On 2015/10/28 16:40:42, mmenke wrote:
> This DCHECK seems wrong - shouldn't it be above the HandleNetData call?

Done. Thanks for explaining!

https://codereview.chromium.org/1410643007/diff/280001/content/browser/servic...
content/browser/service_worker/service_worker_write_to_cache_job.cc:397:
NotifyFinishedCaching(request->status(), kFetchScriptError);
On 2015/10/28 16:40:42, mmenke wrote:
> Seems like moving this above the HandleNetData call as well would also ensure
> NotifyFinishedCaching is called only once.

Done.

https://codereview.chromium.org/1410643007/diff/280001/content/browser/servic...
content/browser/service_worker/service_worker_write_to_cache_job.cc:437:
reported_status = net::URLRequestStatus::FromError(kIdenticalScriptError);
On 2015/10/28 16:40:42, mmenke wrote:
> I don't think this ever triggers on NotifyStartError?  We have to read through
> the entire body before this is relevant.  This should be moved to the same
logic
> in NotifyFinishedCaching, I believe.

Done.

https://codereview.chromium.org/1410643007/diff/280001/mojo/services/network/...
File mojo/services/network/url_loader_impl_apptest.cc (right):

https://codereview.chromium.org/1410643007/diff/280001/mojo/services/network/...
mojo/services/network/url_loader_impl_apptest.cc:61: status_ = result == 0 ?
COMPLETED : STARTED;
On 2015/10/28 16:40:42, mmenke wrote:
> BUG:  result <= 0

Done.

https://codereview.chromium.org/1410643007/diff/280001/net/test/url_request/u...
File net/test/url_request/url_request_failed_job.cc (right):

https://codereview.chromium.org/1410643007/diff/280001/net/test/url_request/u...
net/test/url_request/url_request_failed_job.cc:118: DCHECK_NE(ERR_IO_PENDING,
net_error_);
On 2015/10/28 16:40:42, mmenke wrote:
> nit:  Can remove these - they were for documentation purposes, but now they
just
> match the previous line exactly.

Done.

https://codereview.chromium.org/1410643007/diff/280001/net/test/url_request/u...
File net/test/url_request/url_request_slow_download_job.cc (right):

https://codereview.chromium.org/1410643007/diff/280001/net/test/url_request/u...
net/test/url_request/url_request_slow_download_job.cc:207: return bytes_read;
On 2015/10/28 16:40:42, mmenke wrote:
> Can just merge this with the BUFFER_FILLED case

Done.

[mmenke, 2015-10-28 21:30:16]

I feel I should do another careful pass over this - I think my last pass made it
clear that there are a log of subtle ordering concerns in cases we either have
no tests for, or for issues the tests we do have won't catch, and finding them
now is much better than finding them 5 years down the line when someone finally
feels moved to investigate a random long tail crasher.

Given the size of the CL, I'll do it in two halves again, probably Thursday and
Friday, or Thursday and Monday.

[xunjieli, 2015-10-28 21:39:46]

Sounds good to me! We definitely do not want any bugs lurking in the
background. Thanks for the care taken in this review.

On Wed, Oct 28, 2015 at 5:30 PM, <mmenke@chromium.org> wrote:

> I feel I should do another careful pass over this - I think my last pass
> made it
> clear that there are a log of subtle ordering concerns in cases we either
> have
> no tests for, or for issues the tests we do have won't catch, and finding
> them
> now is much better than finding them 5 years down the line when someone
> finally
> feels moved to investigate a random long tail crasher.
>
> Given the size of the CL, I'll do it in two halves again, probably
> Thursday and
> Friday, or Thursday and Monday.
>
> https://codereview.chromium.org/1410643007/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[mmenke, 2015-10-30 20:06:43]

LGTM.  Didn't have the patience to carefully go through this one again.

https://codereview.chromium.org/1410643007/diff/320001/chrome/browser/chromeo...
File chrome/browser/chromeos/fileapi/external_file_url_request_job.cc (right):

https://codereview.chromium.org/1410643007/diff/320001/chrome/browser/chromeo...
chrome/browser/chromeos/fileapi/external_file_url_request_job.cc:196: void
ExternalFileURLRequestJob::DoStart() {
Does make reviewing harder, but this should be in the same order as it's
declared in, relative to other methods.

https://codereview.chromium.org/1410643007/diff/320001/content/browser/servic...
File content/browser/service_worker/service_worker_read_from_cache_job.cc
(right):

https://codereview.chromium.org/1410643007/diff/320001/content/browser/servic...
content/browser/service_worker/service_worker_read_from_cache_job.cc:49: void
ServiceWorkerReadFromCacheJob::StartAsync() {
Should be in declaration order.

[xunjieli, 2015-10-30 20:33:28]

Thanks! I will add other reviewers now.

https://codereview.chromium.org/1410643007/diff/320001/chrome/browser/chromeo...
File chrome/browser/chromeos/fileapi/external_file_url_request_job.cc (right):

https://codereview.chromium.org/1410643007/diff/320001/chrome/browser/chromeo...
chrome/browser/chromeos/fileapi/external_file_url_request_job.cc:196: void
ExternalFileURLRequestJob::DoStart() {
On 2015/10/30 20:06:43, mmenke wrote:
> Does make reviewing harder, but this should be in the same order as it's
> declared in, relative to other methods.

I considered doing that, but it involves re-ordering methods in this file.
DoStart() is ordered correctly before OnHelperResultObtained(), though not after
ReadRawData().

https://codereview.chromium.org/1410643007/diff/320001/content/browser/servic...
File content/browser/service_worker/service_worker_read_from_cache_job.cc
(right):

https://codereview.chromium.org/1410643007/diff/320001/content/browser/servic...
content/browser/service_worker/service_worker_read_from_cache_job.cc:49: void
ServiceWorkerReadFromCacheJob::StartAsync() {
On 2015/10/30 20:06:43, mmenke wrote:
> Should be in declaration order.

Done. The order in this file doesn't strictly adheres to the declaration order.
I moved it closer to where it is declared.

[xunjieli, 2015-10-30 21:06:31]

Patchset #15 (id:360001) has been deleted

[xunjieli, 2015-10-30 21:38:47]

xunjieli@chromium.org changed reviewers:
+ mnaganov@chromium.org, mtomasz@chromium.org

[xunjieli, 2015-10-30 21:38:47]

mtomasz@: PTAL at
	chrome/browser/chromeos/fileapi/external_file_url_request_job.cc/h

[xunjieli, 2015-10-30 21:48:53]

xunjieli@chromium.org changed reviewers:
+ eugenebut@chromium.org, falken@chromium.org, michaeln@chromium.org,
skyostil@google.com, tzik@chromium.org

[xunjieli, 2015-10-30 21:48:55]

mnaganov: PTAL at
android_webview/browser/net/android_stream_reader_url_request_job.cc

mtomasz@: PTAL at
	chrome/browser/chromeos/fileapi/external_file_url_request_job.cc/h

eugenebut@: PTAL at ios/web/webui/url_data_manager_ios_backend.cc

sky@: PTAL at mojo/services/network/url_loader_impl_apptest.cc

tzik@: PTAL at storage/browser/fileapi/*

falken@: PTAL content/browser/service_worker/

michaeln@: PTAL at content/browser/appcache/ 

skyostil@: PTAL at content/browser/android/

Thanks!

[xunjieli, 2015-10-30 21:50:37]

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

[mnaganov (inactive), 2015-10-30 22:40:23]

android_webview/ LGTM

[Eugene But (OOO till 7-30), 2015-10-31 03:14:35]

ios/web lgtm with changes

https://codereview.chromium.org/1410643007/diff/380001/ios/web/webui/url_data...
File ios/web/webui/url_data_manager_ios_backend.cc (right):

https://codereview.chromium.org/1410643007/diff/380001/ios/web/webui/url_data...
ios/web/webui/url_data_manager_ios_backend.cc:294: // The request completed, and
we have all the data.
This comment belonged to removed code. Please remove the comment as well.

[falken, 2015-11-02 01:22:18]

service_worker lgtm

https://codereview.chromium.org/1410643007/diff/380001/content/browser/servic...
File content/browser/service_worker/service_worker_write_to_cache_job.cc
(right):

https://codereview.chromium.org/1410643007/diff/380001/content/browser/servic...
content/browser/service_worker/service_worker_write_to_cache_job.cc:451:
net::Error result = (net::Error)status.error();
I think Chromium style is static_cast<net::Error>

https://codereview.chromium.org/1410643007/diff/380001/content/browser/servic...
File content/browser/service_worker/service_worker_write_to_cache_job.h (right):

https://codereview.chromium.org/1410643007/diff/380001/content/browser/servic...
content/browser/service_worker/service_worker_write_to_cache_job.h:116: // isn't
IO_PENDING, all of the data in |io_buffer_| has been written back to
This comment is outdated now, HandleNetData no longer sets the job's status. Can
you update it to mention the return value?

[mtomasz, 2015-11-02 05:43:21]

https://codereview.chromium.org/1410643007/diff/380001/chrome/browser/chromeo...
File chrome/browser/chromeos/fileapi/external_file_url_request_job.cc (right):

https://codereview.chromium.org/1410643007/diff/380001/chrome/browser/chromeo...
chrome/browser/chromeos/fileapi/external_file_url_request_job.cc:189: // Post a
task to invoke DoStart asynchronously to avoid re-entering the
Why is this necessary, while it wasn't before? The comment is not clear to me.

https://codereview.chromium.org/1410643007/diff/380001/chrome/browser/chromeo...
File chrome/browser/chromeos/fileapi/external_file_url_request_job.h (right):

https://codereview.chromium.org/1410643007/diff/380001/chrome/browser/chromeo...
chrome/browser/chromeos/fileapi/external_file_url_request_job.h:73: // Called
from an internal helper class defined in drive_url_request_job.cc,
nit: \n between #72 and #73.
nit: Please add a comment. It's not obvious why we have DoStart() besides
Start().

[Sami, 2015-11-02 12:26:03]

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

[Sami, 2015-11-02 12:26:04]

content/browser/android lgtm.

[xunjieli, 2015-11-02 15:29:24]

xunjieli@chromium.org changed reviewers:
- skyostil@google.com

[xunjieli, 2015-11-02 15:37:47]

xunjieli@chromium.org changed reviewers:
+ davidben@chromium.org, estade@chromium.org, zork@chromium.org
- tzik@chromium.org

[xunjieli, 2015-11-02 15:40:12]

Thanks for the review! PTAL.

Additionally requesting OWNER reviews for the following files:

jianli@: storage/browser/blob/blob_url_request_job.h/cc
sky@: mojo/services/network/url_loader_impl_apptest.cc
estade@: content/browser/webui/url_data_manager_backend.cc
zork@: 	content/browser/streams/stream_url_request_job.cc/h
davidben@: content/browser/net/view_http_cache_job_factory.cc
michaeln@: content/browser/appcache/appcache_url_request_job.cc/h  &
content/browser/fileapi/file_writer_delegate_unittest.cc

https://codereview.chromium.org/1410643007/diff/380001/chrome/browser/chromeo...
File chrome/browser/chromeos/fileapi/external_file_url_request_job.cc (right):

https://codereview.chromium.org/1410643007/diff/380001/chrome/browser/chromeo...
chrome/browser/chromeos/fileapi/external_file_url_request_job.cc:189: // Post a
task to invoke DoStart asynchronously to avoid re-entering the
On 2015/11/02 05:43:21, mtomasz wrote:
> Why is this necessary, while it wasn't before? The comment is not clear to me.

Done. NotifyStartError is not legal to call synchronously in Start. Previous
code isn't quite right. I added comment.

https://codereview.chromium.org/1410643007/diff/380001/chrome/browser/chromeo...
File chrome/browser/chromeos/fileapi/external_file_url_request_job.h (right):

https://codereview.chromium.org/1410643007/diff/380001/chrome/browser/chromeo...
chrome/browser/chromeos/fileapi/external_file_url_request_job.h:73: // Called
from an internal helper class defined in drive_url_request_job.cc,
On 2015/11/02 05:43:21, mtomasz wrote:
> nit: \n between #72 and #73.
> nit: Please add a comment. It's not obvious why we have DoStart() besides
> Start().

Done. I also renamed the method to make it clear that it should be invoked
asynchronously in Start.

https://codereview.chromium.org/1410643007/diff/380001/content/browser/servic...
File content/browser/service_worker/service_worker_write_to_cache_job.cc
(right):

https://codereview.chromium.org/1410643007/diff/380001/content/browser/servic...
content/browser/service_worker/service_worker_write_to_cache_job.cc:451:
net::Error result = (net::Error)status.error();
On 2015/11/02 01:22:18, falken wrote:
> I think Chromium style is static_cast<net::Error>

Done.

https://codereview.chromium.org/1410643007/diff/380001/content/browser/servic...
File content/browser/service_worker/service_worker_write_to_cache_job.h (right):

https://codereview.chromium.org/1410643007/diff/380001/content/browser/servic...
content/browser/service_worker/service_worker_write_to_cache_job.h:116: // isn't
IO_PENDING, all of the data in |io_buffer_| has been written back to
On 2015/11/02 01:22:18, falken wrote:
> This comment is outdated now, HandleNetData no longer sets the job's status.
Can
> you update it to mention the return value?

Done.

https://codereview.chromium.org/1410643007/diff/380001/ios/web/webui/url_data...
File ios/web/webui/url_data_manager_ios_backend.cc (right):

https://codereview.chromium.org/1410643007/diff/380001/ios/web/webui/url_data...
ios/web/webui/url_data_manager_ios_backend.cc:294: // The request completed, and
we have all the data.
On 2015/10/31 03:14:35, eugenebut wrote:
> This comment belonged to removed code. Please remove the comment as well.

Done.

[mmenke, 2015-11-02 15:44:59]

https://codereview.chromium.org/1410643007/diff/380001/chrome/browser/chromeo...
File chrome/browser/chromeos/fileapi/external_file_url_request_job.cc (right):

https://codereview.chromium.org/1410643007/diff/380001/chrome/browser/chromeo...
chrome/browser/chromeos/fileapi/external_file_url_request_job.cc:189: // Post a
task to invoke DoStart asynchronously to avoid re-entering the
On 2015/11/02 15:40:11, xunjieli wrote:
> On 2015/11/02 05:43:21, mtomasz wrote:
> > Why is this necessary, while it wasn't before? The comment is not clear to
me.
> 
> Done. NotifyStartError is not legal to call synchronously in Start. Previous
> code isn't quite right. I added comment.

On a side note, I think we may want to allow this at some point, or at least
make it hard fail.

[mtomasz, 2015-11-02 15:56:27]

Thanks. lgtm for external_file_url_request_job.*.

[xunjieli, 2015-11-03 17:16:11]

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

[xunjieli, 2015-11-03 17:19:32]

A friendly ping to get OWNER stamps.
sky@: mojo/services/network/url_loader_impl_apptest.cc
zork@: content/browser/streams/stream_url_request_job.cc/h
davidben@: content/browser/net/view_http_cache_job_factory.cc &
content/browser/webui/url_data_manager_backend.cc
michaeln@: content/browser/appcache/appcache_url_request_job.cc/h  &
content/browser/fileapi/file_writer_delegate_unittest.cc
jianli@: storage/browser/blob/blob_url_request_job.h/cc

PTAL.

[sky, 2015-11-03 17:26:26]

LGTM

[davidben, 2015-11-03 22:50:05]

https://codereview.chromium.org/1410643007/diff/400001/content/browser/androi...
File content/browser/android/url_request_content_job.cc (right):

https://codereview.chromium.org/1410643007/diff/400001/content/browser/androi...
content/browser/android/url_request_content_job.cc:98: void
URLRequestContentJob::SetExtraRequestHeaders(
[Not for this CL, but //net folks on this CL: this API seems kinda weird. That
error results need to be plumbed out is annoying. Seems either this should
return an int or the extra headers should be available to Start() somehow and
then Start() could check there.]

https://codereview.chromium.org/1410643007/diff/400001/content/browser/androi...
content/browser/android/url_request_content_job.cc:199: void
URLRequestContentJob::DidRead(scoped_refptr<net::IOBuffer> buf,
Why does this hold onto buf at all? It doesn't use it. Shouldn't stream_->Read
take a reference it if needs one?

https://codereview.chromium.org/1410643007/diff/400001/content/browser/fileap...
File content/browser/fileapi/file_writer_delegate_unittest.cc (right):

https://codereview.chromium.org/1410643007/diff/400001/content/browser/fileap...
content/browser/fileapi/file_writer_delegate_unittest.cc:13: #include
"base/numerics/safe_conversions.h"
Unused?

https://codereview.chromium.org/1410643007/diff/400001/content/browser/fileap...
content/browser/fileapi/file_writer_delegate_unittest.cc:190: buf_size =
static_cast<int>(remaining_bytes_);
[Existing issue, but the static_cast<int> doesn't seem to do anything.
remaining_bytes_ is already an int.]

https://codereview.chromium.org/1410643007/diff/400001/content/browser/net/vi...
File content/browser/net/view_http_cache_job_factory.cc (right):

https://codereview.chromium.org/1410643007/diff/400001/content/browser/net/vi...
content/browser/net/view_http_cache_job_factory.cc:168: int remaining =
base::checked_cast<int>(data_.size()) - data_offset_;
#include "base/numerics/safe_conversions.h

https://codereview.chromium.org/1410643007/diff/400001/content/browser/webui/...
File content/browser/webui/url_data_manager_backend.cc (right):

https://codereview.chromium.org/1410643007/diff/400001/content/browser/webui/...
content/browser/webui/url_data_manager_backend.cc:195: // bytes read, or < 0 to
indicate an error.
It doesn't look like this function can actually return an error.

https://codereview.chromium.org/1410643007/diff/400001/content/test/net/url_r...
File content/test/net/url_request_abort_on_end_job.cc (right):

https://codereview.chromium.org/1410643007/diff/400001/content/test/net/url_r...
content/test/net/url_request_abort_on_end_job.cc:117: std::min(max_bytes,
base::checked_cast<int>(sizeof(kPageContent)));
#include "base/numerics/safe_conversions.h"

[mmenke, 2015-11-03 22:54:16]

https://codereview.chromium.org/1410643007/diff/400001/content/browser/androi...
File content/browser/android/url_request_content_job.cc (right):

https://codereview.chromium.org/1410643007/diff/400001/content/browser/androi...
content/browser/android/url_request_content_job.cc:98: void
URLRequestContentJob::SetExtraRequestHeaders(
On 2015/11/03 22:50:05, davidben wrote:
> [Not for this CL, but //net folks on this CL: this API seems kinda weird. That
> error results need to be plumbed out is annoying. Seems either this should
> return an int or the extra headers should be available to Start() somehow and
> then Start() could check there.]

I really like the idea of making them an argument to Start().  Fewer methods in
the interface, and fewer implicit assumptions about the ordering they're invoked
in.  I'll file a bug.

[xunjieli, 2015-11-03 23:04:20]

Description was changed from

==========
URLRequestJob: change ReadRawData contract

This CL is patched from ellyjones@ CL at crrev.com/1227893004.

Previously, the interface for URLRequestJob::ReadRawData was as follows:

  bool ReadRawData(IOBuffer*, int, int*)

Subclasses were expected to signal completion of the ReadRawData call by
calling NotifyDone, SetStatus, or maybe one of the other Notify* functions
on URLRequestJob, most of which do internal housekeeping and also drive the
URLRequest's state machine. This made it difficult to reason about the
URLRequestJob's state machine and needlessly complicated most of URLRequestJob.

The new interface is as follows:

  int ReadRawData(IOBuffer*, int)

Subclasses are required to either:

a) Return ERR_IO_PENDING, and call ReadRawDataComplete when the read completes
in any way, or
b) Return a count of bytes read >= 0, indicating synchronous success, or
c) Return another error code < 0, indicating synchronous failure.

This substantially narrows the interface between URLRequestJob and its
subclasses and moves the logic for the URLRequest state machine largely into
URLRequestJob.

Also, the signature of URLRequestJob::ReadFilteredData and some other internal
URLRequestJob helpers changes to propagate detailed error codes instead of
coercing all errors to FAILED.

BUG=474859
==========

to

==========
URLRequestJob: change ReadRawData contract

This CL is patched from ellyjones@ CL at crrev.com/1227893004.

Previously, the interface for URLRequestJob::ReadRawData was as follows:

  bool ReadRawData(IOBuffer*, int, int*)

Subclasses were expected to signal completion of the ReadRawData call by
calling NotifyDone, SetStatus, or maybe one of the other Notify* functions
on URLRequestJob, most of which do internal housekeeping and also drive the
URLRequest's state machine. This made it difficult to reason about the
URLRequestJob's state machine and needlessly complicated most of URLRequestJob.

The new interface is as follows:

  int ReadRawData(IOBuffer*, int)

Subclasses are required to either:

a) Return ERR_IO_PENDING, and call ReadRawDataComplete when the read completes
in any way, or
b) Return a count of bytes read >= 0, indicating synchronous success, or
c) Return another error code < 0, indicating synchronous failure.

This substantially narrows the interface between URLRequestJob and its
subclasses and moves the logic for the URLRequest state machine largely into
URLRequestJob.

Also, the signature of URLRequestJob::ReadFilteredData and some other internal
URLRequestJob helpers changes to propagate detailed error codes instead of
coercing all errors to FAILED.

BUG=474859
BUG=329902
==========

[xunjieli, 2015-11-04 01:26:52]

Thanks for the review! PTAL

https://codereview.chromium.org/1410643007/diff/400001/content/browser/androi...
File content/browser/android/url_request_content_job.cc (right):

https://codereview.chromium.org/1410643007/diff/400001/content/browser/androi...
content/browser/android/url_request_content_job.cc:199: void
URLRequestContentJob::DidRead(scoped_refptr<net::IOBuffer> buf,
On 2015/11/03 22:50:05, davidben wrote:
> Why does this hold onto buf at all? It doesn't use it. Shouldn't stream_->Read
> take a reference it if needs one?

Done.

https://codereview.chromium.org/1410643007/diff/400001/content/browser/fileap...
File content/browser/fileapi/file_writer_delegate_unittest.cc (right):

https://codereview.chromium.org/1410643007/diff/400001/content/browser/fileap...
content/browser/fileapi/file_writer_delegate_unittest.cc:13: #include
"base/numerics/safe_conversions.h"
On 2015/11/03 22:50:05, davidben wrote:
> Unused?

Done.

https://codereview.chromium.org/1410643007/diff/400001/content/browser/fileap...
content/browser/fileapi/file_writer_delegate_unittest.cc:190: buf_size =
static_cast<int>(remaining_bytes_);
On 2015/11/03 22:50:05, davidben wrote:
> [Existing issue, but the static_cast<int> doesn't seem to do anything.
> remaining_bytes_ is already an int.]

Done.

https://codereview.chromium.org/1410643007/diff/400001/content/browser/net/vi...
File content/browser/net/view_http_cache_job_factory.cc (right):

https://codereview.chromium.org/1410643007/diff/400001/content/browser/net/vi...
content/browser/net/view_http_cache_job_factory.cc:168: int remaining =
base::checked_cast<int>(data_.size()) - data_offset_;
On 2015/11/03 22:50:05, davidben wrote:
> #include "base/numerics/safe_conversions.h

Done.

https://codereview.chromium.org/1410643007/diff/400001/content/browser/webui/...
File content/browser/webui/url_data_manager_backend.cc (right):

https://codereview.chromium.org/1410643007/diff/400001/content/browser/webui/...
content/browser/webui/url_data_manager_backend.cc:195: // bytes read, or < 0 to
indicate an error.
On 2015/11/03 22:50:05, davidben wrote:
> It doesn't look like this function can actually return an error.

Done.

https://codereview.chromium.org/1410643007/diff/400001/content/test/net/url_r...
File content/test/net/url_request_abort_on_end_job.cc (right):

https://codereview.chromium.org/1410643007/diff/400001/content/test/net/url_r...
content/test/net/url_request_abort_on_end_job.cc:117: std::min(max_bytes,
base::checked_cast<int>(sizeof(kPageContent)));
On 2015/11/03 22:50:05, davidben wrote:
> #include "base/numerics/safe_conversions.h"

Done.

[davidben, 2015-11-04 17:32:17]

On 2015/11/03 17:19:32, xunjieli wrote:
> davidben@: content/browser/net/view_http_cache_job_factory.cc &
> content/browser/webui/url_data_manager_backend.cc

lgtm

[xunjieli, 2015-11-04 17:33:41]

xunjieli@chromium.org changed reviewers:
- estade@chromium.org

[xunjieli, 2015-11-04 17:36:00]

zork@: content/browser/streams/stream_url_request_job.cc/h
michaeln@: content/browser/appcache/appcache_url_request_job.cc/h  &
content/browser/fileapi/file_writer_delegate_unittest.cc
jianli@: storage/browser/blob/blob_url_request_job.h/cc

PTAL.

[jianli, 2015-11-05 23:55:23]

storage/browser/blob lgtm after addressing one minor comment

https://codereview.chromium.org/1410643007/diff/420001/storage/browser/blob/b...
File storage/browser/blob/blob_url_request_job.cc (right):

https://codereview.chromium.org/1410643007/diff/420001/storage/browser/blob/b...
storage/browser/blob/blob_url_request_job.cc:92:
base::Bind(&BlobURLRequestJob::DidReadRawData,
Why not passing ReadRawDataComplete directly such that
BlobURLRequestJob::DidReadRawData can be removed?

[michaeln, 2015-11-06 00:10:51]

appcache lgtm

https://codereview.chromium.org/1410643007/diff/420001/storage/browser/blob/b...
File storage/browser/blob/blob_url_request_job.cc (right):

https://codereview.chromium.org/1410643007/diff/420001/storage/browser/blob/b...
storage/browser/blob/blob_url_request_job.cc:92:
base::Bind(&BlobURLRequestJob::DidReadRawData,
On 2015/11/05 23:55:23, jianli wrote:
> Why not passing ReadRawDataComplete directly such that
> BlobURLRequestJob::DidReadRawData can be removed?

how would you end the async trace w/o the intermediary?

[michaeln, 2015-11-06 00:17:10]

https://codereview.chromium.org/1410643007/diff/420001/storage/browser/fileap...
File storage/browser/fileapi/file_system_url_request_job.h (right):

https://codereview.chromium.org/1410643007/diff/420001/storage/browser/fileap...
storage/browser/fileapi/file_system_url_request_job.h:74:
std::vector<net::HttpByteRange> byte_ranges_;
lgtm % is byte_ranges_ used anywhere?

[mmenke, 2015-11-06 00:52:34]

https://codereview.chromium.org/1410643007/diff/420001/storage/browser/blob/b...
File storage/browser/blob/blob_url_request_job.cc (right):

https://codereview.chromium.org/1410643007/diff/420001/storage/browser/blob/b...
storage/browser/blob/blob_url_request_job.cc:92:
base::Bind(&BlobURLRequestJob::DidReadRawData,
On 2015/11/06 00:10:51, michaeln wrote:
> On 2015/11/05 23:55:23, jianli wrote:
> > Why not passing ReadRawDataComplete directly such that
> > BlobURLRequestJob::DidReadRawData can be removed?
> 
> how would you end the async trace w/o the intermediary?

You can use weak ptrs with parent class methods, without problems, I believe.

[Zachary Kuznia, 2015-11-06 00:54:11]

content/browser/streams/stream_url_request_job.* LGTM

[michaeln, 2015-11-06 01:22:39]

https://codereview.chromium.org/1410643007/diff/420001/storage/browser/blob/b...
File storage/browser/blob/blob_url_request_job.cc (right):

https://codereview.chromium.org/1410643007/diff/420001/storage/browser/blob/b...
storage/browser/blob/blob_url_request_job.cc:92:
base::Bind(&BlobURLRequestJob::DidReadRawData,
On 2015/11/06 00:52:34, mmenke wrote:
> On 2015/11/06 00:10:51, michaeln wrote:
> > On 2015/11/05 23:55:23, jianli wrote:
> > > Why not passing ReadRawDataComplete directly such that
> > > BlobURLRequestJob::DidReadRawData can be removed?
> > 
> > how would you end the async trace w/o the intermediary?
> 
> You can use weak ptrs with parent class methods, without problems, I believe.

Sure, but the parent class method won't do this...
TRACE_EVENT_ASYNC_END1("Blob", "BlobRequest::ReadRawData", this, "uuid"..

[mmenke, 2015-11-06 01:27:47]

https://codereview.chromium.org/1410643007/diff/420001/storage/browser/blob/b...
File storage/browser/blob/blob_url_request_job.cc (right):

https://codereview.chromium.org/1410643007/diff/420001/storage/browser/blob/b...
storage/browser/blob/blob_url_request_job.cc:92:
base::Bind(&BlobURLRequestJob::DidReadRawData,
On 2015/11/06 01:22:39, michaeln wrote:
> On 2015/11/06 00:52:34, mmenke wrote:
> > On 2015/11/06 00:10:51, michaeln wrote:
> > > On 2015/11/05 23:55:23, jianli wrote:
> > > > Why not passing ReadRawDataComplete directly such that
> > > > BlobURLRequestJob::DidReadRawData can be removed?
> > > 
> > > how would you end the async trace w/o the intermediary?
> > 
> > You can use weak ptrs with parent class methods, without problems, I
believe.
> 
> Sure, but the parent class method won't do this...
> TRACE_EVENT_ASYNC_END1("Blob", "BlobRequest::ReadRawData", this, "uuid"..

Sorry...Missed the "trace."  Saw "end the async event", and thought you were
talking about cancellation.

[xunjieli, 2015-11-06 14:30:03]

Thanks everyone for the reviews!

https://codereview.chromium.org/1410643007/diff/420001/storage/browser/blob/b...
File storage/browser/blob/blob_url_request_job.cc (right):

https://codereview.chromium.org/1410643007/diff/420001/storage/browser/blob/b...
storage/browser/blob/blob_url_request_job.cc:92:
base::Bind(&BlobURLRequestJob::DidReadRawData,
On 2015/11/06 01:27:47, mmenke wrote:
> On 2015/11/06 01:22:39, michaeln wrote:
> > On 2015/11/06 00:52:34, mmenke wrote:
> > > On 2015/11/06 00:10:51, michaeln wrote:
> > > > On 2015/11/05 23:55:23, jianli wrote:
> > > > > Why not passing ReadRawDataComplete directly such that
> > > > > BlobURLRequestJob::DidReadRawData can be removed?
> > > > 
> > > > how would you end the async trace w/o the intermediary?
> > > 
> > > You can use weak ptrs with parent class methods, without problems, I
> believe.
> > 
> > Sure, but the parent class method won't do this...
> > TRACE_EVENT_ASYNC_END1("Blob", "BlobRequest::ReadRawData", this, "uuid"..
> 
> Sorry...Missed the "trace."  Saw "end the async event", and thought you were
> talking about cancellation.

Acknowledged. Looks like we still would like to keep the intermediary in order
to properly end the traces. So no action is needed on my end.

https://codereview.chromium.org/1410643007/diff/420001/storage/browser/fileap...
File storage/browser/fileapi/file_system_url_request_job.h (right):

https://codereview.chromium.org/1410643007/diff/420001/storage/browser/fileap...
storage/browser/fileapi/file_system_url_request_job.h:74:
std::vector<net::HttpByteRange> byte_ranges_;
On 2015/11/06 00:17:10, michaeln wrote:
> lgtm % is byte_ranges_ used anywhere?

Done.

[xunjieli, 2015-11-06 15:24:10]

The CQ bit was checked by xunjieli@chromium.org

[xunjieli, 2015-11-06 15:24:11]

The patchset sent to the CQ was uploaded after l-g-t-m from
rdsmith@chromium.org, mmenke@chromium.org, mnaganov@chromium.org,
skyostil@chromium.org, eugenebut@chromium.org, falken@chromium.org,
mtomasz@chromium.org, sky@chromium.org, michaeln@chromium.org,
davidben@chromium.org, jianli@chromium.org, zork@chromium.org
Link to the patchset: https://codereview.chromium.org/1410643007/#ps440001
(title: "Address michaeln's comment")

[commit-bot: I haz the power, 2015-11-06 15:25:10]

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

[commit-bot: I haz the power, 2015-11-06 15:39:04]

Committed patchset #18 (id:440001)

[commit-bot: I haz the power, 2015-11-06 15:39:58]

Patchset 18 (id:??) landed as
https://crrev.com/a505cbb45ec92d2617fe80490acf7d01ef4d3225
Cr-Commit-Position: refs/heads/master@{#358327}