Defaulting all downloads to go through Chrome network stack

Defaulting all downloads to go through Chrome network stack

In M53, we started a experiment on disabling system DownloadManager.
And In M54, we expect the experiment to be expanded to 100% of the user.
This change removes this field trial flag to default all download through Chrome.
This allows us to unify the download code path between android and desktop.

Android DownloadManager will still be used by OMA downloads.
And will clean up all the code that calls into ChromeDownloadDelegate::EnqueueDownloadManagerRequest() in a separate CL.

BUG=647755
Committed: https://crrev.com/aea2c7d5fcc45b9c606f7595c4c12f652ce03abd
Cr-Commit-Position: refs/heads/master@{#420179}

[qinmin, 2016-09-16 18:57:32]

qinmin@chromium.org changed reviewers:
+ asanka@chromium.org, dfalcantara@chromium.org, thakis@chromium.org

[qinmin, 2016-09-16 18:57:32]

PTAL

[qinmin, 2016-09-16 18:58:25]

Description was changed from

==========
Defaulting all downloads to go through Chrome network stack

In M53, we started a experiment on disabling system DownloadManager.
And In M54, we expect the experiment to be expanded to 100% of the user.
This change removes this field trial flag to default all download
through Chrome.

BUG=647755
==========

to

==========
Defaulting all downloads to go through Chrome network stack

In M53, we started a experiment on disabling system DownloadManager.
And In M54, we expect the experiment to be expanded to 100% of the user.
This change removes this field trial flag to default all download through
Chrome.
This allows us to unify the download code path between android and desktop.

BUG=647755
==========

[gone, 2016-09-19 17:06:11]

lgtm

[Nico, 2016-09-19 17:39:33]

what about
https://cs.chromium.org/chromium/src/chrome/browser/android/chrome_feature_li...
https://cs.chromium.org/chromium/src/chrome/app/generated_resources.grd?q=IDS...
etc?

[qinmin, 2016-09-19 18:03:54]

On 2016/09/19 17:39:33, Nico wrote:
> what about
>
https://cs.chromium.org/chromium/src/chrome/browser/android/chrome_feature_li...
>
https://cs.chromium.org/chromium/src/chrome/app/generated_resources.grd?q=IDS...
> etc?

thanks for catching, removed

[Nico, 2016-09-19 18:04:29]

On 2016/09/19 18:03:54, qinmin wrote:
> On 2016/09/19 17:39:33, Nico wrote:
> > what about
> >
>
https://cs.chromium.org/chromium/src/chrome/browser/android/chrome_feature_li...
> >
>
https://cs.chromium.org/chromium/src/chrome/app/generated_resources.grd?q=IDS...
> > etc?
> 
> thanks for catching, removed

did you see the first of the two links?

[qinmin, 2016-09-19 18:08:27]

On 2016/09/19 18:04:29, Nico wrote:
> On 2016/09/19 18:03:54, qinmin wrote:
> > On 2016/09/19 17:39:33, Nico wrote:
> > > what about
> > >
> >
>
https://cs.chromium.org/chromium/src/chrome/browser/android/chrome_feature_li...
> > >
> >
>
https://cs.chromium.org/chromium/src/chrome/app/generated_resources.grd?q=IDS...
> > > etc?
> > 
> > thanks for catching, removed
> 
> did you see the first of the two links?

missed it, done

[qinmin, 2016-09-19 18:14:21]

Patchset #4 (id:60001) has been deleted

[qinmin, 2016-09-19 18:15:20]

On 2016/09/19 18:08:27, qinmin wrote:
> On 2016/09/19 18:04:29, Nico wrote:
> > On 2016/09/19 18:03:54, qinmin wrote:
> > > On 2016/09/19 17:39:33, Nico wrote:
> > > > what about
> > > >
> > >
> >
>
https://cs.chromium.org/chromium/src/chrome/browser/android/chrome_feature_li...
> > > >
> > >
> >
>
https://cs.chromium.org/chromium/src/chrome/app/generated_resources.grd?q=IDS...
> > > > etc?
> > > 
> > > thanks for catching, removed
> > 
> > did you see the first of the two links?
> 
> missed it, done

wierd, I removed that in chrome_geature_list.h, and git cl upload always skips
that file, I remembered I have done that in ps#1, will figure out why

[qinmin, 2016-09-19 18:17:12]

On 2016/09/19 18:15:20, qinmin wrote:
> On 2016/09/19 18:08:27, qinmin wrote:
> > On 2016/09/19 18:04:29, Nico wrote:
> > > On 2016/09/19 18:03:54, qinmin wrote:
> > > > On 2016/09/19 17:39:33, Nico wrote:
> > > > > what about
> > > > >
> > > >
> > >
> >
>
https://cs.chromium.org/chromium/src/chrome/browser/android/chrome_feature_li...
> > > > >
> > > >
> > >
> >
>
https://cs.chromium.org/chromium/src/chrome/app/generated_resources.grd?q=IDS...
> > > > > etc?
> > > > 
> > > > thanks for catching, removed
> > > 
> > > did you see the first of the two links?
> > 
> > missed it, done
> 
> wierd, I removed that in chrome_geature_list.h, and git cl upload always skips
> that file, I remembered I have done that in ps#1, will figure out why

upload again, this time it is ok

[Nico, 2016-09-19 18:18:44]

lgtm

[qinmin, 2016-09-19 18:33:35]

The CQ bit was checked by qinmin@chromium.org

[qinmin, 2016-09-19 18:33:36]

The patchset sent to the CQ was uploaded after l-g-t-m from
dfalcantara@chromium.org
Link to the patchset: https://codereview.chromium.org/2341643008/#ps80001
(title: "remove extern")

[commit-bot: I haz the power, 2016-09-19 18:34:20]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-19 18:37:59]

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

[commit-bot: I haz the power, 2016-09-19 18:38:00]

Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  chromeos_x86-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[asanka, 2016-09-19 19:29:12]

* Update the ExternalNavigationDelegateImplTest.java to remove tests with
SystemDownloadManager.

* Update the comment in AndroidUIControllerDelegate (download_ui_controller.cc)
which will no longer be valid once Chrome is handling all downloads.

https://codereview.chromium.org/2341643008/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadManagerService.java
(right):

https://codereview.chromium.org/2341643008/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadManagerService.java:1077:
// Open the Android Download Manager.
Why try to open this at all once Chrome is handling all the downloads?

Why is the behavior of openDownloadsPage() sensitive to whether the Chrome
activity is in the foreground or not?

https://codereview.chromium.org/2341643008/diff/80001/chrome/browser/android/...
File chrome/browser/android/intercept_download_resource_throttle.cc (left):

https://codereview.chromium.org/2341643008/diff/80001/chrome/browser/android/...
chrome/browser/android/intercept_download_resource_throttle.cc:40:
UMA_HISTOGRAM_ENUMERATION("MobileDownload.InterceptFailureReason",
Mark this histogram as being deprecated in histograms.xml.

[gone, 2016-09-19 19:33:56]

https://codereview.chromium.org/2341643008/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadManagerService.java
(right):

https://codereview.chromium.org/2341643008/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadManagerService.java:1077:
// Open the Android Download Manager.
On 2016/09/19 19:29:12, asanka wrote:
> Why try to open this at all once Chrome is handling all the downloads?
> 
> Why is the behavior of openDownloadsPage() sensitive to whether the Chrome
> activity is in the foreground or not?

Download Home is tied to a tab: on tablets it's displayed inside a tab and in
general the downloads displayed are keyed on whether the current tab is
incognito (though I guess that's changing soon).  When Clank isn't open, it
probably makes sense to open a new tab in the browser and then send the user
into downloads, though that's clearly not how this is set up currently.

[asanka, 2016-09-19 19:38:50]

Also, could you comment on what will happen to the enqueueDownloadManagerRequest
codepaths? I assume other than OMA downloads and some media downloads, those
codepaths should go away?

[asanka, 2016-09-19 20:05:06]

[sorry one more thing]

qinmin: Would you mind updating the CL description to indicate that some
downloads will continue to go through ADM (OMA and such)?

https://codereview.chromium.org/2341643008/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadManagerService.java
(right):

https://codereview.chromium.org/2341643008/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadManagerService.java:1077:
// Open the Android Download Manager.
On 2016/09/19 at 19:33:56, dfalcantara wrote:
> On 2016/09/19 19:29:12, asanka wrote:
> > Why try to open this at all once Chrome is handling all the downloads?
> > 
> > Why is the behavior of openDownloadsPage() sensitive to whether the Chrome
> > activity is in the foreground or not?
> 
> Download Home is tied to a tab: on tablets it's displayed inside a tab and in
general the downloads displayed are keyed on whether the current tab is
incognito (though I guess that's changing soon).  When Clank isn't open, it
probably makes sense to open a new tab in the browser and then send the user
into downloads, though that's clearly not how this is set up currently.

Right. Though on phones downloads home is a separate activity, correct?

I was wondering more about the fact that this code will open the downloads home
or the Android Download Manager depending on which app is in the foreground.
That seems incorrect when Chrome is handling all downloads since the in-progress
download that triggered the openDownloadsPage call won't be in the ADM.

[gone, 2016-09-19 20:10:42]

https://codereview.chromium.org/2341643008/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadManagerService.java
(right):

https://codereview.chromium.org/2341643008/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadManagerService.java:1077:
// Open the Android Download Manager.
On 2016/09/19 20:05:06, asanka wrote:
> On 2016/09/19 at 19:33:56, dfalcantara wrote:
> > On 2016/09/19 19:29:12, asanka wrote:
> > > Why try to open this at all once Chrome is handling all the downloads?
> > > 
> > > Why is the behavior of openDownloadsPage() sensitive to whether the Chrome
> > > activity is in the foreground or not?
> > 
> > Download Home is tied to a tab: on tablets it's displayed inside a tab and
in
> general the downloads displayed are keyed on whether the current tab is
> incognito (though I guess that's changing soon).  When Clank isn't open, it
> probably makes sense to open a new tab in the browser and then send the user
> into downloads, though that's clearly not how this is set up currently.
> 
> Right. Though on phones downloads home is a separate activity, correct?
> 
> I was wondering more about the fact that this code will open the downloads
home
> or the Android Download Manager depending on which app is in the foreground.
> That seems incorrect when Chrome is handling all downloads since the
in-progress
> download that triggered the openDownloadsPage call won't be in the ADM.

Separate activity, but it's still tied to whether the tab was incognito or not. 
This shouldn't matter once the manager is showing all regular and incognito
downloads in the same list regardless of how it was opened.  As a fallback, we
could change DownloadUtils#showDownloadManager() to pop up Download Home without
any idea of what Chrome's state is.  It already handles null tabs, but it'd need
to be adjusted to account for null activities, as well.

The logic requires some tweaking there, though.  If you file a bug I can try to
tweak it to make more sense.

[qinmin, 2016-09-19 21:07:46]

Description was changed from

==========
Defaulting all downloads to go through Chrome network stack

In M53, we started a experiment on disabling system DownloadManager.
And In M54, we expect the experiment to be expanded to 100% of the user.
This change removes this field trial flag to default all download through
Chrome.
This allows us to unify the download code path between android and desktop.

BUG=647755
==========

to

==========
Defaulting all downloads to go through Chrome network stack

In M53, we started a experiment on disabling system DownloadManager.
And In M54, we expect the experiment to be expanded to 100% of the user.
This change removes this field trial flag to default all download through
Chrome.
This allows us to unify the download code path between android and desktop.

Android DownloadManager will still be used by OMA downloads.
And will clean up all the code that calls into
ChromeDownloadDelegate::EnqueueDownloadManagerRequest() in a separate CL.

BUG=647755
==========

[qinmin, 2016-09-19 21:16:13]

* Updated the ExternalNavigationDelegateImplTest.java to remove a test with
SystemDownloadManager.
* Updated the comment in download_ui_controller.cc.
* Updated the CL description about EnqueueDownloadManagerRequest() call

https://codereview.chromium.org/2341643008/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadManagerService.java
(right):

https://codereview.chromium.org/2341643008/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadManagerService.java:1077:
// Open the Android Download Manager.
On 2016/09/19 19:29:12, asanka wrote:
> Why try to open this at all once Chrome is handling all the downloads?
> 
> Why is the behavior of openDownloadsPage() sensitive to whether the Chrome
> activity is in the foreground or not?

For OMA downloads, which didn't go through Chrome, we need to open it in Android
DownloadManager

https://codereview.chromium.org/2341643008/diff/80001/chrome/browser/android/...
File chrome/browser/android/intercept_download_resource_throttle.cc (left):

https://codereview.chromium.org/2341643008/diff/80001/chrome/browser/android/...
chrome/browser/android/intercept_download_resource_throttle.cc:40:
UMA_HISTOGRAM_ENUMERATION("MobileDownload.InterceptFailureReason",
On 2016/09/19 19:29:12, asanka wrote:
> Mark this histogram as being deprecated in histograms.xml.

Done.

[asanka, 2016-09-20 20:28:02]

lgtm

[qinmin, 2016-09-20 20:47:03]

The CQ bit was checked by qinmin@chromium.org

[qinmin, 2016-09-20 20:47:03]

The patchset sent to the CQ was uploaded after l-g-t-m from thakis@chromium.org,
dfalcantara@chromium.org
Link to the patchset: https://codereview.chromium.org/2341643008/#ps120001
(title: "more clean up")

[commit-bot: I haz the power, 2016-09-20 20:47:39]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-20 20:50:58]

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

[commit-bot: I haz the power, 2016-09-20 20:50:59]

Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[qinmin, 2016-09-20 23:49:02]

The CQ bit was checked by qinmin@chromium.org

[qinmin, 2016-09-20 23:49:03]

The patchset sent to the CQ was uploaded after l-g-t-m from asanka@chromium.org,
thakis@chromium.org, dfalcantara@chromium.org
Link to the patchset: https://codereview.chromium.org/2341643008/#ps140001
(title: "rebase")

[commit-bot: I haz the power, 2016-09-20 23:49:56]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-20 23:59:43]

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

[commit-bot: I haz the power, 2016-09-20 23:59:44]

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

[qinmin, 2016-09-21 00:04:50]

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

[qinmin, 2016-09-21 00:04:50]

+jwd for histogram.xml OWNER

[jwd, 2016-09-21 17:54:39]

lgtm

[qinmin, 2016-09-21 20:37:18]

Patchset #7 (id:140001) has been deleted

[qinmin, 2016-09-21 20:37:57]

The CQ bit was checked by qinmin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-21 20:38:15]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-21 22:10:16]

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

[commit-bot: I haz the power, 2016-09-21 22:10:20]

Dry run: This issue passed the CQ dry run.

[qinmin, 2016-09-21 22:12:00]

The CQ bit was checked by qinmin@chromium.org

[qinmin, 2016-09-21 22:12:00]

The patchset sent to the CQ was uploaded after l-g-t-m from asanka@chromium.org,
thakis@chromium.org, dfalcantara@chromium.org, jwd@chromium.org
Link to the patchset: https://codereview.chromium.org/2341643008/#ps160001
(title: "rebase")

[commit-bot: I haz the power, 2016-09-21 22:12:29]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-21 22:19:56]

Description was changed from

==========
Defaulting all downloads to go through Chrome network stack

In M53, we started a experiment on disabling system DownloadManager.
And In M54, we expect the experiment to be expanded to 100% of the user.
This change removes this field trial flag to default all download through
Chrome.
This allows us to unify the download code path between android and desktop.

Android DownloadManager will still be used by OMA downloads.
And will clean up all the code that calls into
ChromeDownloadDelegate::EnqueueDownloadManagerRequest() in a separate CL.

BUG=647755
==========

to

==========
Defaulting all downloads to go through Chrome network stack

In M53, we started a experiment on disabling system DownloadManager.
And In M54, we expect the experiment to be expanded to 100% of the user.
This change removes this field trial flag to default all download through
Chrome.
This allows us to unify the download code path between android and desktop.

Android DownloadManager will still be used by OMA downloads.
And will clean up all the code that calls into
ChromeDownloadDelegate::EnqueueDownloadManagerRequest() in a separate CL.

BUG=647755
==========

[commit-bot: I haz the power, 2016-09-21 22:19:57]

Committed patchset #7 (id:160001)

[commit-bot: I haz the power, 2016-09-21 22:22:42]

Description was changed from

==========
Defaulting all downloads to go through Chrome network stack

In M53, we started a experiment on disabling system DownloadManager.
And In M54, we expect the experiment to be expanded to 100% of the user.
This change removes this field trial flag to default all download through
Chrome.
This allows us to unify the download code path between android and desktop.

Android DownloadManager will still be used by OMA downloads.
And will clean up all the code that calls into
ChromeDownloadDelegate::EnqueueDownloadManagerRequest() in a separate CL.

BUG=647755
==========

to

==========
Defaulting all downloads to go through Chrome network stack

In M53, we started a experiment on disabling system DownloadManager.
And In M54, we expect the experiment to be expanded to 100% of the user.
This change removes this field trial flag to default all download through
Chrome.
This allows us to unify the download code path between android and desktop.

Android DownloadManager will still be used by OMA downloads.
And will clean up all the code that calls into
ChromeDownloadDelegate::EnqueueDownloadManagerRequest() in a separate CL.

BUG=647755

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

[commit-bot: I haz the power, 2016-09-21 22:22:44]

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