If a download is initiated by download attribute, don't use stream viewer

If a download is initiated by download attribute, don't use stream viewer

BUG=599300
Committed: https://crrev.com/a39db05f58be12ac3ce59cfa62dfd4d3fc4a8df2
Cr-Commit-Position: refs/heads/master@{#385253}

[qinmin, 2016-04-01 00:23:51]

qinmin@chromium.org changed reviewers:
+ boliu@chromium.org

[qinmin, 2016-04-01 00:23:51]

@boliu, I vaguely remember you introduced the stream viewer logic, PTAL

[boliu, 2016-04-01 01:20:43]

Questions.. must_download is set here:
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/lo...

There is no mention of the download attribute there?

https://codereview.chromium.org/1854523002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/download/ChromeDownloadDelegate.java
(right):

https://codereview.chromium.org/1854523002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/download/ChromeDownloadDelegate.java:159:
Intent intent = new Intent(Intent.ACTION_VIEW);
heh, dug up the history, I originally copied this code from the android browser
here https://chrome-internal-review.googlesource.com/313 That's CL #313!!

Maybe it's time to step back and ask, do we ever want this behavior? Why would
we end up in requestHttpGetDownload if we didn't want to download this? If we
always want to download, then shouldn't we just remove this?

[qinmin, 2016-04-01 03:52:59]

On 2016/04/01 01:20:43, boliu wrote:
> Questions.. must_download is set here:
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/lo...
> 
> There is no mention of the download attribute there?
> 

must_download is set to true when download attribute is used:
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/lo....
 HTMLAnchorElement::HandleClick() will send an IPC to the browser process to
invoke ResourceDispatcherHostImpl::BeginDownload() when downloadAttr is present,

>
https://codereview.chromium.org/1854523002/diff/1/chrome/android/java/src/org...
> File
>
chrome/android/java/src/org/chromium/chrome/browser/download/ChromeDownloadDelegate.java
> (right):
> 
>
https://codereview.chromium.org/1854523002/diff/1/chrome/android/java/src/org...
>
chrome/android/java/src/org/chromium/chrome/browser/download/ChromeDownloadDelegate.java:159:
> Intent intent = new Intent(Intent.ACTION_VIEW);
> heh, dug up the history, I originally copied this code from the android
browser
> here https://chrome-internal-review.googlesource.com/313 That's CL #313!!
> 
> Maybe it's time to step back and ask, do we ever want this behavior? Why would
> we end up in requestHttpGetDownload if we didn't want to download this? If we
> always want to download, then shouldn't we just remove this?

[qinmin, 2016-04-01 03:53:11]

https://codereview.chromium.org/1854523002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/download/ChromeDownloadDelegate.java
(right):

https://codereview.chromium.org/1854523002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/download/ChromeDownloadDelegate.java:159:
Intent intent = new Intent(Intent.ACTION_VIEW);
On 2016/04/01 01:20:43, boliu wrote:
> heh, dug up the history, I originally copied this code from the android
browser
> here https://chrome-internal-review.googlesource.com/313 That's CL #313!!
> 
> Maybe it's time to step back and ask, do we ever want this behavior? Why would
> we end up in requestHttpGetDownload if we didn't want to download this? If we
> always want to download, then shouldn't we just remove this?

This is still desired. For example, when clicking a link to a pdf or a media
file, user normally want to view the pdf or video instead of downloading it. If
you put/remove download attribute on a pdf link, you will see the difference

[boliu, 2016-04-01 04:10:07]

https://codereview.chromium.org/1854523002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/download/ChromeDownloadDelegate.java
(right):

https://codereview.chromium.org/1854523002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/download/ChromeDownloadDelegate.java:157:
&& !DownloadManagerService.isAttachment(downloadInfo.getContentDisposition())) {
does the content-disposition check here redundant with mustDownload check?

https://codereview.chromium.org/1854523002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/download/ChromeDownloadDelegate.java:159:
Intent intent = new Intent(Intent.ACTION_VIEW);
On 2016/04/01 03:53:11, qinmin wrote:
> On 2016/04/01 01:20:43, boliu wrote:
> > heh, dug up the history, I originally copied this code from the android
> browser
> > here https://chrome-internal-review.googlesource.com/313 That's CL #313!!
> > 
> > Maybe it's time to step back and ask, do we ever want this behavior? Why
would
> > we end up in requestHttpGetDownload if we didn't want to download this? If
we
> > always want to download, then shouldn't we just remove this?
> 
> This is still desired. For example, when clicking a link to a pdf or a media
> file, user normally want to view the pdf or video instead of downloading it.
If
> you put/remove download attribute on a pdf link, you will see the difference 

How does that end up here, and not chrome's normal intent firing flow for
externally handled links?

[qinmin, 2016-04-01 06:52:57]

https://codereview.chromium.org/1854523002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/download/ChromeDownloadDelegate.java
(right):

https://codereview.chromium.org/1854523002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/download/ChromeDownloadDelegate.java:157:
&& !DownloadManagerService.isAttachment(downloadInfo.getContentDisposition())) {
On 2016/04/01 04:10:07, boliu wrote:
> does the content-disposition check here redundant with mustDownload check?

Yes, if content-disposition== attechment, mustdownload is also true. Removed the
redundant check.

https://codereview.chromium.org/1854523002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/download/ChromeDownloadDelegate.java:159:
Intent intent = new Intent(Intent.ACTION_VIEW);
On 2016/04/01 04:10:07, boliu wrote:
> On 2016/04/01 03:53:11, qinmin wrote:
> > On 2016/04/01 01:20:43, boliu wrote:
> > > heh, dug up the history, I originally copied this code from the android
> > browser
> > > here https://chrome-internal-review.googlesource.com/313 That's CL #313!!
> > > 
> > > Maybe it's time to step back and ask, do we ever want this behavior? Why
> would
> > > we end up in requestHttpGetDownload if we didn't want to download this? If
> we
> > > always want to download, then shouldn't we just remove this?
> > 
> > This is still desired. For example, when clicking a link to a pdf or a media
> > file, user normally want to view the pdf or video instead of downloading it.
> If
> > you put/remove download attribute on a pdf link, you will see the difference

> 
> How does that end up here, and not chrome's normal intent firing flow for
> externally handled links?

This really depends on MIME type. For example, with unified-media-pipeline flag,
clicking on a h.264 video link with nonofficial release build  will trigger a
download, rather than navigating to a MediaDocument. so it is better to launch
Gallery app to play the video

[boliu, 2016-04-01 15:51:33]

https://codereview.chromium.org/1854523002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/download/ChromeDownloadDelegate.java
(left):

https://codereview.chromium.org/1854523002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/download/ChromeDownloadDelegate.java:161:
if (!DownloadManagerService.isAttachment(downloadInfo.getContentDisposition()))
{
you can remove the isAttachment implementation as well, or at least make it
private, this is the only caller outside of DownloadManagerService afaict

https://codereview.chromium.org/1854523002/diff/20001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewDownloadDelegate.java
(right):

https://codereview.chromium.org/1854523002/diff/20001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/ContentViewDownloadDelegate.java:15:
* @param mustDownload Whether the content must be downloded.
Maybe explain why mustDownload might be false.

[qinmin, 2016-04-01 18:11:03]

https://codereview.chromium.org/1854523002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/download/ChromeDownloadDelegate.java
(left):

https://codereview.chromium.org/1854523002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/download/ChromeDownloadDelegate.java:161:
if (!DownloadManagerService.isAttachment(downloadInfo.getContentDisposition()))
{
On 2016/04/01 15:51:33, boliu wrote:
> you can remove the isAttachment implementation as well, or at least make it
> private, this is the only caller outside of DownloadManagerService afaict

Done. Removed isAttachment() call. Also, I don't think isAttachment() should
block the file from auto-opened.

https://codereview.chromium.org/1854523002/diff/20001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewDownloadDelegate.java
(right):

https://codereview.chromium.org/1854523002/diff/20001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/ContentViewDownloadDelegate.java:15:
* @param mustDownload Whether the content must be downloded.
On 2016/04/01 15:51:33, boliu wrote:
> Maybe explain why mustDownload might be false.

Done.

[boliu, 2016-04-01 18:18:08]

https://codereview.chromium.org/1854523002/diff/40001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewDownloadDelegate.java
(right):

https://codereview.chromium.org/1854523002/diff/40001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/ContentViewDownloadDelegate.java:15:
* @param mustDownload Whether the content must be downloded. It is false if
there is the link
s/there is//

Also I was aiming for the explanation you gave me, that if what kind of things
ends up in download, but not mustDownload

[boliu, 2016-04-01 18:18:20]

lgtm % comment

[qinmin, 2016-04-01 18:25:02]

https://codereview.chromium.org/1854523002/diff/40001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewDownloadDelegate.java
(right):

https://codereview.chromium.org/1854523002/diff/40001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/ContentViewDownloadDelegate.java:15:
* @param mustDownload Whether the content must be downloded. It is false if
there is the link
On 2016/04/01 18:18:08, boliu wrote:
> s/there is//
> 
> Also I was aiming for the explanation you gave me, that if what kind of things
> ends up in download, but not mustDownload

Done.

[qinmin, 2016-04-01 18:26:20]

qinmin@chromium.org changed reviewers:
+ yfriedman@chromium.org

[qinmin, 2016-04-01 18:26:21]

+Yaron for OWNER

[Yaron, 2016-04-05 02:58:22]

lgtm

[qinmin, 2016-04-05 04:43:16]

The CQ bit was checked by qinmin@chromium.org

[qinmin, 2016-04-05 04:43:17]

The patchset sent to the CQ was uploaded after l-g-t-m from boliu@chromium.org
Link to the patchset: https://codereview.chromium.org/1854523002/#ps60001
(title: "fix comments")

[commit-bot: I haz the power, 2016-04-05 04:43:36]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1854523002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1854523002/60001

[commit-bot: I haz the power, 2016-04-05 04:53:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-04-05 04:53:51]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[qinmin, 2016-04-05 05:06:12]

qinmin@chromium.org changed reviewers:
+ sky@chromium.org

[qinmin, 2016-04-05 05:06:13]

+ sky for chrome/browser/ OWNER

[sky, 2016-04-05 16:56:38]

On 2016/04/05 05:06:13, qinmin wrote:
> + sky for chrome/browser/ OWNER

I'm assuming you mean only
chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc. As
long as that is correct, LGTM

[qinmin, 2016-04-05 17:14:01]

The CQ bit was checked by qinmin@chromium.org

[qinmin, 2016-04-05 17:14:21]

The CQ bit was unchecked by qinmin@chromium.org

[qinmin, 2016-04-05 18:28:04]

The CQ bit was checked by qinmin@chromium.org

[qinmin, 2016-04-05 18:28:04]

The patchset sent to the CQ was uploaded after l-g-t-m from
yfriedman@chromium.org, boliu@chromium.org, sky@chromium.org
Link to the patchset: https://codereview.chromium.org/1854523002/#ps80001
(title: "rebase")

[commit-bot: I haz the power, 2016-04-05 18:28:41]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1854523002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1854523002/80001

[commit-bot: I haz the power, 2016-04-05 19:24:50]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-04-05 19:27:43]

Description was changed from

==========
If a download is initiated by download attribute, don't use stream viewer

BUG=599300
==========

to

==========
If a download is initiated by download attribute, don't use stream viewer

BUG=599300

Committed: https://crrev.com/a39db05f58be12ac3ce59cfa62dfd4d3fc4a8df2
Cr-Commit-Position: refs/heads/master@{#385253}
==========

[commit-bot: I haz the power, 2016-04-05 19:27:44]

Patchset 5 (id:??) landed as
https://crrev.com/a39db05f58be12ac3ce59cfa62dfd4d3fc4a8df2
Cr-Commit-Position: refs/heads/master@{#385253}