Separate JNI registration with initialization

This is the first patch to separate JNI registration with initialization, currently, we have JNI_OnLoad defined multiple places which has a lot of
duplicated code and JNI registration and initialization are mixed;

This patch
- Added JNIOnLoadDelegate class for each component  specific initialization.
- Added base::android::OnJNIOnLoad() to call a list of delegates
- Migrate testing, content_shell, chrome_shell.

BUG=447393
TBR=thakis

Committed: https://crrev.com/842c972d5011ed671d2486deee78ae7640b0cca6
Cr-Commit-Position: refs/heads/master@{#313592}

[michaelbai, 2015-01-20 20:08:49]

michaelbai@chromium.org changed reviewers:
+ jam@chromium.org, nyquist@chromium.org

[michaelbai, 2015-01-20 20:12:23]

michaelbai@chromium.org changed reviewers:
+ dtrainor@chromium.org

[michaelbai, 2015-01-20 20:12:23]

nyquist@ for base/android

jam@ for content

dtrainor@ chrome/app/android

[David Trainor- moved to gerrit, 2015-01-21 08:18:40]

chrome/app/android lgtm

[jam, 2015-01-21 16:45:30]

i have a few questions, can we chat about this in person? would be faster than
back and forth over email

[michaelbai, 2015-01-21 17:21:13]

On 2015/01/21 16:45:30, jam wrote:
> i have a few questions, can we chat about this in person? would be faster than
> back and forth over email

Sure, I will come by

[michaelbai, 2015-01-23 21:49:42]

jam@, I modified the patch as we discussed, PTAL

[jam, 2015-01-26 22:38:22]

https://codereview.chromium.org/864563002/diff/40001/base/android/base_jni_on...
File base/android/base_jni_onload_delegate.h (right):

https://codereview.chromium.org/864563002/diff/40001/base/android/base_jni_on...
base/android/base_jni_onload_delegate.h:14: class BaseJNIOnLoadDelegate : public
JNIOnLoadDelegate {
since the only user of this is base_jni_onload.cc, why not just inline this
class into that cc file and use it there?

https://codereview.chromium.org/864563002/diff/40001/base/android/jni_onload_...
File base/android/jni_onload_delegate.cc (right):

https://codereview.chromium.org/864563002/diff/40001/base/android/jni_onload_...
base/android/jni_onload_delegate.cc:10: JNIOnLoadDelegate::~JNIOnLoadDelegate()
{}
nit: you can just inline this in the header and then remove this file

https://codereview.chromium.org/864563002/diff/40001/content/app/android/cont...
File content/app/android/content_jni_onload_delegate.h (right):

https://codereview.chromium.org/864563002/diff/40001/content/app/android/cont...
content/app/android/content_jni_onload_delegate.h:14: class
ContentJNIOnLoadDelegate
ditto, inline this in the one file that uses it

https://codereview.chromium.org/864563002/diff/40001/content/public/app/conte...
File content/public/app/content_jni_onload.h (right):

https://codereview.chromium.org/864563002/diff/40001/content/public/app/conte...
content/public/app/content_jni_onload.h:29:
std::vector<base::android::JNIOnLoadDelegate*>* delegates);
nit: no need to take a vector here, since there's only one module that sits
above content. so this method can just be:

bool OnJNIOnLoad(
    JavaVM* vm,
    base::android::JNIOnLoadDelegate* delegate);

https://codereview.chromium.org/864563002/diff/40001/content/shell/android/br...
File
content/shell/android/browsertests_apk/content_browser_tests_jni_onload_delegate.h
(right):

https://codereview.chromium.org/864563002/diff/40001/content/shell/android/br...
content/shell/android/browsertests_apk/content_browser_tests_jni_onload_delegate.h:13:
class ContentBrowserTestsJNIOnLoadDelegate :
ditto: inline too

https://codereview.chromium.org/864563002/diff/40001/content/shell/android/sh...
File content/shell/android/shell_jni_onload_delegate.h (right):

https://codereview.chromium.org/864563002/diff/40001/content/shell/android/sh...
content/shell/android/shell_jni_onload_delegate.h:13: class
ShellJNIOnLoadDelegate : public base::android::JNIOnLoadDelegate {
ditto

https://codereview.chromium.org/864563002/diff/40001/testing/android/native_t...
File testing/android/native_test_jni_onload_delegate.h (right):

https://codereview.chromium.org/864563002/diff/40001/testing/android/native_t...
testing/android/native_test_jni_onload_delegate.h:10: class
NativeTestJNIOnLoadDelegate : public base::android::JNIOnLoadDelegate {
ditto

[michaelbai, 2015-01-27 03:45:32]

jam@, all comments were addressed, PTAL

[jam, 2015-01-27 16:10:34]

lgtm

https://codereview.chromium.org/864563002/diff/60001/base/android/base_jni_on...
File base/android/base_jni_onload.cc (right):

https://codereview.chromium.org/864563002/diff/60001/base/android/base_jni_on...
base/android/base_jni_onload.cc:23: bool
BaseJNIOnLoadDelegate::RegisterJNI(JNIEnv* env) {
nit: it'd be easier to read if these two short methods wre inlined above.. same
for other files

https://codereview.chromium.org/864563002/diff/60001/base/android/jni_onload_...
File base/android/jni_onload_delegate.h (right):

https://codereview.chromium.org/864563002/diff/60001/base/android/jni_onload_...
base/android/jni_onload_delegate.h:27: virtual ~JNIOnLoadDelegate() {};
nit: semicolon not needed

https://codereview.chromium.org/864563002/diff/60001/content/public/app/conte...
File content/public/app/content_jni_onload.h (right):

https://codereview.chromium.org/864563002/diff/60001/content/public/app/conte...
content/public/app/content_jni_onload.h:28: base::android::JNIOnLoadDelegate*
delegates);
nit: delegate instead of delegates

https://codereview.chromium.org/864563002/diff/60001/content/shell/android/br...
File content/shell/android/browsertests_apk/content_browser_tests_jni_onload.cc
(right):

https://codereview.chromium.org/864563002/diff/60001/content/shell/android/br...
content/shell/android/browsertests_apk/content_browser_tests_jni_onload.cc:16:
class ContentBrowserTestsJNIOnLoadDelegate :
nit: 80 chars

[michaelbai, 2015-01-27 20:13:50]

@jam, please see my comments about inline implementation

https://codereview.chromium.org/864563002/diff/60001/base/android/base_jni_on...
File base/android/base_jni_onload.cc (right):

https://codereview.chromium.org/864563002/diff/60001/base/android/base_jni_on...
base/android/base_jni_onload.cc:23: bool
BaseJNIOnLoadDelegate::RegisterJNI(JNIEnv* env) {
On 2015/01/27 16:10:33, jam wrote:
> nit: it'd be easier to read if these two short methods wre inlined above..
same
> for other files

I were told this style is only acceptable in unit tests, is it true?
I expect the method will be more complicated than now after refactoring is done,
is it still ok to have them inline?

https://codereview.chromium.org/864563002/diff/60001/base/android/jni_onload_...
File base/android/jni_onload_delegate.h (right):

https://codereview.chromium.org/864563002/diff/60001/base/android/jni_onload_...
base/android/jni_onload_delegate.h:27: virtual ~JNIOnLoadDelegate() {};
On 2015/01/27 16:10:33, jam wrote:
> nit: semicolon not needed

Done.

https://codereview.chromium.org/864563002/diff/60001/content/shell/android/br...
File content/shell/android/browsertests_apk/content_browser_tests_jni_onload.cc
(right):

https://codereview.chromium.org/864563002/diff/60001/content/shell/android/br...
content/shell/android/browsertests_apk/content_browser_tests_jni_onload.cc:16:
class ContentBrowserTestsJNIOnLoadDelegate :
On 2015/01/27 16:10:33, jam wrote:
> nit: 80 chars

Will exceeds 80 chars if merge them into one line.

[michaelbai, 2015-01-27 23:54:34]

nyquist@  PTAL
dtrainor@ PTAL for both chrome and testing/android

[nyquist, 2015-01-28 01:25:40]

base lgtm

https://codereview.chromium.org/864563002/diff/80001/base/android/jni_onload_...
File base/android/jni_onload_delegate.h (right):

https://codereview.chromium.org/864563002/diff/80001/base/android/jni_onload_...
base/android/jni_onload_delegate.h:24: // RegisterJNI(), so it could be strip
out when JNI registration isn't needed.
Nit: stripped*

https://codereview.chromium.org/864563002/diff/80001/base/android/jni_onload_...
base/android/jni_onload_delegate.h:29: // Returns true if the JNI registration
succeeded.
Nit: s/true if/whether/

https://codereview.chromium.org/864563002/diff/80001/base/android/jni_onload_...
base/android/jni_onload_delegate.h:32: // Returns true if the initialization
succeeded. This method is called after
Nit: s/true if/whether/

[nyquist, 2015-01-28 01:26:03]

https://codereview.chromium.org/864563002/diff/80001/base/android/base_jni_on...
File base/android/base_jni_onload.h (right):

https://codereview.chromium.org/864563002/diff/80001/base/android/base_jni_on...
base/android/base_jni_onload.h:18: // Returns true if JNI registration and
initialization succeeded. Caller shall
Nit: s/true if/whether/

[David Trainor- moved to gerrit, 2015-01-28 19:13:25]

chrome/ and testing/ lgtm with small nits

https://codereview.chromium.org/864563002/diff/80001/chrome/app/android/chrom...
File chrome/app/android/chrome_jni_onload.cc (right):

https://codereview.chromium.org/864563002/diff/80001/chrome/app/android/chrom...
chrome/app/android/chrome_jni_onload.cc:14: virtual bool RegisterJNI(JNIEnv*
env) override;
I'm not 100% sure, but I just looked at web_contents_impl.h, to check.  I don't
think you need virtual if you use override (the new non-macro version).  Same
with method below.

https://codereview.chromium.org/864563002/diff/80001/testing/android/native_t...
File testing/android/native_test_jni_onload.cc (right):

https://codereview.chromium.org/864563002/diff/80001/testing/android/native_t...
testing/android/native_test_jni_onload.cc:14: virtual bool RegisterJNI(JNIEnv*
env) override;
Same as above

[michaelbai, 2015-01-28 19:50:58]

you are right, virtual is not necessary

https://codereview.chromium.org/864563002/diff/80001/chrome/app/android/chrom...
File chrome/app/android/chrome_jni_onload.cc (right):

https://codereview.chromium.org/864563002/diff/80001/chrome/app/android/chrom...
chrome/app/android/chrome_jni_onload.cc:14: virtual bool RegisterJNI(JNIEnv*
env) override;
On 2015/01/28 19:13:25, David Trainor wrote:
> I'm not 100% sure, but I just looked at web_contents_impl.h, to check.  I
don't
> think you need virtual if you use override (the new non-macro version).  Same
> with method below.

Done.

[michaelbai, 2015-01-28 20:16:11]

The CQ bit was checked by michaelbai@chromium.org

[commit-bot: I haz the power, 2015-01-28 20:16:25]

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

[commit-bot: I haz the power, 2015-01-28 21:41:06]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-01-28 21:42:23]

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

[jam, 2015-01-28 21:48:49]

https://codereview.chromium.org/864563002/diff/60001/base/android/base_jni_on...
File base/android/base_jni_onload.cc (right):

https://codereview.chromium.org/864563002/diff/60001/base/android/base_jni_on...
base/android/base_jni_onload.cc:23: bool
BaseJNIOnLoadDelegate::RegisterJNI(JNIEnv* env) {
On 2015/01/27 20:13:50, michaelbai wrote:
> On 2015/01/27 16:10:33, jam wrote:
> > nit: it'd be easier to read if these two short methods wre inlined above..
> same
> > for other files
> 
> I were told this style is only acceptable in unit tests, is it true?

I just noticed the wiki. this is the first time I see it, but it's not a
requirement to follow that page. i'll follow up with peter. we also have tons of
examples to the contrary. 

> I expect the method will be more complicated than now after refactoring is
done,
> is it still ok to have them inline?

[Peter Kasting, 2015-01-28 23:40:46]

On 2015/01/27 20:13:50, michaelbai wrote:
>
https://codereview.chromium.org/864563002/diff/60001/base/android/base_jni_on...
> File base/android/base_jni_onload.cc (right):
> 
>
https://codereview.chromium.org/864563002/diff/60001/base/android/base_jni_on...
> base/android/base_jni_onload.cc:23: bool
> BaseJNIOnLoadDelegate::RegisterJNI(JNIEnv* env) {
> On 2015/01/27 16:10:33, jam wrote:
> > nit: it'd be easier to read if these two short methods wre inlined above..
> same
> > for other files
> 
> I were told this style is only acceptable in unit tests, is it true?
> I expect the method will be more complicated than now after refactoring is
done,
> is it still ok to have them inline?

For reference: inlining code into class declarations is acceptable in .cc files;
it's primarily a judgment call between author and reviewers what is ultimately
most readable.

The C++ Dos and Don'ts page (
https://sites.google.com/a/chromium.org/dev/developers/coding-style/cpp-dos-a...
) has been clarified to make this more explicit.

[michaelbai, 2015-01-28 23:51:04]

On 2015/01/28 23:40:46, Peter Kasting wrote:
> On 2015/01/27 20:13:50, michaelbai wrote:
> >
>
https://codereview.chromium.org/864563002/diff/60001/base/android/base_jni_on...
> > File base/android/base_jni_onload.cc (right):
> > 
> >
>
https://codereview.chromium.org/864563002/diff/60001/base/android/base_jni_on...
> > base/android/base_jni_onload.cc:23: bool
> > BaseJNIOnLoadDelegate::RegisterJNI(JNIEnv* env) {
> > On 2015/01/27 16:10:33, jam wrote:
> > > nit: it'd be easier to read if these two short methods wre inlined above..
> > same
> > > for other files
> > 
> > I were told this style is only acceptable in unit tests, is it true?
> > I expect the method will be more complicated than now after refactoring is
> done,
> > is it still ok to have them inline?
> 
> For reference: inlining code into class declarations is acceptable in .cc
files;
> it's primarily a judgment call between author and reviewers what is ultimately
> most readable.
> 
> The C++ Dos and Don'ts page (
>
https://sites.google.com/a/chromium.org/dev/developers/coding-style/cpp-dos-a...
> ) has been clarified to make this more explicit.


Thanks for clarifying this!!