Add a helper method to truncate SourceStream's length

Add a helper method to truncate SourceStream's length

To support half open requests, we need to truncate all SourceStreams when they came.
This CL adds a helper method to truncate the SourceStream when they are active.
And it fixes an issue that the usage of 0 can be very unclear.
Also this CL makes the initial request half open so we can get the correct content-length.

BUG=644352
Review-Url: https://codereview.chromium.org/2770303002
Cr-Commit-Position: refs/heads/master@{#459603}
Committed: https://chromium.googlesource.com/chromium/src/+/aa5d2f477221a57cc51cdef53279c3b1453f63d3

[qinmin, 2017-03-24 18:31:47]

qinmin@chromium.org changed reviewers:
+ dtrainor@chromium.org, xingliu@chromium.org

[qinmin, 2017-03-24 18:31:48]

PTAL

[xingliu, 2017-03-24 19:13:47]

Some discussion.

https://codereview.chromium.org/2770303002/diff/1/content/browser/download/do...
File content/browser/download/download_file_impl.cc (right):

https://codereview.chromium.org/2770303002/diff/1/content/browser/download/do...
content/browser/download/download_file_impl.cc:74: if (offset < offset_) {
nit%: Should we also do it when offset == offset_ ?

https://codereview.chromium.org/2770303002/diff/1/content/browser/download/do...
content/browser/download/download_file_impl.cc:75: if (offset + bytes_written >
offset_)
nit%: what about if (..&& ..)

https://codereview.chromium.org/2770303002/diff/1/content/browser/download/do...
content/browser/download/download_file_impl.cc:82: length_ = offset - offset;
should it be (offset_ - offset) ?

Also if length_ is already kNoBytesToWrite, is it possible to flush it with a
new valid value, since this function calls on every ReceivedBytes.

[qinmin, 2017-03-24 20:39:01]

https://codereview.chromium.org/2770303002/diff/1/content/browser/download/do...
File content/browser/download/download_file_impl.cc (right):

https://codereview.chromium.org/2770303002/diff/1/content/browser/download/do...
content/browser/download/download_file_impl.cc:74: if (offset < offset_) {
On 2017/03/24 19:13:47, xingliu wrote:
> nit%: Should we also do it when offset == offset_ ?
Done. == is originally captured in the next if block, moved it here.
Results are the same, but we can remove the 
if (length_ == 0) block below

https://codereview.chromium.org/2770303002/diff/1/content/browser/download/do...
content/browser/download/download_file_impl.cc:75: if (offset + bytes_written >
offset_)
On 2017/03/24 19:13:47, xingliu wrote:
> nit%: what about if (..&& ..)

No, we want to early return here if offset < offset_.
Otherwise, it will go into the next if block since (length > offset- offset_) is
always true

https://codereview.chromium.org/2770303002/diff/1/content/browser/download/do...
content/browser/download/download_file_impl.cc:82: length_ = offset - offset;
On 2017/03/24 19:13:47, xingliu wrote:
> should it be (offset_ - offset) ?
> 
> Also if length_ is already kNoBytesToWrite, is it possible to flush it with a
> new valid value, since this function calls on every ReceivedBytes.

No, (offset_-offset) is negative in in this if block.  If length_ is
kNoBytesToWrite, it will not satisfy any of the conditions here, so it shouldn't
change.

Added an early return condition at the top so we don't proceed if length_is
already kNoBytesToWrite.

[xingliu, 2017-03-24 21:14:08]

https://codereview.chromium.org/2770303002/diff/1/content/browser/download/do...
File content/browser/download/download_file_impl.cc (right):

https://codereview.chromium.org/2770303002/diff/1/content/browser/download/do...
content/browser/download/download_file_impl.cc:75: if (offset + bytes_written >
offset_)
On 2017/03/24 20:39:01, qinmin wrote:
> On 2017/03/24 19:13:47, xingliu wrote:
> > nit%: what about if (..&& ..)
> 
> No, we want to early return here if offset < offset_.
> Otherwise, it will go into the next if block since (length > offset- offset_)
is
> always true

My bad, I see.

https://codereview.chromium.org/2770303002/diff/1/content/browser/download/do...
content/browser/download/download_file_impl.cc:82: length_ = offset - offset;
On 2017/03/24 20:39:01, qinmin wrote:
> On 2017/03/24 19:13:47, xingliu wrote:
> > should it be (offset_ - offset) ?
> > 
> > Also if length_ is already kNoBytesToWrite, is it possible to flush it with
a
> > new valid value, since this function calls on every ReceivedBytes.
> 
> No, (offset_-offset) is negative in in this if block.  If length_ is
> kNoBytesToWrite, it will not satisfy any of the conditions here, so it
shouldn't
> change.
> 
> Added an early return condition at the top so we don't proceed if length_is
> already kNoBytesToWrite.

offset - offset is always 0, I think we are trying to truncate here?

[qinmin, 2017-03-24 21:38:31]

https://codereview.chromium.org/2770303002/diff/1/content/browser/download/do...
File content/browser/download/download_file_impl.cc (right):

https://codereview.chromium.org/2770303002/diff/1/content/browser/download/do...
content/browser/download/download_file_impl.cc:82: length_ = offset - offset;
On 2017/03/24 21:14:08, xingliu wrote:
> On 2017/03/24 20:39:01, qinmin wrote:
> > On 2017/03/24 19:13:47, xingliu wrote:
> > > should it be (offset_ - offset) ?
> > > 
> > > Also if length_ is already kNoBytesToWrite, is it possible to flush it
with
> a
> > > new valid value, since this function calls on every ReceivedBytes.
> > 
> > No, (offset_-offset) is negative in in this if block.  If length_ is
> > kNoBytesToWrite, it will not satisfy any of the conditions here, so it
> shouldn't
> > change.
> > 
> > Added an early return condition at the top so we don't proceed if length_is
> > already kNoBytesToWrite.
> 
> offset - offset is always 0, I think we are trying to truncate here?

should be offset-offset_, fixed.

[xingliu, 2017-03-24 22:17:42]

lgtm,

From now on, we need to check kNoBytesToWrite when SourceStream::length() is
used.

[qinmin, 2017-03-24 22:47:13]

The CQ bit was checked by qinmin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-24 22:47:37]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-24 23:39:07]

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

[commit-bot: I haz the power, 2017-03-24 23:39:07]

Dry run: This issue passed the CQ dry run.

[qinmin, 2017-03-24 23:49:05]

The CQ bit was checked by qinmin@chromium.org

[commit-bot: I haz the power, 2017-03-24 23:49:45]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-24 23:55:11]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1490399345032830,
"parent_rev": "fa2b8f683b4ed28047f8bcb6f159b28478f68191", "commit_rev":
"aa5d2f477221a57cc51cdef53279c3b1453f63d3"}

[commit-bot: I haz the power, 2017-03-24 23:55:54]

Description was changed from

==========
Add a helper method to truncate SourceStream's length

To support half open requests, we need to truncate all SourceStreams when they
came.
This CL adds a helper method to truncate the SourceStream when they are active.
And it fixes an issue that the usage of 0 can be very unclear.
Also this CL makes the initial request half open so we can get the correct
content-length.

BUG=644352
==========

to

==========
Add a helper method to truncate SourceStream's length

To support half open requests, we need to truncate all SourceStreams when they
came.
This CL adds a helper method to truncate the SourceStream when they are active.
And it fixes an issue that the usage of 0 can be very unclear.
Also this CL makes the initial request half open so we can get the correct
content-length.

BUG=644352

Review-Url: https://codereview.chromium.org/2770303002
Cr-Commit-Position: refs/heads/master@{#459603}
Committed:
https://chromium.googlesource.com/chromium/src/+/aa5d2f477221a57cc51cdef53279...
==========

[commit-bot: I haz the power, 2017-03-24 23:55:55]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/aa5d2f477221a57cc51cdef53279...