android: Allow registering multiple CreationParams

android: Allow registering multiple CreationParams

This is first step to avoid webapk overwriting default
CreationParams at run time. For now it's only used in
tests, and native side just hard codes the default ID.

Also added a check that warm up connection cannot be
used if the creation params do not match.

Other clean ups:
* Hide methods that don't need to be public.
* Remove copy. Params is already immutable.
* Remove redundant registerDefault call from
  Activities. Monochrome application already registers
  default in its onCreate.

BUG=664530
Review-Url: https://codereview.chromium.org/2705133002
Cr-Commit-Position: refs/heads/master@{#453428}
Committed: https://chromium.googlesource.com/chromium/src/+/f2e4ff7579e19d3f48d3aca2513470e5afdff01f

[boliu, 2017-02-20 22:49:40]

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

[commit-bot: I haz the power, 2017-02-20 22:49:52]

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, 2017-02-20 23:56:14]

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

[commit-bot: I haz the power, 2017-02-20 23:56:15]

Dry run: This issue passed the CQ dry run.

[boliu, 2017-02-22 01:37:37]

Description was changed from

==========
android: Allow registering multiple CreationParams

Only used in tests for now.

Other clean ups:
* Hide methods that don't need to be public.
* Remove copy. Params is already immutable.

BUG=664530
==========

to

==========
android: Allow registering multiple CreationParams

This is first step to avoid webapk overwriting default
CreationParams at run time. For now it's only used in
tests, and native side just hard codes the default ID.

Also added a check that warm up connection cannot be
used if the creation params do not match.

Other clean ups:
* Hide methods that don't need to be public.
* Remove copy. Params is already immutable.

BUG=664530
==========

[boliu, 2017-02-22 01:43:29]

boliu@chromium.org changed reviewers:
+ mariakhomenko@chromium.org, yfriedman@chromium.org

[boliu, 2017-02-22 01:43:29]

ptal

maria for content
yfriedman for stamping chrome, though you might want to read the whole thing.

Follow up that actually uses this code in
https://codereview.chromium.org/2706933003, though that's not ready to be
reviewed yet

[boliu, 2017-02-22 05:13:47]

Description was changed from

==========
android: Allow registering multiple CreationParams

This is first step to avoid webapk overwriting default
CreationParams at run time. For now it's only used in
tests, and native side just hard codes the default ID.

Also added a check that warm up connection cannot be
used if the creation params do not match.

Other clean ups:
* Hide methods that don't need to be public.
* Remove copy. Params is already immutable.

BUG=664530
==========

to

==========
android: Allow registering multiple CreationParams

This is first step to avoid webapk overwriting default
CreationParams at run time. For now it's only used in
tests, and native side just hard codes the default ID.

Also added a check that warm up connection cannot be
used if the creation params do not match.

Other clean ups:
* Hide methods that don't need to be public.
* Remove copy. Params is already immutable.
* Remove redundant registerDefault call from
  Activities. Monochrome application already registers
  default in its onCreate.

BUG=664530
==========

[boliu, 2017-02-22 21:25:01]

boliu@chromium.org changed reviewers:
+ tedchoc@chromium.org
- yfriedman@chromium.org

[boliu, 2017-02-22 21:25:02]

On 2017/02/22 01:43:29, boliu wrote:
> ptal
> 
> maria for content
> yfriedman for stamping chrome, though you might want to read the whole thing.
> 
> Follow up that actually uses this code in
> https://codereview.chromium.org/2706933003, though that's not ready to be
> reviewed yet

s/yaron/ted/ for chrome

[boliu, 2017-02-23 17:52:57]

ping

[Maria, 2017-02-24 05:32:42]

lgtm

It seems to me like you may want to include someone from webapk team on this
review.

https://codereview.chromium.org/2705133002/diff/80001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/ChildProcessCreationParams.java
(right):

https://codereview.chromium.org/2705133002/diff/80001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/ChildProcessCreationParams.java:27:
public static void registerDefault(ChildProcessCreationParams params) {
public methods should have javadoc here and below

[boliu, 2017-02-24 16:26:05]

On 2017/02/24 05:32:42, Maria wrote:
> lgtm
> 
> It seems to me like you may want to include someone from webapk team on this
> review.


I'll ping the bug, but I think the interesting stuff for webapk is in the next
CL, not this one.

[boliu, 2017-02-25 00:49:46]

https://codereview.chromium.org/2705133002/diff/80001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/ChildProcessCreationParams.java
(right):

https://codereview.chromium.org/2705133002/diff/80001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/ChildProcessCreationParams.java:27:
public static void registerDefault(ChildProcessCreationParams params) {
On 2017/02/24 05:32:42, Maria wrote:
> public methods should have javadoc here and below

Added one line javadoc to methods that are meant to be public.
getDefault() can be made package after webapk is fixed in next CL, so let a TODO
get() is made package

Also added an assert that default is never unregistered

[Ted C, 2017-02-27 22:25:18]

lgtm

https://codereview.chromium.org/2705133002/diff/100001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/ChildProcessCreationParams.java
(right):

https://codereview.chromium.org/2705133002/diff/100001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/ChildProcessCreationParams.java:25:
private static int sNextId = 1; // 0 is reserved for DEFAULT_ID.
nit put this below the finals

https://codereview.chromium.org/2705133002/diff/100001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/ChildProcessCreationParams.java:31:
sParamMap.append(DEFAULT_ID, params);
maybe add a TODO to do something like:

assert getDefault() == null;

?

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

https://codereview.chromium.org/2705133002/diff/100001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:646:
if (paramId != ChildProcessCreationParams.DEFAULT_ID && params == null) {
why do we want to allow DEFAULT_ID not to exist in the map?

Seems like we should force DEFAULT_ID to be passed in, and it should always
exist in the map.

i.e. why isn't this just if (params == null) FREAK OUT!

https://codereview.chromium.org/2705133002/diff/100001/content/public/android...
File
content/public/android/javatests/src/org/chromium/content/browser/ChildProcessLauncherTest.java
(right):

https://codereview.chromium.org/2705133002/diff/100001/content/public/android...
content/public/android/javatests/src/org/chromium/content/browser/ChildProcessLauncherTest.java:504:
private static void startChildProcess(
maybe startRendererProcess since this hardcodes the type to that?

[boliu, 2017-02-27 23:43:02]

https://codereview.chromium.org/2705133002/diff/100001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/ChildProcessCreationParams.java
(right):

https://codereview.chromium.org/2705133002/diff/100001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/ChildProcessCreationParams.java:25:
private static int sNextId = 1; // 0 is reserved for DEFAULT_ID.
On 2017/02/27 22:25:17, Ted C wrote:
> nit put this below the finals

Done.

https://codereview.chromium.org/2705133002/diff/100001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/ChildProcessCreationParams.java:31:
sParamMap.append(DEFAULT_ID, params);
On 2017/02/27 22:25:17, Ted C wrote:
> maybe add a TODO to do something like:
> 
> assert getDefault() == null;
> 
> ?

Done.

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

https://codereview.chromium.org/2705133002/diff/100001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:646:
if (paramId != ChildProcessCreationParams.DEFAULT_ID && params == null) {
On 2017/02/27 22:25:17, Ted C wrote:
> why do we want to allow DEFAULT_ID not to exist in the map?
> 
> Seems like we should force DEFAULT_ID to be passed in, and it should always
> exist in the map.
> 
> i.e. why isn't this just if (params == null) FREAK OUT!

regular chrome (ie not monochrome) doesn't need it, and I don't think we should
force chrome to set one just for fun..

(but if we do.. I'd want to get rid of "default params" entirely, and force
everything to pass down a specific param id)

https://codereview.chromium.org/2705133002/diff/100001/content/public/android...
File
content/public/android/javatests/src/org/chromium/content/browser/ChildProcessLauncherTest.java
(right):

https://codereview.chromium.org/2705133002/diff/100001/content/public/android...
content/public/android/javatests/src/org/chromium/content/browser/ChildProcessLauncherTest.java:504:
private static void startChildProcess(
On 2017/02/27 22:25:18, Ted C wrote:
> maybe startRendererProcess since this hardcodes the type to that?

Done.

[boliu, 2017-02-27 23:43:56]

The CQ bit was checked by boliu@chromium.org

[boliu, 2017-02-27 23:43:56]

The patchset sent to the CQ was uploaded after l-g-t-m from
mariakhomenko@chromium.org, tedchoc@chromium.org
Link to the patchset: https://codereview.chromium.org/2705133002/#ps120001
(title: "ted review")

[commit-bot: I haz the power, 2017-02-27 23:44:21]

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, 2017-02-27 23:47:14]

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

[commit-bot: I haz the power, 2017-02-27 23:47:15]

Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[boliu, 2017-02-28 00:13:03]

The CQ bit was checked by boliu@chromium.org

[boliu, 2017-02-28 00:13:04]

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

[commit-bot: I haz the power, 2017-02-28 00:13:30]

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, 2017-02-28 01:00:35]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1488240783899110,
"parent_rev": "7660559e277f55e66210695dadc2508deb0bf13d", "commit_rev":
"f2e4ff7579e19d3f48d3aca2513470e5afdff01f"}

[commit-bot: I haz the power, 2017-02-28 01:01:14]

Description was changed from

==========
android: Allow registering multiple CreationParams

This is first step to avoid webapk overwriting default
CreationParams at run time. For now it's only used in
tests, and native side just hard codes the default ID.

Also added a check that warm up connection cannot be
used if the creation params do not match.

Other clean ups:
* Hide methods that don't need to be public.
* Remove copy. Params is already immutable.
* Remove redundant registerDefault call from
  Activities. Monochrome application already registers
  default in its onCreate.

BUG=664530
==========

to

==========
android: Allow registering multiple CreationParams

This is first step to avoid webapk overwriting default
CreationParams at run time. For now it's only used in
tests, and native side just hard codes the default ID.

Also added a check that warm up connection cannot be
used if the creation params do not match.

Other clean ups:
* Hide methods that don't need to be public.
* Remove copy. Params is already immutable.
* Remove redundant registerDefault call from
  Activities. Monochrome application already registers
  default in its onCreate.

BUG=664530

Review-Url: https://codereview.chromium.org/2705133002
Cr-Commit-Position: refs/heads/master@{#453428}
Committed:
https://chromium.googlesource.com/chromium/src/+/f2e4ff7579e19d3f48d3aca25134...
==========

[commit-bot: I haz the power, 2017-02-28 01:01:15]

Committed patchset #8 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/f2e4ff7579e19d3f48d3aca25134...