Consolidate JNI registrations for mojo system.

Consolidate JNI registrations for mojo system.

This CL adds a jni registrar consolidating all jni registrations for
mojo system:
  RegisterCoreImpl
  RegisterWatcherImpl
So that mojo users can just use this only one registrar rather than
calling all above registration functions.

BUG=none

Review-Url: https://codereview.chromium.org/2784493003
Cr-Commit-Position: refs/heads/master@{#463166}
Committed: https://chromium.googlesource.com/chromium/src/+/7e01649ec8b61ba9f5e934bf036eb996530e7d7b

[leonhsl(Using Gerrit), 2017-03-29 12:52:46]

The CQ bit was checked by leon.han@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-29 12:53:04]

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

[leonhsl(Using Gerrit), 2017-03-29 13:04:20]

leon.han@intel.com changed reviewers:
+ blundell@chromium.org, rockot@chromium.org, tedchoc@chromium.org

[leonhsl(Using Gerrit), 2017-03-29 13:04:20]

Hi, this CL comes from Colin's comment at
https://codereview.chromium.org/2774783003/#msg86, PTAL, thanks.

blundell@: overall

rockot@: overall and mojo/* OWNER review

tedchoc@: content/browser/android/* OWNER review

[commit-bot: I haz the power, 2017-03-29 13:50:42]

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

[commit-bot: I haz the power, 2017-03-29 13:50:43]

Dry run: This issue passed the CQ dry run.

[Ted C, 2017-03-29 16:09:38]

On 2017/03/29 13:04:20, leonhsl wrote:
> Hi, this CL comes from Colin's comment at
> https://codereview.chromium.org/2774783003/#msg86, PTAL, thanks.
> 
> blundell@: overall
> 
> rockot@: overall and mojo/* OWNER review
> 
> tedchoc@: content/browser/android/* OWNER review

content/browser/android/ - lgtm

[blundell, 2017-03-29 17:31:57]

https://codereview.chromium.org/2784493003/diff/1/content/browser/android/bro...
File content/browser/android/browser_jni_registrar.cc (right):

https://codereview.chromium.org/2784493003/diff/1/content/browser/android/bro...
content/browser/android/browser_jni_registrar.cc:61: {"MojoSystem",
mojo::android::RegisterSystemJni},
nit: Instead of being in this list, this should be called at all the callsites
of RegisterBrowserJni(), right?

[leonhsl(Using Gerrit), 2017-03-30 08:04:14]

https://codereview.chromium.org/2784493003/diff/1/content/browser/android/bro...
File content/browser/android/browser_jni_registrar.cc (right):

https://codereview.chromium.org/2784493003/diff/1/content/browser/android/bro...
content/browser/android/browser_jni_registrar.cc:61: {"MojoSystem",
mojo::android::RegisterSystemJni},
On 2017/03/29 17:31:57, blundell wrote:
> nit: Instead of being in this list, this should be called at all the callsites
> of RegisterBrowserJni(), right?

The mojo system JNI is necessary to functions in
content/browser/android/java_interfaces_impl.cc, so I suppose we'd better still
keep mojo JNI registration here? 
Some callsites of RegisterBrowserJni() may have no direct need to mojo JNI by
themselves, if we let them register mojo JNI, seems somehow unnatural.. Or I'm
missing some other rules that we should do that way? Thanks!

[blundell, 2017-03-30 08:42:15]

On 2017/03/30 08:04:14, leonhsl wrote:
>
https://codereview.chromium.org/2784493003/diff/1/content/browser/android/bro...
> File content/browser/android/browser_jni_registrar.cc (right):
> 
>
https://codereview.chromium.org/2784493003/diff/1/content/browser/android/bro...
> content/browser/android/browser_jni_registrar.cc:61: {"MojoSystem",
> mojo::android::RegisterSystemJni},
> On 2017/03/29 17:31:57, blundell wrote:
> > nit: Instead of being in this list, this should be called at all the
callsites
> > of RegisterBrowserJni(), right?
> 
> The mojo system JNI is necessary to functions in
> content/browser/android/java_interfaces_impl.cc, so I suppose we'd better
still
> keep mojo JNI registration here? 
> Some callsites of RegisterBrowserJni() may have no direct need to mojo JNI by
> themselves, if we let them register mojo JNI, seems somehow unnatural.. Or I'm
> missing some other rules that we should do that way? Thanks!

From looking at the code, the understanding that I've developed of how the
Register*Jni() hierarchy works is that every top-level app explicitly calls
Register<Foo>Jni() for all the modules that it needs. Otherwise why doesn't
RegisterBrowserJni() also register the //base JNI (or the JNI of any of
//content/browser/android's other dependencies), for example? Is there something
I'm missing about why the Mojo Jni is different from all the other Jni
registered in //content/app/android/library_loader_hooks.cc?

[leonhsl(Using Gerrit), 2017-03-30 09:52:04]

The CQ bit was checked by leon.han@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-30 09:52:19]

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

[leonhsl(Using Gerrit), 2017-03-30 10:01:50]

The CQ bit was checked by leon.han@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-30 10:02:14]

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

[leonhsl(Using Gerrit), 2017-03-30 10:38:26]

On 2017/03/30 08:42:15, blundell wrote:
> On 2017/03/30 08:04:14, leonhsl wrote:
> >
>
https://codereview.chromium.org/2784493003/diff/1/content/browser/android/bro...
> > File content/browser/android/browser_jni_registrar.cc (right):
> > 
> >
>
https://codereview.chromium.org/2784493003/diff/1/content/browser/android/bro...
> > content/browser/android/browser_jni_registrar.cc:61: {"MojoSystem",
> > mojo::android::RegisterSystemJni},
> > On 2017/03/29 17:31:57, blundell wrote:
> > > nit: Instead of being in this list, this should be called at all the
> callsites
> > > of RegisterBrowserJni(), right?
> > 
> > The mojo system JNI is necessary to functions in
> > content/browser/android/java_interfaces_impl.cc, so I suppose we'd better
> still
> > keep mojo JNI registration here? 
> > Some callsites of RegisterBrowserJni() may have no direct need to mojo JNI
by
> > themselves, if we let them register mojo JNI, seems somehow unnatural.. Or
I'm
> > missing some other rules that we should do that way? Thanks!
> 
> From looking at the code, the understanding that I've developed of how the
> Register*Jni() hierarchy works is that every top-level app explicitly calls
> Register<Foo>Jni() for all the modules that it needs. Otherwise why doesn't
> RegisterBrowserJni() also register the //base JNI (or the JNI of any of
> //content/browser/android's other dependencies), for example? Is there
something
> I'm missing about why the Mojo Jni is different from all the other Jni
> registered in //content/app/android/library_loader_hooks.cc?

Yeah given the example for //base JNIs, I do agree the rule is that every
top-level app itself register JNIs of all necessary modules. Thanks for detailed
explanations! Uploaded ps#2 to address this, PTAnL.

[commit-bot: I haz the power, 2017-03-30 10:56:55]

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

[commit-bot: I haz the power, 2017-03-30 10:56:56]

Dry run: This issue passed the CQ dry run.

[Ted C, 2017-03-30 16:43:25]

On 2017/03/30 10:38:26, leonhsl wrote:
> On 2017/03/30 08:42:15, blundell wrote:
> > On 2017/03/30 08:04:14, leonhsl wrote:
> > >
> >
>
https://codereview.chromium.org/2784493003/diff/1/content/browser/android/bro...
> > > File content/browser/android/browser_jni_registrar.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2784493003/diff/1/content/browser/android/bro...
> > > content/browser/android/browser_jni_registrar.cc:61: {"MojoSystem",
> > > mojo::android::RegisterSystemJni},
> > > On 2017/03/29 17:31:57, blundell wrote:
> > > > nit: Instead of being in this list, this should be called at all the
> > callsites
> > > > of RegisterBrowserJni(), right?
> > > 
> > > The mojo system JNI is necessary to functions in
> > > content/browser/android/java_interfaces_impl.cc, so I suppose we'd better
> > still
> > > keep mojo JNI registration here? 
> > > Some callsites of RegisterBrowserJni() may have no direct need to mojo JNI
> by
> > > themselves, if we let them register mojo JNI, seems somehow unnatural.. Or
> I'm
> > > missing some other rules that we should do that way? Thanks!
> > 
> > From looking at the code, the understanding that I've developed of how the
> > Register*Jni() hierarchy works is that every top-level app explicitly calls
> > Register<Foo>Jni() for all the modules that it needs. Otherwise why doesn't
> > RegisterBrowserJni() also register the //base JNI (or the JNI of any of
> > //content/browser/android's other dependencies), for example? Is there
> something
> > I'm missing about why the Mojo Jni is different from all the other Jni
> > registered in //content/app/android/library_loader_hooks.cc?
> 
> Yeah given the example for //base JNIs, I do agree the rule is that every
> top-level app itself register JNIs of all necessary modules. Thanks for
detailed
> explanations! Uploaded ps#2 to address this, PTAnL.

:-) indeed...much better. moar lgtm

[leonhsl(Using Gerrit), 2017-03-30 23:57:31]

On 2017/03/30 16:43:25, Ted C wrote:
> On 2017/03/30 10:38:26, leonhsl wrote:
> > On 2017/03/30 08:42:15, blundell wrote:
> > > On 2017/03/30 08:04:14, leonhsl wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/2784493003/diff/1/content/browser/android/bro...
> > > > File content/browser/android/browser_jni_registrar.cc (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/2784493003/diff/1/content/browser/android/bro...
> > > > content/browser/android/browser_jni_registrar.cc:61: {"MojoSystem",
> > > > mojo::android::RegisterSystemJni},
> > > > On 2017/03/29 17:31:57, blundell wrote:
> > > > > nit: Instead of being in this list, this should be called at all the
> > > callsites
> > > > > of RegisterBrowserJni(), right?
> > > > 
> > > > The mojo system JNI is necessary to functions in
> > > > content/browser/android/java_interfaces_impl.cc, so I suppose we'd
better
> > > still
> > > > keep mojo JNI registration here? 
> > > > Some callsites of RegisterBrowserJni() may have no direct need to mojo
JNI
> > by
> > > > themselves, if we let them register mojo JNI, seems somehow unnatural..
Or
> > I'm
> > > > missing some other rules that we should do that way? Thanks!
> > > 
> > > From looking at the code, the understanding that I've developed of how the
> > > Register*Jni() hierarchy works is that every top-level app explicitly
calls
> > > Register<Foo>Jni() for all the modules that it needs. Otherwise why
doesn't
> > > RegisterBrowserJni() also register the //base JNI (or the JNI of any of
> > > //content/browser/android's other dependencies), for example? Is there
> > something
> > > I'm missing about why the Mojo Jni is different from all the other Jni
> > > registered in //content/app/android/library_loader_hooks.cc?
> > 
> > Yeah given the example for //base JNIs, I do agree the rule is that every
> > top-level app itself register JNIs of all necessary modules. Thanks for
> detailed
> > explanations! Uploaded ps#2 to address this, PTAnL.
> 
> :-) indeed...much better. moar lgtm

Thank you Ted :)

[blundell, 2017-03-31 11:28:18]

lgtm, thanks!

[leonhsl(Using Gerrit), 2017-03-31 12:18:49]

leon.han@intel.com changed reviewers:
+ avi@chromium.org

[leonhsl(Using Gerrit), 2017-03-31 12:18:49]

+Avi, would you PTAL for OWNER review of content/*? Thanks.

[Avi (use Gerrit), 2017-03-31 15:37:22]

lgtm

[leonhsl(Using Gerrit), 2017-04-04 01:32:44]

leon.han@intel.com changed reviewers:
+ yzshen@chromium.org

[leonhsl(Using Gerrit), 2017-04-04 01:32:44]

+Yuzhu, would you PTAL for mojo/ OWNER review? Thanks!

[yzshen1, 2017-04-10 00:01:24]

On 2017/04/04 01:32:44, leonhsl wrote:
> +Yuzhu, would you PTAL for mojo/ OWNER review? Thanks!

mojo/ LGTM

[leonhsl(Using Gerrit), 2017-04-10 01:42:43]

The CQ bit was checked by leon.han@intel.com

[commit-bot: I haz the power, 2017-04-10 01:42:53]

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-04-10 02:41:02]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1491788563860820,
"parent_rev": "115a1494e75fec1c214329b8e8f0be9b35194785", "commit_rev":
"7e01649ec8b61ba9f5e934bf036eb996530e7d7b"}

[commit-bot: I haz the power, 2017-04-10 02:42:14]

Description was changed from

==========
Consolidate JNI registrations for mojo system.

This CL adds a jni registrar consolidating all jni registrations for
mojo system:
  RegisterCoreImpl
  RegisterWatcherImpl
So that mojo users can just use this only one registrar rather than
calling all above registration functions.

BUG=none
==========

to

==========
Consolidate JNI registrations for mojo system.

This CL adds a jni registrar consolidating all jni registrations for
mojo system:
  RegisterCoreImpl
  RegisterWatcherImpl
So that mojo users can just use this only one registrar rather than
calling all above registration functions.

BUG=none

Review-Url: https://codereview.chromium.org/2784493003
Cr-Commit-Position: refs/heads/master@{#463166}
Committed:
https://chromium.googlesource.com/chromium/src/+/7e01649ec8b61ba9f5e934bf036e...
==========

[commit-bot: I haz the power, 2017-04-10 02:42:19]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/7e01649ec8b61ba9f5e934bf036e...