reintroduce InterceptDownloadResourceThrottle for some OMA DRM downloads

reintroduce InterceptDownloadResourceThrottle for some OMA DRM downloads

For OMA DRM downloads, if download descriptor file doesn't exist, Android DownloadManager should be used.
This CL reverts some of the changes in https://codereview.chromium.org/2341643008 to intercept such downloads.

BUG=675559
Review-Url: https://codereview.chromium.org/2642683005
Cr-Commit-Position: refs/heads/master@{#449100}
Committed: https://chromium.googlesource.com/chromium/src/+/1dfa16b2126e9776a3e26f00b4c767b83f2645b5

[qinmin, 2017-01-19 23:18:30]

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

[qinmin, 2017-01-19 23:18:30]

PTAL

dtrainor@ for chrome/browser/android/
thakis@ for chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc
OWNER

[asanka, 2017-01-19 23:59:13]

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

[asanka, 2017-01-19 23:59:14]

https://codereview.chromium.org/2642683005/diff/1/chrome/browser/loader/chrom...
File chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc (right):

https://codereview.chromium.org/2642683005/diff/1/chrome/browser/loader/chrom...
chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc:550: #endif
This is the wrong place to add this resource throttle since it bypasses the
SafeBrowsing checks for explicit downloads. It should be added after
AppendStandardResourceThrottles() below.

Note that is_new_request and is_content_initiated are both true for <a download>
type downloads.

[qinmin, 2017-01-20 00:06:10]

https://codereview.chromium.org/2642683005/diff/1/chrome/browser/loader/chrom...
File chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc (right):

https://codereview.chromium.org/2642683005/diff/1/chrome/browser/loader/chrom...
chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc:550: #endif
On 2017/01/19 23:59:13, asanka wrote:
> This is the wrong place to add this resource throttle since it bypasses the
> SafeBrowsing checks for explicit downloads. It should be added after
> AppendStandardResourceThrottles() below.
> 
> Note that is_new_request and is_content_initiated are both true for <a
download>
> type downloads.

Done.

[qinmin, 2017-01-23 19:37:10]

ping, dtrainor@, thakis@, would you mind take a look?

[David Trainor- moved to gerrit, 2017-01-24 17:13:35]

https://codereview.chromium.org/2642683005/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/download/ChromeDownloadDelegate.java
(right):

https://codereview.chromium.org/2642683005/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/download/ChromeDownloadDelegate.java:324:
String contentDisposition, String mimeType, String cookie, String referer) {
s/referer/referrer?

https://codereview.chromium.org/2642683005/diff/20001/chrome/browser/BUILD.gn
File chrome/browser/BUILD.gn (right):

https://codereview.chromium.org/2642683005/diff/20001/chrome/browser/BUILD.gn...
chrome/browser/BUILD.gn:2635:
"android/download/intercept_download_resource_throttle.cc",
Were these files removed?  I don't see them on the CL.

https://codereview.chromium.org/2642683005/diff/20001/chrome/browser/android/...
File chrome/browser/android/download/download_controller.cc (right):

https://codereview.chromium.org/2642683005/diff/20001/chrome/browser/android/...
chrome/browser/android/download/download_controller.cc:261: return;
Should this case log a similar error to StartAndroidDownload()?

https://codereview.chromium.org/2642683005/diff/20001/chrome/browser/android/...
File chrome/browser/android/download/download_controller_base.h (right):

https://codereview.chromium.org/2642683005/diff/20001/chrome/browser/android/...
chrome/browser/android/download/download_controller_base.h:82: virtual void
CreateGETDownload(
Who calls this?

[qinmin, 2017-01-24 17:32:24]

https://codereview.chromium.org/2642683005/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/download/ChromeDownloadDelegate.java
(right):

https://codereview.chromium.org/2642683005/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/download/ChromeDownloadDelegate.java:324:
String contentDisposition, String mimeType, String cookie, String referer) {
On 2017/01/24 17:13:35, David Trainor wrote:
> s/referer/referrer?

Done.

https://codereview.chromium.org/2642683005/diff/20001/chrome/browser/BUILD.gn
File chrome/browser/BUILD.gn (right):

https://codereview.chromium.org/2642683005/diff/20001/chrome/browser/BUILD.gn...
chrome/browser/BUILD.gn:2635:
"android/download/intercept_download_resource_throttle.cc",
On 2017/01/24 17:13:35, David Trainor wrote:
> Were these files removed?  I don't see them on the CL.

sorry, the files are now added

https://codereview.chromium.org/2642683005/diff/20001/chrome/browser/android/...
File chrome/browser/android/download/download_controller.cc (right):

https://codereview.chromium.org/2642683005/diff/20001/chrome/browser/android/...
chrome/browser/android/download/download_controller.cc:261: return;
On 2017/01/24 17:13:35, David Trainor wrote:
> Should this case log a similar error to StartAndroidDownload()?

Done.

https://codereview.chromium.org/2642683005/diff/20001/chrome/browser/android/...
File chrome/browser/android/download/download_controller_base.h (right):

https://codereview.chromium.org/2642683005/diff/20001/chrome/browser/android/...
chrome/browser/android/download/download_controller_base.h:82: virtual void
CreateGETDownload(
On 2017/01/24 17:13:35, David Trainor wrote:
> Who calls this?

InterceptDownloadResourceThrottle calls this to let Android Downloadmanager
intercept the download

[qinmin, 2017-01-25 07:28:53]

Patchset #3 (id:40001) has been deleted

[qinmin, 2017-01-25 19:30:42]

ping, dtrainor@, thakis@, PTAL again

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

asanka@chromium.org changed reviewers:
- asanka@chromium.org

[David Trainor- moved to gerrit, 2017-01-27 17:16:47]

lgtm

https://codereview.chromium.org/2642683005/diff/80001/chrome/browser/android/...
File chrome/browser/android/download/intercept_download_resource_throttle.cc
(right):

https://codereview.chromium.org/2642683005/diff/80001/chrome/browser/android/...
chrome/browser/android/download/intercept_download_resource_throttle.cc:25:
InterceptDownloadResourceThrottle::~InterceptDownloadResourceThrottle() {
does = default fail here because complex destructor?

https://codereview.chromium.org/2642683005/diff/80001/chrome/browser/android/...
File chrome/browser/android/download/intercept_download_resource_throttle.h
(right):

https://codereview.chromium.org/2642683005/diff/80001/chrome/browser/android/...
chrome/browser/android/download/intercept_download_resource_throttle.h:35: //
Set to true when if we want chrome to handle the download.
What is set to true?

[qinmin, 2017-01-27 21:29:54]

https://codereview.chromium.org/2642683005/diff/80001/chrome/browser/android/...
File chrome/browser/android/download/intercept_download_resource_throttle.cc
(right):

https://codereview.chromium.org/2642683005/diff/80001/chrome/browser/android/...
chrome/browser/android/download/intercept_download_resource_throttle.cc:25:
InterceptDownloadResourceThrottle::~InterceptDownloadResourceThrottle() {
On 2017/01/27 17:16:46, David Trainor wrote:
> does = default fail here because complex destructor?

Done.

https://codereview.chromium.org/2642683005/diff/80001/chrome/browser/android/...
File chrome/browser/android/download/intercept_download_resource_throttle.h
(right):

https://codereview.chromium.org/2642683005/diff/80001/chrome/browser/android/...
chrome/browser/android/download/intercept_download_resource_throttle.h:35: //
Set to true when if we want chrome to handle the download.
On 2017/01/27 17:16:47, David Trainor wrote:
> What is set to true?

removed

[qinmin, 2017-01-30 17:13:00]

@thakis, would you please take a quick look at
chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc? thanks

On 2017/01/27 21:29:54, qinmin wrote:
>
https://codereview.chromium.org/2642683005/diff/80001/chrome/browser/android/...
> File chrome/browser/android/download/intercept_download_resource_throttle.cc
> (right):
> 
>
https://codereview.chromium.org/2642683005/diff/80001/chrome/browser/android/...
> chrome/browser/android/download/intercept_download_resource_throttle.cc:25:
> InterceptDownloadResourceThrottle::~InterceptDownloadResourceThrottle() {
> On 2017/01/27 17:16:46, David Trainor wrote:
> > does = default fail here because complex destructor?
> 
> Done.
> 
>
https://codereview.chromium.org/2642683005/diff/80001/chrome/browser/android/...
> File chrome/browser/android/download/intercept_download_resource_throttle.h
> (right):
> 
>
https://codereview.chromium.org/2642683005/diff/80001/chrome/browser/android/...
> chrome/browser/android/download/intercept_download_resource_throttle.h:35: //
> Set to true when if we want chrome to handle the download.
> On 2017/01/27 17:16:47, David Trainor wrote:
> > What is set to true?
> 
> removed

[Nico, 2017-02-07 15:34:45]

Does this have any test coverage?

Other than that, my file lgtm

[qinmin, 2017-02-07 19:26:45]

On 2017/02/07 15:34:45, Nico wrote:
> Does this have any test coverage?
> 
> Other than that, my file lgtm

we have a test in DownloadTest.java for .dm files,  the new CL should not break
that.

[qinmin, 2017-02-07 19:26:49]

The CQ bit was checked by qinmin@chromium.org

[qinmin, 2017-02-07 19:26:50]

The patchset sent to the CQ was uploaded after l-g-t-m from
dtrainor@chromium.org
Link to the patchset: https://codereview.chromium.org/2642683005/#ps100001
(title: "nit")

[commit-bot: I haz the power, 2017-02-07 19:27:25]

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-07 21:09:48]

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

[commit-bot: I haz the power, 2017-02-07 21:09:49]

Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)

[qinmin, 2017-02-08 20:20:46]

The CQ bit was checked by qinmin@chromium.org

[qinmin, 2017-02-08 20:20:46]

The patchset sent to the CQ was uploaded after l-g-t-m from
dtrainor@chromium.org, thakis@chromium.org
Link to the patchset: https://codereview.chromium.org/2642683005/#ps120001
(title: "function name change to avoid findbugs confusion")

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

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-08 21:59:54]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1486585246646230,
"parent_rev": "b85585f7cb1ae6e55a1e7204c8955604df898164", "commit_rev":
"1dfa16b2126e9776a3e26f00b4c767b83f2645b5"}

[commit-bot: I haz the power, 2017-02-08 22:00:34]

Description was changed from

==========
reintroduce InterceptDownloadResourceThrottle for some OMA DRM downloads

For OMA DRM downloads, if download descriptor file doesn't exist, Android
DownloadManager should be used.
This CL reverts some of the changes in
https://codereview.chromium.org/2341643008 to intercept such downloads.

BUG=675559
==========

to

==========
reintroduce InterceptDownloadResourceThrottle for some OMA DRM downloads

For OMA DRM downloads, if download descriptor file doesn't exist, Android
DownloadManager should be used.
This CL reverts some of the changes in
https://codereview.chromium.org/2341643008 to intercept such downloads.

BUG=675559

Review-Url: https://codereview.chromium.org/2642683005
Cr-Commit-Position: refs/heads/master@{#449100}
Committed:
https://chromium.googlesource.com/chromium/src/+/1dfa16b2126e9776a3e26f00b4c7...
==========

[commit-bot: I haz the power, 2017-02-08 22:00:35]

Committed patchset #6 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/1dfa16b2126e9776a3e26f00b4c7...