Add control to use If-Range header for range request.

Add control to use If-Range header for range request.

Currently If-Range is used in all half opened request, including normal
download resumption, and parallel download subsequent requests.

However parallel download subsequent requests may not necessarily use
If-Range header.

This CL adds a parameter to control if If-Range is sent in the request.

Also refactored the code to attach partial request headers.

BUG=706181, 644352
Review-Url: https://codereview.chromium.org/2786523002
Cr-Commit-Position: refs/heads/master@{#460930}
Committed: https://chromium.googlesource.com/chromium/src/+/b791e0b97910713700bf8e4f3396521cff290d3b

[xingliu, 2017-03-29 02:11:11]

Description was changed from

==========
Add control to use If-Range header for range request.

Currently If-Range is used in all half opened request, including normal
download resumption, and parallel download subsequent requests.

However parallel download subsequent requests can directly get http 416.

So add a parameter to control if If-Range is sent in the request.

Also refactored the code to attach partial request headers.

BUG=706181, 644352
==========

to

==========
Add control to use If-Range header for range request.

Currently If-Range is used in all half opened request, including normal
download resumption, and parallel download subsequent requests.

However parallel download subsequent requests may not necessarily use
If-Range header.

This CL adds a parameter to control if If-Range is sent in the request.

Also refactored the code to attach partial request headers.

BUG=706181, 644352
==========

[xingliu, 2017-03-29 02:11:15]

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

[xingliu, 2017-03-29 02:11:52]

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

[xingliu, 2017-03-29 02:11:53]

Hi, PTAL, thanks.

[commit-bot: I haz the power, 2017-03-29 02:12:08]

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

[commit-bot: I haz the power, 2017-03-29 04:13:19]

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

[commit-bot: I haz the power, 2017-03-29 04:13:20]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_TIMED_OUT, build hasn't
started yet, builder probably lacks capacity)

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

On 2017/03/29 02:11:53, xingliu wrote:
> Hi, PTAL, thanks.

Can we check the response code for if-range? if it is 206, we can add the byte
stream, otherwise, we simply ignore it?

[xingliu, 2017-03-29 17:27:46]

On 2017/03/29 17:09:38, qinmin wrote:
> On 2017/03/29 02:11:53, xingliu wrote:
> > Hi, PTAL, thanks.
> 
> Can we check the response code for if-range? if it is 206, we can add the byte
> stream, otherwise, we simply ignore it?

Make sense. I can do this in this patch.

Initial request accept both 206 and 200(strong validator mismatched).

The worker will accept 206 and 416(Range error).

So can I add this logic in worker layer, to propagate the 416 to worker and
intercept there.

or download request core, to pass in a bool like ignore_precondition_fail(and
remove use_if_range), so when If-Range precondition check fails, just ignore it.

which way is better, wdyt?

[xingliu, 2017-03-29 17:58:23]

On 2017/03/29 17:09:38, qinmin wrote:
> On 2017/03/29 02:11:53, xingliu wrote:
> > Hi, PTAL, thanks.
> 
> Can we check the response code for if-range? if it is 206, we can add the byte
> stream, otherwise, we simply ignore it?

Done, thanks for this suggestion.

Handle the 416 in worker, I feel it's a bit better than still letting the worker
use
If-Range and let DownloadRequestCore handle everything.

Since http server can just return a header and don't need to read the file. And
416 fits better than a 200 with If-Range in this case.

[xingliu, 2017-03-29 18:18:58]

To correct the comment here, the worker will accept 206, 412(precondition fail),
416(range not satisfied).
412 will interrupt the download instead of return 200 with full content. 416
will be ignored.

[xingliu, 2017-03-29 19:39:48]

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

[commit-bot: I haz the power, 2017-03-29 19:40: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-03-29 19:45:03]

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

[commit-bot: I haz the power, 2017-03-29 19:45:04]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  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_...)

[asanka, 2017-03-29 21:57:59]

lgtm

[David Trainor- moved to gerrit, 2017-03-30 04:27:20]

lgtm

[qinmin, 2017-03-30 05:21:43]

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

https://codereview.chromium.org/2786523002/diff/40001/content/browser/downloa...
content/browser/download/download_worker.cc:99: if (create_info->result ==
{} needed as this if statement spans multiple lines

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

https://codereview.chromium.org/2786523002/diff/40001/content/browser/downloa...
content/browser/download/parallel_download_job.cc:192:
download_params->set_use_if_range(false);
still need this?

[xingliu, 2017-03-30 17:16:47]

Thanks for the feedback. PTAL.

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

https://codereview.chromium.org/2786523002/diff/40001/content/browser/downloa...
content/browser/download/download_worker.cc:99: if (create_info->result ==
On 2017/03/30 05:21:43, qinmin wrote:
> {} needed as this if statement spans multiple lines

Done.

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

https://codereview.chromium.org/2786523002/diff/40001/content/browser/downloa...
content/browser/download/parallel_download_job.cc:192:
download_params->set_use_if_range(false);
On 2017/03/30 05:21:43, qinmin wrote:
> still need this?

Yes, I think so.

Precondition failed for initial request and subsequent requests are different.
The initial request will handle the full content returned with http 200 but
subsequent requests need to drop it.

We previously distinguish initial request and subsequent requests based on their
length cap. But subsequent requests now can also be half opened.

Another option is to check the length difference in worker layer, which is not
ideal according to previous review. I guess the main reason is we'd better use
http protocol instead of relying on length checking in worker layer.

[qinmin, 2017-03-30 17:46:07]

lgtm

[xingliu, 2017-03-30 20:59:02]

The CQ bit was checked by xingliu@chromium.org

[xingliu, 2017-03-30 20:59:03]

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

[commit-bot: I haz the power, 2017-03-30 21:00:24]

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

[commit-bot: I haz the power, 2017-03-30 21:25:27]

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

[commit-bot: I haz the power, 2017-03-30 21:25:28]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[xingliu, 2017-03-30 21:56:40]

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

[xingliu, 2017-03-30 21:56:41]

@alexmos, please help take a look at change at:
	content/public/browser/download_url_parameters.h
	content/public/browser/download_url_parameters.cc

Thanks.

[alexmos, 2017-03-30 22:01:45]

content/public/browser/ LGTM

[xingliu, 2017-03-30 22:32:08]

The CQ bit was checked by xingliu@chromium.org

[commit-bot: I haz the power, 2017-03-30 22:33:55]

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

[commit-bot: I haz the power, 2017-03-30 23:39:02]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1490913128399100,
"parent_rev": "569fe6657d6887a1dab9e6476450ff403031832c", "commit_rev":
"b791e0b97910713700bf8e4f3396521cff290d3b"}

[commit-bot: I haz the power, 2017-03-30 23:39:49]

Description was changed from

==========
Add control to use If-Range header for range request.

Currently If-Range is used in all half opened request, including normal
download resumption, and parallel download subsequent requests.

However parallel download subsequent requests may not necessarily use
If-Range header.

This CL adds a parameter to control if If-Range is sent in the request.

Also refactored the code to attach partial request headers.

BUG=706181, 644352
==========

to

==========
Add control to use If-Range header for range request.

Currently If-Range is used in all half opened request, including normal
download resumption, and parallel download subsequent requests.

However parallel download subsequent requests may not necessarily use
If-Range header.

This CL adds a parameter to control if If-Range is sent in the request.

Also refactored the code to attach partial request headers.

BUG=706181, 644352

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

[commit-bot: I haz the power, 2017-03-30 23:39:50]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/b791e0b97910713700bf8e4f3396...