URLRequestJob: change ReadRawData contract

URLRequestJob: change ReadRawData contract

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,mnaganov@chromium.org,skyostil@chromium.org,eugenebut@chromium.org,davidben@chromium.org,falken@chromium.org,mtomasz@chromium.org, sky@chromium.org,jianli@chromium.org,zork@chromium.org,rdsmith@chromium.org
BUG=553300
Committed: https://crrev.com/26ede96bb45057dbb767ba41abde6ee29b1fb1de
Cr-Commit-Position: refs/heads/master@{#361158}

[xunjieli, 2015-11-20 18:30:54]

Description was changed from

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

BUG=
==========

to

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

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

[xunjieli, 2015-11-23 15:00:12]

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

[xunjieli, 2015-11-23 15:00:13]

Matt, I am planning to TBR other owners since the rest of the files did not
change. Randy is out. Could you take a look? I have confirmed that I do not see
crbug.com/558912 and crbug.com/558224. Not sure if we want to wait until after
thanksgiving though. PTAL.

[xunjieli, 2015-11-23 15:37:56]

https://codereview.chromium.org/1467603002/diff/20001/net/http/http_transacti...
File net/http/http_transaction_test_util.cc (right):

https://codereview.chromium.org/1467603002/diff/20001/net/http/http_transacti...
net/http/http_transaction_test_util.cc:214: CHECK(!done_reading_called_);
I can't add CHECK(!done_reading_called_) here, because done_reading_called is a
member of MockNetworkTransaction.

[mmenke, 2015-11-23 16:06:02]

https://codereview.chromium.org/1467603002/diff/20001/net/http/http_transacti...
File net/http/http_transaction_test_util.cc (right):

https://codereview.chromium.org/1467603002/diff/20001/net/http/http_transacti...
net/http/http_transaction_test_util.cc:214: CHECK(!done_reading_called_);
On 2015/11/23 15:37:56, xunjieli wrote:
> I can't add CHECK(!done_reading_called_) here, because done_reading_called is
a
> member of MockNetworkTransaction.

Sorry, I meant in MockNetworkTransaction::Read.

[mmenke, 2015-11-23 16:07:55]

On 2015/11/23 16:06:02, mmenke wrote:
>
https://codereview.chromium.org/1467603002/diff/20001/net/http/http_transacti...
> File net/http/http_transaction_test_util.cc (right):
> 
>
https://codereview.chromium.org/1467603002/diff/20001/net/http/http_transacti...
> net/http/http_transaction_test_util.cc:214: CHECK(!done_reading_called_);
> On 2015/11/23 15:37:56, xunjieli wrote:
> > I can't add CHECK(!done_reading_called_) here, because done_reading_called
is
> a
> > member of MockNetworkTransaction.
> 
> Sorry, I meant in MockNetworkTransaction::Read.

Oh, and I think we should go ahead and try to land today.  If we can't get it in
today, should wait until next Monday.

[xunjieli, 2015-11-23 16:15:58]

SGTM. PTAL.

https://codereview.chromium.org/1467603002/diff/20001/net/http/http_transacti...
File net/http/http_transaction_test_util.cc (right):

https://codereview.chromium.org/1467603002/diff/20001/net/http/http_transacti...
net/http/http_transaction_test_util.cc:214: CHECK(!done_reading_called_);
On 2015/11/23 16:06:02, mmenke wrote:
> On 2015/11/23 15:37:56, xunjieli wrote:
> > I can't add CHECK(!done_reading_called_) here, because done_reading_called
is
> a
> > member of MockNetworkTransaction.
> 
> Sorry, I meant in MockNetworkTransaction::Read.

Sorry, I meant to say it is a member of MockNetworkLayer. I have added a
done_reading_called_ to MockNetworkTransaction.

[mmenke, 2015-11-23 16:43:04]

On 2015/11/23 16:15:58, xunjieli wrote:
> SGTM. PTAL.
> 
>
https://codereview.chromium.org/1467603002/diff/20001/net/http/http_transacti...
> File net/http/http_transaction_test_util.cc (right):
> 
>
https://codereview.chromium.org/1467603002/diff/20001/net/http/http_transacti...
> net/http/http_transaction_test_util.cc:214: CHECK(!done_reading_called_);
> On 2015/11/23 16:06:02, mmenke wrote:
> > On 2015/11/23 15:37:56, xunjieli wrote:
> > > I can't add CHECK(!done_reading_called_) here, because done_reading_called
> is
> > a
> > > member of MockNetworkTransaction.
> > 
> > Sorry, I meant in MockNetworkTransaction::Read.
> 
> Sorry, I meant to say it is a member of MockNetworkLayer. I have added a
> done_reading_called_ to MockNetworkTransaction.

LGTM

[xunjieli, 2015-11-23 16:45:18]

The CQ bit was checked by xunjieli@chromium.org

[commit-bot: I haz the power, 2015-11-23 16:46:35]

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

[xunjieli, 2015-11-23 16:46:53]

The CQ bit was unchecked by xunjieli@chromium.org

[xunjieli, 2015-11-23 16:47:23]

Description was changed from

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

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

to

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

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,mnaganov@chromium.org,skyostil@chromium.org,eugeneb...,
sky@chromium.org,jianli@chromium.org,zork@chromium.org,rdsmith@chromium.org
BUG=553300
==========

[xunjieli, 2015-11-23 16:47:30]

The CQ bit was checked by xunjieli@chromium.org

[commit-bot: I haz the power, 2015-11-23 16:48:04]

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

[commit-bot: I haz the power, 2015-11-23 18:34:20]

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

[commit-bot: I haz the power, 2015-11-23 18:34:21]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[xunjieli, 2015-11-23 18:51:33]

The CQ bit was checked by xunjieli@chromium.org

[commit-bot: I haz the power, 2015-11-23 18:52:54]

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

[commit-bot: I haz the power, 2015-11-23 19:39:21]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-11-23 19:40:22]

Patchset 4 (id:??) landed as
https://crrev.com/26ede96bb45057dbb767ba41abde6ee29b1fb1de
Cr-Commit-Position: refs/heads/master@{#361158}