Introduce ParallelDownloadJob.

Introduce ParallelDownloadJob.

1. Add ParallelDownloadJob and DownloadUrlJob, both inherits DownloadJob
owned by DownloadItemImpl, currently we only do necessary refactoring
for parallel download feature, and failure handling or wiring to file
IO logic is not implemented yet.

2. Modify DownloadJob, remove any function we don't need. Since we need
to access many things in DownloadItemImpl, now it cache a raw pointer,
and is a friend class of DownloadItemImpl.

3. ParallelDownloadJob holds a list of DownloadUrlTask, which uses
UrlDownloader to send requests.

4. Tweak for UrlDownloader, previously UrlDownloader is coupled with
DownloadManagerImpl and everything has to go through
DownloadManagerImpl, now it uses a weak pointer delegate to communicate
to its owner, i.e. DownloadManagerImpl or DownloadUrlTask.

5. Pull out MockDownloadItemImpl to a seperate source file.

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

[xingliu, 2017-02-14 19:53:46]

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

[commit-bot: I haz the power, 2017-02-14 19:54:11]

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

[xingliu, 2017-02-14 19:54:39]

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

[xingliu, 2017-02-14 19:54:40]

Hi, PHTAL, thanks.

[commit-bot: I haz the power, 2017-02-14 20:29:12]

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

[commit-bot: I haz the power, 2017-02-14 20:29:13]

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

[xingliu, 2017-02-14 20:32:07]

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

[commit-bot: I haz the power, 2017-02-14 20:33:30]

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-02-14 22:04:12]

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

[commit-bot: I haz the power, 2017-02-14 22:04:13]

Dry run: This issue passed the CQ dry run.

[xingliu, 2017-02-15 17:37:02]

Description was changed from

==========
Introduce ParallelDownloadJob.

1. Add ParallelDownloadJob and DownloadUrlJob, both inherits DownloadJob
owned by DownloadItemImpl, currently we only do necessary refactoring
for parallel download feature, and failure handling or wiring to file
IO logic is not implemented yet.

2. Modify DownloadJob, remove any function we don't need. Since we need
to access many things in DownloadItemImpl, now it cache a raw pointer,
and is a friend class of DownloadItemImpl.

3. ParallelDownloadJob holds a list of DownloadUrlTask, which uses
UrlDownloader to send requests.

4. Tweak for UrlDownloader, previously UrlDownloader is coupled with
DownloadManagerImpl and everything has to go through
DownloadManagerImpl, now it uses a weak pointer delegate to communicate
to its owner, i.e. DownloadManagerImpl or DownloadUrlTask.

5. Pull out MockDownloadItemImpl to a seperate source file.
==========

to

==========
Introduce ParallelDownloadJob.

1. Add ParallelDownloadJob and DownloadUrlJob, both inherits DownloadJob
owned by DownloadItemImpl, currently we only do necessary refactoring
for parallel download feature, and failure handling or wiring to file
IO logic is not implemented yet.

2. Modify DownloadJob, remove any function we don't need. Since we need
to access many things in DownloadItemImpl, now it cache a raw pointer,
and is a friend class of DownloadItemImpl.

3. ParallelDownloadJob holds a list of DownloadUrlTask, which uses
UrlDownloader to send requests.

4. Tweak for UrlDownloader, previously UrlDownloader is coupled with
DownloadManagerImpl and everything has to go through
DownloadManagerImpl, now it uses a weak pointer delegate to communicate
to its owner, i.e. DownloadManagerImpl or DownloadUrlTask.

5. Pull out MockDownloadItemImpl to a seperate source file.

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

[asanka, 2017-02-16 16:27:40]

You can disregard the lower level comments. I think we'll need to figure out the
higher level details before diving into the nitty gritty :-)

Let's layout the division of responsibilities across the classes first.
Currently there are too many classes purporting to download a URL, which is a
bit confusing. So let's have a rough sketch of the nomenclature. That'll make it
easier for everyone to understand what's going on.

DownloadRequestCore: common logic for observing network events, dealing with
server response, constructing the primary data pipe, and notifying its delegate
that a new download is starting.

UrlDownloader: is currently tasked with owning and driving a URLRequest and
invoking DownlaodRequestCore. It's purpose in life is to absorb the interface
details of URLRequest, maintain flow control, and translate events into a form
that DownloadRequestCore can understand. This is only used when the downloads
system owns and drives a URLRequest, which is the case for parallel downloads.

DownloadRequestHandler: currently attaches itself to the resource loader stack
and has the same responsibilities as UrlDownloader w/r/t translating events into
something DownloadRequestCore can understand.

DownloadManagerImpl: Both UrlDownloader and DownlaodRequestHandler notify
DownloadManagerImpl whne a new download is starting. Responsible for
constructing a DownloadFile, attaching the reader end of the data pipe to said
DownloadFile, constructing a DownloadItemImpl, and then passing along all the
download metadata so that DownloadItemImpl can start its resource downloading
state machine.

DownloadItemImpl: takes a DownloadFile, a DownloadRequestHandleInterface, and
some metadata and invokes its state machine. Also responsible for handling
resumption by invoking DownloadRequestCore to create a new request and then
handing that over to DownloadManager to dispatch via UrlDownloader.

So in the new world, we need to decide:

* Who is responsible for constructing and owning a DownloadFile? (assuming that
there's only going to be one DownloadFile per DownloadItemImpl)

* Who is responsible for download logistics like target determination?

* Who is responsible for active download workflow (e.g. keeping score of how
much of the file has been downloaded, and dealing with pause/resume/cancel)?

Once we answer those, let's start by moving that logic to the respective classes
without breaking anything. Then we can add the parallelizing logic on top, at
which point we'll need to deal with things like:

* How is resumption going to be handled when the partial file consists of a
bunch of disjoint ranges instead of one contiguous range rooted at the start of
the file?

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

https://codereview.chromium.org/2689373003/diff/20001/content/browser/downloa...
content/browser/download/download_item_impl.cc:375: is_paused_ = true;
Consider removing is_paused_ and having the job_ keep track of the paused state.

https://codereview.chromium.org/2689373003/diff/20001/content/browser/downloa...
content/browser/download/download_item_impl.cc:1245: job_->Attach(this);
Why is there a separate Attach() method?

It would be reasonable to assume that the job would be bound to the same
DownloadItemImpl for its entire lifetime, and that DownloadItemImpl will outlive
the job. This implies that we would be able associate a Job with a
DownloadItemImpl at creation and leave it at that.

https://codereview.chromium.org/2689373003/diff/20001/content/browser/downloa...
content/browser/download/download_item_impl.cc:1249: void
DownloadItemImpl::InitializeDownloadFile() {
This isn't so much initializing the download file, but setting in motion the
entire series of events that result in the file being downloaded.

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

https://codereview.chromium.org/2689373003/diff/20001/content/browser/downloa...
content/browser/download/download_job.cc:20:
download_item_->InitializeDownloadFile();
It would be great if we could have calls going one way. I.e. does it make sense
for DownloadItemImpl to retain ownership of the DownloadFile? Or should
DownloadJob be the owner of the DownloadFile.

DownloadFile is only supposed to contain state for an active download, which
maps to the lifetime of a Job.

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

https://codereview.chromium.org/2689373003/diff/20001/content/browser/downloa...
content/browser/download/download_job_factory.cc:27: bool has_strong_validator =
Not the place to determine this.

See
https://cs.chromium.org/chromium/src/net/http/http_response_headers.cc?rcl=7b...

DownloadRequestCore has access to this information, and consequently etag and
last_modified would only be non-empty if they are considered strong validators
:-). There would be no point in keeping those around if they weren't.

Another option is to also check for Accept-Ranges and only store validators if
they could be used for range requests. Once again, there's no point in keeping
validators around for downloads if they couldn't be used for range requests.

https://codereview.chromium.org/2689373003/diff/20001/content/browser/downloa...
content/browser/download/download_job_factory.cc:44: job =
base::MakeUnique<ParallelDownloadJob>(std::move(req_handle));
return base::MakeUnique<...>(...)

No need to store it in job.

https://codereview.chromium.org/2689373003/diff/20001/content/browser/downloa...
content/browser/download/download_job_factory.cc:49: job =
base::MakeUnique<DownloadUrlJob>(std::move(req_handle));
Let's rename DownloadUrlJob to something else so that the distinction between
ParallelDownloadJob and DownloadUrlJob is clear.

https://codereview.chromium.org/2689373003/diff/20001/content/browser/downloa...
File content/browser/download/download_url_job.h (right):

https://codereview.chromium.org/2689373003/diff/20001/content/browser/downloa...
content/browser/download/download_url_job.h:18: DownloadUrlJob(
explicit

https://codereview.chromium.org/2689373003/diff/20001/content/browser/downloa...
File content/browser/download/download_url_task.h (right):

https://codereview.chromium.org/2689373003/diff/20001/content/browser/downloa...
content/browser/download/download_url_task.h:23: class DownloadUrlTask : public
UrlDownloader::Delegate {
This class name is pretty confusing given that we have a DownloadUrlJob /
UrlDownloader / DownloadUrlTask/ etc..

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

https://codereview.chromium.org/2689373003/diff/20001/content/browser/downloa...
content/browser/download/parallel_download_job.cc:18: // request.
It'll be quite unlikely that we'd find a number that works well for everyone. We
may need to consider dynamically ramping up or down depending on the overall
throughput we are seeing. That said, 2 is a good starting number.

https://codereview.chromium.org/2689373003/diff/20001/content/browser/downloa...
content/browser/download/parallel_download_job.cc:19: const int
kParallelRequestNumber = 2;
kParallelRequestCount or somesuch. Parallel request number implies it's an
identifier.

https://codereview.chromium.org/2689373003/diff/20001/content/browser/downloa...
content/browser/download/parallel_download_job.cc:54: int64_t total_bytes) {
This differs from the "list of slices" model that's landed for restoring
downloads from history. I.e. we should be able to deal with a download file that
has a bunch of subranges that have already been downloaded. This API assumes
that the portion to be downloaded is always at the end, which is not the case.

https://codereview.chromium.org/2689373003/diff/20001/content/browser/downloa...
content/browser/download/parallel_download_job.cc:61: int64_t piece_size =
std::max(bytes_left / request_num_, min_length_);
Let's use consistent terminology. Elsewhere we've used 'slice' to refer to a
part of a download file.

https://codereview.chromium.org/2689373003/diff/20001/content/browser/downloa...
content/browser/download/parallel_download_job.cc:64: // We may produce less
requests if (bytes_left / request_num_) is less than
s/less requests/fewer requests/

https://codereview.chromium.org/2689373003/diff/20001/content/browser/downloa...
content/browser/download/parallel_download_job.cc:93:
download_params->set_length(length);
Worth adding a comment here that the length parameter is the one thing that
causes this request to be treated as a slice.

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

https://codereview.chromium.org/2689373003/diff/20001/content/browser/downloa...
content/browser/download/parallel_download_job.h:25:
std::unique_ptr<DownloadRequestHandleInterface> request_handle);
explicit

[xingliu, 2017-02-16 19:12:33]

@asanka thank you for the review. Sorry I should write a doc for discussion
first. Doc is coming soon.

In high level, I agree DownloadFile eventually will be in DownloadJob or
DownloadUrlJob(naming will change), which maps to the current active download.
There might be other DownloadJob subclass that only needs a byte stream and
provide their own sink and state machine.

Most of the download process cascade methods are directly performing operation
on a DownloadFile and related to the internal state machine. So if DownloadFile
should live in a DownloadJob, I think it's reasonable to move those method and
also the state machine to DownloadJob subclass(DownloadUrlJob) as well. This is
also the original goal for DownloadJob.

For methods NOT directly operate on DownloadFile like DetermineDownloadTarget,
evetually it will call method in a callback chain to call DownloadFile
functions, so it's probably make sense to put them into DownloadJob subclass as
well. Or leave them in DownloadItemImpl, and expose methods on DownloadJob to
let later callbacks to access DownloadFile via DownloadJob. 

But in general, moving DownloadFile and the state machine are substantial
changes, probably we can do it in several CL.

In the end, ParallelDownloadJob will use the same internal state machine and
sink, so I think it can a subclass of DownloadUrlJob, which eventually owns the
DownloadFile and the state machine.

[xingliu, 2017-02-18 02:37:56]

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

[commit-bot: I haz the power, 2017-02-18 02:38:51]

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-02-18 04:40:14]

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

[commit-bot: I haz the power, 2017-02-18 04:40:15]

Dry run: Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
no build URL)

[xingliu, 2017-02-20 18:59:11]

Hi, thanks for the review. please help take another look.

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

https://codereview.chromium.org/2689373003/diff/20001/content/browser/downloa...
content/browser/download/download_item_impl.cc:375: is_paused_ = true;
On 2017/02/16 16:27:39, asanka wrote:
> Consider removing is_paused_ and having the job_ keep track of the paused
state.

Done. Makes sense.

https://codereview.chromium.org/2689373003/diff/20001/content/browser/downloa...
content/browser/download/download_item_impl.cc:1245: job_->Attach(this);
On 2017/02/16 16:27:39, asanka wrote:
> Why is there a separate Attach() method?
> 
> It would be reasonable to assume that the job would be bound to the same
> DownloadItemImpl for its entire lifetime, and that DownloadItemImpl will
outlive
> the job. This implies that we would be able associate a Job with a
> DownloadItemImpl at creation and leave it at that.

Done. Move to pass the item in ctor.

https://codereview.chromium.org/2689373003/diff/20001/content/browser/downloa...
content/browser/download/download_item_impl.cc:1249: void
DownloadItemImpl::InitializeDownloadFile() {
On 2017/02/16 16:27:39, asanka wrote:
> This isn't so much initializing the download file, but setting in motion the
> entire series of events that result in the file being downloaded.

Done, renamed to StartDownloadProgress.

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

https://codereview.chromium.org/2689373003/diff/20001/content/browser/downloa...
content/browser/download/download_job.cc:20:
download_item_->InitializeDownloadFile();
On 2017/02/16 16:27:39, asanka wrote:
> It would be great if we could have calls going one way. I.e. does it make
sense
> for DownloadItemImpl to retain ownership of the DownloadFile? Or should
> DownloadJob be the owner of the DownloadFile.
> 
> DownloadFile is only supposed to contain state for an active download, which
> maps to the lifetime of a Job.

Ideally the file should live with DownloadJob subclass, can we do it in a
separate CL, since this will involve a lot of refactoring.

Also I'm fine with adding the parallel logic in DownloadItemImpl, maybe adding
all these DownloadJob classes are over abstraction for parallel download, wdyt?

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

https://codereview.chromium.org/2689373003/diff/20001/content/browser/downloa...
content/browser/download/download_job_factory.cc:27: bool has_strong_validator =
On 2017/02/16 16:27:39, asanka wrote:
> Not the place to determine this.
> 
> See
>
https://cs.chromium.org/chromium/src/net/http/http_response_headers.cc?rcl=7b...
> 
> DownloadRequestCore has access to this information, and consequently etag and
> last_modified would only be non-empty if they are considered strong validators
> :-). There would be no point in keeping those around if they weren't.
> 
> Another option is to also check for Accept-Ranges and only store validators if
> they could be used for range requests. Once again, there's no point in keeping
> validators around for downloads if they couldn't be used for range requests.

Oh, I see. Thanks.

https://codereview.chromium.org/2689373003/diff/20001/content/browser/downloa...
content/browser/download/download_job_factory.cc:44: job =
base::MakeUnique<ParallelDownloadJob>(std::move(req_handle));
On 2017/02/16 16:27:39, asanka wrote:
> return base::MakeUnique<...>(...)
> 
> No need to store it in job.

Done.

https://codereview.chromium.org/2689373003/diff/20001/content/browser/downloa...
content/browser/download/download_job_factory.cc:49: job =
base::MakeUnique<DownloadUrlJob>(std::move(req_handle));
On 2017/02/16 16:27:39, asanka wrote:
> Let's rename DownloadUrlJob to something else so that the distinction between
> ParallelDownloadJob and DownloadUrlJob is clear.

Done. Renamed to DownloadJobImpl.

https://codereview.chromium.org/2689373003/diff/20001/content/browser/downloa...
File content/browser/download/download_url_job.h (right):

https://codereview.chromium.org/2689373003/diff/20001/content/browser/downloa...
content/browser/download/download_url_job.h:18: DownloadUrlJob(
On 2017/02/16 16:27:39, asanka wrote:
> explicit

Done, but omitted the explicit. Now we also pass DownloadItemImpl, so the ctor
has 2 parameters.

https://codereview.chromium.org/2689373003/diff/20001/content/browser/downloa...
File content/browser/download/download_url_task.h (right):

https://codereview.chromium.org/2689373003/diff/20001/content/browser/downloa...
content/browser/download/download_url_task.h:23: class DownloadUrlTask : public
UrlDownloader::Delegate {
On 2017/02/16 16:27:39, asanka wrote:
> This class name is pretty confusing given that we have a DownloadUrlJob /
> UrlDownloader / DownloadUrlTask/ etc..

Renamed to DownloadWorker in this patch.

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

https://codereview.chromium.org/2689373003/diff/20001/content/browser/downloa...
content/browser/download/parallel_download_job.cc:19: const int
kParallelRequestNumber = 2;
On 2017/02/16 16:27:39, asanka wrote:
> kParallelRequestCount or somesuch. Parallel request number implies it's an
> identifier.

Done.

https://codereview.chromium.org/2689373003/diff/20001/content/browser/downloa...
content/browser/download/parallel_download_job.cc:54: int64_t total_bytes) {
On 2017/02/16 16:27:39, asanka wrote:
> This differs from the "list of slices" model that's landed for restoring
> downloads from history. I.e. we should be able to deal with a download file
that
> has a bunch of subranges that have already been downloaded. This API assumes
> that the portion to be downloaded is always at the end, which is not the case.

Renamed the function name. Added some comment in the header.

This function is to arrange slices for a new download. |bytes_received| here is
from the original request. Also we didn't hook this to the history db to add new
records yet.

Resumption from history db will have another function unimplemented yet.

https://codereview.chromium.org/2689373003/diff/20001/content/browser/downloa...
content/browser/download/parallel_download_job.cc:61: int64_t piece_size =
std::max(bytes_left / request_num_, min_length_);
On 2017/02/16 16:27:39, asanka wrote:
> Let's use consistent terminology. Elsewhere we've used 'slice' to refer to a
> part of a download file.

Done.

https://codereview.chromium.org/2689373003/diff/20001/content/browser/downloa...
content/browser/download/parallel_download_job.cc:64: // We may produce less
requests if (bytes_left / request_num_) is less than
On 2017/02/16 16:27:39, asanka wrote:
> s/less requests/fewer requests/

Done.

https://codereview.chromium.org/2689373003/diff/20001/content/browser/downloa...
content/browser/download/parallel_download_job.cc:93:
download_params->set_length(length);
On 2017/02/16 16:27:39, asanka wrote:
> Worth adding a comment here that the length parameter is the one thing that
> causes this request to be treated as a slice.

Done.

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

https://codereview.chromium.org/2689373003/diff/20001/content/browser/downloa...
content/browser/download/parallel_download_job.h:25:
std::unique_ptr<DownloadRequestHandleInterface> request_handle);
On 2017/02/16 16:27:39, asanka wrote:
> explicit

Done. Same as DownloadUrlJob, now it also pass DownloadItemImpl.

[xingliu, 2017-02-20 18:59:44]

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

[commit-bot: I haz the power, 2017-02-20 18:59:52]

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-02-20 20:19:57]

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

[commit-bot: I haz the power, 2017-02-20 20:19:59]

Dry run: This issue passed the CQ dry run.

[qinmin, 2017-02-22 19:51:23]

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

https://codereview.chromium.org/2689373003/diff/60001/content/browser/downloa...
content/browser/download/download_item_impl.cc:399: job_->Resume();
job_ can be null here, right?

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

https://codereview.chromium.org/2689373003/diff/60001/content/browser/downloa...
content/browser/download/mock_download_job.h:21:
MockDownloadJob(DownloadItemImpl* download_item);
explicit

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

https://codereview.chromium.org/2689373003/diff/60001/content/browser/downloa...
content/browser/download/parallel_download_job.h:53: int64_t min_length_;
why is this a member variable? it feels like this is a constant, do you want to
configure this by finch?
Even so, if there are holes in the file, we might still request something less
than min_length_

[xingliu, 2017-02-22 21:20:53]

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

[commit-bot: I haz the power, 2017-02-22 21:22:01]

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

[xingliu, 2017-02-22 21:22:41]

Thanks for the feedback, PTAL.

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

https://codereview.chromium.org/2689373003/diff/60001/content/browser/downloa...
content/browser/download/download_item_impl.cc:399: job_->Resume();
On 2017/02/22 19:51:23, qinmin wrote:
> job_ can be null here, right?

Done, it should have returned in previous IsPause() check. But add a check is
probably better in case we remove the previous check.

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

https://codereview.chromium.org/2689373003/diff/60001/content/browser/downloa...
content/browser/download/mock_download_job.h:21:
MockDownloadJob(DownloadItemImpl* download_item);
On 2017/02/22 19:51:23, qinmin wrote:
> explicit

Done.

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

https://codereview.chromium.org/2689373003/diff/60001/content/browser/downloa...
content/browser/download/parallel_download_job.h:53: int64_t min_length_;
On 2017/02/22 19:51:23, qinmin wrote:
> why is this a member variable? it feels like this is a constant, do you want
to
> configure this by finch?
> Even so, if there are holes in the file, we might still request something less
> than min_length_

Done. Thanks for this suggestion.

Yeah, this one should be only used for new download and doesn't make too much
sense to do it here then. Removed this variable and only check the whole file
size in download_job_factory.cc.

[commit-bot: I haz the power, 2017-02-22 21:44:14]

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

[commit-bot: I haz the power, 2017-02-22 21:44:15]

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_...)

[xingliu, 2017-02-22 22:35:14]

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

[commit-bot: I haz the power, 2017-02-22 22:36:13]

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-02-23 01:33:37]

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

[commit-bot: I haz the power, 2017-02-23 01:33:38]

Dry run: This issue passed the CQ dry run.

[qinmin, 2017-02-23 07:26:57]

overall looks good, more comments

https://codereview.chromium.org/2689373003/diff/100001/content/browser/downlo...
File content/browser/download/download_item_impl.cc (right):

https://codereview.chromium.org/2689373003/diff/100001/content/browser/downlo...
content/browser/download/download_item_impl.cc:1237: DCHECK(job_.get());
nit: DCHECK(job_);

https://codereview.chromium.org/2689373003/diff/100001/content/browser/downlo...
content/browser/download/download_item_impl.cc:1644: if (job_.get())
nit: if (job_)

https://codereview.chromium.org/2689373003/diff/100001/content/browser/downlo...
content/browser/download/download_item_impl.cc:1959: job_->set_is_paused(false);
shouldn't this be calling job_->Resume()?

https://codereview.chromium.org/2689373003/diff/100001/content/browser/downlo...
File content/browser/download/download_item_impl.h (right):

https://codereview.chromium.org/2689373003/diff/100001/content/browser/downlo...
content/browser/download/download_item_impl.h:398: void StartDownloadProgress();
nit: StartDownloadProgress() doesn't sounds like a commonly used phrase. Either
StartDownload(), or StartDownloadProcess() or StartFileDownload() sounds much
better.

https://codereview.chromium.org/2689373003/diff/100001/content/browser/downlo...
File content/browser/download/download_job.h (right):

https://codereview.chromium.org/2689373003/diff/100001/content/browser/downlo...
content/browser/download/download_job.h:24: DownloadJob(DownloadItemImpl*
download_item);
explicit

https://codereview.chromium.org/2689373003/diff/100001/content/browser/downlo...
content/browser/download/download_job.h:34: void set_is_paused(bool is_paused) {
is_paused_ = is_paused; }
this is strange.  shouldn't the caller use either Pause() or Resume() to change
the state of is_paused_?

https://codereview.chromium.org/2689373003/diff/100001/content/browser/downlo...
content/browser/download/download_job.h:50: bool is_paused_ = false;
the assignment is already in initializer list

https://codereview.chromium.org/2689373003/diff/100001/content/browser/downlo...
File content/browser/download/download_job_factory.cc (right):

https://codereview.chromium.org/2689373003/diff/100001/content/browser/downlo...
content/browser/download/download_job_factory.cc:21: const int64_t
kMinFileSizeParallelDownload = 2048;
Bytes or KB? 2048 bytes is too small to kick off parallel downloading, 2048 KB
sounds more reasonable

https://codereview.chromium.org/2689373003/diff/100001/content/browser/downlo...
content/browser/download/download_job_factory.cc:47:
std::unique_ptr<DownloadJob> job;
seems this is not needed

https://codereview.chromium.org/2689373003/diff/100001/content/browser/downlo...
File content/browser/download/download_job_factory.h (right):

https://codereview.chromium.org/2689373003/diff/100001/content/browser/downlo...
content/browser/download/download_job_factory.h:26: };
DISALLOW_COPY_AND_ASSIGN(DownloadJobFactory)

[xingliu, 2017-02-24 23:37:02]

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

[commit-bot: I haz the power, 2017-02-24 23:37:45]

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

[xingliu, 2017-02-24 23:43:13]

Thanks for the feedback, PTAL.

https://codereview.chromium.org/2689373003/diff/100001/content/browser/downlo...
File content/browser/download/download_item_impl.cc (right):

https://codereview.chromium.org/2689373003/diff/100001/content/browser/downlo...
content/browser/download/download_item_impl.cc:1237: DCHECK(job_.get());
On 2017/02/23 07:26:56, qinmin wrote:
> nit: DCHECK(job_);

Done.

https://codereview.chromium.org/2689373003/diff/100001/content/browser/downlo...
content/browser/download/download_item_impl.cc:1644: if (job_.get())
On 2017/02/23 07:26:57, qinmin wrote:
> nit: if (job_) 

Done.

https://codereview.chromium.org/2689373003/diff/100001/content/browser/downlo...
content/browser/download/download_item_impl.cc:1959: job_->set_is_paused(false);
On 2017/02/23 07:26:57, qinmin wrote:
> shouldn't this be calling job_->Resume()?

Done, not sure if this is totally correct.

job_ may contains a living request, then job_->Resume may resume the request and
continue to pull some data and set is_pause to false.

The previous call will asychronously swap the item with a new job which contains
a new request handle by eventually calling DownloadItemImpl::Start.

So probably the old request will continue to download some data and be killed
when the previous call callback to UI thread, and new request will download
those data again.

https://codereview.chromium.org/2689373003/diff/100001/content/browser/downlo...
File content/browser/download/download_item_impl.h (right):

https://codereview.chromium.org/2689373003/diff/100001/content/browser/downlo...
content/browser/download/download_item_impl.h:398: void StartDownloadProgress();
On 2017/02/23 07:26:57, qinmin wrote:
> nit: StartDownloadProgress() doesn't sounds like a commonly used phrase.
Either
> StartDownload(), or StartDownloadProcess() or StartFileDownload() sounds much
> better.

Done, changed to StartDownload.

https://codereview.chromium.org/2689373003/diff/100001/content/browser/downlo...
File content/browser/download/download_job.h (right):

https://codereview.chromium.org/2689373003/diff/100001/content/browser/downlo...
content/browser/download/download_job.h:24: DownloadJob(DownloadItemImpl*
download_item);
On 2017/02/23 07:26:57, qinmin wrote:
> explicit

Done.

https://codereview.chromium.org/2689373003/diff/100001/content/browser/downlo...
content/browser/download/download_job.h:34: void set_is_paused(bool is_paused) {
is_paused_ = is_paused; }
On 2017/02/23 07:26:57, qinmin wrote:
> this is strange.  shouldn't the caller use either Pause() or Resume() to
change
> the state of is_paused_?

Removed.

https://codereview.chromium.org/2689373003/diff/100001/content/browser/downlo...
content/browser/download/download_job.h:50: bool is_paused_ = false;
On 2017/02/23 07:26:57, qinmin wrote:
> the assignment is already in initializer list

Done.

https://codereview.chromium.org/2689373003/diff/100001/content/browser/downlo...
File content/browser/download/download_job_factory.cc (right):

https://codereview.chromium.org/2689373003/diff/100001/content/browser/downlo...
content/browser/download/download_job_factory.cc:21: const int64_t
kMinFileSizeParallelDownload = 2048;
On 2017/02/23 07:26:57, qinmin wrote:
> Bytes or KB? 2048 bytes is too small to kick off parallel downloading, 2048 KB
> sounds more reasonable

Done, yeah, make sense.

https://codereview.chromium.org/2689373003/diff/100001/content/browser/downlo...
content/browser/download/download_job_factory.cc:47:
std::unique_ptr<DownloadJob> job;
On 2017/02/23 07:26:57, qinmin wrote:
> seems this is not needed

Done. Sorry I should change it in the previous patch.

https://codereview.chromium.org/2689373003/diff/100001/content/browser/downlo...
File content/browser/download/download_job_factory.h (right):

https://codereview.chromium.org/2689373003/diff/100001/content/browser/downlo...
content/browser/download/download_job_factory.h:26: };
On 2017/02/23 07:26:57, qinmin wrote:
> DISALLOW_COPY_AND_ASSIGN(DownloadJobFactory)

Done.

[xingliu, 2017-02-25 00:12:13]

More discussion on is_pause.

https://codereview.chromium.org/2689373003/diff/100001/content/browser/downlo...
File content/browser/download/download_item_impl.cc (right):

https://codereview.chromium.org/2689373003/diff/100001/content/browser/downlo...
content/browser/download/download_item_impl.cc:1959: job_->set_is_paused(false);
On 2017/02/24 23:43:13, xingliu wrote:
> On 2017/02/23 07:26:57, qinmin wrote:
> > shouldn't this be calling job_->Resume()?
> 
> Done, not sure if this is totally correct.
> 
> job_ may contains a living request, then job_->Resume may resume the request
and
> continue to pull some data and set is_pause to false.
> 
> The previous call will asychronously swap the item with a new job which
contains
> a new request handle by eventually calling DownloadItemImpl::Start.
> 
> So probably the old request will continue to download some data and be killed
> when the previous call callback to UI thread, and new request will download
> those data again.

Maybe we should just remove this call, since the previous call will swap a new
job_ with is_pause set to initial value, false.

The time between this line and a new job_ coming, the state will always be
RESUMING_INTERNAL, it doesn't use is_pause in the existing code. So we probably
don't need to worry about this.

[commit-bot: I haz the power, 2017-02-25 00:16:19]

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

[commit-bot: I haz the power, 2017-02-25 00:16:20]

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...)

[xingliu, 2017-02-25 00:47:31]

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

[commit-bot: I haz the power, 2017-02-25 00:47:43]

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-02-25 00:49:19]

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

[commit-bot: I haz the power, 2017-02-25 00:49:20]

Dry run: Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  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_...)

[xingliu, 2017-02-25 01:29:27]

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

[commit-bot: I haz the power, 2017-02-25 01:29:41]

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-02-25 01:32:25]

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

[commit-bot: I haz the power, 2017-02-25 01:32:26]

Dry run: Try jobs failed on following builders:
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[qinmin, 2017-02-27 17:15:46]

https://codereview.chromium.org/2689373003/diff/100001/content/browser/downlo...
File content/browser/download/download_item_impl.cc (right):

https://codereview.chromium.org/2689373003/diff/100001/content/browser/downlo...
content/browser/download/download_item_impl.cc:1959: job_->set_is_paused(false);
On 2017/02/25 00:12:13, xingliu wrote:
> On 2017/02/24 23:43:13, xingliu wrote:
> > On 2017/02/23 07:26:57, qinmin wrote:
> > > shouldn't this be calling job_->Resume()?
> > 
> > Done, not sure if this is totally correct.
> > 
> > job_ may contains a living request, then job_->Resume may resume the request
> and
> > continue to pull some data and set is_pause to false.
> > 
> > The previous call will asychronously swap the item with a new job which
> contains
> > a new request handle by eventually calling DownloadItemImpl::Start.
> > 
> > So probably the old request will continue to download some data and be
killed
> > when the previous call callback to UI thread, and new request will download
> > those data again.
> 
> Maybe we should just remove this call, since the previous call will swap a new
> job_ with is_pause set to initial value, false.
> 
> The time between this line and a new job_ coming, the state will always be
> RESUMING_INTERNAL, it doesn't use is_pause in the existing code. So we
probably
> don't need to worry about this.

I would rather change the DownloadJob::Resume() call to take a boolean param,
something like start_new_request, so that we don't remove the logic here.

[xingliu, 2017-02-27 18:11:52]

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

[commit-bot: I haz the power, 2017-02-27 18:12:23]

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

[xingliu, 2017-02-27 19:16:22]

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

[commit-bot: I haz the power, 2017-02-27 19:17:03]

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-02-27 20:37:18]

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

[commit-bot: I haz the power, 2017-02-27 20:37:19]

Dry run: This issue passed the CQ dry run.

[xingliu, 2017-02-27 20:44:52]

PHTAL, please diff with patch 9, which is a clean rebase on master without code
changes.

The only change here is the Resume function parameter.

https://codereview.chromium.org/2689373003/diff/100001/content/browser/downlo...
File content/browser/download/download_item_impl.cc (right):

https://codereview.chromium.org/2689373003/diff/100001/content/browser/downlo...
content/browser/download/download_item_impl.cc:1959: job_->set_is_paused(false);
On 2017/02/27 17:15:46, qinmin wrote:
> On 2017/02/25 00:12:13, xingliu wrote:
> > On 2017/02/24 23:43:13, xingliu wrote:
> > > On 2017/02/23 07:26:57, qinmin wrote:
> > > > shouldn't this be calling job_->Resume()?
> > > 
> > > Done, not sure if this is totally correct.
> > > 
> > > job_ may contains a living request, then job_->Resume may resume the
request
> > and
> > > continue to pull some data and set is_pause to false.
> > > 
> > > The previous call will asychronously swap the item with a new job which
> > contains
> > > a new request handle by eventually calling DownloadItemImpl::Start.
> > > 
> > > So probably the old request will continue to download some data and be
> killed
> > > when the previous call callback to UI thread, and new request will
download
> > > those data again.
> > 
> > Maybe we should just remove this call, since the previous call will swap a
new
> > job_ with is_pause set to initial value, false.
> > 
> > The time between this line and a new job_ coming, the state will always be
> > RESUMING_INTERNAL, it doesn't use is_pause in the existing code. So we
> probably
> > don't need to worry about this.
> 
> I would rather change the DownloadJob::Resume() call to take a boolean param,
> something like start_new_request, so that we don't remove the logic here.

Done, now job_->Resume(false) will set is_paused to false without resuming the
request.

[qinmin, 2017-02-27 21:54:46]

lgtm % nits

https://codereview.chromium.org/2689373003/diff/180001/content/browser/downlo...
File content/browser/download/download_job_factory.cc (right):

https://codereview.chromium.org/2689373003/diff/180001/content/browser/downlo...
content/browser/download/download_job_factory.cc:21: const int64_t
kMinFileSizeParallelDownload = 2097152;
nit: make this finch configurable, or add a TODO for that. We need to study the
UMA on how long it tooks to download a file of a particular size. If it is too
short, user won't notice any changes in download speed.

https://codereview.chromium.org/2689373003/diff/180001/content/browser/downlo...
File content/browser/download/download_worker.h (right):

https://codereview.chromium.org/2689373003/diff/180001/content/browser/downlo...
content/browser/download/download_worker.h:16: #include
"content/public/browser/download_interrupt_reasons.h"
nit: still need this here? or only the cc file?

https://codereview.chromium.org/2689373003/diff/180001/content/browser/downlo...
File content/browser/download/parallel_download_job.h (right):

https://codereview.chromium.org/2689373003/diff/180001/content/browser/downlo...
content/browser/download/parallel_download_job.h:23: typedef
std::vector<std::unique_ptr<DownloadWorker>> WorkerList;
nit: make it private

[xingliu, 2017-02-27 22:45:59]

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

[xingliu, 2017-02-27 22:46:00]

Hi, Alex, PTAL BUILD.gn, thanks.

alexmos@chromium.org: Please review changes in 
content/browser/BUILD.gn.

[alexmos, 2017-02-27 23:12:15]

content/browser/BUILD.gn LGTM

[xingliu, 2017-02-27 23:53:26]

Thanks for the review, some minor polish, and start to put it to CQ.

https://codereview.chromium.org/2689373003/diff/180001/content/browser/downlo...
File content/browser/download/download_job_factory.cc (right):

https://codereview.chromium.org/2689373003/diff/180001/content/browser/downlo...
content/browser/download/download_job_factory.cc:21: const int64_t
kMinFileSizeParallelDownload = 2097152;
On 2017/02/27 21:54:46, qinmin wrote:
> nit: make this finch configurable, or add a TODO for that. We need to study
the
> UMA on how long it tooks to download a file of a particular size. If it is too
> short, user won't notice any changes in download speed.

Done.

https://codereview.chromium.org/2689373003/diff/180001/content/browser/downlo...
File content/browser/download/download_worker.h (right):

https://codereview.chromium.org/2689373003/diff/180001/content/browser/downlo...
content/browser/download/download_worker.h:16: #include
"content/public/browser/download_interrupt_reasons.h"
On 2017/02/27 21:54:46, qinmin wrote:
> nit: still need this here? or only the cc file?

Done.

https://codereview.chromium.org/2689373003/diff/180001/content/browser/downlo...
File content/browser/download/parallel_download_job.h (right):

https://codereview.chromium.org/2689373003/diff/180001/content/browser/downlo...
content/browser/download/parallel_download_job.h:23: typedef
std::vector<std::unique_ptr<DownloadWorker>> WorkerList;
On 2017/02/27 21:54:46, qinmin wrote:
> nit: make it private

Done.

[xingliu, 2017-02-28 00:53:33]

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

[commit-bot: I haz the power, 2017-02-28 00:54:16]

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-02-28 02:16:14]

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

[commit-bot: I haz the power, 2017-02-28 02:16:15]

Dry run: This issue passed the CQ dry run.

[xingliu, 2017-02-28 02:51:02]

The CQ bit was checked by xingliu@chromium.org

[xingliu, 2017-02-28 02:51:03]

The patchset sent to the CQ was uploaded after l-g-t-m from qinmin@chromium.org,
alexmos@chromium.org
Link to the patchset: https://codereview.chromium.org/2689373003/#ps200001
(title: "nits.")

[commit-bot: I haz the power, 2017-02-28 02:51:38]

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-02-28 02:59:31]

CQ is committing da patch.

Bot data: {"patchset_id": 200001, "attempt_start_ts": 1488250262753170,
"parent_rev": "d492a16c9bbbe163d48cb0b2976304787f21c5d1", "commit_rev":
"468824d8e6ce6def7e144e9a72ae706f4b0b1b2b"}

[commit-bot: I haz the power, 2017-02-28 03:00:06]

Description was changed from

==========
Introduce ParallelDownloadJob.

1. Add ParallelDownloadJob and DownloadUrlJob, both inherits DownloadJob
owned by DownloadItemImpl, currently we only do necessary refactoring
for parallel download feature, and failure handling or wiring to file
IO logic is not implemented yet.

2. Modify DownloadJob, remove any function we don't need. Since we need
to access many things in DownloadItemImpl, now it cache a raw pointer,
and is a friend class of DownloadItemImpl.

3. ParallelDownloadJob holds a list of DownloadUrlTask, which uses
UrlDownloader to send requests.

4. Tweak for UrlDownloader, previously UrlDownloader is coupled with
DownloadManagerImpl and everything has to go through
DownloadManagerImpl, now it uses a weak pointer delegate to communicate
to its owner, i.e. DownloadManagerImpl or DownloadUrlTask.

5. Pull out MockDownloadItemImpl to a seperate source file.

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

to

==========
Introduce ParallelDownloadJob.

1. Add ParallelDownloadJob and DownloadUrlJob, both inherits DownloadJob
owned by DownloadItemImpl, currently we only do necessary refactoring
for parallel download feature, and failure handling or wiring to file
IO logic is not implemented yet.

2. Modify DownloadJob, remove any function we don't need. Since we need
to access many things in DownloadItemImpl, now it cache a raw pointer,
and is a friend class of DownloadItemImpl.

3. ParallelDownloadJob holds a list of DownloadUrlTask, which uses
UrlDownloader to send requests.

4. Tweak for UrlDownloader, previously UrlDownloader is coupled with
DownloadManagerImpl and everything has to go through
DownloadManagerImpl, now it uses a weak pointer delegate to communicate
to its owner, i.e. DownloadManagerImpl or DownloadUrlTask.

5. Pull out MockDownloadItemImpl to a seperate source file.

BUG=644352

Review-Url: https://codereview.chromium.org/2689373003
Cr-Commit-Position: refs/heads/master@{#453478}
Committed:
https://chromium.googlesource.com/chromium/src/+/468824d8e6ce6def7e144e9a72ae...
==========

[commit-bot: I haz the power, 2017-02-28 03:00:09]

Committed patchset #11 (id:200001) as
https://chromium.googlesource.com/chromium/src/+/468824d8e6ce6def7e144e9a72ae...