Chrome Android PDF Downloads: Pass in HasUserGesture param to Java side.

Chrome Android PDF Downloads: Pass in HasUserGesture param to Java side.

BUG=395194
TEST=manual, it works

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

[tommycli, 2014-07-29 23:00:33]

boliu, benm: PTAL

This change passes the HasUserGesture param up to the Java side for Chrome
Android downloads. This allows us to judiciously auto-open certain mimetypes
securely.

Thanks!

[tommycli, 2014-07-29 23:05:08]

+rsesek: This is part of the change we discussed earlier today to only auto-open
PDFs if there is a user gesture.

[boliu, 2014-07-29 23:26:33]

lg2m

Not as complicated as I thought :)

https://codereview.chromium.org/417263004/diff/40001/content/browser/android/...
File content/browser/android/download_controller_android_impl.cc (right):

https://codereview.chromium.org/417263004/diff/40001/content/browser/android/...
content/browser/android/download_controller_android_impl.cc:424:
content::ResourceRequestInfo::ForRequest(request));
nit: use assignment syntax? Using this on raw pointer feels a bit weird.

[tommycli, 2014-07-29 23:59:40]

boliu: Yeah I thought it would be really involved also, but it turned out to be
pretty reasonable. Your email pointed me to the right set of files to look at.

It's kind of confusing that there's both the native download flow and the
Android Download Manager flow. Not sure why there's both.

https://codereview.chromium.org/417263004/diff/40001/content/browser/android/...
File content/browser/android/download_controller_android_impl.cc (right):

https://codereview.chromium.org/417263004/diff/40001/content/browser/android/...
content/browser/android/download_controller_android_impl.cc:424:
content::ResourceRequestInfo::ForRequest(request));
On 2014/07/29 23:26:33, boliu wrote:
> nit: use assignment syntax? Using this on raw pointer feels a bit weird.

Done.

[boliu, 2014-07-30 00:04:06]

On 2014/07/29 23:59:40, tommycli wrote:
> boliu: Yeah I thought it would be really involved also, but it turned out to
be
> pretty reasonable. Your email pointed me to the right set of files to look at.
> 
> It's kind of confusing that there's both the native download flow and the
> Android Download Manager flow. Not sure why there's both.

History is I implemented the android download manager path (internship). Then
realized there are cases it can't handle, so brought back the chrome path.

Benefit of android download manager is it can do downloads in the background
even if chrome is killed. Although it would be interesting to see collect stats
and check if large file downloads happens often enough to be worth maintaining
this path.

[tommycli, 2014-07-30 00:08:06]

On 2014/07/30 00:04:06, boliu wrote:
> On 2014/07/29 23:59:40, tommycli wrote:
> > boliu: Yeah I thought it would be really involved also, but it turned out to
> be
> > pretty reasonable. Your email pointed me to the right set of files to look
at.
> > 
> > It's kind of confusing that there's both the native download flow and the
> > Android Download Manager flow. Not sure why there's both.
> 
> History is I implemented the android download manager path (internship). Then
> realized there are cases it can't handle, so brought back the chrome path.
> 
> Benefit of android download manager is it can do downloads in the background
> even if chrome is killed. Although it would be interesting to see collect
stats
> and check if large file downloads happens often enough to be worth maintaining
> this path.

Whoa. That's a pretty piece for an intern project. Good to know the history ;)

[tommycli, 2014-07-30 18:11:09]

+tedchoc, +yfriedman, also qualified to review this

[Yaron, 2014-07-30 18:18:28]

lgtm

There are other reasons we use the chrome download path (one being
authentication).

https://codereview.chromium.org/417263004/diff/60001/content/public/android/j...
File
content/public/android/java/src/org/chromium/content/browser/DownloadController.java
(right):

https://codereview.chromium.org/417263004/diff/60001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/DownloadController.java:72:
Log.w(LOGTAG, String.format("Has user gesture: %d", hasUserGesture));
Remove log

https://codereview.chromium.org/417263004/diff/60001/content/public/android/j...
File
content/public/android/java/src/org/chromium/content/browser/DownloadInfo.java
(right):

https://codereview.chromium.org/417263004/diff/60001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/DownloadInfo.java:97:
public boolean getHasUserGesture() {
nit: just call this hasUserGesture

[tommycli, 2014-07-30 18:49:15]

yaron: Thanks!

https://codereview.chromium.org/417263004/diff/60001/content/public/android/j...
File
content/public/android/java/src/org/chromium/content/browser/DownloadController.java
(right):

https://codereview.chromium.org/417263004/diff/60001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/DownloadController.java:72:
Log.w(LOGTAG, String.format("Has user gesture: %d", hasUserGesture));
On 2014/07/30 18:18:28, Yaron wrote:
> Remove log

Done.

https://codereview.chromium.org/417263004/diff/60001/content/public/android/j...
File
content/public/android/java/src/org/chromium/content/browser/DownloadInfo.java
(right):

https://codereview.chromium.org/417263004/diff/60001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/DownloadInfo.java:97:
public boolean getHasUserGesture() {
On 2014/07/30 18:18:28, Yaron wrote:
> nit: just call this hasUserGesture

Done.

[tommycli, 2014-07-30 19:11:06]

The CQ bit was checked by tommycli@chromium.org

[commit-bot: I haz the power, 2014-07-30 19:12:55]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/tommycli@chromium.org/417263004/100001

[commit-bot: I haz the power, 2014-07-30 19:42:09]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/35471)
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/40502)
  android_dbg_triggered_tests on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tri...)
  ios_dbg_simulator on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device/bu...)
  ios_rel_device_ninja on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)

[tommycli, 2014-07-30 20:55:33]

The CQ bit was unchecked by tommycli@chromium.org

[tommycli, 2014-07-30 20:55:34]

The CQ bit was checked by tommycli@chromium.org

[commit-bot: I haz the power, 2014-07-30 20:59:51]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/tommycli@chromium.org/417263004/100001

[commit-bot: I haz the power, 2014-07-30 22:31:54]

Change committed as 286618