Introduce background Download process to android

Introduce background Download process to android

This change adds a new download process to android.
This process will handle download in the background and may outlive chrome
Unlike other ChildProcessService, DownloadProcessService is a started service as it manage its own lifecycle:
When a download started, the download service will start running
When all download finishes, the Download service will be stopped

BUG=545640
Committed: https://crrev.com/ee805c1eeab8ed56cdac0020ba41948279bdbd05
Cr-Commit-Position: refs/heads/master@{#373608}

[qinmin, 2016-01-23 00:47:47]

qinmin@chromium.org changed reviewers:
+ asanka@chromium.org, sievers@chromium.org, svaldez@chromium.org

[qinmin, 2016-01-23 00:47:47]

PTAL:
sievers@ for review and OWNER

[svaldez, 2016-01-25 15:33:44]

Initial pass.

https://codereview.chromium.org/1622743005/diff/1/chrome/android/java/Android...
File chrome/android/java/AndroidManifest.xml (right):

https://codereview.chromium.org/1622743005/diff/1/chrome/android/java/Android...
chrome/android/java/AndroidManifest.xml:650: android:process=":download"/>
nit: Reorder to match ordering of previous service definitions:

service android:name=... android:process=... android permission=...
android:isolatedProcess=... android:exported=...

https://codereview.chromium.org/1622743005/diff/1/content/app/content_main_ru...
File content/app/content_main_runner.cc (right):

https://codereview.chromium.org/1622743005/diff/1/content/app/content_main_ru...
content/app/content_main_runner.cc:365: {switches::kDownloadProcess,    
DownloadMain},
nit: Space "DownloadMain }"

https://codereview.chromium.org/1622743005/diff/1/content/browser/android/dow...
File content/browser/android/download_process_host.cc (right):

https://codereview.chromium.org/1622743005/diff/1/content/browser/android/dow...
content/browser/android/download_process_host.cc:21: class
DownloadSandboxedProcessLauncherDelegate
Is this actually a Sandboxed process? (We're creatingit NoSandbox)

https://codereview.chromium.org/1622743005/diff/1/content/browser/android/dow...
content/browser/android/download_process_host.cc:40: if (started_)
Possibly crash if StartProcess is called again.

https://codereview.chromium.org/1622743005/diff/1/content/browser/android/dow...
File content/browser/android/download_process_host.h (right):

https://codereview.chromium.org/1622743005/diff/1/content/browser/android/dow...
content/browser/android/download_process_host.h:1: // Copyright 2015 The
Chromium Authors. All rights reserved.
2016, same with any other dates.

https://codereview.chromium.org/1622743005/diff/1/content/public/android/java...
File
content/public/android/java/src/org/chromium/content/app/ChildProcessService.java
(right):

https://codereview.chromium.org/1622743005/diff/1/content/public/android/java...
content/public/android/java/src/org/chromium/content/app/ChildProcessService.java:295:
mFdInfos = getFileDescriptorInfo(bundle);
Move to DownloadServiceProcess, so that ChildServiceProcess doesn't need to
worry about 'getFileDescriptorInfo' and 'isBoundService'?

Also is it correct to ignore the new descriptors from getFileDescriptorInfo if
we have some passed as part of the parcel?

https://codereview.chromium.org/1622743005/diff/1/content/public/android/java...
File
content/public/android/java/src/org/chromium/content/app/DownloadProcessService.java
(right):

https://codereview.chromium.org/1622743005/diff/1/content/public/android/java...
content/public/android/java/src/org/chromium/content/app/DownloadProcessService.java:42:
assert !isBoundService();
Remove isBoundsService. (Seems to only state whether or not we are an instance
of DownloadProcessService)

https://codereview.chromium.org/1622743005/diff/1/content/public/android/java...
content/public/android/java/src/org/chromium/content/app/DownloadProcessService.java:44:
getServiceInfo(intent.getExtras());
Locally override getServiceInfo instead of modifying ChildProcessInfo.

https://codereview.chromium.org/1622743005/diff/1/content/public/android/java...
File
content/public/android/java/src/org/chromium/content/browser/ChildProcessConstants.java
(right):

https://codereview.chromium.org/1622743005/diff/1/content/public/android/java...
content/public/android/java/src/org/chromium/content/browser/ChildProcessConstants.java:25:
public static final int DOWNLOAD_PROCESS_NOTIFICATION_ID = 1797;
I believe the right way of using IDs is to add it to the resource file and then
reference that:
chrome/android/java/res/values/ids.xml

That way we don't end up accidentally having overlapping IDs.

https://codereview.chromium.org/1622743005/diff/1/content/public/android/java...
File
content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java
(right):

https://codereview.chromium.org/1622743005/diff/1/content/public/android/java...
content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:707:
protected static Bundle createsServiceBundle(
createDownloadServiceBundle

https://codereview.chromium.org/1622743005/diff/1/content/public/common/proce...
File content/public/common/process_type.h (right):

https://codereview.chromium.org/1622743005/diff/1/content/public/common/proce...
content/public/common/process_type.h:32: PROCESS_TYPE_DOWNLOAD,
I believe we aren't allowed to ifdef here, since otherwise the numbering for
histograms between Android and Desktop would be off.

[asanka, 2016-01-25 16:44:50]

Started looking at this, but I think we should hash out the role of the
background service. Based on previous OOB comments regarding persistence of
download request data, I was under the impression that the single background
process would be able to reschedule itself if it were to be killed.

Shall we come up with a reasonably concrete set of  requirements in
https://docs.google.com/a/google.com/document/d/1-BdH1GU9TkHQ3DFtZVpE9oElGwOK...
(sorry, Google internal link)?

https://codereview.chromium.org/1622743005/diff/1/chrome/android/java/Android...
File chrome/android/java/AndroidManifest.xml (right):

https://codereview.chromium.org/1622743005/diff/1/chrome/android/java/Android...
chrome/android/java/AndroidManifest.xml:646: when user kills chrome -->
Is this comment accurate? And if so, was there consensus that the download
process would outlive the browser process in the case where the user explicitly
kills the app (as opposed to Chrome being killed due to other reasons)?

I'm okay if that was the decision, but I want to make sure we are not violating
platform norms or user expectations.

https://codereview.chromium.org/1622743005/diff/1/chrome/android/java/Android...
chrome/android/java/AndroidManifest.xml:649: android:permission="{{
manifest_package }}.permission.CHILD_SERVICE"
Should this be left unset? What protections do we get from setting this to
CHILD_SERVICE?

Note that we want to restrict all other processes other than the browser process
from binding to this.

https://codereview.chromium.org/1622743005/diff/1/content/browser/android/dow...
File content/browser/android/download_process_host.cc (right):

https://codereview.chromium.org/1622743005/diff/1/content/browser/android/dow...
content/browser/android/download_process_host.cc:48: std::string channel_id =
process_->GetHost()->CreateChannel();
Do you intend for more than one browser process instance to share a background
download process? The control IPC channel you create here can be bound to only
the initiating browser process instance IIUC.

[qinmin, 2016-01-25 22:48:23]

https://codereview.chromium.org/1622743005/diff/1/chrome/android/java/Android...
File chrome/android/java/AndroidManifest.xml (right):

https://codereview.chromium.org/1622743005/diff/1/chrome/android/java/Android...
chrome/android/java/AndroidManifest.xml:646: when user kills chrome -->
On 2016/01/25 16:44:50, asanka wrote:
> Is this comment accurate? And if so, was there consensus that the download
> process would outlive the browser process in the case where the user
explicitly
> kills the app (as opposed to Chrome being killed due to other reasons)?
> 
> I'm okay if that was the decision, but I want to make sure we are not
violating
> platform norms or user expectations.

I think this should be the use case and should be expected by user, we can
confirm it with Sidv and Grace. 
The current behavior with Android DownloadManager is that when user explicitly
killing chrome, the download will continue. And we should match that behavior.

https://codereview.chromium.org/1622743005/diff/1/chrome/android/java/Android...
chrome/android/java/AndroidManifest.xml:649: android:permission="{{
manifest_package }}.permission.CHILD_SERVICE"
On 2016/01/25 16:44:50, asanka wrote:
> Should this be left unset? What protections do we get from setting this to
> CHILD_SERVICE?
> 
> Note that we want to restrict all other processes other than the browser
process
> from binding to this.

This specifies that only processes with CHILD_SERVICE permission can start this
service, which means other apps cannot launch this. Since the GPU and renderer
process will or is already isolated process, they don't have any permissions,
and will not be able to bind or start this service

https://codereview.chromium.org/1622743005/diff/1/chrome/android/java/Android...
chrome/android/java/AndroidManifest.xml:650: android:process=":download"/>
On 2016/01/25 15:33:43, svaldez wrote:
> nit: Reorder to match ordering of previous service definitions:
> 
> service android:name=... android:process=... android permission=...
> android:isolatedProcess=... android:exported=...

Done.

https://codereview.chromium.org/1622743005/diff/1/content/app/content_main_ru...
File content/app/content_main_runner.cc (right):

https://codereview.chromium.org/1622743005/diff/1/content/app/content_main_ru...
content/app/content_main_runner.cc:365: {switches::kDownloadProcess,    
DownloadMain},
On 2016/01/25 15:33:43, svaldez wrote:
> nit: Space "DownloadMain }"

Done.

https://codereview.chromium.org/1622743005/diff/1/content/browser/android/dow...
File content/browser/android/download_process_host.cc (right):

https://codereview.chromium.org/1622743005/diff/1/content/browser/android/dow...
content/browser/android/download_process_host.cc:21: class
DownloadSandboxedProcessLauncherDelegate
On 2016/01/25 15:33:43, svaldez wrote:
> Is this actually a Sandboxed process? (We're creatingit NoSandbox)

No. Unfortunately, BrowserChildProcessHost::Launch() needs an instance of
SandboxedProcessLauncherDelegate. However, in SandboxedProcessLauncherDelegate,
the sandbox policy doesn't necessarily needs to be strictly sandboxed. Or we can
define out own sandbox policy in sandbox_type.h

https://codereview.chromium.org/1622743005/diff/1/content/browser/android/dow...
content/browser/android/download_process_host.cc:40: if (started_)
On 2016/01/25 15:33:43, svaldez wrote:
> Possibly crash if StartProcess is called again.

This function will be called when the first time a download starts in chrome,
and we should terminate the background process when there are no downloads in
progress. As a result, we should either expose a
DownloadProcessHost::IsStarted() call to the caller so that the caller can check
if download service is running, or do nothing when
DownloadProcessHost::startProcess() is called. This CL chooses the latter.

https://codereview.chromium.org/1622743005/diff/1/content/browser/android/dow...
content/browser/android/download_process_host.cc:48: std::string channel_id =
process_->GetHost()->CreateChannel();
On 2016/01/25 16:44:50, asanka wrote:
> Do you intend for more than one browser process instance to share a background
> download process? The control IPC channel you create here can be bound to only
> the initiating browser process instance IIUC.

Yes, the download process can outlive chrome. As a result, if chrome gets killed
and got relauched, we should get a new IPC fd from the browser process.

https://codereview.chromium.org/1622743005/diff/1/content/browser/android/dow...
File content/browser/android/download_process_host.h (right):

https://codereview.chromium.org/1622743005/diff/1/content/browser/android/dow...
content/browser/android/download_process_host.h:1: // Copyright 2015 The
Chromium Authors. All rights reserved.
On 2016/01/25 15:33:43, svaldez wrote:
> 2016, same with any other dates.

Done.

https://codereview.chromium.org/1622743005/diff/1/content/public/android/java...
File
content/public/android/java/src/org/chromium/content/app/ChildProcessService.java
(right):

https://codereview.chromium.org/1622743005/diff/1/content/public/android/java...
content/public/android/java/src/org/chromium/content/app/ChildProcessService.java:295:
mFdInfos = getFileDescriptorInfo(bundle);
On 2016/01/25 15:33:43, svaldez wrote:
> Move to DownloadServiceProcess, so that ChildServiceProcess doesn't need to
> worry about 'getFileDescriptorInfo' and 'isBoundService'?
> 
> Also is it correct to ignore the new descriptors from getFileDescriptorInfo if
> we have some passed as part of the parcel?

Let the DownloadServiceProcess override getServiceInfo() function? Then there
are lot of duplicated code. see my comments in DownloadProcessService.

If we passed the FDs from the parcel, then getFileDescriptorInfo() should not
get called. For bound service, they can pass FDs inside the parcel, but for a
started service, we cannot do that.

https://codereview.chromium.org/1622743005/diff/1/content/public/android/java...
File
content/public/android/java/src/org/chromium/content/app/DownloadProcessService.java
(right):

https://codereview.chromium.org/1622743005/diff/1/content/public/android/java...
content/public/android/java/src/org/chromium/content/app/DownloadProcessService.java:42:
assert !isBoundService();
On 2016/01/25 15:33:43, svaldez wrote:
> Remove isBoundsService. (Seems to only state whether or not we are an instance
> of DownloadProcessService)

Done.

https://codereview.chromium.org/1622743005/diff/1/content/public/android/java...
content/public/android/java/src/org/chromium/content/app/DownloadProcessService.java:44:
getServiceInfo(intent.getExtras());
On 2016/01/25 15:33:43, svaldez wrote:
> Locally override getServiceInfo instead of modifying ChildProcessInfo.

hmm... That's a lot of duplicated code. And i need to access parent class's
synchronization variable mMainThread here, which is very wierd.
Also need to change lots of member variables from private to be protected. I
doubt that will give us any gain here.

https://codereview.chromium.org/1622743005/diff/1/content/public/android/java...
File
content/public/android/java/src/org/chromium/content/browser/ChildProcessConstants.java
(right):

https://codereview.chromium.org/1622743005/diff/1/content/public/android/java...
content/public/android/java/src/org/chromium/content/browser/ChildProcessConstants.java:25:
public static final int DOWNLOAD_PROCESS_NOTIFICATION_ID = 1797;
On 2016/01/25 15:33:43, svaldez wrote:
> I believe the right way of using IDs is to add it to the resource file and
then
> reference that:
> chrome/android/java/res/values/ids.xml
> 
> That way we don't end up accidentally having overlapping IDs.

If you check ids.xml, the ids are mostly for UI. The problem is that different
builds(content_shell, chrome, chrome_public) uses different xmls, so for a
single Id, we need to add it to different xmls, which makes is even harder to
maintain.

https://codereview.chromium.org/1622743005/diff/1/content/public/android/java...
File
content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java
(right):

https://codereview.chromium.org/1622743005/diff/1/content/public/android/java...
content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:707:
protected static Bundle createsServiceBundle(
On 2016/01/25 15:33:43, svaldez wrote:
> createDownloadServiceBundle

This function is also used in ChildProcessConnectionImpl class, I forget to add
that file, readded in the new patch. So this is not solely used for download
purpose.

https://codereview.chromium.org/1622743005/diff/1/content/public/common/proce...
File content/public/common/process_type.h (right):

https://codereview.chromium.org/1622743005/diff/1/content/public/common/proce...
content/public/common/process_type.h:32: PROCESS_TYPE_DOWNLOAD,
On 2016/01/25 15:33:43, svaldez wrote:
> I believe we aren't allowed to ifdef here, since otherwise the numbering for
> histograms between Android and Desktop would be off.

Done.

[qinmin, 2016-01-26 18:22:00]

ping, sievers@, would you please take a look? thanks

[no sievers, 2016-01-27 00:33:36]

https://codereview.chromium.org/1622743005/diff/20001/content/browser/android...
File content/browser/android/download_process_host.h (right):

https://codereview.chromium.org/1622743005/diff/20001/content/browser/android...
content/browser/android/download_process_host.h:15: public:
This class is unused.

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

https://codereview.chromium.org/1622743005/diff/20001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:667:
startDownloadProcess(context, filesToBeMapped, commandLine,
chromiumLinkerParams);
This ignores |clientContext| so it never triggers the proper callbacks to the
native ChildProcessLauncher for success or failure.

https://codereview.chromium.org/1622743005/diff/20001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:719:
* Create the background download service. The download service controls its own
life
Can you explain how the lifetime is managed?

Do you want Chrome to be able to reference and keep this alive at all as long as
downloads are active and Chrome is not killed (as in manage it in the native
code)?

Or will Chrome not care about this after a download is submitted, in other words
is this some sort of service that we just submit work to and don't want to be
involved with lifetime management otherwise?

Do you plan on talking to the service through Chromium's IPC channel or Binder?

https://codereview.chromium.org/1622743005/diff/20001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:889:
List<FileDescriptorInfo> info = sDownloadFilesMap.get(channelId);
This potentially allows a malicious renderer to hijack the IPC channel to the
browser and steal cookies and what not.

You should at least verify that this is CALLBACK_FOR_DOWNLOAD_PROCESS.

Or is there maybe a better way so that we can pass it to the child instead of
this way around? (like we do for the other process types)

[qinmin, 2016-01-28 00:58:31]

sievers@, PTAL again.

Chatted with sievers@, removed all dependencies on BrowserProcessHost and
ChildProcessLauncher since the process manages its own life cycle.
We are no longer passing IPC fds to the download service. Browser process will
use Intent to communicate with the download process, and the download process
will use callbacks to inform the download process about whether a download can
proceed or not.

https://codereview.chromium.org/1622743005/diff/20001/content/browser/android...
File content/browser/android/download_process_host.h (right):

https://codereview.chromium.org/1622743005/diff/20001/content/browser/android...
content/browser/android/download_process_host.h:15: public:
On 2016/01/27 00:33:36, sievers wrote:
> This class is unused.

This is the entry point to create the download process. Moved all the
implementation into child_process_launcher_android, and we no longer need this.
In the future, DownloadControllerAndroidImpl will call startDownloadProcess() to
start the process if it is not created.

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

https://codereview.chromium.org/1622743005/diff/20001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:667:
startDownloadProcess(context, filesToBeMapped, commandLine,
chromiumLinkerParams);
On 2016/01/27 00:33:36, sievers wrote:
> This ignores |clientContext| so it never triggers the proper callbacks to the
> native ChildProcessLauncher for success or failure.

The new patch no longer uses ChildProcessLauncher, so there is no need to
callback the native to clean things up.

https://codereview.chromium.org/1622743005/diff/20001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:719:
* Create the background download service. The download service controls its own
life
On 2016/01/27 00:33:36, sievers wrote:
> Can you explain how the lifetime is managed?
> 
> Do you want Chrome to be able to reference and keep this alive at all as long
as
> downloads are active and Chrome is not killed (as in manage it in the native
> code)?
> 
> Or will Chrome not care about this after a download is submitted, in other
words
> is this some sort of service that we just submit work to and don't want to be
> involved with lifetime management otherwise?
> 
> Do you plan on talking to the service through Chromium's IPC channel or
Binder?

Yes, Chrome will not involved in the lifetime management of this new process.
To communicate with the service, we will use callbacks for the purpose of accept
or reject a download.

https://codereview.chromium.org/1622743005/diff/20001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:889:
List<FileDescriptorInfo> info = sDownloadFilesMap.get(channelId);
On 2016/01/27 00:33:36, sievers wrote:
> This potentially allows a malicious renderer to hijack the IPC channel to the
> browser and steal cookies and what not.
> 
> You should at least verify that this is CALLBACK_FOR_DOWNLOAD_PROCESS.
> 
> Or is there maybe a better way so that we can pass it to the child instead of
> this way around? (like we do for the other process types)

The new patch no long uses a callback to get the IPC FD. Added a verification
for CALLBACK_FOR_DOWNLOAD_PROCESS

[qinmin, 2016-01-29 17:42:51]

Description was changed from

==========
Introduce background Download process to android

This change adds a new download process type on android.
This process will handle download in the background and may outlive chrome
Unlike other ChildProcessService, DownloadProcessService is a started service as
to manage its own lifecycle

BUG=545640
==========

to

==========
Introduce background Download process to android

This change adds a new download process type on android.
This process will handle download in the background and may outlive chrome
Unlike other ChildProcessService, DownloadProcessService is a started service as
it manage its own lifecycle:
When a download started, the download service will start running
When all download finishes, the Download service will be stopped


BUG=545640
==========

[qinmin, 2016-01-29 23:28:13]

Description was changed from

==========
Introduce background Download process to android

This change adds a new download process type on android.
This process will handle download in the background and may outlive chrome
Unlike other ChildProcessService, DownloadProcessService is a started service as
it manage its own lifecycle:
When a download started, the download service will start running
When all download finishes, the Download service will be stopped


BUG=545640
==========

to

==========
Introduce background Download process to android

This change adds a new download process to android.
This process will handle download in the background and may outlive chrome
Unlike other ChildProcessService, DownloadProcessService is a started service as
it manage its own lifecycle:
When a download started, the download service will start running
When all download finishes, the Download service will be stopped


BUG=545640
==========

[no sievers, 2016-02-02 01:37:38]

https://codereview.chromium.org/1622743005/diff/40001/content/app/android/dow...
File content/app/android/download_main.cc (right):

https://codereview.chromium.org/1622743005/diff/40001/content/app/android/dow...
content/app/android/download_main.cc:13: // process.
Does this TODO still apply?

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

https://codereview.chromium.org/1622743005/diff/40001/content/browser/android...
content/browser/android/child_process_launcher_android.cc:99:
StartChildProcess(cmd_line->argv(), child_process_id,
How about a different Java entry point, if all the arguments don't apply?

https://codereview.chromium.org/1622743005/diff/40001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/app/ChildProcessService.java
(right):

https://codereview.chromium.org/1622743005/diff/40001/content/public/android/...
content/public/android/java/src/org/chromium/content/app/ChildProcessService.java:293:
mFdInfos = EMPTY_FILE_DESCRIPTOR_INFO;
can we assert it's the download process? or at least leave a comment.

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

https://codereview.chromium.org/1622743005/diff/40001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:450:
private static Map<String, List<FileDescriptorInfo>> sDownloadFilesMap =
unused?

[qinmin, 2016-02-02 20:51:41]

PTAL again, thanks

https://codereview.chromium.org/1622743005/diff/40001/content/app/android/dow...
File content/app/android/download_main.cc (right):

https://codereview.chromium.org/1622743005/diff/40001/content/app/android/dow...
content/app/android/download_main.cc:13: // process.
On 2016/02/02 01:37:38, sievers wrote:
> Does this TODO still apply?

Done. Removed

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

https://codereview.chromium.org/1622743005/diff/40001/content/browser/android...
content/browser/android/child_process_launcher_android.cc:99:
StartChildProcess(cmd_line->argv(), child_process_id,
On 2016/02/02 01:37:38, sievers wrote:
> How about a different Java entry point, if all the arguments don't apply?

Done. Call startDownloadProcess() in the java class.

https://codereview.chromium.org/1622743005/diff/40001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/app/ChildProcessService.java
(right):

https://codereview.chromium.org/1622743005/diff/40001/content/public/android/...
content/public/android/java/src/org/chromium/content/app/ChildProcessService.java:293:
mFdInfos = EMPTY_FILE_DESCRIPTOR_INFO;
On 2016/02/02 01:37:38, sievers wrote:
> can we assert it's the download process? or at least leave a comment.

Done. Refactored the SWITCHES_* variables in ChildProcessLauncher

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

https://codereview.chromium.org/1622743005/diff/40001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:450:
private static Map<String, List<FileDescriptorInfo>> sDownloadFilesMap =
On 2016/02/02 01:37:38, sievers wrote:
> unused?

Removed.

[no sievers, 2016-02-03 22:46:52]

ok just a few more nits

https://codereview.chromium.org/1622743005/diff/60001/content/app/android/dow...
File content/app/android/download_main.cc (right):

https://codereview.chromium.org/1622743005/diff/60001/content/app/android/dow...
content/app/android/download_main.cc:1: // Copyright 2015 The Chromium Authors.
All rights reserved.
nit: 2016

https://codereview.chromium.org/1622743005/diff/60001/content/browser/android...
File content/browser/android/child_process_launcher_android.cc (left):

https://codereview.chromium.org/1622743005/diff/60001/content/browser/android...
content/browser/android/child_process_launcher_android.cc:119: DCHECK(file_count
> 0);
This DCHECK() can stay now, right?

https://codereview.chromium.org/1622743005/diff/60001/content/browser/android...
File content/browser/android/child_process_launcher_android.cc (right):

https://codereview.chromium.org/1622743005/diff/60001/content/browser/android...
content/browser/android/child_process_launcher_android.cc:136: int
child_process_id = ChildProcessHostImpl::GenerateChildProcessUniqueId();
This is not needed right? It's not a normal ChildProcess after all.

https://codereview.chromium.org/1622743005/diff/60001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/app/DownloadProcessService.java
(right):

https://codereview.chromium.org/1622743005/diff/60001/content/public/android/...
content/public/android/java/src/org/chromium/content/app/DownloadProcessService.java:1:
// Copyright 2015 The Chromium Authors. All rights reserved.
nit: 2016

https://codereview.chromium.org/1622743005/diff/60001/content/public/android/...
content/public/android/java/src/org/chromium/content/app/DownloadProcessService.java:54:
*/
@CalledByNative

https://codereview.chromium.org/1622743005/diff/60001/content/public/android/...
content/public/android/java/src/org/chromium/content/app/DownloadProcessService.java:69:
private void onDownloadCompleted(boolean success) {
@CalledByNative

https://codereview.chromium.org/1622743005/diff/60001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/ChildProcessConstants.java
(right):

https://codereview.chromium.org/1622743005/diff/60001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/ChildProcessConstants.java:25:
public static final int DOWNLOAD_PROCESS_NOTIFICATION_ID = 1797;
Can you put this into chromium//src/chrome/android/java/res/values/ids.xml where
all the other notification ids for startForegorund() are and then use
|R.id.download_process_notification| here?

https://codereview.chromium.org/1622743005/diff/60001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java
(right):

https://codereview.chromium.org/1622743005/diff/60001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:629:
* can outlive chrome.
What happens if this gets called repeatedly?
Do you eventually expect this to happen (then maybe it should be
'startDownloadProcessIfNecessary' or so)? Or how do you know that it's still
there?

[qinmin, 2016-02-03 23:37:37]

https://codereview.chromium.org/1622743005/diff/60001/content/app/android/dow...
File content/app/android/download_main.cc (right):

https://codereview.chromium.org/1622743005/diff/60001/content/app/android/dow...
content/app/android/download_main.cc:1: // Copyright 2015 The Chromium Authors.
All rights reserved.
On 2016/02/03 22:46:51, sievers wrote:
> nit: 2016

Done.

https://codereview.chromium.org/1622743005/diff/60001/content/browser/android...
File content/browser/android/child_process_launcher_android.cc (left):

https://codereview.chromium.org/1622743005/diff/60001/content/browser/android...
content/browser/android/child_process_launcher_android.cc:119: DCHECK(file_count
> 0);
On 2016/02/03 22:46:51, sievers wrote:
> This DCHECK() can stay now, right?

Done.

https://codereview.chromium.org/1622743005/diff/60001/content/browser/android...
File content/browser/android/child_process_launcher_android.cc (right):

https://codereview.chromium.org/1622743005/diff/60001/content/browser/android...
content/browser/android/child_process_launcher_android.cc:136: int
child_process_id = ChildProcessHostImpl::GenerateChildProcessUniqueId();
On 2016/02/03 22:46:51, sievers wrote:
> This is not needed right? It's not a normal ChildProcess after all.

removed. I was just not sure whether we need any flags from
AppendExtraCommandLineSwitches(), but we can add that later if it is needed.

https://codereview.chromium.org/1622743005/diff/60001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/app/DownloadProcessService.java
(right):

https://codereview.chromium.org/1622743005/diff/60001/content/public/android/...
content/public/android/java/src/org/chromium/content/app/DownloadProcessService.java:1:
// Copyright 2015 The Chromium Authors. All rights reserved.
On 2016/02/03 22:46:52, sievers wrote:
> nit: 2016

Done.

https://codereview.chromium.org/1622743005/diff/60001/content/public/android/...
content/public/android/java/src/org/chromium/content/app/DownloadProcessService.java:54:
*/
On 2016/02/03 22:46:52, sievers wrote:
> @CalledByNative

added @CalledByNative in the comments. This is not yet called, and adding
@CalledByNative will cause jni generator to generate the stub in _jni file.

https://codereview.chromium.org/1622743005/diff/60001/content/public/android/...
content/public/android/java/src/org/chromium/content/app/DownloadProcessService.java:69:
private void onDownloadCompleted(boolean success) {
On 2016/02/03 22:46:52, sievers wrote:
> @CalledByNative

same as above

https://codereview.chromium.org/1622743005/diff/60001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/ChildProcessConstants.java
(right):

https://codereview.chromium.org/1622743005/diff/60001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/ChildProcessConstants.java:25:
public static final int DOWNLOAD_PROCESS_NOTIFICATION_ID = 1797;
On 2016/02/03 22:46:52, sievers wrote:
> Can you put this into chromium//src/chrome/android/java/res/values/ids.xml
where
> all the other notification ids for startForegorund() are and then use
> |R.id.download_process_notification| here?

This is in content, so I added a new ids.xml under content.

https://codereview.chromium.org/1622743005/diff/60001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java
(right):

https://codereview.chromium.org/1622743005/diff/60001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:629:
* can outlive chrome.
On 2016/02/03 22:46:52, sievers wrote:
> What happens if this gets called repeatedly?
> Do you eventually expect this to happen (then maybe it should be
> 'startDownloadProcessIfNecessary' or so)? Or how do you know that it's still
> there?


This could get called repeatedly, keeping track of whether download process is
alive from the browser process doesn't match our description that download
process should manage its own life cycle.
When the 2nd time this gets called, the process should get the download request
from the intent, and update the callback so that it can callback the new browser
process.
Changed the function name to startDownloadProcessIfNecessary

[no sievers, 2016-02-03 23:45:11]

lgtm

[qinmin, 2016-02-03 23:46:15]

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

[commit-bot: I haz the power, 2016-02-03 23:47:38]

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

[qinmin, 2016-02-03 23:50:10]

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

[qinmin, 2016-02-03 23:50:11]

+tedchoc for chrome/android/ OWNER

[commit-bot: I haz the power, 2016-02-04 00:47:19]

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

[commit-bot: I haz the power, 2016-02-04 00:47:20]

Dry run: Try jobs failed on following builders:
  android_chromium_gn_compile_rel on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_chro...)

[qinmin, 2016-02-04 06:30:13]

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

[commit-bot: I haz the power, 2016-02-04 06:33:13]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1622743005/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1622743005/100001

[commit-bot: I haz the power, 2016-02-04 07:35:19]

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

[commit-bot: I haz the power, 2016-02-04 07:35:20]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Ted C, 2016-02-04 19:27:47]

On 2016/02/04 07:35:20, commit-bot: I haz the power wrote:
> Dry run: Try jobs failed on following builders:
>   mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

chrome/android - lgtm

[qinmin, 2016-02-04 19:28:14]

The CQ bit was checked by qinmin@chromium.org

[qinmin, 2016-02-04 19:28:14]

The patchset sent to the CQ was uploaded after l-g-t-m from sievers@chromium.org
Link to the patchset: https://codereview.chromium.org/1622743005/#ps100001
(title: "fix gn build")

[commit-bot: I haz the power, 2016-02-04 19:29:45]

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

[commit-bot: I haz the power, 2016-02-04 20:38:03]

Description was changed from

==========
Introduce background Download process to android

This change adds a new download process to android.
This process will handle download in the background and may outlive chrome
Unlike other ChildProcessService, DownloadProcessService is a started service as
it manage its own lifecycle:
When a download started, the download service will start running
When all download finishes, the Download service will be stopped


BUG=545640
==========

to

==========
Introduce background Download process to android

This change adds a new download process to android.
This process will handle download in the background and may outlive chrome
Unlike other ChildProcessService, DownloadProcessService is a started service as
it manage its own lifecycle:
When a download started, the download service will start running
When all download finishes, the Download service will be stopped


BUG=545640
==========

[commit-bot: I haz the power, 2016-02-04 20:38:04]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-02-04 20:38:55]

Description was changed from

==========
Introduce background Download process to android

This change adds a new download process to android.
This process will handle download in the background and may outlive chrome
Unlike other ChildProcessService, DownloadProcessService is a started service as
it manage its own lifecycle:
When a download started, the download service will start running
When all download finishes, the Download service will be stopped


BUG=545640
==========

to

==========
Introduce background Download process to android

This change adds a new download process to android.
This process will handle download in the background and may outlive chrome
Unlike other ChildProcessService, DownloadProcessService is a started service as
it manage its own lifecycle:
When a download started, the download service will start running
When all download finishes, the Download service will be stopped

BUG=545640

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

[commit-bot: I haz the power, 2016-02-04 20:38:56]

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