permissionrequest API for guest Download.

permissionrequest API for guest Download.

Exposed event:
event.type = 'download'
event.requestMethod = 'GET'/'POST'...
event.url = url


BUG=141204
TEST=Pending: osx + win (b/c there is a *ViewGuest change).
Added browser_tests:WebViewTest.Download, ran unit_tests:DownloadRequestLimiterTest*

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

[lazyboy, 2013-03-25 16:31:51]

Fady, can you take a quick initial look? I still have to write browser_test and
unit test for browser/download changes. Ty.

[asanka, 2013-03-25 18:15:11]

Drive by.

https://chromiumcodereview.appspot.com/13037003/diff/2001/content/public/brow...
File content/public/browser/web_contents_delegate.h (right):

https://chromiumcodereview.appspot.com/13037003/diff/2001/content/public/brow...
content/public/browser/web_contents_delegate.h:226: virtual void
CanDownloadAsync(RenderViewHost* render_view_host,
Why are you adding another callback instead of making CanDownload async? 

With this CL, CanDownloadAsync() is only called when
WebContents::GetRenderProcessHost()->IsGuest() is true. And CanDownload() is
only called when that condition is false.

[lazyboy, 2013-03-25 18:36:40]

https://chromiumcodereview.appspot.com/13037003/diff/2001/content/public/brow...
File content/public/browser/web_contents_delegate.h (right):

https://chromiumcodereview.appspot.com/13037003/diff/2001/content/public/brow...
content/public/browser/web_contents_delegate.h:226: virtual void
CanDownloadAsync(RenderViewHost* render_view_host,
On 2013/03/25 18:15:11, asanka wrote:
> Why are you adding another callback instead of making CanDownload async? 
> 
> With this CL, CanDownloadAsync() is only called when
> WebContents::GetRenderProcessHost()->IsGuest() is true. And CanDownload() is
> only called when that condition is false.

I was worried of slowing down !IsGuest() case (which should be by far the most
common case) by making them async.
Is it acceptable to make CanDownload async for all cases?
Ty.

[asanka, 2013-03-25 18:58:11]

https://chromiumcodereview.appspot.com/13037003/diff/2001/content/public/brow...
File content/public/browser/web_contents_delegate.h (right):

https://chromiumcodereview.appspot.com/13037003/diff/2001/content/public/brow...
content/public/browser/web_contents_delegate.h:226: virtual void
CanDownloadAsync(RenderViewHost* render_view_host,
On 2013/03/25 18:36:40, lazyboy wrote:
> On 2013/03/25 18:15:11, asanka wrote:
> > Why are you adding another callback instead of making CanDownload async? 
> > 
> > With this CL, CanDownloadAsync() is only called when
> > WebContents::GetRenderProcessHost()->IsGuest() is true. And CanDownload() is
> > only called when that condition is false.
> 
> I was worried of slowing down !IsGuest() case (which should be by far the most
> common case) by making them async.
> Is it acceptable to make CanDownload async for all cases?
> Ty.

Yes. The delegate can always invoke the callback synchronously if it can respond
immediately.

Part of the problem is that the distinction between the two callbacks is not
clear at all from the content/ API. On the surface it looks like it's sync vs.
async. But in reality it's based on whether or not the render process host is
associated with a guest renderer.

Having a single API is simpler. Please indicate here that it's permissible to
invoke the callback synchronously.

[lazyboy, 2013-03-25 20:13:05]

https://chromiumcodereview.appspot.com/13037003/diff/2001/content/public/brow...
File content/public/browser/web_contents_delegate.h (right):

https://chromiumcodereview.appspot.com/13037003/diff/2001/content/public/brow...
content/public/browser/web_contents_delegate.h:226: virtual void
CanDownloadAsync(RenderViewHost* render_view_host,
On 2013/03/25 18:58:11, asanka wrote:
> On 2013/03/25 18:36:40, lazyboy wrote:
> > On 2013/03/25 18:15:11, asanka wrote:
> > > Why are you adding another callback instead of making CanDownload async? 
> > > 
> > > With this CL, CanDownloadAsync() is only called when
> > > WebContents::GetRenderProcessHost()->IsGuest() is true. And CanDownload()
is
> > > only called when that condition is false.
> > 
> > I was worried of slowing down !IsGuest() case (which should be by far the
most
> > common case) by making them async.
> > Is it acceptable to make CanDownload async for all cases?
> > Ty.
> 
> Yes. The delegate can always invoke the callback synchronously if it can
respond
> immediately.
> 
> Part of the problem is that the distinction between the two callbacks is not
> clear at all from the content/ API. On the surface it looks like it's sync vs.
> async. But in reality it's based on whether or not the render process host is
> associated with a guest renderer.
> 
> Having a single API is simpler. Please indicate here that it's permissible to
> invoke the callback synchronously.

Changed back to single CanDownload() function.
Done.

[Fady Samuel, 2013-03-26 15:34:34]

This looks fine to me from a browser_plugin perspective. A test would be nice.

content/public changes should be reviewed by jam@.

https://codereview.chromium.org/13037003/diff/13002/content/browser/web_conte...
File content/browser/web_contents/web_contents_view_guest.cc (right):

https://codereview.chromium.org/13037003/diff/13002/content/browser/web_conte...
content/browser/web_contents/web_contents_view_guest.cc:39: return
platform_view_->GetNativeView();
Why is this necessary?

[lazyboy, 2013-03-26 16:04:36]

I'll go ahead and add tests for this CL then, assuming there's no red flag on
downloading from chrome apps. I'll add reviewers for /download and
content/public when its ready. Ty.

https://codereview.chromium.org/13037003/diff/13002/content/browser/web_conte...
File content/browser/web_contents/web_contents_view_guest.cc (right):

https://codereview.chromium.org/13037003/diff/13002/content/browser/web_conte...
content/browser/web_contents/web_contents_view_guest.cc:39: return
platform_view_->GetNativeView();
On 2013/03/26 15:34:34, Fady Samuel wrote:
> Why is this necessary?
The file dialog code path uses this.

[lazyboy, 2013-03-27 00:59:27]

I've added tests, Fady can you take another look?
@asanka, Can you review the downloads changes?
Thanks.

[asanka, 2013-03-27 15:34:53]

https://chromiumcodereview.appspot.com/13037003/diff/20001/chrome/browser/dow...
File chrome/browser/download/download_request_limiter.cc (right):

https://chromiumcodereview.appspot.com/13037003/diff/20001/chrome/browser/dow...
chrome/browser/download/download_request_limiter.cc:286: originating_contents,
request_id, request_method, callback);
|originating_contents| might go away before OnCanDownloadDecided is invoked.
Could you look it up by render_process_host_id, render_view_id?

https://chromiumcodereview.appspot.com/13037003/diff/20001/chrome/browser/dow...
chrome/browser/download/download_request_limiter.cc:290: // take care of it.
Nit: Android also overrides CanDownload to cancel the download and allow the
Download app to handle it. There are overrides for pre-rendering and instant to
suppress downloads from invisible or transient pages. In addition, there's an
override in BrowserPluginGuest that you are working with in this CL. I think
this comment is stale and not all that useful.

https://chromiumcodereview.appspot.com/13037003/diff/20001/chrome/browser/dow...
chrome/browser/download/download_request_limiter.cc:302: const Callback&
orig_callback, bool allow) {
Can you add a DCHECK here to check whether this callback is getting invoked on
the UI thread?

[lazyboy, 2013-03-27 17:09:22]

https://chromiumcodereview.appspot.com/13037003/diff/20001/chrome/browser/dow...
File chrome/browser/download/download_request_limiter.cc (right):

https://chromiumcodereview.appspot.com/13037003/diff/20001/chrome/browser/dow...
chrome/browser/download/download_request_limiter.cc:286: originating_contents,
request_id, request_method, callback);
On 2013/03/27 15:34:53, asanka wrote:
> |originating_contents| might go away before OnCanDownloadDecided is invoked.
> Could you look it up by render_process_host_id, render_view_id?

Done.

https://chromiumcodereview.appspot.com/13037003/diff/20001/chrome/browser/dow...
chrome/browser/download/download_request_limiter.cc:290: // take care of it.
On 2013/03/27 15:34:53, asanka wrote:
> Nit: Android also overrides CanDownload to cancel the download and allow the
> Download app to handle it. There are overrides for pre-rendering and instant
to
> suppress downloads from invisible or transient pages. In addition, there's an
> override in BrowserPluginGuest that you are working with in this CL. I think
> this comment is stale and not all that useful.
OK, removed.

https://chromiumcodereview.appspot.com/13037003/diff/20001/chrome/browser/dow...
chrome/browser/download/download_request_limiter.cc:302: const Callback&
orig_callback, bool allow) {
On 2013/03/27 15:34:53, asanka wrote:
> Can you add a DCHECK here to check whether this callback is getting invoked on
> the UI thread?

Done.

[asanka, 2013-03-28 21:30:04]

chrome/browser/download LGTM

[lazyboy, 2013-03-28 21:39:27]

John, can you review content/public/* changes? I'll add other OWNERS as
reviewers afterwards. Ty.

[jam, 2013-03-29 00:32:50]

lgtm

[lazyboy, 2013-03-29 01:01:13]

+torne@ for android_webview/
+gavinp@ for chrome/browser/prerender
+sky@ for chrome/browser/{ui,views}

[lazyboy, 2013-03-29 01:02:27]

Actually adding torne@ this time.

[gavinp, 2013-03-29 01:19:13]

lgtm

[sky, 2013-03-29 14:57:22]

LGTM

[Torne, 2013-04-02 14:45:49]

android_webview/ LGTM

[Fady Samuel, 2013-04-02 14:47:55]

Exciting stuff. browser_plugin LGTM. Looks like this might land first before
Michael's permission refactor. I'll let him know.

[commit-bot: I haz the power, 2013-04-03 04:59:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/lazyboy@chromium.org/13037003/45001

[commit-bot: I haz the power, 2013-04-03 09:01:56]

Change committed as 192029