Add android download controller.

Add android download controller.

The android download controller is responsible for sending GET
downloads to the JAVA side (to be handled by Android DownloadManager).

 - It also prepares all the info needed for the download (cookies etc)
 - POST downloads are handled natively, the download controller can be
   used to pass start/completed notifications to the JAVA side.

BUG=
TEST=


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

[nilesh, 2012-05-25 17:14:47]

Hi John,
Please take a look.

DownloadController::NewGetDownload will be called by the android implementation
of WebContentsDelegate::CanDownload

ContentViewClient (which implements WebContentsDelegate) is not upstream yet.

[jochen (gone - plz use gerrit), 2012-05-25 21:29:36]

http://codereview.chromium.org/10449017/diff/10001/content/browser/android/do...
File content/browser/android/download_controller.cc (right):

http://codereview.chromium.org/10449017/diff/10001/content/browser/android/do...
content/browser/android/download_controller.cc:1: // Copyright (c) 2011 The
Chromium Authors. All rights reserved.
2012

http://codereview.chromium.org/10449017/diff/10001/content/browser/android/do...
content/browser/android/download_controller.cc:38: static void Init(JNIEnv* env,
jobject obj) {
where is this invoked from? Can it be in the anonymous namespace below?

http://codereview.chromium.org/10449017/diff/10001/content/browser/android/do...
content/browser/android/download_controller.cc:44: static const char*
kDownloadControllerClassPathName =
no static required

http://codereview.chromium.org/10449017/diff/10001/content/browser/android/do...
content/browser/android/download_controller.cc:56: jweak obj;
variables after methods

http://codereview.chromium.org/10449017/diff/10001/content/browser/android/do...
content/browser/android/download_controller.cc:92: render_process_id,
request_id);
should fit on previous line?

http://codereview.chromium.org/10449017/diff/10001/content/browser/android/do...
content/browser/android/download_controller.cc:117:
request->context()->cookie_store()->GetCookiesWithOptionsAsync(
please ask the request context' network delegate whether you can get cookies

http://codereview.chromium.org/10449017/diff/10001/content/browser/android/do...
content/browser/android/download_controller.cc:163:
Java_DownloadController_newHttpGetDownload(env,
env should go on the next line

http://codereview.chromium.org/10449017/diff/10001/content/browser/android/do...
File content/browser/android/download_controller.h (right):

http://codereview.chromium.org/10449017/diff/10001/content/browser/android/do...
content/browser/android/download_controller.h:1: // Copyright (c) 2011 The
Chromium Authors. All rights reserved.
2012

http://codereview.chromium.org/10449017/diff/10001/content/browser/android/do...
content/browser/android/download_controller.h:19: #ifndef
CONTENT_BROWSER_ANDROID_DOWNLOAD_CONTROLLER_H_
should this file go into content/browser/download/android/ ?

http://codereview.chromium.org/10449017/diff/10001/content/browser/android/do...
content/browser/android/download_controller.h:42: class DownloadController :
public content::DownloadItem::Observer {
code in src/content should be in the content namespace

http://codereview.chromium.org/10449017/diff/10001/content/browser/android/do...
content/browser/android/download_controller.h:61: virtual void
OnDownloadOpened(content::DownloadItem* item) OVERRIDE { }
virtual methods can't be inlined, therefore, the body should be in the .cc file

http://codereview.chromium.org/10449017/diff/10001/content/browser/android/do...
content/browser/android/download_controller.h:69: struct DownloadInfoAndroid {
definitions come before ctor/dtor

http://codereview.chromium.org/10449017/diff/10001/content/browser/android/do...
content/browser/android/download_controller.h:103: jobject GetContentView(int
render_process_id, int render_view_id);
empty line between method decls and variables

http://codereview.chromium.org/10449017/diff/10001/content/browser/android/do...
content/browser/android/download_controller.h:104: struct JavaObject;
type declarations come before methods

http://codereview.chromium.org/10449017/diff/10001/content/browser/android/do...
content/browser/android/download_controller.h:108: JavaObject* java_object();
methods before member variables

http://codereview.chromium.org/10449017/diff/10001/content/browser/android/do...
content/browser/android/download_controller.h:109: };
disallow copy/assign

http://codereview.chromium.org/10449017/diff/10001/content/browser/android/do...
content/browser/android/download_controller.h:111: bool
RegisterDownloadController(JNIEnv* env);
can this be a static method on DownloadController instead?

[nilesh, 2012-05-25 23:10:22]

@jochen: Thanks for the comments. PTAL.

http://codereview.chromium.org/10449017/diff/10001/content/browser/android/do...
File content/browser/android/download_controller.cc (right):

http://codereview.chromium.org/10449017/diff/10001/content/browser/android/do...
content/browser/android/download_controller.cc:1: // Copyright (c) 2011 The
Chromium Authors. All rights reserved.
On 2012/05/25 21:29:37, jochen wrote:
> 2012

Done.

http://codereview.chromium.org/10449017/diff/10001/content/browser/android/do...
content/browser/android/download_controller.cc:38: static void Init(JNIEnv* env,
jobject obj) {
On 2012/05/25 21:29:37, jochen wrote:
> where is this invoked from? Can it be in the anonymous namespace below?

Its called via JNI from DownloadController.java (constructor calls
nativeInit()). It is declared (and mapped to nativeInit) in the generated
jni/download_controller_jni.h included above.

http://codereview.chromium.org/10449017/diff/10001/content/browser/android/do...
content/browser/android/download_controller.cc:44: static const char*
kDownloadControllerClassPathName =
On 2012/05/25 21:29:37, jochen wrote:
> no static required

Done.

http://codereview.chromium.org/10449017/diff/10001/content/browser/android/do...
content/browser/android/download_controller.cc:56: jweak obj;
On 2012/05/25 21:29:37, jochen wrote:
> variables after methods

Done.

http://codereview.chromium.org/10449017/diff/10001/content/browser/android/do...
content/browser/android/download_controller.cc:92: render_process_id,
request_id);
On 2012/05/25 21:29:37, jochen wrote:
> should fit on previous line?

Done.

http://codereview.chromium.org/10449017/diff/10001/content/browser/android/do...
content/browser/android/download_controller.cc:117:
request->context()->cookie_store()->GetCookiesWithOptionsAsync(
On 2012/05/25 21:29:37, jochen wrote:
> please ask the request context' network delegate whether you can get cookies

NetworkDelegate::CanGetCookies needs a CookieList. I am not sure I have one
here. Can you explain a bit more? Thanks.

http://codereview.chromium.org/10449017/diff/10001/content/browser/android/do...
content/browser/android/download_controller.cc:163:
Java_DownloadController_newHttpGetDownload(env,
On 2012/05/25 21:29:37, jochen wrote:
> env should go on the next line

Done.

http://codereview.chromium.org/10449017/diff/10001/content/browser/android/do...
File content/browser/android/download_controller.h (right):

http://codereview.chromium.org/10449017/diff/10001/content/browser/android/do...
content/browser/android/download_controller.h:1: // Copyright (c) 2011 The
Chromium Authors. All rights reserved.
On 2012/05/25 21:29:37, jochen wrote:
> 2012

Done.

http://codereview.chromium.org/10449017/diff/10001/content/browser/android/do...
content/browser/android/download_controller.h:19: #ifndef
CONTENT_BROWSER_ANDROID_DOWNLOAD_CONTROLLER_H_
On 2012/05/25 21:29:37, jochen wrote:
> should this file go into content/browser/download/android/ ?

We dont plan to have multiple files here.
Maybe content/browser/download/download_controller_android.h ?

Other examples:
content/browser/device_orientation/data_fetcher_impl_android.h
content/browser/geolocation/location_api_adapter_android.h

What do you think?

http://codereview.chromium.org/10449017/diff/10001/content/browser/android/do...
content/browser/android/download_controller.h:42: class DownloadController :
public content::DownloadItem::Observer {
On 2012/05/25 21:29:37, jochen wrote:
> code in src/content should be in the content namespace

Done.

http://codereview.chromium.org/10449017/diff/10001/content/browser/android/do...
content/browser/android/download_controller.h:61: virtual void
OnDownloadOpened(content::DownloadItem* item) OVERRIDE { }
On 2012/05/25 21:29:37, jochen wrote:
> virtual methods can't be inlined, therefore, the body should be in the .cc
file

Done.

http://codereview.chromium.org/10449017/diff/10001/content/browser/android/do...
content/browser/android/download_controller.h:69: struct DownloadInfoAndroid {
On 2012/05/25 21:29:37, jochen wrote:
> definitions come before ctor/dtor

Done.

http://codereview.chromium.org/10449017/diff/10001/content/browser/android/do...
content/browser/android/download_controller.h:103: jobject GetContentView(int
render_process_id, int render_view_id);
On 2012/05/25 21:29:37, jochen wrote:
> empty line between method decls and variables

Done.

http://codereview.chromium.org/10449017/diff/10001/content/browser/android/do...
content/browser/android/download_controller.h:104: struct JavaObject;
On 2012/05/25 21:29:37, jochen wrote:
> type declarations come before methods

Done.

http://codereview.chromium.org/10449017/diff/10001/content/browser/android/do...
content/browser/android/download_controller.h:108: JavaObject* java_object();
On 2012/05/25 21:29:37, jochen wrote:
> methods before member variables

Done.

http://codereview.chromium.org/10449017/diff/10001/content/browser/android/do...
content/browser/android/download_controller.h:109: };
On 2012/05/25 21:29:37, jochen wrote:
> disallow copy/assign

Done.

http://codereview.chromium.org/10449017/diff/10001/content/browser/android/do...
content/browser/android/download_controller.h:111: bool
RegisterDownloadController(JNIEnv* env);
On 2012/05/25 21:29:37, jochen wrote:
> can this be a static method on DownloadController instead?

Done.

[jochen (gone - plz use gerrit), 2012-05-26 10:12:44]

http://codereview.chromium.org/10449017/diff/12002/content/browser/android/do...
File content/browser/android/download_controller.cc (right):

http://codereview.chromium.org/10449017/diff/12002/content/browser/android/do...
content/browser/android/download_controller.cc:38: static void Init(JNIEnv* env,
jobject obj) {
if it's invoked within this compilation unit, you could make it non-static and
move it into the anon namespace, but that's optional

http://codereview.chromium.org/10449017/diff/12002/content/browser/android/do...
content/browser/android/download_controller.cc:114:
request->context()->cookie_store()->GetCookiesWithOptionsAsync(
see
http://code.google.com/searchframe#OAMlx_jo-ck/src/net/url_request/url_reques...
for the pattern to use

[nilesh, 2012-05-29 18:21:22]

PTAL.

http://codereview.chromium.org/10449017/diff/12002/content/browser/android/do...
File content/browser/android/download_controller.cc (right):

http://codereview.chromium.org/10449017/diff/12002/content/browser/android/do...
content/browser/android/download_controller.cc:38: static void Init(JNIEnv* env,
jobject obj) {
On 2012/05/26 10:12:44, jochen wrote:
> if it's invoked within this compilation unit, you could make it non-static and
> move it into the anon namespace, but that's optional

The method is declared in jni/download_controller_jni.h
I cannot move it into anonymous namespace.

http://codereview.chromium.org/10449017/diff/12002/content/browser/android/do...
content/browser/android/download_controller.cc:114:
request->context()->cookie_store()->GetCookiesWithOptionsAsync(
On 2012/05/26 10:12:44, jochen wrote:
> see
>
http://code.google.com/searchframe#OAMlx_jo-ck/src/net/url_request/url_reques...
> for the pattern to use

Done. Thanks for the pointer.

[jam, 2012-05-29 21:48:05]

http://codereview.chromium.org/10449017/diff/12004/content/browser/android/do...
File content/browser/android/download_controller.cc (right):

http://codereview.chromium.org/10449017/diff/12004/content/browser/android/do...
content/browser/android/download_controller.cc:19: #include
"content/public/browser/web_contents.h"
nit: for all these includes, our convention is to include the impl files in code
in content

http://codereview.chromium.org/10449017/diff/12004/content/browser/android/do...
content/browser/android/download_controller.cc:35: using content::WebContents;
these include statements don't appear to be needed since the code is in the
content namespace

http://codereview.chromium.org/10449017/diff/12004/content/browser/android/do...
content/browser/android/download_controller.cc:85: content::RenderViewHost*
render_view_host,
nit: get rid of all the "content::" since you're in content namespace already

http://codereview.chromium.org/10449017/diff/12004/content/browser/android/do...
File content/browser/android/download_controller.h (right):

http://codereview.chromium.org/10449017/diff/12004/content/browser/android/do...
content/browser/android/download_controller.h:40: class DownloadController :
public content::DownloadItem::Observer {
nit: no content:: since you're in content namespace, also below

http://codereview.chromium.org/10449017/diff/12004/content/browser/android/do...
content/browser/android/download_controller.h:50: void
NewGetDownload(content::RenderViewHost* source,
nit: this function name isn't consistent with how we usually name things. i.e.
since it's doing an action, it should start with a verb, so CreateDownload. Get
is usually GET afaik, since CreateGetDownload sounds a little confusing, so it
would be CreateGETDownload

http://codereview.chromium.org/10449017/diff/12004/content/browser/android/do...
content/browser/android/download_controller.h:117: JavaObject* java_object();
nit: per style guide, since this is defined in the cc file it should be
GetJavaObject()

[nilesh, 2012-05-30 00:29:57]

Addressed comments. PTAL.

http://codereview.chromium.org/10449017/diff/12004/content/browser/android/do...
File content/browser/android/download_controller.cc (right):

http://codereview.chromium.org/10449017/diff/12004/content/browser/android/do...
content/browser/android/download_controller.cc:19: #include
"content/public/browser/web_contents.h"
On 2012/05/29 21:48:05, John Abd-El-Malek wrote:
> nit: for all these includes, our convention is to include the impl files in
code
> in content

Done.

http://codereview.chromium.org/10449017/diff/12004/content/browser/android/do...
content/browser/android/download_controller.cc:35: using content::WebContents;
On 2012/05/29 21:48:05, John Abd-El-Malek wrote:
> these include statements don't appear to be needed since the code is in the
> content namespace

Done.

http://codereview.chromium.org/10449017/diff/12004/content/browser/android/do...
content/browser/android/download_controller.cc:85: content::RenderViewHost*
render_view_host,
On 2012/05/29 21:48:05, John Abd-El-Malek wrote:
> nit: get rid of all the "content::" since you're in content namespace already

Done. I forgot to remove all these after I moved the class in content namespace.

http://codereview.chromium.org/10449017/diff/12004/content/browser/android/do...
File content/browser/android/download_controller.h (right):

http://codereview.chromium.org/10449017/diff/12004/content/browser/android/do...
content/browser/android/download_controller.h:40: class DownloadController :
public content::DownloadItem::Observer {
On 2012/05/29 21:48:05, John Abd-El-Malek wrote:
> nit: no content:: since you're in content namespace, also below

Done.

http://codereview.chromium.org/10449017/diff/12004/content/browser/android/do...
content/browser/android/download_controller.h:50: void
NewGetDownload(content::RenderViewHost* source,
On 2012/05/29 21:48:05, John Abd-El-Malek wrote:
> nit: this function name isn't consistent with how we usually name things. i.e.
> since it's doing an action, it should start with a verb, so CreateDownload.
Get
> is usually GET afaik, since CreateGetDownload sounds a little confusing, so it
> would be CreateGETDownload

Done.

http://codereview.chromium.org/10449017/diff/12004/content/browser/android/do...
content/browser/android/download_controller.h:117: JavaObject* java_object();
On 2012/05/29 21:48:05, John Abd-El-Malek wrote:
> nit: per style guide, since this is defined in the cc file it should be
> GetJavaObject()

Done.

[jam, 2012-05-30 14:59:49]

lgtm

http://codereview.chromium.org/10449017/diff/2006/content/browser/android/dow...
File content/browser/android/download_controller.cc (right):

http://codereview.chromium.org/10449017/diff/2006/content/browser/android/dow...
content/browser/android/download_controller.cc:176:
BrowserThread::PostTask(BrowserThread::UI, FROM_HERE,
nit: to follow the style guide, for each line with parameters, the first
parameter has to start on the same indentation. so move BrowserThread::UI to the
next line

[nilesh, 2012-05-30 17:04:09]

Thanks for the review.

http://codereview.chromium.org/10449017/diff/2006/content/browser/android/dow...
File content/browser/android/download_controller.cc (right):

http://codereview.chromium.org/10449017/diff/2006/content/browser/android/dow...
content/browser/android/download_controller.cc:176:
BrowserThread::PostTask(BrowserThread::UI, FROM_HERE,
On 2012/05/30 14:59:49, John Abd-El-Malek wrote:
> nit: to follow the style guide, for each line with parameters, the first
> parameter has to start on the same indentation. so move BrowserThread::UI to
the
> next line

Done.

[nilesh, 2012-05-30 17:06:03]

On 2012/05/30 17:04:09, nileshagrawal1 wrote:
> Thanks for the review.
> 
>
http://codereview.chromium.org/10449017/diff/2006/content/browser/android/dow...
> File content/browser/android/download_controller.cc (right):
> 
>
http://codereview.chromium.org/10449017/diff/2006/content/browser/android/dow...
> content/browser/android/download_controller.cc:176:
> BrowserThread::PostTask(BrowserThread::UI, FROM_HERE,
> On 2012/05/30 14:59:49, John Abd-El-Malek wrote:
> > nit: to follow the style guide, for each line with parameters, the first
> > parameter has to start on the same indentation. so move BrowserThread::UI to
> the
> > next line
> 
> Done.

@jochen: I will go ahead and submit this. Let me know if you have more comments.
I will address them in  another CL.

[commit-bot: I haz the power, 2012-05-30 17:38:12]

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

[jochen (gone - plz use gerrit), 2012-05-30 17:56:53]

I'm fine, thank you!

On Wed, May 30, 2012 at 7:38 PM, <commit-bot@chromium.org> wrote:

> CQ is trying da patch. Follow status at
> https://chromium-status.**appspot.com/cq/nileshagrawal@**
>
chromium.org/10449017/17002<https://chromium-status.appspot.com/cq/nileshagrawal@chromium.org/10449017/17002>
>
>
>
https://chromiumcodereview.**appspot.com/10449017/<https://chromiumcodereview...
>

[commit-bot: I haz the power, 2012-05-30 19:21:11]

Change committed as 139600