Separate OnJNIOnLoad

Separate OnJNIOnLoad

Separate it into OnJNIOnLoadRegisterJNI() and OnJNIOnLoadInit(), so they
could be called by embedder when needed.

BUG=447393
Committed: https://crrev.com/5237cb89a8fc1a743b483bc2be5cdbe9669d9d86
Cr-Commit-Position: refs/heads/master@{#317479}

[michaelbai, 2015-02-19 20:51:35]

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

[michaelbai, 2015-02-19 20:52:42]

jam@ overall
nyquist@ base/android
David@ chrome/android

[michaelbai, 2015-02-19 20:53:11]

gunsch@ chromecast
torne@ android_webview

[michaelbai, 2015-02-19 20:54:06]

nyquist@ also testing

[gunsch, 2015-02-19 20:56:25]

On 2015/02/19 20:54:06, michaelbai wrote:
> nyquist@ also testing

can you also: "git cl try -m tryserver.chromium.linux -b cast_shell_apk" ?

[gunsch, 2015-02-19 20:56:48]

On 2015/02/19 20:56:25, gunsch wrote:
> On 2015/02/19 20:54:06, michaelbai wrote:
> > nyquist@ also testing
> 
> can you also: "git cl try -m tryserver.chromium.linux -b cast_shell_apk" ?

but generally chromecast/ lgtm

[jam, 2015-02-19 22:17:56]

https://codereview.chromium.org/935413004/diff/1/base/android/base_jni_onload.h
File base/android/base_jni_onload.h (right):

https://codereview.chromium.org/935413004/diff/1/base/android/base_jni_onload...
base/android/base_jni_onload.h:21: BASE_EXPORT bool OnJNIOnLoadRegisterJNI(
nit: the JNI suffix seems redundant

https://codereview.chromium.org/935413004/diff/1/base/android/base_jni_onload...
base/android/base_jni_onload.h:28: BASE_EXPORT bool
OnJNIOnLoadInit(std::vector<JNIOnLoadDelegate*>* delegates);
now that there are two methods, it's a bit confusing that they each take an
interface that contains callbacks for the other methods. it would be clearer to
have each one take callbacks. i.e.

typedef base::Callback<bool(JNIEnv*)> RegisterCallback;
BASE_EXPORT bool OnJNIOnLoadRegister(
  JavaVM* vm,
  std::vector<RegisterCallback> callbacks);

typedef base::Callback<bool(void)> InitCallback;
BASE_EXPORT bool OnJNIOnLoadInit(std::vector<InitCallback> callbacks);

[David Trainor- moved to gerrit, 2015-02-19 22:19:23]

chrome/app/android lgtm

[michaelbai, 2015-02-20 00:50:08]

New patchsets have been uploaded after l-g-t-m from
gunsch@chromium.org,dtrainor@chromium.org

[michaelbai, 2015-02-20 00:58:36]

@jam, PTAL

https://codereview.chromium.org/935413004/diff/1/base/android/base_jni_onload.h
File base/android/base_jni_onload.h (right):

https://codereview.chromium.org/935413004/diff/1/base/android/base_jni_onload...
base/android/base_jni_onload.h:21: BASE_EXPORT bool OnJNIOnLoadRegisterJNI(
On 2015/02/19 22:17:55, jam wrote:
> nit: the JNI suffix seems redundant

JNIOnLoad() is entry point of android shared library, I want keep the name to
indicate the methods are called in JNI_OnLoad()

https://codereview.chromium.org/935413004/diff/1/base/android/base_jni_onload...
base/android/base_jni_onload.h:28: BASE_EXPORT bool
OnJNIOnLoadInit(std::vector<JNIOnLoadDelegate*>* delegates);
On 2015/02/19 22:17:55, jam wrote:
> now that there are two methods, it's a bit confusing that they each take an
> interface that contains callbacks for the other methods. it would be clearer
to
> have each one take callbacks. i.e.
> 
> typedef base::Callback<bool(JNIEnv*)> RegisterCallback;
> BASE_EXPORT bool OnJNIOnLoadRegister(
>   JavaVM* vm,
>   std::vector<RegisterCallback> callbacks);
> 
> typedef base::Callback<bool(void)> InitCallback;
> BASE_EXPORT bool OnJNIOnLoadInit(std::vector<InitCallback> callbacks);

Done.

[jam, 2015-02-20 06:37:37]

lgtm

https://codereview.chromium.org/935413004/diff/20001/android_webview/lib/main...
File android_webview/lib/main/webview_entry_point.cc (right):

https://codereview.chromium.org/935413004/diff/20001/android_webview/lib/main...
android_webview/lib/main/webview_entry_point.cc:5: #include
"android_webview/lib/main/webview_jni_onload.h"
nit: why not just inline these two methods in this file and get rid of that
header and cc file?

[michaelbai, 2015-02-20 17:00:17]

David and @gunsch,
The patch had significant changes since you reviewed, PTAL 

@nyquist, @torne
could you review this today?

https://codereview.chromium.org/935413004/diff/20001/android_webview/lib/main...
File android_webview/lib/main/webview_entry_point.cc (right):

https://codereview.chromium.org/935413004/diff/20001/android_webview/lib/main...
android_webview/lib/main/webview_entry_point.cc:5: #include
"android_webview/lib/main/webview_jni_onload.h"
On 2015/02/20 06:37:37, jam wrote:
> nit: why not just inline these two methods in this file and get rid of that
> header and cc file?

These methods will be used by the monochrome's JNI_OnLoad() method.

[michaelbai, 2015-02-20 17:06:15]

https://codereview.chromium.org/935413004/diff/20001/android_webview/lib/main...
File android_webview/lib/main/webview_entry_point.cc (right):

https://codereview.chromium.org/935413004/diff/20001/android_webview/lib/main...
android_webview/lib/main/webview_entry_point.cc:5: #include
"android_webview/lib/main/webview_jni_onload.h"
On 2015/02/20 17:00:17, michaelbai wrote:
> On 2015/02/20 06:37:37, jam wrote:
> > nit: why not just inline these two methods in this file and get rid of that
> > header and cc file?
> 
> These methods will be used by the monochrome's JNI_OnLoad() method.

or I will have android_webview::OnJNIOnLoadInit(), might refactorying this part
in monochrome's CL.

[gunsch, 2015-02-20 17:37:31]

chromecast/ still lgtm, can you give the "cast_shell_apk" trybot another whirl
for good measure? (it's still in a probationary stage and not yet automatic)

[Torne, 2015-02-20 18:24:10]

LGTM.

Sorry for not noticing the problem with the previous approach in the other CLs
you sent out; thanks a lot for doing this refactoring.

https://codereview.chromium.org/935413004/diff/20001/android_webview/lib/main...
File android_webview/lib/main/webview_entry_point.cc (right):

https://codereview.chromium.org/935413004/diff/20001/android_webview/lib/main...
android_webview/lib/main/webview_entry_point.cc:5: #include
"android_webview/lib/main/webview_jni_onload.h"
Yeah, we will almost certainly need to reuse these methods in the monochrome
entry point as Michael says. It's better for us if this file contains nothing
but JNI_OnLoad doing as little as possible by calling other functions.

[michaelbai, 2015-02-20 18:54:42]

New patchsets have been uploaded after l-g-t-m from
jam@chromium.org,gunsch@chromium.org,torne@chromium.org

[nyquist, 2015-02-20 19:45:16]

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

https://codereview.chromium.org/935413004/diff/40001/base/android/base_jni_on...
base/android/base_jni_onload.cc:31: if (!i->Run(env))
Is there a reason why we don't pop_back() before returning false?

[michaelbai, 2015-02-20 19:59:20]

PTAL, thanks

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

https://codereview.chromium.org/935413004/diff/40001/base/android/base_jni_on...
base/android/base_jni_onload.cc:31: if (!i->Run(env))
On 2015/02/20 19:45:16, nyquist wrote:
> Is there a reason why we don't pop_back() before returning false?

Thanks, fixed it

[michaelbai, 2015-02-20 20:11:13]

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

https://codereview.chromium.org/935413004/diff/40001/base/android/base_jni_on...
base/android/base_jni_onload.cc:31: if (!i->Run(env))
On 2015/02/20 19:59:20, michaelbai wrote:
> On 2015/02/20 19:45:16, nyquist wrote:
> > Is there a reason why we don't pop_back() before returning false?
> 
> Thanks, fixed it

Actually, callbacks has been copied, I don't think I need to pop_back() now.

[nyquist, 2015-02-20 21:43:41]

ok. lgtm

[michaelbai, 2015-02-20 21:44:29]

The CQ bit was checked by michaelbai@chromium.org

[commit-bot: I haz the power, 2015-02-20 21:45:28]

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

[michaelbai, 2015-02-20 22:06:49]

The CQ bit was unchecked by michaelbai@chromium.org

[michaelbai, 2015-02-20 23:29:50]

New patchsets have been uploaded after l-g-t-m from nyquist@chromium.org

[michaelbai, 2015-02-20 23:31:38]

The CQ bit was checked by michaelbai@chromium.org

[commit-bot: I haz the power, 2015-02-20 23:33:09]

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

[commit-bot: I haz the power, 2015-02-21 01:29:47]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-02-21 01:30:19]

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