Glue parallel download job and download file together.

Glue parallel download job and download file together.

1. Connect the parallel download job to download file, so we can
generate requests and push the byte streams to download file.

2. Move 2 utility functions into parallel_download_util.

3. Tweak the test fixture for parallel download job, now it can take
a ReceivedSlice. And we target to test BuildParallelRequests now.
So we can add unittests for resumption later. The unittests for
communication between classes will be added later.

4. Refactor download_file_unittests, now it may accept more streams.

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

[xingliu, 2017-03-10 22:31:25]

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

[xingliu, 2017-03-10 22:31:26]

Hi, PHTAL, thanks.

[qinmin, 2017-03-11 06:08:22]

https://codereview.chromium.org/2742093002/diff/1/content/browser/download/pa...
File content/browser/download/parallel_download_job.cc (right):

https://codereview.chromium.org/2742093002/diff/1/content/browser/download/pa...
content/browser/download/parallel_download_job.cc:74: initial_request_offset_,
content_length_, GetParallelRequestCount());
nit: shouldn't the initial_request_offset_ always be 0 here if there are no
received slices? In that case, FindSlicesForNewDownload() only needs 2 params.
However, if we want to use FindSlicesForNewDownload() to solve the TODO below,
then we probably should rename it to sth like
FindSlicesToDownloadForRemainingContent()

https://codereview.chromium.org/2742093002/diff/1/content/browser/download/pa...
content/browser/download/parallel_download_job.cc:96: for (auto it =
slices_to_download.begin() + 1; it != slices_to_download.end();
const auto&

https://codereview.chromium.org/2742093002/diff/1/content/browser/download/pa...
content/browser/download/parallel_download_job.cc:98: CreateRequest(it->offset,
it->received_bytes);
you need {} since here since the for loop spans multiple lines

https://codereview.chromium.org/2742093002/diff/1/content/browser/download/pa...
File content/browser/download/parallel_download_job.h (right):

https://codereview.chromium.org/2742093002/diff/1/content/browser/download/pa...
content/browser/download/parallel_download_job.h:44: typedef
std::unordered_map<int64_t, std::unique_ptr<DownloadWorker>>
nit: using WorkerMap = std::...

https://codereview.chromium.org/2742093002/diff/1/content/browser/download/pa...
File content/browser/download/parallel_download_utils.cc (right):

https://codereview.chromium.org/2742093002/diff/1/content/browser/download/pa...
content/browser/download/parallel_download_utils.cc:64: for (int i = 0,
num_requests = bytes_left / slice_size; i < num_requests - 1;
shouldn't this just use request_count? why using a new num_requests?

https://codereview.chromium.org/2742093002/diff/1/content/browser/download/pa...
content/browser/download/parallel_download_utils.cc:72:
slices_to_download.emplace_back(current_offset, 0);
s/0/DownloadSaveInfo::kLengthFullContent/

https://codereview.chromium.org/2742093002/diff/1/content/public/browser/down...
File content/public/browser/download_item.h (right):

https://codereview.chromium.org/2742093002/diff/1/content/public/browser/down...
content/public/browser/download_item.h:120: typedef
std::vector<DownloadItem::ReceivedSlice> ReceivedSlices;
nit: using ReceivedSlices = std::vector<DownloadItem::ReceivedSlice>

[xingliu, 2017-03-13 17:53:17]

Thanks for the feedback, PTAL.

https://codereview.chromium.org/2742093002/diff/1/content/browser/download/pa...
File content/browser/download/parallel_download_job.cc (right):

https://codereview.chromium.org/2742093002/diff/1/content/browser/download/pa...
content/browser/download/parallel_download_job.cc:74: initial_request_offset_,
content_length_, GetParallelRequestCount());
On 2017/03/11 06:08:22, qinmin wrote:
> nit: shouldn't the initial_request_offset_ always be 0 here if there are no
> received slices? In that case, FindSlicesForNewDownload() only needs 2 params.
> However, if we want to use FindSlicesForNewDownload() to solve the TODO below,
> then we probably should rename it to sth like
> FindSlicesToDownloadForRemainingContent()

Thanks for this suggestion.

Yeah, this function can be used to chunk the last slice.

One question about ReceivedSlice, would a normal download also get a
ReceivedSlice? This makes sense but just not sure our plan here.

https://codereview.chromium.org/2742093002/diff/1/content/browser/download/pa...
content/browser/download/parallel_download_job.cc:96: for (auto it =
slices_to_download.begin() + 1; it != slices_to_download.end();
On 2017/03/11 06:08:22, qinmin wrote:
> const auto&

Done. use ++it here so const will yield compiling error.

Added a size check since (slices_to_download.begin() + 1) may throw exception.

https://codereview.chromium.org/2742093002/diff/1/content/browser/download/pa...
content/browser/download/parallel_download_job.cc:98: CreateRequest(it->offset,
it->received_bytes);
On 2017/03/11 06:08:22, qinmin wrote:
> you need {} since here since the for loop spans multiple lines

Done.

https://codereview.chromium.org/2742093002/diff/1/content/browser/download/pa...
File content/browser/download/parallel_download_job.h (right):

https://codereview.chromium.org/2742093002/diff/1/content/browser/download/pa...
content/browser/download/parallel_download_job.h:44: typedef
std::unordered_map<int64_t, std::unique_ptr<DownloadWorker>>
On 2017/03/11 06:08:22, qinmin wrote:
> nit: using WorkerMap = std::...

Done.

https://codereview.chromium.org/2742093002/diff/1/content/browser/download/pa...
File content/browser/download/parallel_download_utils.cc (right):

https://codereview.chromium.org/2742093002/diff/1/content/browser/download/pa...
content/browser/download/parallel_download_utils.cc:64: for (int i = 0,
num_requests = bytes_left / slice_size; i < num_requests - 1;
On 2017/03/11 06:08:22, qinmin wrote:
> shouldn't this just use request_count? why using a new num_requests?

This logic considers an edge case, like 3 bytes left with 4 request count, then
slice_size will be 1, and num_requests is 3 instead of 4.

https://codereview.chromium.org/2742093002/diff/1/content/browser/download/pa...
content/browser/download/parallel_download_utils.cc:72:
slices_to_download.emplace_back(current_offset, 0);
On 2017/03/11 06:08:22, qinmin wrote:
> s/0/DownloadSaveInfo::kLengthFullContent/

Done.

https://codereview.chromium.org/2742093002/diff/1/content/public/browser/down...
File content/public/browser/download_item.h (right):

https://codereview.chromium.org/2742093002/diff/1/content/public/browser/down...
content/public/browser/download_item.h:120: typedef
std::vector<DownloadItem::ReceivedSlice> ReceivedSlices;
On 2017/03/11 06:08:22, qinmin wrote:
> nit: using ReceivedSlices = std::vector<DownloadItem::ReceivedSlice>

Done.

[xingliu, 2017-03-13 18:24:13]

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

[commit-bot: I haz the power, 2017-03-13 18:25:10]

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-13 19:44:51]

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

[commit-bot: I haz the power, 2017-03-13 19:44:52]

Dry run: This issue passed the CQ dry run.

[David Trainor- moved to gerrit, 2017-03-14 17:59:10]

https://codereview.chromium.org/2742093002/diff/20001/content/browser/downloa...
File content/browser/download/download_file_unittest.cc (right):

https://codereview.chromium.org/2742093002/diff/20001/content/browser/downloa...
content/browser/download/download_file_unittest.cc:212: const
std::vector<DownloadItem::ReceivedSlice>& received_slices =
Should we pull this out to a def at some point?  e.g. "using SliceList =
std::vector<...>;"?

https://codereview.chromium.org/2742093002/diff/20001/content/browser/downloa...
File content/browser/download/download_job.cc (right):

https://codereview.chromium.org/2742093002/diff/20001/content/browser/downloa...
content/browser/download/download_job.cc:37: return;
When do we expect this to happen?

https://codereview.chromium.org/2742093002/diff/20001/content/public/browser/...
File content/public/browser/download_item.h (right):

https://codereview.chromium.org/2742093002/diff/20001/content/public/browser/...
content/public/browser/download_item.h:120: using ReceivedSlices =
std::vector<DownloadItem::ReceivedSlice>;
Aha!

[xingliu, 2017-03-14 18:26:13]

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

[xingliu, 2017-03-14 18:26:31]

Thanks for the feedback, PTAL.

https://codereview.chromium.org/2742093002/diff/20001/content/browser/downloa...
File content/browser/download/download_file_unittest.cc (right):

https://codereview.chromium.org/2742093002/diff/20001/content/browser/downloa...
content/browser/download/download_file_unittest.cc:212: const
std::vector<DownloadItem::ReceivedSlice>& received_slices =
On 2017/03/14 17:59:10, David Trainor-ping if over 24h wrote:
> Should we pull this out to a def at some point?  e.g. "using SliceList =
> std::vector<...>;"?

Done, forgot to use DownloadItem::ReceivedSlices in here.

https://codereview.chromium.org/2742093002/diff/20001/content/browser/downloa...
File content/browser/download/download_job.cc (right):

https://codereview.chromium.org/2742093002/diff/20001/content/browser/downloa...
content/browser/download/download_job.cc:37: return;
On 2017/03/14 17:59:10, David Trainor-ping if over 24h wrote:
> When do we expect this to happen?

Done, added a VLOG.

download_file is invalidated when DownloadItemImpl calls ReleaseDownloadFile.
 
It happens if the download is interrupted or complete, or restart a download
after user cancel the download.

The AddByteStream still may be called since the server response may arrive later
after something goes wrong or the original request complete the download.

We probably need to test this when put everything together. A cancel call may be
needed to kill the url_request in job class.

[commit-bot: I haz the power, 2017-03-14 18:26:38]

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-14 19:25:01]

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

[commit-bot: I haz the power, 2017-03-14 19:25:02]

Dry run: This issue passed the CQ dry run.

[asanka, 2017-03-14 19:44:02]

https://codereview.chromium.org/2742093002/diff/40001/content/browser/downloa...
File content/browser/download/download_create_info.h (right):

https://codereview.chromium.org/2742093002/diff/40001/content/browser/downloa...
content/browser/download/download_create_info.h:66: int64_t offset;
Do you expect this to be different from save_info->offset?

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

https://codereview.chromium.org/2742093002/diff/40001/content/browser/downloa...
content/browser/download/download_file_impl.cc:141:
stream->SetByteStream(std::move(stream_reader));
Are all SourceStream constructions immediately followed by SetByteStream? If so
we could move the byte stream into the constructor and get rid of SetByteStream.

https://codereview.chromium.org/2742093002/diff/40001/content/browser/downloa...
File content/browser/download/download_file_impl.h (right):

https://codereview.chromium.org/2742093002/diff/40001/content/browser/downloa...
content/browser/download/download_file_impl.h:206: // file thread.
Doesn't need to be restated here. The thread restriction applies to all
DownloadFileImpl members.

https://codereview.chromium.org/2742093002/diff/40001/content/browser/downloa...
File content/browser/download/download_file_unittest.cc (right):

https://codereview.chromium.org/2742093002/diff/40001/content/browser/downloa...
content/browser/download/download_file_unittest.cc:904: DCHECK(input_stream_1_);
DCHECK isn't necessary since the code will reliably crash if it's nullptr.

https://codereview.chromium.org/2742093002/diff/40001/content/browser/downloa...
File content/browser/download/download_job.cc (right):

https://codereview.chromium.org/2742093002/diff/40001/content/browser/downloa...
content/browser/download/download_job.cc:43: // safe to use base::Unretained
here.
to be precise, download_file_ is owned by download_item_ on the UI thread and is
always deleted on the FILE thread. I.e. the task that we schedule below will
always run prior to the download_file_ destructor.

https://codereview.chromium.org/2742093002/diff/40001/content/browser/downloa...
File content/browser/download/parallel_download_utils.cc (right):

https://codereview.chromium.org/2742093002/diff/40001/content/browser/downloa...
content/browser/download/parallel_download_utils.cc:54: int request_count) {
This assumes that the file consists of a contiguous range from offset 0 through
bytes_received. Is this a safe assumption?

https://codereview.chromium.org/2742093002/diff/40001/content/public/browser/...
File content/public/browser/download_item.h (right):

https://codereview.chromium.org/2742093002/diff/40001/content/public/browser/...
content/public/browser/download_item.h:120: using ReceivedSlices =
std::vector<DownloadItem::ReceivedSlice>;
Does this need to be a part of DownloadItem?

[xingliu, 2017-03-14 22:48:29]

Thanks for the review, PTAL.

https://codereview.chromium.org/2742093002/diff/40001/content/browser/downloa...
File content/browser/download/download_create_info.h (right):

https://codereview.chromium.org/2742093002/diff/40001/content/browser/downloa...
content/browser/download/download_create_info.h:66: int64_t offset;
On 2017/03/14 19:44:01, asanka wrote:
> Do you expect this to be different from save_info->offset?

No, but the problem is save_info will be moved to DownloadFile before sending to
DownloadItem, so DownloadItem and Job will lose this meta data.

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

https://codereview.chromium.org/2742093002/diff/40001/content/browser/downloa...
content/browser/download/download_file_impl.cc:141:
stream->SetByteStream(std::move(stream_reader));
On 2017/03/14 19:44:01, asanka wrote:
> Are all SourceStream constructions immediately followed by SetByteStream? If
so
> we could move the byte stream into the constructor and get rid of
SetByteStream.

Done,

Thanks for this suggestion, yeah, this makes sense now.

https://codereview.chromium.org/2742093002/diff/40001/content/browser/downloa...
File content/browser/download/download_file_impl.h (right):

https://codereview.chromium.org/2742093002/diff/40001/content/browser/downloa...
content/browser/download/download_file_impl.h:206: // file thread.
On 2017/03/14 19:44:01, asanka wrote:
> Doesn't need to be restated here. The thread restriction applies to all
> DownloadFileImpl members.

Done.

https://codereview.chromium.org/2742093002/diff/40001/content/browser/downloa...
File content/browser/download/download_file_unittest.cc (right):

https://codereview.chromium.org/2742093002/diff/40001/content/browser/downloa...
content/browser/download/download_file_unittest.cc:904: DCHECK(input_stream_1_);
On 2017/03/14 19:44:01, asanka wrote:
> DCHECK isn't necessary since the code will reliably crash if it's nullptr.

Done.

https://codereview.chromium.org/2742093002/diff/40001/content/browser/downloa...
File content/browser/download/download_job.cc (right):

https://codereview.chromium.org/2742093002/diff/40001/content/browser/downloa...
content/browser/download/download_job.cc:43: // safe to use base::Unretained
here.
On 2017/03/14 19:44:01, asanka wrote:
> to be precise, download_file_ is owned by download_item_ on the UI thread and
is
> always deleted on the FILE thread. I.e. the task that we schedule below will
> always run prior to the download_file_ destructor.

Done, polished the comment here.

https://codereview.chromium.org/2742093002/diff/40001/content/browser/downloa...
File content/browser/download/parallel_download_utils.cc (right):

https://codereview.chromium.org/2742093002/diff/40001/content/browser/downloa...
content/browser/download/parallel_download_utils.cc:54: int request_count) {
On 2017/03/14 19:44:01, asanka wrote:
> This assumes that the file consists of a contiguous range from offset 0
through
> bytes_received. Is this a safe assumption?

This function is mainly to chunk a slice into smaller ones. Currently used for
new download.

If there is more ReceivedSlice and may contains holes, another function is used,
mainly to find "holes".

https://codereview.chromium.org/2742093002/diff/40001/content/public/browser/...
File content/public/browser/download_item.h (right):

https://codereview.chromium.org/2742093002/diff/40001/content/public/browser/...
content/public/browser/download_item.h:120: using ReceivedSlices =
std::vector<DownloadItem::ReceivedSlice>;
On 2017/03/14 19:44:01, asanka wrote:
> Does this need to be a part of DownloadItem?

Put it here to align with ReceivedSlice, 

But good point, maybe DownloadItemImpl is better. wdyt?

[xingliu, 2017-03-16 00:13:06]

ping~

[David Trainor- moved to gerrit, 2017-03-17 07:15:27]

lgtm % other reviewers.

https://codereview.chromium.org/2742093002/diff/20001/content/browser/downloa...
File content/browser/download/download_job.cc (right):

https://codereview.chromium.org/2742093002/diff/20001/content/browser/downloa...
content/browser/download/download_job.cc:37: return;
On 2017/03/14 18:26:31, xingliu wrote:
> On 2017/03/14 17:59:10, David Trainor-ping if over 24h wrote:
> > When do we expect this to happen?
> 
> Done, added a VLOG.
> 
> download_file is invalidated when DownloadItemImpl calls ReleaseDownloadFile.
>  
> It happens if the download is interrupted or complete, or restart a download
> after user cancel the download.
> 
> The AddByteStream still may be called since the server response may arrive
later
> after something goes wrong or the original request complete the download.
> 
> We probably need to test this when put everything together. A cancel call may
be
> needed to kill the url_request in job class.

sg.  add a TODO to investigate/file a bug?

[qinmin, 2017-03-17 07:54:36]

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

https://codereview.chromium.org/2742093002/diff/60001/content/browser/downloa...
content/browser/download/download_file_impl.cc:80: save_info_->offset,
DownloadSaveInfo::kLengthFullContent,
actually, this is no longer true. For resumption, the length might not be
DownloadSaveInfo::kLengthFullContent.

see
https://cs.chromium.org/chromium/src/content/browser/download/download_item_i....

You either need to remove that line to make the initial request half open, or
set the length here.

[asanka, 2017-03-17 17:10:10]

LGTM % nits. Let's see what happens when we put everything together. :-)

I wasn't super detailed in the review since I'm not sure which parts are
intentionally missing. I'd suggest going over the whole feature again once all
the crucial bits are in place.

https://codereview.chromium.org/2742093002/diff/60001/content/browser/downloa...
File content/browser/download/download_create_info.h (right):

https://codereview.chromium.org/2742093002/diff/60001/content/browser/downloa...
content/browser/download/download_create_info.h:63: int64_t total_bytes;
This is now the size of the response body, not the total download size.

https://codereview.chromium.org/2742093002/diff/60001/content/browser/downloa...
content/browser/download/download_create_info.h:65: // The starting position of
the request to save to the target file.
Can you add a comment indicating that this must match save_info->offset if
save_info is non-null?

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

https://codereview.chromium.org/2742093002/diff/60001/content/browser/downloa...
content/browser/download/download_file_impl.cc:130: callback,
DOWNLOAD_INTERRUPT_REASON_NONE));
(not for this CL, but since you are in the neighborhood :-) )

Consider moving this before the RegisterAndActivateStream() call. Otherwise any
resulting DestinationUpdates/DestinationErrors will be scheduled ahead of the
DownloadFileInitialized call being scheduled here.

Not a big deal since this is already the case. But it's something that's
counterintuitive to someone reading the code.

https://codereview.chromium.org/2742093002/diff/60001/content/browser/downloa...
File content/browser/download/parallel_download_job.h (right):

https://codereview.chromium.org/2742093002/diff/60001/content/browser/downloa...
content/browser/download/parallel_download_job.h:68: // content-length header.
At this point, we should take care to differentiate between the following
concepts:

* Expected content length of entire download.  This would be the Content-Length
header value for an uncompressed entity body when requesting the whole download.

* Expected length of slice. This would be the Content-Length header value for an
uncompressed entity body when requesting slice. Also true for half-open slices.

https://codereview.chromium.org/2742093002/diff/60001/content/browser/downloa...
File content/browser/download/parallel_download_utils.cc (right):

https://codereview.chromium.org/2742093002/diff/60001/content/browser/downloa...
content/browser/download/parallel_download_utils.cc:37: // 3. (Undetermined)
Http/1.1 protocol.
Probably needs to be plumbed up from DownloadRequestCore.

This is available via url_request()->response_info()->connection_info.

https://codereview.chromium.org/2742093002/diff/60001/content/browser/downloa...
content/browser/download/parallel_download_utils.cc:38: // 4. (Undetermined) Not
under http proxy, e.g. data saver.
I'd suggest being a bit conservative about avoiding proxies since those are
common.

Excluding H2 / QUIC will automatically eliminate proxies we are accessing via
multiplexed connections. That might be enough to avoid parallel requests via the
data reduction proxy.

[xingliu, 2017-03-17 19:08:26]

Thanks for the feedbacks. 

I manually test this CL a little bit on eclipse download site, we can see from
the log that two streams write to the same sink, the sha256 of the file is
correct.

with some other recent CLs landed, streams should be able to stop at the correct
time.

We still have many things to do, to deal with edge cases, errors, clean up the
requests after failure, etc.

https://codereview.chromium.org/2742093002/diff/20001/content/browser/downloa...
File content/browser/download/download_job.cc (right):

https://codereview.chromium.org/2742093002/diff/20001/content/browser/downloa...
content/browser/download/download_job.cc:37: return;
On 2017/03/17 07:15:27, David Trainor-ping if over 24h wrote:
> On 2017/03/14 18:26:31, xingliu wrote:
> > On 2017/03/14 17:59:10, David Trainor-ping if over 24h wrote:
> > > When do we expect this to happen?
> > 
> > Done, added a VLOG.
> > 
> > download_file is invalidated when DownloadItemImpl calls
ReleaseDownloadFile.
> >  
> > It happens if the download is interrupted or complete, or restart a download
> > after user cancel the download.
> > 
> > The AddByteStream still may be called since the server response may arrive
> later
> > after something goes wrong or the original request complete the download.
> > 
> > We probably need to test this when put everything together. A cancel call
may
> be
> > needed to kill the url_request in job class.
> 
> sg.  add a TODO to investigate/file a bug?

Done.

https://codereview.chromium.org/2742093002/diff/60001/content/browser/downloa...
File content/browser/download/download_create_info.h (right):

https://codereview.chromium.org/2742093002/diff/60001/content/browser/downloa...
content/browser/download/download_create_info.h:63: int64_t total_bytes;
On 2017/03/17 17:10:09, asanka wrote:
> This is now the size of the response body, not the total download size.

Done, change the comment here, to address this is the content-length response
header.

https://codereview.chromium.org/2742093002/diff/60001/content/browser/downloa...
content/browser/download/download_create_info.h:65: // The starting position of
the request to save to the target file.
On 2017/03/17 17:10:09, asanka wrote:
> Can you add a comment indicating that this must match save_info->offset if
> save_info is non-null?

Done, also put a TODO, I think if we refactor job to own download file, this can
be removed.

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

https://codereview.chromium.org/2742093002/diff/60001/content/browser/downloa...
content/browser/download/download_file_impl.cc:80: save_info_->offset,
DownloadSaveInfo::kLengthFullContent,
On 2017/03/17 07:54:36, qinmin wrote:
> actually, this is no longer true. For resumption, the length might not be
> DownloadSaveInfo::kLengthFullContent.
> 
> see
>
https://cs.chromium.org/chromium/src/content/browser/download/download_item_i....
> 
> You either need to remove that line to make the initial request half open, or
> set the length here.

Good point, done.

https://codereview.chromium.org/2742093002/diff/60001/content/browser/downloa...
content/browser/download/download_file_impl.cc:130: callback,
DOWNLOAD_INTERRUPT_REASON_NONE));
On 2017/03/17 17:10:09, asanka wrote:
> (not for this CL, but since you are in the neighborhood :-) )
> 
> Consider moving this before the RegisterAndActivateStream() call. Otherwise
any
> resulting DestinationUpdates/DestinationErrors will be scheduled ahead of the
> DownloadFileInitialized call being scheduled here.
> 
> Not a big deal since this is already the case. But it's something that's
> counterintuitive to someone reading the code.

Done,

Oh, I see, thanks for this suggestion.

https://codereview.chromium.org/2742093002/diff/60001/content/browser/downloa...
File content/browser/download/parallel_download_job.h (right):

https://codereview.chromium.org/2742093002/diff/60001/content/browser/downloa...
content/browser/download/parallel_download_job.h:68: // content-length header.
On 2017/03/17 17:10:09, asanka wrote:
> At this point, we should take care to differentiate between the following
> concepts:
> 
> * Expected content length of entire download.  This would be the
Content-Length
> header value for an uncompressed entity body when requesting the whole
download.
> 
> * Expected length of slice. This would be the Content-Length header value for
an
> uncompressed entity body when requesting slice. Also true for half-open
slices.

Thanks for this suggestion.

Polished the comment here, well I actually didn't realize this, fixed the
function to chunk the file.

https://codereview.chromium.org/2742093002/diff/60001/content/browser/downloa...
File content/browser/download/parallel_download_utils.cc (right):

https://codereview.chromium.org/2742093002/diff/60001/content/browser/downloa...
content/browser/download/parallel_download_utils.cc:37: // 3. (Undetermined)
Http/1.1 protocol.
On 2017/03/17 17:10:09, asanka wrote:
> Probably needs to be plumbed up from DownloadRequestCore.
> 
> This is available via url_request()->response_info()->connection_info.

Thanks, will do it in a separate CL.

https://codereview.chromium.org/2742093002/diff/60001/content/browser/downloa...
content/browser/download/parallel_download_utils.cc:38: // 4. (Undetermined) Not
under http proxy, e.g. data saver.
On 2017/03/17 17:10:09, asanka wrote:
> I'd suggest being a bit conservative about avoiding proxies since those are
> common.
> 
> Excluding H2 / QUIC will automatically eliminate proxies we are accessing via
> multiplexed connections. That might be enough to avoid parallel requests via
the
> data reduction proxy.

Make sense, removed the http proxy line.

[xingliu, 2017-03-17 20:47:42]

xingliu@chromium.org changed reviewers:
+ alexmos@chromium.org

[xingliu, 2017-03-17 20:47:42]

@alexmos, Hi, alex can you help take a look at
content/public/browser/download_item.h

Thanks.

[alexmos, 2017-03-17 20:54:02]

content/public/browser/download_item.h LGTM

[qinmin, 2017-03-17 21:58:39]

lgtm

[xingliu, 2017-03-18 02:35:22]

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

[commit-bot: I haz the power, 2017-03-18 02:35:53]

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-18 03:09:05]

The CQ bit was unchecked by xingliu@chromium.org

[xingliu, 2017-03-18 03:09:18]

The CQ bit was checked by xingliu@chromium.org

[xingliu, 2017-03-18 03:09:18]

The patchset sent to the CQ was uploaded after l-g-t-m from asanka@chromium.org,
dtrainor@chromium.org, qinmin@chromium.org, alexmos@chromium.org
Link to the patchset: https://codereview.chromium.org/2742093002/#ps100001
(title: "Rebase, and adjust unittests with recent landed CLs.")

[commit-bot: I haz the power, 2017-03-18 03:09:43]

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-18 03:45:26]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1489806558163330,
"parent_rev": "4aab7a540a5ce356f6ddd9455c9ac35b83315a29", "commit_rev":
"6719c20584c6c916a4f5820a0f3b458ab612dfad"}

[commit-bot: I haz the power, 2017-03-18 03:46:06]

Description was changed from

==========
Glue parallel download job and download file together.

1. Connect the parallel download job to download file, so we can
generate requests and push the byte streams to download file.

2. Move 2 utility functions into parallel_download_util.

3. Tweak the test fixture for parallel download job, now it can take
a ReceivedSlice. And we target to test BuildParallelRequests now.
So we can add unittests for resumption later. The unittests for
communication between classes will be added later.

4. Refactor download_file_unittests, now it may accept more streams.

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

to

==========
Glue parallel download job and download file together.

1. Connect the parallel download job to download file, so we can
generate requests and push the byte streams to download file.

2. Move 2 utility functions into parallel_download_util.

3. Tweak the test fixture for parallel download job, now it can take
a ReceivedSlice. And we target to test BuildParallelRequests now.
So we can add unittests for resumption later. The unittests for
communication between classes will be added later.

4. Refactor download_file_unittests, now it may accept more streams.

BUG=644352

Review-Url: https://codereview.chromium.org/2742093002
Cr-Commit-Position: refs/heads/master@{#457952}
Committed:
https://chromium.googlesource.com/chromium/src/+/6719c20584c6c916a4f5820a0f3b...
==========

[commit-bot: I haz the power, 2017-03-18 03:46:08]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/6719c20584c6c916a4f5820a0f3b...