Introduce DownloadJob for DownloadItem refactoring.

Introduce DownloadJob for DownloadItem refactoring.

1. DownloadJob is to delegate actual download logic from
DownloadItemImpl. Also it can be used
for parallel download feature, which is a DownloadJob manages
list of smaller tasks(pieces of parallel download segments).

2. DownloadJob base class provides an content internal interface.
The public interface of DownloadItem is intact.

3. Add unit test and mock structure for DownloadJob.

This CL only checks in the interface. Pulling out
DownloadItemImpl logic and subclass of DownloadJob will come
in later CL.

BUG=682350
Review-Url: https://codereview.chromium.org/2645133003
Cr-Commit-Position: refs/heads/master@{#446806}
Committed: https://chromium.googlesource.com/chromium/src/+/f61d39f8a151e11949079994228823ca9720cba1

[xingliu, 2017-01-20 18:52:44]

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

[xingliu, 2017-01-20 18:52:45]

The DownloadJob interface, please help take a look.

[xingliu, 2017-01-20 18:54:33]

Description was changed from

==========
Introduce DownloadJob for DownloadItem refactoring.

1. DownloadJob is to split the normal download and save page
download logic from DownloadItemImpl.

2. DownloadJob base class provides an content internal interface.
The public interface of DownloadItem is intact.

3. Add unit test and mock structure for DownloadJob.

This CL only checks in the interface. Pulling out
DownloadItemImpl logic and subclasses of DownloadJob will come
in later CL.

BUG=682350
==========

to

==========
Introduce DownloadJob for DownloadItem refactoring.

1. DownloadJob is to split the normal download and save page
download logic from DownloadItemImpl. Also it can be used
for parallel download feature, which is a DownloadJob manages
list of DownloadJobs(pieces of parallel download segments).

2. DownloadJob base class provides an content internal interface.
The public interface of DownloadItem is intact.

3. Add unit test and mock structure for DownloadJob.

This CL only checks in the interface. Pulling out
DownloadItemImpl logic and subclasses of DownloadJob will come
in later CL.

BUG=682350
==========

[xingliu, 2017-01-20 19:06:52]

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

[commit-bot: I haz the power, 2017-01-20 19:07:18]

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-01-20 20:29:05]

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

[commit-bot: I haz the power, 2017-01-20 20:29:06]

Dry run: This issue passed the CQ dry run.

[xingliu, 2017-01-23 18:37:52]

Cc to khushal, DownloadJob probably have 4 subclasses evetually,

DownloadJobImpl, SavePageJob, ParallelDownloadJob, PwaDownloadJob.

[qinmin, 2017-01-23 21:46:35]

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

https://codereview.chromium.org/2645133003/diff/20001/content/browser/downloa...
content/browser/download/download_job.h:45: virtual void
StartSaving(DownloadJob* download_job) = 0;
Shouldn't the manager just call download_job_->StartSaving()? what is the
purpose for the passed in param?

same below

[xingliu, 2017-01-23 22:59:09]

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

https://codereview.chromium.org/2645133003/diff/20001/content/browser/downloa...
content/browser/download/download_job.h:45: virtual void
StartSaving(DownloadJob* download_job) = 0;
On 2017/01/23 21:46:35, qinmin wrote:
> Shouldn't the manager just call download_job_->StartSaving()? what is the
> purpose for the passed in param?
> 
> same below

These are the callbacks to DownloadItemImpl which implements
DownloadJob::Manager, as something happens in DownloadJob, like the http
response is processed, it notifies DownloadItemImpl.

In my understanding, DownloadItemImpl eventually only know external state:
DownloadState, and meta data. Internal states should be in DownloadJobs.

[qinmin, 2017-01-23 23:03:15]

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

https://codereview.chromium.org/2645133003/diff/20001/content/browser/downloa...
content/browser/download/download_job.h:45: virtual void
StartSaving(DownloadJob* download_job) = 0;
On 2017/01/23 22:59:09, xingliu wrote:
> On 2017/01/23 21:46:35, qinmin wrote:
> > Shouldn't the manager just call download_job_->StartSaving()? what is the
> > purpose for the passed in param?
> > 
> > same below
> 
> These are the callbacks to DownloadItemImpl which implements
> DownloadJob::Manager, as something happens in DownloadJob, like the http
> response is processed, it notifies DownloadItemImpl.
> 
> In my understanding, DownloadItemImpl eventually only know external state:
> DownloadState, and meta data. Internal states should be in DownloadJobs.

Then shouldn't the function name be OnSavingStarted or StartedSaving?

[xingliu, 2017-01-23 23:26:08]

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

https://codereview.chromium.org/2645133003/diff/20001/content/browser/downloa...
content/browser/download/download_job.h:45: virtual void
StartSaving(DownloadJob* download_job) = 0;
On 2017/01/23 23:03:15, qinmin wrote:
> On 2017/01/23 22:59:09, xingliu wrote:
> > On 2017/01/23 21:46:35, qinmin wrote:
> > > Shouldn't the manager just call download_job_->StartSaving()? what is the
> > > purpose for the passed in param?
> > > 
> > > same below
> > 
> > These are the callbacks to DownloadItemImpl which implements
> > DownloadJob::Manager, as something happens in DownloadJob, like the http
> > response is processed, it notifies DownloadItemImpl.
> > 
> > In my understanding, DownloadItemImpl eventually only know external state:
> > DownloadState, and meta data. Internal states should be in DownloadJobs.
> 
> Then shouldn't the function name be OnSavingStarted or StartedSaving? 

Done, changed to OnSavingStarted aligned with other functions here.

[xingliu, 2017-01-24 22:09:00]

Ping~

[xingliu, 2017-01-24 22:09:09]

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

[commit-bot: I haz the power, 2017-01-24 22:09:51]

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

[xingliu, 2017-01-24 22:14:47]

Description was changed from

==========
Introduce DownloadJob for DownloadItem refactoring.

1. DownloadJob is to split the normal download and save page
download logic from DownloadItemImpl. Also it can be used
for parallel download feature, which is a DownloadJob manages
list of DownloadJobs(pieces of parallel download segments).

2. DownloadJob base class provides an content internal interface.
The public interface of DownloadItem is intact.

3. Add unit test and mock structure for DownloadJob.

This CL only checks in the interface. Pulling out
DownloadItemImpl logic and subclasses of DownloadJob will come
in later CL.

BUG=682350
==========

to

==========
Introduce DownloadJob for DownloadItem refactoring.

1. DownloadJob is to delegate actual download logic from
DownloadItemImpl. Also it can be used
for parallel download feature, which is a DownloadJob manages
list of smaller tasks(pieces of parallel download segments).

2. DownloadJob base class provides an content internal interface.
The public interface of DownloadItem is intact.

3. Add unit test and mock structure for DownloadJob.

This CL only checks in the interface. Pulling out
DownloadItemImpl logic and subclass of DownloadJob will come
in later CL.

BUG=682350
==========

[commit-bot: I haz the power, 2017-01-24 23:36:10]

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

[commit-bot: I haz the power, 2017-01-24 23:36:10]

Dry run: This issue passed the CQ dry run.

[qinmin, 2017-01-24 23:57:49]

lgtm % comments

https://codereview.chromium.org/2645133003/diff/80001/content/browser/downloa...
File content/browser/download/download_job.h (right):

https://codereview.chromium.org/2645133003/diff/80001/content/browser/downloa...
content/browser/download/download_job.h:31: URL_DOWNLOAD = 0,   // Normal
download.
this can be removed now

https://codereview.chromium.org/2645133003/diff/80001/content/browser/downloa...
content/browser/download/download_job.h:39: virtual void
OnSavingStarted(DownloadJob* download_job) = 0;
should there be a responseInfo param?

[xingliu, 2017-01-25 01:18:07]

https://codereview.chromium.org/2645133003/diff/80001/content/browser/downloa...
File content/browser/download/download_job.h (right):

https://codereview.chromium.org/2645133003/diff/80001/content/browser/downloa...
content/browser/download/download_job.h:31: URL_DOWNLOAD = 0,   // Normal
download.
On 2017/01/24 23:57:49, qinmin wrote:
> this can be removed now

Done.

https://codereview.chromium.org/2645133003/diff/80001/content/browser/downloa...
content/browser/download/download_job.h:39: virtual void
OnSavingStarted(DownloadJob* download_job) = 0;
On 2017/01/24 23:57:49, qinmin wrote:
> should there be a responseInfo param?

In the current DownloadItemImpl code, the ResponseInfo fields are passed into
the ctor of DownloadItemImpl, since the item is created after handled the http
response or read from a history db. I think it makes more sense than passing the
info here.

[David Trainor- moved to gerrit, 2017-01-25 03:46:09]

lgtm % nits

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

https://codereview.chromium.org/2645133003/diff/100001/content/browser/downlo...
content/browser/download/download_job.h:63: // See |DownloadItem::
CanOpenDownload|.
remove space after ::

https://codereview.chromium.org/2645133003/diff/100001/content/browser/downlo...
content/browser/download/download_job.h:76: virtual bool IsPause() const = 0;
s/Pause/Paused?

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

https://codereview.chromium.org/2645133003/diff/100001/content/browser/downlo...
content/browser/download/mock_download_job.cc:9:
MockDownloadJob::MockDownloadJob() {}
= default?  Or does this not work because of the mocks?

[asanka, 2017-01-27 01:45:49]

lgtm % nits.

We'll need to move this out to //content/public at some point, but we can keep
it here until we iron out the API.

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

https://codereview.chromium.org/2645133003/diff/120001/content/browser/downlo...
content/browser/download/download_job.h:30: class CONTENT_EXPORT Manager {
Customary to call this Delegate.

[xingliu, 2017-01-27 18:33:47]

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

[xingliu, 2017-01-27 18:33:48]

@alexmos, Hi, PHTAL for 

content/browser/BUILD.gn.

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

https://codereview.chromium.org/2645133003/diff/100001/content/browser/downlo...
content/browser/download/mock_download_job.cc:9:
MockDownloadJob::MockDownloadJob() {}
On 2017/01/25 03:46:09, David Trainor wrote:
> = default?  Or does this not work because of the mocks?

Done.

[alexmos, 2017-01-27 18:38:16]

content/browser/BUILD.gn LGTM

[xingliu, 2017-01-27 18:50:58]

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

[commit-bot: I haz the power, 2017-01-27 18:51:32]

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-01-27 20:33:47]

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

[commit-bot: I haz the power, 2017-01-27 20:33:48]

Dry run: This issue passed the CQ dry run.

[xingliu, 2017-01-27 21:56:07]

The CQ bit was checked by xingliu@chromium.org

[xingliu, 2017-01-27 21:56:07]

The patchset sent to the CQ was uploaded after l-g-t-m from qinmin@chromium.org,
dtrainor@chromium.org, asanka@chromium.org
Link to the patchset: https://codereview.chromium.org/2645133003/#ps140001
(title: "Conditionally call to delegate.")

[commit-bot: I haz the power, 2017-01-27 21:56:57]

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-01-27 22:46:51]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1485554167051370,
"parent_rev": "ee272d6c846e19f2019bf8a0ba09316c1d561469", "commit_rev":
"f61d39f8a151e11949079994228823ca9720cba1"}

[commit-bot: I haz the power, 2017-01-27 22:47:31]

Description was changed from

==========
Introduce DownloadJob for DownloadItem refactoring.

1. DownloadJob is to delegate actual download logic from
DownloadItemImpl. Also it can be used
for parallel download feature, which is a DownloadJob manages
list of smaller tasks(pieces of parallel download segments).

2. DownloadJob base class provides an content internal interface.
The public interface of DownloadItem is intact.

3. Add unit test and mock structure for DownloadJob.

This CL only checks in the interface. Pulling out
DownloadItemImpl logic and subclass of DownloadJob will come
in later CL.

BUG=682350
==========

to

==========
Introduce DownloadJob for DownloadItem refactoring.

1. DownloadJob is to delegate actual download logic from
DownloadItemImpl. Also it can be used
for parallel download feature, which is a DownloadJob manages
list of smaller tasks(pieces of parallel download segments).

2. DownloadJob base class provides an content internal interface.
The public interface of DownloadItem is intact.

3. Add unit test and mock structure for DownloadJob.

This CL only checks in the interface. Pulling out
DownloadItemImpl logic and subclass of DownloadJob will come
in later CL.

BUG=682350

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

[commit-bot: I haz the power, 2017-01-27 22:47:32]

Committed patchset #8 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/f61d39f8a151e119490799942288...