Android: Use a ResourceThrottle to intercept GET downloads.

Android: Use a ResourceThrottle to intercept GET downloads.

Earlier we used to pass all the relevant bits to WebContentsDelegate::CanDownload
This also allows for DownloadRequestLimiter to work on Android (show the infobar when multiple downloads are initiated from a page)

BUG=


Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=192740

[joth, 2013-04-05 00:54:45]

https://codereview.chromium.org/13649009/diff/4001/chrome/browser/renderer_ho...
File chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc
(right):

https://codereview.chromium.org/13649009/diff/4001/chrome/browser/renderer_ho...
chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:329: //
If it's from the web, we don't trust it, so we push the throttle on.
sure you want your new throttle contained inside this if() too?

[nilesh, 2013-04-05 01:03:37]

https://codereview.chromium.org/13649009/diff/4001/chrome/browser/renderer_ho...
File chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc
(right):

https://codereview.chromium.org/13649009/diff/4001/chrome/browser/renderer_ho...
chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:329: //
If it's from the web, we don't trust it, so we push the throttle on.
On 2013/04/05 00:54:45, joth wrote:
> sure you want your new throttle contained inside this if() too?

Earlier this functionality was in DownloadResourceThrottle and this is the only
place when a DownloadResourceThrottle is created. So we are not changing which
requests will be intercepted. Am I missing something?

[joth, 2013-04-05 01:10:35]

On 4 April 2013 18:03, <nileshagrawal@chromium.org> wrote:

>
> https://codereview.chromium.**org/13649009/diff/4001/chrome/**
>
browser/renderer_host/chrome_**resource_dispatcher_host_**delegate.cc<https://codereview.chromium.org/13649009/diff/4001/chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc>
> File
> chrome/browser/renderer_host/**chrome_resource_dispatcher_**
> host_delegate.cc
> (right):
>
> https://codereview.chromium.**org/13649009/diff/4001/chrome/**
> browser/renderer_host/chrome_**resource_dispatcher_host_**
>
delegate.cc#newcode329<https://codereview.chromium.org/13649009/diff/4001/chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc#newcode329>
> chrome/browser/renderer_host/**chrome_resource_dispatcher_**
> host_delegate.cc:329:
> // If it's from the web, we don't trust it, so we push the throttle on.
> On 2013/04/05 00:54:45, joth wrote:
>
>> sure you want your new throttle contained inside this if() too?
>>
>
> Earlier this functionality was in DownloadResourceThrottle and this is
> the only place when a DownloadResourceThrottle is created. So we are not
> changing which requests will be intercepted. Am I missing something?
>
>
More likely I am!  Why wouldn't we want to use the DownloadManager for
user-initiated downloads? or is this if() doing something quite different
to what I understand from the comment. there must be another path to
initiate the DownloadManager somewhere further down the line.


>
https://codereview.chromium.**org/13649009/<https://codereview.chromium.org/1...
>

[asanka, 2013-04-05 16:57:35]

A couple of questions.

Do you intend this CL to not introduce behavior changes? In which case the
issues raised could be addressed separately.

https://codereview.chromium.org/13649009/diff/4001/chrome/browser/android/int...
File chrome/browser/android/intercept_download_resource_throttle.cc (right):

https://codereview.chromium.org/13649009/diff/4001/chrome/browser/android/int...
chrome/browser/android/intercept_download_resource_throttle.cc:37: if
(request_->method() != net::HttpRequestHeaders::kGetMethod)
What if the scheme isn't http or https?

https://codereview.chromium.org/13649009/diff/4001/chrome/browser/renderer_ho...
File chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc
(right):

https://codereview.chromium.org/13649009/diff/4001/chrome/browser/renderer_ho...
chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:329: //
If it's from the web, we don't trust it, so we push the throttle on.
On 2013/04/05 01:03:38, nilesh wrote:
> On 2013/04/05 00:54:45, joth wrote:
> > sure you want your new throttle contained inside this if() too?
> 
> Earlier this functionality was in DownloadResourceThrottle and this is the
only
> place when a DownloadResourceThrottle is created. So we are not changing which
> requests will be intercepted. Am I missing something?

That's actually a good question. The DownloadResourceThrottle was only meant to
prevent carpet bombing and not as a generic go/no-go filter for downloads. The
|is_content_initiated| bit is there to separate downloads that were initiated by
content from other downloads.

|is_content_initated| is false for downloads initiated by:
- 'Save page' on a resource that's not text, HTML, CSS or javascript.
- 'Save URL' on a hyperlink.
- HTML5 Drag and drop.
- Downloads extension API.
- WebStore intaller.
- Plugin installer.

... is true for downloads initiated by:
- Navigation to a resource that can't be rendered (e.g. unhandled content type).
- Response requests download via 'Content-Disposition: attachment'.
- Other forced downloads (e.g. response is a user script).

It sounds like InterceptDownloadResourceThrottle wants to handle at least some
types of downloads for which |is_content_initiated| is false.

[nilesh, 2013-04-05 17:17:20]

On 2013/04/05 16:57:35, asanka wrote:
> A couple of questions.
> 
> Do you intend this CL to not introduce behavior changes? In which case the
> issues raised could be addressed separately.

Well there is some behavior change, as now we allow DownloadRequestLimiter to do
its thing (prompt the user if needed), as the new throttle runs after
DownloadResourceThrottle.

> 
>
https://codereview.chromium.org/13649009/diff/4001/chrome/browser/android/int...
> File chrome/browser/android/intercept_download_resource_throttle.cc (right):
> 
>
https://codereview.chromium.org/13649009/diff/4001/chrome/browser/android/int...
> chrome/browser/android/intercept_download_resource_throttle.cc:37: if
> (request_->method() != net::HttpRequestHeaders::kGetMethod)
> What if the scheme isn't http or https?
> 
>
https://codereview.chromium.org/13649009/diff/4001/chrome/browser/renderer_ho...
> File chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc
> (right):
> 
>
https://codereview.chromium.org/13649009/diff/4001/chrome/browser/renderer_ho...
> chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:329:
//
> If it's from the web, we don't trust it, so we push the throttle on.
> On 2013/04/05 01:03:38, nilesh wrote:
> > On 2013/04/05 00:54:45, joth wrote:
> > > sure you want your new throttle contained inside this if() too?
> > 
> > Earlier this functionality was in DownloadResourceThrottle and this is the
> only
> > place when a DownloadResourceThrottle is created. So we are not changing
which
> > requests will be intercepted. Am I missing something?
> 
> That's actually a good question. The DownloadResourceThrottle was only meant
to
> prevent carpet bombing and not as a generic go/no-go filter for downloads. The
> |is_content_initiated| bit is there to separate downloads that were initiated
by
> content from other downloads.
> 
> |is_content_initated| is false for downloads initiated by:
> - 'Save page' on a resource that's not text, HTML, CSS or javascript.
> - 'Save URL' on a hyperlink.
> - HTML5 Drag and drop.
> - Downloads extension API.
> - WebStore intaller.
> - Plugin installer.

None of the above are applicable here for Android. 'Save' link/image are
directly handled by the app and sent to Android DownloadManager and others are
not supported on Android.
I think, there should be no behavior change even if we move this outside
|is_content_initiated| unless there may be multiple calls to ::DownloadStarting
for the same request with different values of |is_content_initiated|.

> 
> ... is true for downloads initiated by:
> - Navigation to a resource that can't be rendered (e.g. unhandled content
type).
> - Response requests download via 'Content-Disposition: attachment'.
> - Other forced downloads (e.g. response is a user script).
> 
> It sounds like InterceptDownloadResourceThrottle wants to handle at least some
> types of downloads for which |is_content_initiated| is false.

[asanka, 2013-04-05 17:33:47]

LGTM.

[nilesh, 2013-04-05 17:45:45]

Nico: chrome/chrome_browser.gypi OWNERS

[Nico, 2013-04-06 01:28:37]

gypi lgtm

(as chrome/OWNERS states, it's ok to tbr cls like this)

[commit-bot: I haz the power, 2013-04-06 04:44:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/nileshagrawal@chromium.org/13649009/4001

[commit-bot: I haz the power, 2013-04-06 10:33:57]

Change committed as 192740