Reland: URLRequestJob: change ReadRawData contract

Reland: URLRequestJob: change ReadRawData contract

This CL is to reland crrev.com/1410643007 which was
reverted in crrev.com/1437523002.

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.

TBR=michaeln@chromium.org,mtomasz@chromium.org,jianli@chromium.org,zork@chromium.org

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

[xunjieli, 2015-11-16 16:27:55]

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

[xunjieli, 2015-11-16 16:27:56]

Hi Randy and Matt,

I am trying to reland the refactoring CL.

In addition to create a revert of crrev.com/1437523002:

- I have added a regression test for crbug.com/553300 in
url_request_job_unittest.cc.
- I have also reverted the fix for crbug.com/553270. ReadRawDataComplete is a
completion callback. It doesn't make sense to call it with ERR_IO_PENDING. I
have added a DCHECK_NE instead, and the bug seen in 553270 and 553268 is not
reproducible. I think fixing 553300 somehow fixed this as a side effect.

PTAL.

[xunjieli, 2015-11-16 16:29:00]

Description was changed from

==========
Reland: 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
==========

to

==========
Reland: 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-16 16:29:00]

xunjieli@chromium.org changed reviewers:
+ mmenke@chromium.org
- mmenke@google.com

[xunjieli, 2015-11-17 17:44:59]

Description was changed from

==========
Reland: 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
==========

to

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

This CL is to reland crrev.com/1410643007 which was
reverted in crrev.com/1437523002.

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-17 18:00:42]

xunjieli@chromium.org changed reviewers:
+ davidben@chromium.org, eugenebut@chromium.org, falken@chromium.org,
jianli@chromium.org, michaeln@chromium.org, mnaganov@chromium.org,
mtomasz@chromium.org, sky@chromium.org, skyostil@chromium.org,
tzik@chromium.org, zork@chromium.org

[xunjieli, 2015-11-17 18:09:35]

This is to reland the refactoring CL which was reverted because there was an
issue in the URLRequestJob::ReadRawDataComplete (see crbug.com/553300 and
crbug.com/553270 for more info). I am relanding this after the M48 branch point
so this refactoring can have more brewing time. Only
storage/browser/fileapi/file_system_dir_url_request_job.cc and
mojo/services/network/url_loader_impl_apptest.cc have new changes (which I've
sync-ed and rebased) since last review. OWNERs PTAL nevertheless. Thanks! 

mnaganov@:
android_webview/browser/net/android_stream_reader_url_request_job.cc

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

tzik@:
storage/browser/fileapi/file_system_dir_url_request_job.cc/h
storage/browser/fileapi/file_system_url_request_job.h/h

eugenebut@:
ios/web/webui/url_data_manager_ios_backend.cc/h

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

falken@:
content/browser/service_worker/*

michaeln@:
content/browser/appcache/appcache_url_request_job.cc/h
content/browser/fileapi/file_writer_delegate_unittest.cc

skyostil@:
content/browser/android/url_request_content_job.h/cc

jianli@:
storage/browser/blob/blob_url_request_job.h/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

[Eugene But (OOO till 7-30), 2015-11-17 18:13:00]

On 2015/11/17 18:09:35, xunjieli wrote:
> This is to reland the refactoring CL which was reverted because there was an
> issue in the URLRequestJob::ReadRawDataComplete (see crbug.com/553300 and
> crbug.com/553270 for more info). I am relanding this after the M48 branch
point
> so this refactoring can have more brewing time. Only
> storage/browser/fileapi/file_system_dir_url_request_job.cc and
> mojo/services/network/url_loader_impl_apptest.cc have new changes (which I've
> sync-ed and rebased) since last review. OWNERs PTAL nevertheless. Thanks! 
> 
> mnaganov@:
> android_webview/browser/net/android_stream_reader_url_request_job.cc
> 
> mtomasz@: 
> chrome/browser/chromeos/fileapi/external_file_url_request_job.cc/h
> 
> tzik@:
> storage/browser/fileapi/file_system_dir_url_request_job.cc/h
> storage/browser/fileapi/file_system_url_request_job.h/h
> 
> eugenebut@:
> ios/web/webui/url_data_manager_ios_backend.cc/h
> 
> sky@:
> mojo/services/network/url_loader_impl_apptest.cc
> 
> falken@:
> content/browser/service_worker/*
> 
> michaeln@:
> content/browser/appcache/appcache_url_request_job.cc/h
> content/browser/fileapi/file_writer_delegate_unittest.cc
> 
> skyostil@:
> content/browser/android/url_request_content_job.h/cc
> 
> jianli@:
> storage/browser/blob/blob_url_request_job.h/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
Can you make CL with 2 patches?:
1. Original CL which was reverted
2. Fix

Otherwise it's hard to say what was changed as a part of the fix.

[mnaganov (inactive), 2015-11-17 18:14:48]

android_webview/ LGTM

[xunjieli, 2015-11-17 18:15:03]

Patchset #2 (id:20001) has been deleted

[xunjieli, 2015-11-17 18:15:22]

Patchset #2 (id:40001) has been deleted

[xunjieli, 2015-11-17 18:17:47]

On 2015/11/17 18:13:00, eugenebut wrote:
> On 2015/11/17 18:09:35, xunjieli wrote:
> > This is to reland the refactoring CL which was reverted because there was an
> > issue in the URLRequestJob::ReadRawDataComplete (see crbug.com/553300 and
> > crbug.com/553270 for more info). I am relanding this after the M48 branch
> point
> > so this refactoring can have more brewing time. Only
> > storage/browser/fileapi/file_system_dir_url_request_job.cc and
> > mojo/services/network/url_loader_impl_apptest.cc have new changes (which
I've
> > sync-ed and rebased) since last review. OWNERs PTAL nevertheless. Thanks! 
> > 
> > mnaganov@:
> > android_webview/browser/net/android_stream_reader_url_request_job.cc
> > 
> > mtomasz@: 
> > chrome/browser/chromeos/fileapi/external_file_url_request_job.cc/h
> > 
> > tzik@:
> > storage/browser/fileapi/file_system_dir_url_request_job.cc/h
> > storage/browser/fileapi/file_system_url_request_job.h/h
> > 
> > eugenebut@:
> > ios/web/webui/url_data_manager_ios_backend.cc/h
> > 
> > sky@:
> > mojo/services/network/url_loader_impl_apptest.cc
> > 
> > falken@:
> > content/browser/service_worker/*
> > 
> > michaeln@:
> > content/browser/appcache/appcache_url_request_job.cc/h
> > content/browser/fileapi/file_writer_delegate_unittest.cc
> > 
> > skyostil@:
> > content/browser/android/url_request_content_job.h/cc
> > 
> > jianli@:
> > storage/browser/blob/blob_url_request_job.h/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
> Can you make CL with 2 patches?:
> 1. Original CL which was reverted
> 2. Fix
> 
> Otherwise it's hard to say what was changed as a part of the fix.

Great suggestion! The first patch now is the revert of the revert. The second
patch is new fixes. PTAL. Thanks!

[Sami, 2015-11-17 18:20:48]

content/browser/android/ lgtm.

[Eugene But (OOO till 7-30), 2015-11-17 18:22:40]

Thanks for updating CL. Please make sure that CL description fits 72 symbols,
otherwise lgtm.

[sky, 2015-11-17 18:42:00]

mojo LGTM

[mmenke, 2015-11-17 18:57:15]

On 2015/11/17 18:42:00, sky wrote:
> mojo LGTM

LGTM.

Helen:  You can generally just TBR people when re-landing a CL if their part of
the review didn't change (I get a review from people who owned the changed
files, TBR the others and post a message to them that I'm TBRing and why, and
then land ~24 hours later if I don't hear back from them all by then).

[mmenke, 2015-11-17 18:57:32]

https://codereview.chromium.org/1439953006/diff/60001/net/url_request/url_req...
File net/url_request/url_request_job_unittest.cc (right):

https://codereview.chromium.org/1439953006/diff/60001/net/url_request/url_req...
net/url_request/url_request_job_unittest.cc:79: const MockTransaction
kEmptyBody_Transaction = {
Maybe kEmptyBodyGzip_Transaction?

[Randy Smith (Not in Mondays), 2015-11-17 19:04:12]

LGTM.

[xunjieli, 2015-11-17 19:29:40]

Patchset #4 (id:100001) has been deleted

[davidben, 2015-11-17 19:54:07]

lgtm with comment

https://codereview.chromium.org/1439953006/diff/60001/content/browser/net/vie...
File content/browser/net/view_http_cache_job_factory.cc (right):

https://codereview.chromium.org/1439953006/diff/60001/content/browser/net/vie...
content/browser/net/view_http_cache_job_factory.cc:169: int remaining =
base::checked_cast<int>(data_.size()) - data_offset_;
Oh oops. I switched data_offset_ to size_t but intended to do so only after you
landed this one. I missed that yours had since been reverted. Since data_offset_
is also a size_t now let's do

  DCHECK_LE(data_offset_, data_.size());
  int remaining = base::checked_cast<int>(data_.size() - data_offset_);
  [...]
  data_offset_ += base::checked_cast<size_t>(buf_size);

We'll sort out the rest of the size_t / int confusion later since this interface
is now fairly int-specific in the return value.

[davidben, 2015-11-17 19:55:02]

https://codereview.chromium.org/1439953006/diff/60001/content/browser/net/vie...
File content/browser/net/view_http_cache_job_factory.cc (right):

https://codereview.chromium.org/1439953006/diff/60001/content/browser/net/vie...
content/browser/net/view_http_cache_job_factory.cc:169: int remaining =
base::checked_cast<int>(data_.size()) - data_offset_;
On 2015/11/17 19:54:06, davidben (slow) wrote:
> Oh oops. I switched data_offset_ to size_t but intended to do so only after
you
> landed this one. I missed that yours had since been reverted. Since
data_offset_
> is also a size_t now let's do
> 
>   DCHECK_LE(data_offset_, data_.size());
>   int remaining = base::checked_cast<int>(data_.size() - data_offset_);
>   [...]
>   data_offset_ += base::checked_cast<size_t>(buf_size);
> 
> We'll sort out the rest of the size_t / int confusion later since this
interface
> is now fairly int-specific in the return value.

(Er, I guess negative buf_size would have already exploded, so we can leave the
data_offset_ line alone.)

[xunjieli, 2015-11-17 20:28:36]

Hi michaeln, tzik, jianli, falken, mtomasz, zork,

Since this is a pretty mechanical re-landing of a previous reviewed CL (except a
fix to url_request_job which rdsmith and mmenke have reviewed), I will TBR you
if I don't hear back tomorrow. Please take your time. Thank you!

> Helen:  You can generally just TBR people when re-landing a CL if their part
of
> the review didn't change (I get a review from people who owned the changed
> files, TBR the others and post a message to them that I'm TBRing and why, and
> then land ~24 hours later if I don't hear back from them all by then).

Got it. Thanks!

https://codereview.chromium.org/1439953006/diff/60001/content/browser/net/vie...
File content/browser/net/view_http_cache_job_factory.cc (right):

https://codereview.chromium.org/1439953006/diff/60001/content/browser/net/vie...
content/browser/net/view_http_cache_job_factory.cc:169: int remaining =
base::checked_cast<int>(data_.size()) - data_offset_;
On 2015/11/17 19:55:02, davidben (slow) wrote:
> On 2015/11/17 19:54:06, davidben (slow) wrote:
> > Oh oops. I switched data_offset_ to size_t but intended to do so only after
> you
> > landed this one. I missed that yours had since been reverted. Since
> data_offset_
> > is also a size_t now let's do
> > 
> >   DCHECK_LE(data_offset_, data_.size());
> >   int remaining = base::checked_cast<int>(data_.size() - data_offset_);
> >   [...]
> >   data_offset_ += base::checked_cast<size_t>(buf_size);
> > 
> > We'll sort out the rest of the size_t / int confusion later since this
> interface
> > is now fairly int-specific in the return value.
> 
> (Er, I guess negative buf_size would have already exploded, so we can leave
the
> data_offset_ line alone.)

Acknowledged. Thanks, David.

https://codereview.chromium.org/1439953006/diff/60001/net/url_request/url_req...
File net/url_request/url_request_job_unittest.cc (right):

https://codereview.chromium.org/1439953006/diff/60001/net/url_request/url_req...
net/url_request/url_request_job_unittest.cc:79: const MockTransaction
kEmptyBody_Transaction = {
On 2015/11/17 18:57:32, mmenke wrote:
> Maybe kEmptyBodyGzip_Transaction?

Done.

[xunjieli, 2015-11-17 20:37:55]

https://codereview.chromium.org/1439953006/diff/60001/content/browser/net/vie...
File content/browser/net/view_http_cache_job_factory.cc (right):

https://codereview.chromium.org/1439953006/diff/60001/content/browser/net/vie...
content/browser/net/view_http_cache_job_factory.cc:169: int remaining =
base::checked_cast<int>(data_.size()) - data_offset_;
On 2015/11/17 19:55:02, davidben (slow) wrote:
> On 2015/11/17 19:54:06, davidben (slow) wrote:
> > Oh oops. I switched data_offset_ to size_t but intended to do so only after
> you
> > landed this one. I missed that yours had since been reverted. Since
> data_offset_
> > is also a size_t now let's do
> > 
> >   DCHECK_LE(data_offset_, data_.size());
> >   int remaining = base::checked_cast<int>(data_.size() - data_offset_);
> >   [...]
> >   data_offset_ += base::checked_cast<size_t>(buf_size);
> > 
> > We'll sort out the rest of the size_t / int confusion later since this
> interface
> > is now fairly int-specific in the return value.
> 
> (Er, I guess negative buf_size would have already exploded, so we can leave
the
> data_offset_ line alone.)

Done.

[falken, 2015-11-18 01:16:01]

service worker lgtm

[tzik, 2015-11-18 03:46:46]

fileapi LGTM

[xunjieli, 2015-11-18 14:27:04]

Description was changed from

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

This CL is to reland crrev.com/1410643007 which was
reverted in crrev.com/1437523002.

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
==========

to

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

This CL is to reland crrev.com/1410643007 which was
reverted in crrev.com/1437523002.

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.

TBR=michaeln@chromium.org, mtomasz@chromium.org,
jianli@chromium.org,zork@chromium.org

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

[xunjieli, 2015-11-18 14:27:24]

Description was changed from

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

This CL is to reland crrev.com/1410643007 which was
reverted in crrev.com/1437523002.

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.

TBR=michaeln@chromium.org, mtomasz@chromium.org,
jianli@chromium.org,zork@chromium.org

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

to

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

This CL is to reland crrev.com/1410643007 which was
reverted in crrev.com/1437523002.

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.

TBR=michaeln@chromium.org,mtomasz@chromium.org,jianli@chromium.org,zork@chrom...

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

[xunjieli, 2015-11-18 14:27:36]

The CQ bit was checked by xunjieli@chromium.org

[xunjieli, 2015-11-18 14:27:37]

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

[commit-bot: I haz the power, 2015-11-18 14:27:58]

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

[xunjieli, 2015-11-18 14:31:11]

I have stamps for files (url_request_job, mojo, service worker, fileapi) that
have changed since last review now. I am TBRing the other owners. Thank you all!

[commit-bot: I haz the power, 2015-11-18 14:34:15]

Committed patchset #5 (id:140001)

[commit-bot: I haz the power, 2015-11-18 14:35:03]

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