Adding a call to support download initiated context menu

Adding a call to support download initiated context menu

The android download manager does not support authentication.
Therefore, use chrome path for download initiated by context menu.
gerrit change: 35490

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

[qinmin, 2013-04-12 00:35:51]

PTAL

[nilesh, 2013-04-12 16:27:45]

https://codereview.chromium.org/13859009/diff/1/content/browser/android/downl...
File content/browser/android/download_controller_android_impl.cc (right):

https://codereview.chromium.org/13859009/diff/1/content/browser/android/downl...
content/browser/android/download_controller_android_impl.cc:325:
DownloadManager* dlm = content::BrowserContext::GetDownloadManager(
dont need content::
here and below

https://codereview.chromium.org/13859009/diff/1/content/browser/android/downl...
content/browser/android/download_controller_android_impl.cc:325:
DownloadManager* dlm = content::BrowserContext::GetDownloadManager(
We are supposed to use the _impl version inside content.
DownloadManagerImpl* dlm =
static_cast<DownloadManagerImpl*>(BrowserContext::GetDownloadManager(
          web_contents()->GetBrowserContext());
I am not sure how strictly this is followed though.

Also please explicitly include what you use. 
DownloadManagerImpl, DownloadUrlParameters, Referrer

https://codereview.chromium.org/13859009/diff/1/content/browser/android/downl...
content/browser/android/download_controller_android_impl.cc:331: if (!is_link)
if (is_link) .. else ..
I find this easier to read.

https://codereview.chromium.org/13859009/diff/1/content/browser/android/downl...
content/browser/android/download_controller_android_impl.cc:334:
dl_params->set_referrer_encoding(params.frame_charset);
Why is referrer encoding only set for is_link?

https://codereview.chromium.org/13859009/diff/1/content/public/browser/androi...
File content/public/browser/android/download_controller_android.h (right):

https://codereview.chromium.org/13859009/diff/1/content/public/browser/androi...
content/public/browser/android/download_controller_android.h:34: bool is_link) =
0;
looking at the caller, is_link can be decided based on ContextMenuParams. Dont
need this extra param.

[nilesh, 2013-04-12 16:44:14]

https://codereview.chromium.org/13859009/diff/1/content/public/browser/androi...
File content/public/browser/android/download_controller_android.h (right):

https://codereview.chromium.org/13859009/diff/1/content/public/browser/androi...
content/public/browser/android/download_controller_android.h:29: virtual void
OnPostDownloadStarted(DownloadItem* download_item) = 0;
Also rename this to OnDownloadStarted. With this and your earlier change for
http_auth, this methods is called for more than just Post downloads.

[nilesh, 2013-04-12 16:50:07]

https://codereview.chromium.org/13859009/diff/1/content/public/browser/androi...
File content/public/browser/android/download_controller_android.h (right):

https://codereview.chromium.org/13859009/diff/1/content/public/browser/androi...
content/public/browser/android/download_controller_android.h:29: virtual void
OnPostDownloadStarted(DownloadItem* download_item) = 0;
actually you can add a note in the comment and a TODO. POST is used in the
method names on the java side too (DownloadController and
ContentViewDownloadDelegate)

[qinmin, 2013-04-15 18:56:51]

https://codereview.chromium.org/13859009/diff/1/content/browser/android/downl...
File content/browser/android/download_controller_android_impl.cc (right):

https://codereview.chromium.org/13859009/diff/1/content/browser/android/downl...
content/browser/android/download_controller_android_impl.cc:325:
DownloadManager* dlm = content::BrowserContext::GetDownloadManager(
On 2013/04/12 16:27:45, nilesh wrote:
> dont need content::
> here and below

Done.

https://codereview.chromium.org/13859009/diff/1/content/browser/android/downl...
content/browser/android/download_controller_android_impl.cc:325:
DownloadManager* dlm = content::BrowserContext::GetDownloadManager(
On 2013/04/12 16:27:45, nilesh wrote:
> We are supposed to use the _impl version inside content.
> DownloadManagerImpl* dlm =
> static_cast<DownloadManagerImpl*>(BrowserContext::GetDownloadManager(
>           web_contents()->GetBrowserContext());
> I am not sure how strictly this is followed though.
> 
> Also please explicitly include what you use. 
> DownloadManagerImpl, DownloadUrlParameters, Referrer

Done.

https://codereview.chromium.org/13859009/diff/1/content/browser/android/downl...
content/browser/android/download_controller_android_impl.cc:331: if (!is_link)
On 2013/04/12 16:27:45, nilesh wrote:
> if (is_link) .. else ..
> I find this easier to read.

Done.

https://codereview.chromium.org/13859009/diff/1/content/browser/android/downl...
content/browser/android/download_controller_android_impl.cc:334:
dl_params->set_referrer_encoding(params.frame_charset);
This is from the upstream saving logic. I think for image/video files, referrer
doesn't make too much sense. You can embed a url from any sites on ur page, so
the referrer doesn't matter.

On 2013/04/12 16:27:45, nilesh wrote:
> Why is referrer encoding only set for is_link?

https://codereview.chromium.org/13859009/diff/1/content/public/browser/androi...
File content/public/browser/android/download_controller_android.h (right):

https://codereview.chromium.org/13859009/diff/1/content/public/browser/androi...
content/public/browser/android/download_controller_android.h:29: virtual void
OnPostDownloadStarted(DownloadItem* download_item) = 0;
On 2013/04/12 16:44:14, nilesh wrote:
> Also rename this to OnDownloadStarted. With this and your earlier change for
> http_auth, this methods is called for more than just Post downloads.

Done.

https://codereview.chromium.org/13859009/diff/1/content/public/browser/androi...
content/public/browser/android/download_controller_android.h:29: virtual void
OnPostDownloadStarted(DownloadItem* download_item) = 0;
Fixed naming in all the related classes, and also the comments.
On 2013/04/12 16:50:07, nilesh wrote:
> actually you can add a note in the comment and a TODO. POST is used in the
> method names on the java side too (DownloadController and
> ContentViewDownloadDelegate)

https://codereview.chromium.org/13859009/diff/1/content/public/browser/androi...
content/public/browser/android/download_controller_android.h:34: bool is_link) =
0;
we still need this. We need to know whether "save image" or "save image link" is
selected.

On 2013/04/12 16:27:45, nilesh wrote:
> looking at the caller, is_link can be decided based on ContextMenuParams. Dont
> need this extra param.

[nilesh, 2013-04-15 20:26:46]

LGTM
do check with joth/boliu to find out if changing the ContentViewDownloadDelegate
changes anything for webview.

[boliu, 2013-04-15 20:38:19]

On 2013/04/15 20:26:46, nilesh wrote:
> LGTM
> do check with joth/boliu to find out if changing the
ContentViewDownloadDelegate
> changes anything for webview.

Webview is not using all these chrome download features, so should not need to
change anything for webview.

[qinmin, 2013-04-15 21:36:05]

@Yaron, would you please take a look?

[qinmin, 2013-04-15 21:49:58]

+asanka for OWNER

[asanka, 2013-04-15 22:15:58]

/chrome/browser/download LGTM. Thanks!

[Yaron, 2013-04-16 02:03:12]

lgtm

[qinmin, 2013-04-16 16:17:33]

Committed patchset #2 manually as r194366 (presubmit successful).