remove is_sparse_file_ from DownloadFileImpl

remove is_sparse_file_ from DownloadFileImpl

|received_slices_| vector and |source_streams_| can both tell whether the file is sparse.
For non parallel download, received_slices_ vector is always empty
If a writer is added, we will start constructing that vector.

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

[qinmin, 2017-03-28 22:44:24]

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

[qinmin, 2017-03-28 22:44:25]

PTAL

[David Trainor- moved to gerrit, 2017-03-28 23:58:50]

lgtm % pulling out helper method.

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

https://codereview.chromium.org/2782033002/diff/1/content/browser/download/do...
content/browser/download/download_file_impl.cc:142: received_slices_.size() >
0);
Seems like sometimes we use received_slices_ and sometimes we use
source_streams_.  Can we add a private helper IsSparseFile() that just checks
one or both that we can use throughout the file?

[qinmin, 2017-03-29 16:52:08]

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

[commit-bot: I haz the power, 2017-03-29 16:52:35]

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-29 17:03:50]

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

[commit-bot: I haz the power, 2017-03-29 17:03:50]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[qinmin, 2017-03-29 17:21:02]

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

[commit-bot: I haz the power, 2017-03-29 17:21:28]

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-29 17:33:16]

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

[commit-bot: I haz the power, 2017-03-29 17:33:19]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[qinmin, 2017-03-29 17:38:26]

Patchset #2 (id:20001) has been deleted

[qinmin, 2017-03-29 17:38:32]

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

[commit-bot: I haz the power, 2017-03-29 17:38:56]

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-29 18:45:01]

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

[commit-bot: I haz the power, 2017-03-29 18:45:02]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[qinmin, 2017-03-29 19:12:06]

Patchset #3 (id:60001) has been deleted

[qinmin, 2017-03-29 19:16:24]

qinmin@chromium.org changed reviewers:
+ phajdan.jr@chromium.org

[qinmin, 2017-03-29 19:16:25]

+phajdan.jr for content/public/test OWNER.

phajdan.jr@, would you please take a look at this 1 line change? thanks

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

https://codereview.chromium.org/2782033002/diff/1/content/browser/download/do...
content/browser/download/download_file_impl.cc:142: received_slices_.size() >
0);
On 2017/03/28 23:58:49, David Trainor-ping if over 24h wrote:
> Seems like sometimes we use received_slices_ and sometimes we use
> source_streams_.  Can we add a private helper IsSparseFile() that just checks
> one or both that we can use throughout the file?

Done.

[qinmin, 2017-03-29 19:16:41]

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

[commit-bot: I haz the power, 2017-03-29 19:17:24]

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

[xingliu, 2017-03-29 20:14:02]

lgtm with nit%

https://codereview.chromium.org/2782033002/diff/80001/content/browser/downloa...
File content/browser/download/download_file_impl.cc (right):

https://codereview.chromium.org/2782033002/diff/80001/content/browser/downloa...
content/browser/download/download_file_impl.cc:404:
AddNewSlice(source_stream->offset(), bytes_to_write);
nit%:  I might not fully understand the logic here, please ignore the comment if
I'm wrong.

The slice for initial request is also added in AddByteStream.

If the last function call in the task runner is AddByteStream for a second
stream, the ReceivedSlice for the initial request may be created twice if this
is the first disk IO for initial request.

[commit-bot: I haz the power, 2017-03-29 20:18:47]

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

[commit-bot: I haz the power, 2017-03-29 20:18:48]

Dry run: This issue passed the CQ dry run.

[qinmin, 2017-03-29 20:44:08]

https://codereview.chromium.org/2782033002/diff/80001/content/browser/downloa...
File content/browser/download/download_file_impl.cc (right):

https://codereview.chromium.org/2782033002/diff/80001/content/browser/downloa...
content/browser/download/download_file_impl.cc:404:
AddNewSlice(source_stream->offset(), bytes_to_write);
On 2017/03/29 20:14:02, xingliu wrote:
> nit%:  I might not fully understand the logic here, please ignore the comment
if
> I'm wrong.
> 
> The slice for initial request is also added in AddByteStream.
> 
> If the last function call in the task runner is AddByteStream for a second
> stream, the ReceivedSlice for the initial request may be created twice if this
> is the first disk IO for initial request.

Shouldn't AddByteStream only called on parallel requests? The response for the
initial request is added in the ctor, and once another stream arrives, we will
create the received_slice vector

[xingliu, 2017-03-29 21:06:28]

https://codereview.chromium.org/2782033002/diff/80001/content/browser/downloa...
File content/browser/download/download_file_impl.cc (right):

https://codereview.chromium.org/2782033002/diff/80001/content/browser/downloa...
content/browser/download/download_file_impl.cc:404:
AddNewSlice(source_stream->offset(), bytes_to_write);
On 2017/03/29 20:44:08, qinmin wrote:
> On 2017/03/29 20:14:02, xingliu wrote:
> > nit%:  I might not fully understand the logic here, please ignore the
comment
> if
> > I'm wrong.
> > 
> > The slice for initial request is also added in AddByteStream.
> > 
> > If the last function call in the task runner is AddByteStream for a second
> > stream, the ReceivedSlice for the initial request may be created twice if
this
> > is the first disk IO for initial request.
> 
> Shouldn't AddByteStream only called on parallel requests? The response for the
> initial request is added in the ctor, and once another stream arrives, we will
> create the received_slice vector

Yeah, I mean (bytes_to_write > 0 && prev_bytes_written == 0) may pass for the
first source stream and try to add the slice twice. 

This is probably ok since new slice is merged to the vector of ReceivedSlice.

[qinmin, 2017-03-29 21:19:30]

On 2017/03/29 21:06:28, xingliu wrote:
>
https://codereview.chromium.org/2782033002/diff/80001/content/browser/downloa...
> File content/browser/download/download_file_impl.cc (right):
> 
>
https://codereview.chromium.org/2782033002/diff/80001/content/browser/downloa...
> content/browser/download/download_file_impl.cc:404:
> AddNewSlice(source_stream->offset(), bytes_to_write);
> On 2017/03/29 20:44:08, qinmin wrote:
> > On 2017/03/29 20:14:02, xingliu wrote:
> > > nit%:  I might not fully understand the logic here, please ignore the
> comment
> > if
> > > I'm wrong.
> > > 
> > > The slice for initial request is also added in AddByteStream.
> > > 
> > > If the last function call in the task runner is AddByteStream for a second
> > > stream, the ReceivedSlice for the initial request may be created twice if
> this
> > > is the first disk IO for initial request.
> > 
> > Shouldn't AddByteStream only called on parallel requests? The response for
the
> > initial request is added in the ctor, and once another stream arrives, we
will
> > create the received_slice vector
> 
> Yeah, I mean (bytes_to_write > 0 && prev_bytes_written == 0) may pass for the
> first source stream and try to add the slice twice. 
> 
> This is probably ok since new slice is merged to the vector of ReceivedSlice.

This won't happen.
If  the first request has written some data before the first AddByteStream()
call, prev_bytes_written will not be 0, and thus we will not call AddNewSlice
here.
Otherwise, a slice prior to the first request's offset should have been created.
And the AddNewSlice() call here will merge the new slice with that one if it is
called by the first request

[Paweł Hajdan Jr., 2017-03-30 08:03:38]

LGTM

[qinmin, 2017-03-30 14:17:19]

The CQ bit was checked by qinmin@chromium.org

[qinmin, 2017-03-30 14:17:19]

The patchset sent to the CQ was uploaded after l-g-t-m from
dtrainor@chromium.org
Link to the patchset: https://codereview.chromium.org/2782033002/#ps80001
(title: "add IsSparseFile() helper method")

[commit-bot: I haz the power, 2017-03-30 14:17:37]

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-30 14:22:23]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1490883439451990,
"parent_rev": "459465b1d6d569a612b16526a30af878a8df1b10", "commit_rev":
"6be2821a3321d1a56d1c2b0424903c0015fb4590"}

[commit-bot: I haz the power, 2017-03-30 14:23:10]

Description was changed from

==========
remove is_sparse_file_ from DownloadFileImpl

|received_slices_| vector and |source_streams_| can both tell whether the file
is sparse.
For non parallel download, received_slices_ vector is always empty
If a writer is added, we will start constructing that vector.

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

to

==========
remove is_sparse_file_ from DownloadFileImpl

|received_slices_| vector and |source_streams_| can both tell whether the file
is sparse.
For non parallel download, received_slices_ vector is always empty
If a writer is added, we will start constructing that vector.

BUG=644352

Review-Url: https://codereview.chromium.org/2782033002
Cr-Commit-Position: refs/heads/master@{#460749}
Committed:
https://chromium.googlesource.com/chromium/src/+/6be2821a3321d1a56d1c2b042490...
==========

[commit-bot: I haz the power, 2017-03-30 14:23:11]

Committed patchset #3 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/6be2821a3321d1a56d1c2b042490...