Add DownloadControllerAndroid public interface for android.

Add DownloadControllerAndroid public interface for android.


BUG=


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=161964

[nilesh, 2012-10-12 18:48:29]

PTAL.

[joth, 2012-10-12 18:55:13]

lg, I only skimmed the moved/renamed classes

http://codereview.chromium.org/11112022/diff/1/chrome/browser/component/web_c...
File
chrome/browser/component/web_contents_delegate_android/web_contents_delegate_android.cc
(right):

http://codereview.chromium.org/11112022/diff/1/chrome/browser/component/web_c...
chrome/browser/component/web_contents_delegate_android/web_contents_delegate_android.cc:294:
source, download);
I'd suggest deleting these 2 methods from here, put them in the two sub-classes
instead (browser & webview)

[boliu, 2012-10-12 19:11:02]

lgtm but I'm not an owner :)

Would suggest trying the android_fyi_dbg bot since it has the instrumentation
tests.

[nilesh, 2012-10-12 19:23:22]

Addressed comments. PTAL

http://codereview.chromium.org/11112022/diff/1/chrome/browser/component/web_c...
File
chrome/browser/component/web_contents_delegate_android/web_contents_delegate_android.cc
(right):

http://codereview.chromium.org/11112022/diff/1/chrome/browser/component/web_c...
chrome/browser/component/web_contents_delegate_android/web_contents_delegate_android.cc:294:
source, download);
I assume the plan is to move everything out of this file in the long term.
Otherwise it does not make sense to have two copies of the same implementation.

[nilesh, 2012-10-12 19:24:56]

Avi for content/content_browser.gypi OWNER

[nilesh, 2012-10-12 20:30:52]

Joi for chrome/browser/component

[Avi (use Gerrit), 2012-10-12 20:35:11]

This touches content API; I'd like John to take a look.

[joth, 2012-10-12 20:50:03]

On 2012/10/12 19:23:22, nileshagrawal1 wrote:
> Addressed comments. PTAL
> 
>
http://codereview.chromium.org/11112022/diff/1/chrome/browser/component/web_c...
> File
>
chrome/browser/component/web_contents_delegate_android/web_contents_delegate_android.cc
> (right):
> 
>
http://codereview.chromium.org/11112022/diff/1/chrome/browser/component/web_c...
>
chrome/browser/component/web_contents_delegate_android/web_contents_delegate_android.cc:294:
> source, download);
> I assume the plan is to move everything out of this file in the long term.
> Otherwise it does not make sense to have two copies of the same
implementation.

yeah... the main role for WebContentsDelegateAndroid is to forward WCD calls
from native to java. (This allows us to have a java-mirror of the WCD no-op
base-class which is convenient way to find all the different java implementers
of a given function across all our configs)
Mixing policy decisions in here is kind-of fuzzying the purpose of this class.
and i think we already do want differing policies in the two sub-classes...

http://codereview.chromium.org/11112022/diff/16002/android_webview/native/aw_...
File android_webview/native/aw_web_contents_delegate.cc (right):

http://codereview.chromium.org/11112022/diff/16002/android_webview/native/aw_...
android_webview/native/aw_web_contents_delegate.cc:12: #include
"content/public/browser/download_item.h"
nit: forward declare DownloadItem rather than #include.

http://codereview.chromium.org/11112022/diff/16002/android_webview/native/aw_...
android_webview/native/aw_web_contents_delegate.cc:61: return true;
always return false here, as we (currently) never want to have the chrome stack
deal with the download.

http://codereview.chromium.org/11112022/diff/16002/android_webview/native/aw_...
android_webview/native/aw_web_contents_delegate.cc:67: source, download);
replace this with
NOTREACHED();  // We always return false in CanDownload.

[nilesh, 2012-10-12 21:07:45]

PTAL.

http://codereview.chromium.org/11112022/diff/16002/android_webview/native/aw_...
File android_webview/native/aw_web_contents_delegate.cc (right):

http://codereview.chromium.org/11112022/diff/16002/android_webview/native/aw_...
android_webview/native/aw_web_contents_delegate.cc:12: #include
"content/public/browser/download_item.h"
On 2012/10/12 20:50:03, joth wrote:
> nit: forward declare DownloadItem rather than #include.

Done.

http://codereview.chromium.org/11112022/diff/16002/android_webview/native/aw_...
android_webview/native/aw_web_contents_delegate.cc:61: return true;
On 2012/10/12 20:50:03, joth wrote:
> always return false here, as we (currently) never want to have the chrome
stack
> deal with the download.

Done.

http://codereview.chromium.org/11112022/diff/16002/android_webview/native/aw_...
android_webview/native/aw_web_contents_delegate.cc:67: source, download);
On 2012/10/12 20:50:03, joth wrote:
> replace this with
> NOTREACHED();  // We always return false in CanDownload.

Done.

[nilesh, 2012-10-12 21:31:18]

http://codereview.chromium.org/11112022/diff/16002/android_webview/native/aw_...
File android_webview/native/aw_web_contents_delegate.cc (right):

http://codereview.chromium.org/11112022/diff/16002/android_webview/native/aw_...
android_webview/native/aw_web_contents_delegate.cc:61: return true;
I think you meant return false always, but still handle GET downloads. Fixing.

[joth, 2012-10-12 21:33:26]

On 2012/10/12 21:31:18, nileshagrawal1 wrote:
>
http://codereview.chromium.org/11112022/diff/16002/android_webview/native/aw_...
> File android_webview/native/aw_web_contents_delegate.cc (right):
> 
>
http://codereview.chromium.org/11112022/diff/16002/android_webview/native/aw_...
> android_webview/native/aw_web_contents_delegate.cc:61: return true;
> I think you meant return false always, but still handle GET downloads. Fixing.

yep exactly :) TIA

[nilesh, 2012-10-12 21:38:11]

On 2012/10/12 21:33:26, joth wrote:
> On 2012/10/12 21:31:18, nileshagrawal1 wrote:
> >
>
http://codereview.chromium.org/11112022/diff/16002/android_webview/native/aw_...
> > File android_webview/native/aw_web_contents_delegate.cc (right):
> > 
> >
>
http://codereview.chromium.org/11112022/diff/16002/android_webview/native/aw_...
> > android_webview/native/aw_web_contents_delegate.cc:61: return true;
> > I think you meant return false always, but still handle GET downloads.
Fixing.
> 
> yep exactly :) TIA

Done.

[joth, 2012-10-12 23:03:48]

lgtm

http://codereview.chromium.org/11112022/diff/34001/android_webview/native/aw_...
File android_webview/native/aw_web_contents_delegate.cc (right):

http://codereview.chromium.org/11112022/diff/34001/android_webview/native/aw_...
android_webview/native/aw_web_contents_delegate.cc:56: bool
AwWebContentsDelegate::CanDownload(content::RenderViewHost* source,
as per my comment over in
https://codereview.chromium.org/11146002/diff/2001/content/public/browser/res...
I can't even see how this method will get called at all if we're not using
chrome/ layer code.
Could you add a TODO here to check this out?

[joth, 2012-10-12 23:04:41]

or rather than TODO, i can just raise a bug.


On 12 October 2012 16:03, <joth@chromium.org> wrote:

> lgtm
>
>
> http://codereview.chromium.**org/11112022/diff/34001/**
>
android_webview/native/aw_web_**contents_delegate.cc<http://codereview.chromium.org/11112022/diff/34001/android_webview/native/aw_web_contents_delegate.cc>
> File android_webview/native/aw_web_**contents_delegate.cc (right):
>
> http://codereview.chromium.**org/11112022/diff/34001/**
>
android_webview/native/aw_web_**contents_delegate.cc#newcode56<http://codereview.chromium.org/11112022/diff/34001/android_webview/native/aw_web_contents_delegate.cc#newcode56>
> android_webview/native/aw_web_**contents_delegate.cc:56: bool
> AwWebContentsDelegate::**CanDownload(content::**RenderViewHost* source,
> as per my comment over in
> https://codereview.chromium.**org/11146002/diff/2001/**
>
content/public/browser/**resource_dispatcher_host_**delegate.h#newcode55<https://codereview.chromium.org/11146002/diff/2001/content/public/browser/resource_dispatcher_host_delegate.h#newcode55>
> I can't even see how this method will get called at all if we're not
> using chrome/ layer code.
> Could you add a TODO here to check this out?
>
>
http://codereview.chromium.**org/11112022/<http://codereview.chromium.org/111...
>

[nilesh, 2012-10-12 23:10:16]

http://codereview.chromium.org/11112022/diff/34001/android_webview/native/aw_...
File android_webview/native/aw_web_contents_delegate.cc (right):

http://codereview.chromium.org/11112022/diff/34001/android_webview/native/aw_...
android_webview/native/aw_web_contents_delegate.cc:56: bool
AwWebContentsDelegate::CanDownload(content::RenderViewHost* source,
This will only be called if you use the Chrome Implementation of
ResourceDispatcherHost::DownloadStarting.

[Jói, 2012-10-13 15:20:22]

Please add an appropriate OWNERS file to 	
chrome/browser/component/web_contents_delegate_android.  I've asked for this as
a "to do" in a couple of reviews already, so this time I'll hold my L-G-T-M
until it's been added to this change :)

Cheers,
Jói

[jam, 2012-10-15 06:45:04]

content lgtm

http://codereview.chromium.org/11112022/diff/34001/android_webview/native/aw_...
File android_webview/native/aw_web_contents_delegate.cc (right):

http://codereview.chromium.org/11112022/diff/34001/android_webview/native/aw_...
android_webview/native/aw_web_contents_delegate.cc:18: class DownloadItem;
nit: unnecessary

http://codereview.chromium.org/11112022/diff/34001/chrome/browser/android/chr...
File chrome/browser/android/chrome_web_contents_delegate_android.cc (right):

http://codereview.chromium.org/11112022/diff/34001/chrome/browser/android/chr...
chrome/browser/android/chrome_web_contents_delegate_android.cc:33: }
nit: unnecessary

[nilesh, 2012-10-15 17:13:51]

John- Addressed nits.
Joi,joth - Added OWNERS file, PTAL

http://codereview.chromium.org/11112022/diff/34001/android_webview/native/aw_...
File android_webview/native/aw_web_contents_delegate.cc (right):

http://codereview.chromium.org/11112022/diff/34001/android_webview/native/aw_...
android_webview/native/aw_web_contents_delegate.cc:18: class DownloadItem;
On 2012/10/15 06:45:04, John Abd-El-Malek wrote:
> nit: unnecessary
Removed

http://codereview.chromium.org/11112022/diff/34001/chrome/browser/android/chr...
File chrome/browser/android/chrome_web_contents_delegate_android.cc (right):

http://codereview.chromium.org/11112022/diff/34001/chrome/browser/android/chr...
chrome/browser/android/chrome_web_contents_delegate_android.cc:33: }
Removed

[Jói, 2012-10-15 17:14:33]

LGTM

[commit-bot: I haz the power, 2012-10-15 19:05:55]

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

[commit-bot: I haz the power, 2012-10-15 21:22:59]

Change committed as 161964