Fix an issue that we didn't clean url request properly.

Fix an issue that we didn't clean url request properly.

The ByteStreamReader we used in DownloadFile does not support Close()
api, so we need to close the url request on UI thread if
"ByteStreamReader::Close" should be called.

Before we implement parallel download, we null out the callback on
ByteStreamReader during download interruption, and on UI thread, we
cancel the url request. But for parallel download, at some point, we
won't interrupt the download but need to cancel a particular request.

There is another CL that implements a browser test for parallel download and it will test the code here.
https://codereview.chromium.org/2819483002/

BUG=710576, 644352
Review-Url: https://codereview.chromium.org/2811293004
Cr-Commit-Position: refs/heads/master@{#465289}
Committed: https://chromium.googlesource.com/chromium/src/+/b444a98187571cf9ec627ac52fee7e81ffcecb93

[xingliu, 2017-04-13 03:50:16]

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

[commit-bot: I haz the power, 2017-04-13 03:50:59]

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

[xingliu, 2017-04-13 03:51:58]

Description was changed from

==========
Fix an issue that we didn't clean url request properly.

The ByteStreamReader we used in DownloadFile does not support Close()
api, so we need to close the url request on UI thread if
"ByteStreamReader::Close" should be called.

Before we implement parallel download, we null out the callback on
ByteStreamReader during download interruption, and on UI thread, we
cancel the url request. But for parallel download, at some point, we
won't interrupt the download but need to cancel a particular request.


BUG=710576, 644352
==========

to

==========
Fix an issue that we didn't clean url request properly.

The ByteStreamReader we used in DownloadFile does not support Close()
api, so we need to close the url request on UI thread if
"ByteStreamReader::Close" should be called.

Before we implement parallel download, we null out the callback on
ByteStreamReader during download interruption, and on UI thread, we
cancel the url request. But for parallel download, at some point, we
won't interrupt the download but need to cancel a particular request.

There is another CL that implements a browser test for parallel download and it
will test the code here.
https://codereview.chromium.org/2819483002/

BUG=710576, 644352
==========

[xingliu, 2017-04-13 03:51:59]

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

[xingliu, 2017-04-13 03:52:18]

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

[xingliu, 2017-04-13 03:52:19]

Hi, PTAL, thanks.

[commit-bot: I haz the power, 2017-04-13 04:01:42]

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

[commit-bot: I haz the power, 2017-04-13 04:01:43]

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...)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[xingliu, 2017-04-13 21:14:50]

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

[commit-bot: I haz the power, 2017-04-13 21:15:28]

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

[commit-bot: I haz the power, 2017-04-13 23:21:36]

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

[commit-bot: I haz the power, 2017-04-13 23:21:37]

Dry run: This issue passed the CQ dry run.

[qinmin, 2017-04-14 20:04:44]

Overall looks ok. 
But just one question, will it be better to a callback to DownloadFileImpl and
let DownloadFileImpl to execute that callback on UI thread to cancel the
request?

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

https://codereview.chromium.org/2811293004/diff/20001/content/browser/downloa...
content/browser/download/download_file_impl.cc:82: length_ = kNoBytesToWrite;
the stream_to_close can be added here rather than waiting for the StreamActive
call

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

https://codereview.chromium.org/2811293004/diff/20001/content/browser/downloa...
content/browser/download/download_item_impl.h:237: const
std::unordered_set<int64_t>& stream_to_close) override;
I would use finished_stream_offsets instead of stream_to_close for 2 reasons:
1. the param is offset, not streams.
2. these streams are considered finished.

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

https://codereview.chromium.org/2811293004/diff/20001/content/browser/downloa...
content/browser/download/download_job.h:48: virtual void CancelRequest(int64_t
offset);
nit:s/CancelRequest/CancelRequestWithOffset/

[xingliu, 2017-04-14 21:31:02]

On 2017/04/14 20:04:44, qinmin wrote:
> Overall looks ok. 
> But just one question, will it be better to a callback to DownloadFileImpl and
> let DownloadFileImpl to execute that callback on UI thread to cancel the
> request?
> 
>
https://codereview.chromium.org/2811293004/diff/20001/content/browser/downloa...
> File content/browser/download/download_file_impl.cc (right):
> 
>
https://codereview.chromium.org/2811293004/diff/20001/content/browser/downloa...
> content/browser/download/download_file_impl.cc:82: length_ = kNoBytesToWrite;
> the stream_to_close can be added here rather than waiting for the StreamActive
> call
> 
>
https://codereview.chromium.org/2811293004/diff/20001/content/browser/downloa...
> File content/browser/download/download_item_impl.h (right):
> 
>
https://codereview.chromium.org/2811293004/diff/20001/content/browser/downloa...
> content/browser/download/download_item_impl.h:237: const
> std::unordered_set<int64_t>& stream_to_close) override;
> I would use finished_stream_offsets instead of stream_to_close for 2 reasons:
> 1. the param is offset, not streams.
> 2. these streams are considered finished.
> 
>
https://codereview.chromium.org/2811293004/diff/20001/content/browser/downloa...
> File content/browser/download/download_job.h (right):
> 
>
https://codereview.chromium.org/2811293004/diff/20001/content/browser/downloa...
> content/browser/download/download_job.h:48: virtual void CancelRequest(int64_t
> offset);
> nit:s/CancelRequest/CancelRequestWithOffset/

The callback is probably better, since DestinationUpdate is called too
frequently. I'll make the change.

[xingliu, 2017-04-15 00:26:30]

Thanks for the review.

For this issue, the long term solution is probably to implement
ByteStreamReader::Close.

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

https://codereview.chromium.org/2811293004/diff/20001/content/browser/downloa...
content/browser/download/download_file_impl.cc:82: length_ = kNoBytesToWrite;
On 2017/04/14 20:04:43, qinmin wrote:
> the stream_to_close can be added here rather than waiting for the StreamActive
> call

We also truncate the length in next block, line 90. In this case, we also need
to cancel the request, but after that much data being written to disk.

So we only cancel the request when |should_terminate| is true in StreamActive.
When the stream is complete by itself, the writer side should close the url
request.

But I agree on the idea to let SourceStream to do a bit more things, and it
needs some clean up and refactoring. Like SourceStream::Close as a temporary
solution to "ByteStreamReader::Close".

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

https://codereview.chromium.org/2811293004/diff/20001/content/browser/downloa...
content/browser/download/download_item_impl.h:237: const
std::unordered_set<int64_t>& stream_to_close) override;
On 2017/04/14 20:04:43, qinmin wrote:
> I would use finished_stream_offsets instead of stream_to_close for 2 reasons:
> 1. the param is offset, not streams.
> 2. these streams are considered finished.

Done, changed to use a callback.

Callback looks cleaner than passing the offset here, thanks for the suggestion.

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

https://codereview.chromium.org/2811293004/diff/20001/content/browser/downloa...
content/browser/download/download_job.h:48: virtual void CancelRequest(int64_t
offset);
On 2017/04/14 20:04:44, qinmin wrote:
> nit:s/CancelRequest/CancelRequestWithOffset/

Done.

[qinmin, 2017-04-17 21:39:59]

lgtm % comments

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

https://codereview.chromium.org/2811293004/diff/40001/content/browser/downloa...
content/browser/download/download_file.h:51: // Upon completion, |callback| will
be called on the UI
s/callback/initialize_callback/

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

https://codereview.chromium.org/2811293004/diff/40001/content/browser/downloa...
content/browser/download/parallel_download_job.cc:102:
DownloadJobImpl::Cancel(false);
early return?

[xingliu, 2017-04-17 23:41:51]

Thanks for the review. Polished on details.

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

https://codereview.chromium.org/2811293004/diff/40001/content/browser/downloa...
content/browser/download/download_file.h:51: // Upon completion, |callback| will
be called on the UI
On 2017/04/17 21:39:59, qinmin wrote:
> s/callback/initialize_callback/

Done.

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

https://codereview.chromium.org/2811293004/diff/40001/content/browser/downloa...
content/browser/download/parallel_download_job.cc:102:
DownloadJobImpl::Cancel(false);
On 2017/04/17 21:39:59, qinmin wrote:
> early return?

Done, make sense.

[xingliu, 2017-04-18 00:34:05]

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

[commit-bot: I haz the power, 2017-04-18 00:34:34]

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

[xingliu, 2017-04-18 01:13:10]

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

[xingliu, 2017-04-18 01:13:11]

alexmos@chromium.org: Could you help to take a look at the change in this file?

content/public/test/test_file_error_injector.cc

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

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

[commit-bot: I haz the power, 2017-04-18 01:15:41]

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-04-18 02:22:06]

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

[commit-bot: I haz the power, 2017-04-18 02:22: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-04-18 03:53:48]

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

[commit-bot: I haz the power, 2017-04-18 03:53:49]

Dry run: This issue passed the CQ dry run.

[alexmos, 2017-04-18 16:49:06]

On 2017/04/18 01:13:11, xingliu wrote:
> mailto:alexmos@chromium.org: Could you help to take a look at the change in
this file?
> 
> content/public/test/test_file_error_injector.cc

LGTM

[xingliu, 2017-04-18 18:03:51]

The CQ bit was checked by xingliu@chromium.org

[xingliu, 2017-04-18 18:03:52]

The patchset sent to the CQ was uploaded after l-g-t-m from qinmin@chromium.org
Link to the patchset: https://codereview.chromium.org/2811293004/#ps120001
(title: "Removed a DCHECK for unit test.")

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

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-04-18 18:09:52]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1492538631698260,
"parent_rev": "6a3aeb67d211a7c37fc2ce6355898d27fdfdad04", "commit_rev":
"b444a98187571cf9ec627ac52fee7e81ffcecb93"}

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

Description was changed from

==========
Fix an issue that we didn't clean url request properly.

The ByteStreamReader we used in DownloadFile does not support Close()
api, so we need to close the url request on UI thread if
"ByteStreamReader::Close" should be called.

Before we implement parallel download, we null out the callback on
ByteStreamReader during download interruption, and on UI thread, we
cancel the url request. But for parallel download, at some point, we
won't interrupt the download but need to cancel a particular request.

There is another CL that implements a browser test for parallel download and it
will test the code here.
https://codereview.chromium.org/2819483002/

BUG=710576, 644352
==========

to

==========
Fix an issue that we didn't clean url request properly.

The ByteStreamReader we used in DownloadFile does not support Close()
api, so we need to close the url request on UI thread if
"ByteStreamReader::Close" should be called.

Before we implement parallel download, we null out the callback on
ByteStreamReader during download interruption, and on UI thread, we
cancel the url request. But for parallel download, at some point, we
won't interrupt the download but need to cancel a particular request.

There is another CL that implements a browser test for parallel download and it
will test the code here.
https://codereview.chromium.org/2819483002/

BUG=710576, 644352

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

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

Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/b444a98187571cf9ec627ac52fee...

[asanka, 2017-04-21 15:09:28]

https://codereview.chromium.org/2811293004/diff/120001/content/browser/downlo...
File content/browser/download/download_file_impl.cc (right):

https://codereview.chromium.org/2811293004/diff/120001/content/browser/downlo...
content/browser/download/download_file_impl.cc:690: void
DownloadFileImpl::CancelRequestOnUIThread(int64_t offset) {
Nit : The naming convention is to name the function based on which thread it
runs on if it differs from surrounding code. In this case the function runs on
the IO thread. That reduces the chance of accidentally calling a function that
needs to be scheduled on a different thread.

In this case, you could just call this CancelRequest() since it shares the
thread affinity with the surrounding code.

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

https://codereview.chromium.org/2811293004/diff/120001/content/browser/downlo...
content/browser/download/download_item_impl.cc:1067:
job_->CancelRequestWithOffset(offset);
It is possible that this will cancel the only request, correct? In which case
the download should be canceled?

I ask because I don't see any tests that exercise that possibility. The control
paths that are affected are quite convoluted and currently in flux. There should
be tests for these cases or there's a pretty good chance that things will break.

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

https://codereview.chromium.org/2811293004/diff/120001/content/browser/downlo...
content/browser/download/download_job.cc:69: void
DownloadJob::CancelRequestWithOffset(int64_t offset) {}
Nit: Add a NOTREACHED()

https://codereview.chromium.org/2811293004/diff/120001/content/browser/downlo...
File content/browser/download/parallel_download_job.cc (right):

https://codereview.chromium.org/2811293004/diff/120001/content/browser/downlo...
content/browser/download/parallel_download_job.cc:102:
DownloadJobImpl::Cancel(false);
I guess the expectation is that this will go through the DownloadJobImpl ->
DownloadResourceHandle -> ResourceDispatcherHost -> ResourceLoader ->
net::URLRequest -> ResourceLoader -> DownloadResourceHandler ->
DownloadRequestCore -> DownloadFileImpl -> DownloadItemImpl and cancel the
download? Can you add a test for this case because that's a pretty big chunk of
code with a good chance of breaking. I don't know if this even works for
anything other than http/s.

https://codereview.chromium.org/2811293004/diff/120001/content/browser/downlo...
content/browser/download/parallel_download_job.cc:108: it->second->Cancel();
Is there a reason to consider the |it == workers_.end()| as a valid outcome
here? It looks like workers are added to workers_, but never removed.

[xingliu, 2017-04-21 16:59:20]

On 2017/04/21 15:09:28, asanka wrote:
>
https://codereview.chromium.org/2811293004/diff/120001/content/browser/downlo...
> File content/browser/download/download_file_impl.cc (right):
> 
>
https://codereview.chromium.org/2811293004/diff/120001/content/browser/downlo...
> content/browser/download/download_file_impl.cc:690: void
> DownloadFileImpl::CancelRequestOnUIThread(int64_t offset) {
> Nit : The naming convention is to name the function based on which thread it
> runs on if it differs from surrounding code. In this case the function runs on
> the IO thread. That reduces the chance of accidentally calling a function that
> needs to be scheduled on a different thread.
> 
> In this case, you could just call this CancelRequest() since it shares the
> thread affinity with the surrounding code.
> 
>
https://codereview.chromium.org/2811293004/diff/120001/content/browser/downlo...
> File content/browser/download/download_item_impl.cc (right):
> 
>
https://codereview.chromium.org/2811293004/diff/120001/content/browser/downlo...
> content/browser/download/download_item_impl.cc:1067:
> job_->CancelRequestWithOffset(offset);
> It is possible that this will cancel the only request, correct? In which case
> the download should be canceled?
> 
> I ask because I don't see any tests that exercise that possibility. The
control
> paths that are affected are quite convoluted and currently in flux. There
should
> be tests for these cases or there's a pretty good chance that things will
break.
> 
>
https://codereview.chromium.org/2811293004/diff/120001/content/browser/downlo...
> File content/browser/download/download_job.cc (right):
> 
>
https://codereview.chromium.org/2811293004/diff/120001/content/browser/downlo...
> content/browser/download/download_job.cc:69: void
> DownloadJob::CancelRequestWithOffset(int64_t offset) {}
> Nit: Add a NOTREACHED()
> 
>
https://codereview.chromium.org/2811293004/diff/120001/content/browser/downlo...
> File content/browser/download/parallel_download_job.cc (right):
> 
>
https://codereview.chromium.org/2811293004/diff/120001/content/browser/downlo...
> content/browser/download/parallel_download_job.cc:102:
> DownloadJobImpl::Cancel(false);
> I guess the expectation is that this will go through the DownloadJobImpl ->
> DownloadResourceHandle -> ResourceDispatcherHost -> ResourceLoader ->
> net::URLRequest -> ResourceLoader -> DownloadResourceHandler ->
> DownloadRequestCore -> DownloadFileImpl -> DownloadItemImpl and cancel the
> download? Can you add a test for this case because that's a pretty big chunk
of
> code with a good chance of breaking. I don't know if this even works for
> anything other than http/s.
> 
>
https://codereview.chromium.org/2811293004/diff/120001/content/browser/downlo...
> content/browser/download/parallel_download_job.cc:108: it->second->Cancel();
> Is there a reason to consider the |it == workers_.end()| as a valid outcome
> here? It looks like workers are added to workers_, but never removed.

Hi, thanks for the review, will address these issues in another CL soon.

[xingliu, 2017-04-21 18:07:00]

https://codereview.chromium.org/2811293004/diff/120001/content/browser/downlo...
File content/browser/download/parallel_download_job.cc (right):

https://codereview.chromium.org/2811293004/diff/120001/content/browser/downlo...
content/browser/download/parallel_download_job.cc:102:
DownloadJobImpl::Cancel(false);
On 2017/04/21 15:09:28, asanka wrote:
> I guess the expectation is that this will go through the DownloadJobImpl ->
> DownloadResourceHandle -> ResourceDispatcherHost -> ResourceLoader ->
> net::URLRequest -> ResourceLoader -> DownloadResourceHandler ->
> DownloadRequestCore -> DownloadFileImpl -> DownloadItemImpl and cancel the
> download? Can you add a test for this case because that's a pretty big chunk
of
> code with a good chance of breaking. I don't know if this even works for
> anything other than http/s.

ParallelDownloadComplete browser test covers this flow. Will add more unit tests
in later CL.


The flow is used when we need to close the ByteStream in DownloadFileImpl, so
the beginning point of the flow is actually DownloadFileImpl.

After we refactor the download file to be owned by DownloadJob, this function
will be removed.

One question about the non http cases, do we mean quic here? or other protocol
also supported in UrlRequest like ftp?

[xingliu, 2017-04-22 00:07:24]

Thanks for the feedback.

Please refer to 
https://codereview.chromium.org/2829853011/#
for change.

https://codereview.chromium.org/2811293004/diff/120001/content/browser/downlo...
File content/browser/download/download_file_impl.cc (right):

https://codereview.chromium.org/2811293004/diff/120001/content/browser/downlo...
content/browser/download/download_file_impl.cc:690: void
DownloadFileImpl::CancelRequestOnUIThread(int64_t offset) {
On 2017/04/21 15:09:28, asanka wrote:
> Nit : The naming convention is to name the function based on which thread it
> runs on if it differs from surrounding code. In this case the function runs on
> the IO thread. That reduces the chance of accidentally calling a function that
> needs to be scheduled on a different thread.
> 
> In this case, you could just call this CancelRequest() since it shares the
> thread affinity with the surrounding code.

Done. Thanks.

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

https://codereview.chromium.org/2811293004/diff/120001/content/browser/downlo...
content/browser/download/download_item_impl.cc:1067:
job_->CancelRequestWithOffset(offset);
On 2017/04/21 15:09:28, asanka wrote:
> It is possible that this will cancel the only request, correct? In which case
> the download should be canceled?
> 
> I ask because I don't see any tests that exercise that possibility. The
control
> paths that are affected are quite convoluted and currently in flux. There
should
> be tests for these cases or there's a pretty good chance that things will
break.

If this call cancelled the only request, the download file should inform the
download item interruption or completion right after that.

It's not expected to cancel the download with this function in any case, but to
clean up a particular request, since the way we null out the ByteStreamReader in
DownloadFileImpl does not actually close the url request.

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

https://codereview.chromium.org/2811293004/diff/120001/content/browser/downlo...
content/browser/download/download_job.cc:69: void
DownloadJob::CancelRequestWithOffset(int64_t offset) {}
On 2017/04/21 15:09:28, asanka wrote:
> Nit: Add a NOTREACHED()

Done.

https://codereview.chromium.org/2811293004/diff/120001/content/browser/downlo...
File content/browser/download/parallel_download_job.cc (right):

https://codereview.chromium.org/2811293004/diff/120001/content/browser/downlo...
content/browser/download/parallel_download_job.cc:102:
DownloadJobImpl::Cancel(false);
On 2017/04/21 18:07:00, xingliu wrote:
> On 2017/04/21 15:09:28, asanka wrote:
> > I guess the expectation is that this will go through the DownloadJobImpl ->
> > DownloadResourceHandle -> ResourceDispatcherHost -> ResourceLoader ->
> > net::URLRequest -> ResourceLoader -> DownloadResourceHandler ->
> > DownloadRequestCore -> DownloadFileImpl -> DownloadItemImpl and cancel the
> > download? Can you add a test for this case because that's a pretty big chunk
> of
> > code with a good chance of breaking. I don't know if this even works for
> > anything other than http/s.
> 
> ParallelDownloadComplete browser test covers this flow. Will add more unit
tests
> in later CL.
> 
> 
> The flow is used when we need to close the ByteStream in DownloadFileImpl, so
> the beginning point of the flow is actually DownloadFileImpl.
> 
> After we refactor the download file to be owned by DownloadJob, this function
> will be removed.
> 
> One question about the non http cases, do we mean quic here? or other protocol
> also supported in UrlRequest like ftp?

The flow is DownloadFileImpl ==> DownloadItemImpl ==> DownloadJob ==> close a
particular connection.

https://codereview.chromium.org/2811293004/diff/120001/content/browser/downlo...
content/browser/download/parallel_download_job.cc:108: it->second->Cancel();
On 2017/04/21 15:09:28, asanka wrote:
> Is there a reason to consider the |it == workers_.end()| as a valid outcome
> here? It looks like workers are added to workers_, but never removed.

Done, added a DCHECK.

In the long term, after the job owns the download file, job's life cycle may be
changed to be the similar to download file.