refactoring Android Webview JNI_OnLoad interface

refactoring Android Webview JNI_OnLoad interface to make it consistent with others.

BUG=464008
Committed: https://crrev.com/0fb835f016fa4c63bdb3a2bf363d67eb8525a121
Cr-Commit-Position: refs/heads/master@{#320174}

[michaelbai, 2015-03-04 23:05:47]

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

[Torne, 2015-03-05 18:56:09]

https://codereview.chromium.org/975253002/diff/20001/android_webview/android_...
File android_webview/android_webview.gyp (right):

https://codereview.chromium.org/975253002/diff/20001/android_webview/android_...
android_webview/android_webview.gyp:352: 'target_name': 'android_webview',
This target is unnecessary - it is exactly the same thing as
android_webview_common.

android_webview_build==1 is only defined downstream in Android and isn't
relevant to any work going forward; as far as anyone in chromium needs to be
concerned now, this condition is always false and I intend to permanently remove
them all from the sources soon.

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

https://codereview.chromium.org/975253002/diff/20001/android_webview/lib/main...
android_webview/lib/main/webview_jni_onload.cc:57:
base::android::DisableManualJniRegistration();
I think we should leave this in JNI_OnLoad, since whether it's safe to do this
or not is a property of the final linked .so. It is not automatically the case
that any binary which includes webview_jni_onload.cc is able to use native JNI
exports.

[michaelbai, 2015-03-05 19:28:53]

PTAL

https://codereview.chromium.org/975253002/diff/20001/android_webview/android_...
File android_webview/android_webview.gyp (right):

https://codereview.chromium.org/975253002/diff/20001/android_webview/android_...
android_webview/android_webview.gyp:352: 'target_name': 'android_webview',
On 2015/03/05 18:56:09, Torne wrote:
> This target is unnecessary - it is exactly the same thing as
> android_webview_common.
> 
> android_webview_build==1 is only defined downstream in Android and isn't
> relevant to any work going forward; as far as anyone in chromium needs to be
> concerned now, this condition is always false and I intend to permanently
remove
> them all from the sources soon.

Sounds good, I reverted this part

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

https://codereview.chromium.org/975253002/diff/20001/android_webview/lib/main...
android_webview/lib/main/webview_jni_onload.cc:57:
base::android::DisableManualJniRegistration();
On 2015/03/05 18:56:09, Torne wrote:
> I think we should leave this in JNI_OnLoad, since whether it's safe to do this
> or not is a property of the final linked .so. It is not automatically the case
> that any binary which includes webview_jni_onload.cc is able to use native JNI
> exports.

Will WebView always disable JNI Registration no matter what shared library it is
in?
If the shared library doesn't support native JNI export, it shouldn't include
WebView.

[Torne, 2015-03-05 20:12:48]

On 2015/03/05 19:28:53, michaelbai wrote:
> PTAL
> 
>
https://codereview.chromium.org/975253002/diff/20001/android_webview/android_...
> File android_webview/android_webview.gyp (right):
> 
>
https://codereview.chromium.org/975253002/diff/20001/android_webview/android_...
> android_webview/android_webview.gyp:352: 'target_name': 'android_webview',
> On 2015/03/05 18:56:09, Torne wrote:
> > This target is unnecessary - it is exactly the same thing as
> > android_webview_common.
> > 
> > android_webview_build==1 is only defined downstream in Android and isn't
> > relevant to any work going forward; as far as anyone in chromium needs to be
> > concerned now, this condition is always false and I intend to permanently
> remove
> > them all from the sources soon.
> 
> Sounds good, I reverted this part

Can you mention which JNI_OnLoad you are updating in the commit message? It
reads a bit oddly at the moment :)

>
https://codereview.chromium.org/975253002/diff/20001/android_webview/lib/main...
> File android_webview/lib/main/webview_jni_onload.cc (right):
> 
>
https://codereview.chromium.org/975253002/diff/20001/android_webview/lib/main...
> android_webview/lib/main/webview_jni_onload.cc:57:
> base::android::DisableManualJniRegistration();
> On 2015/03/05 18:56:09, Torne wrote:
> > I think we should leave this in JNI_OnLoad, since whether it's safe to do
this
> > or not is a property of the final linked .so. It is not automatically the
case
> > that any binary which includes webview_jni_onload.cc is able to use native
JNI
> > exports.
> 
> Will WebView always disable JNI Registration no matter what shared library it
is
> in?
> If the shared library doesn't support native JNI export, it shouldn't include
> WebView.

It just seems like we should keep the disabling of JNI registration in the same
place that actually makes it possible, namely the actual shared library target
(gyp/gn settings are required on the actual target to make it work). If this
code gets linked into another binary which doesn't set the right settings then
it will make everything crash.

[michaelbai, 2015-03-05 20:33:16]

On 2015/03/05 20:12:48, Torne wrote:
> On 2015/03/05 19:28:53, michaelbai wrote:
> > PTAL
> > 
> >
>
https://codereview.chromium.org/975253002/diff/20001/android_webview/android_...
> > File android_webview/android_webview.gyp (right):
> > 
> >
>
https://codereview.chromium.org/975253002/diff/20001/android_webview/android_...
> > android_webview/android_webview.gyp:352: 'target_name': 'android_webview',
> > On 2015/03/05 18:56:09, Torne wrote:
> > > This target is unnecessary - it is exactly the same thing as
> > > android_webview_common.
> > > 
> > > android_webview_build==1 is only defined downstream in Android and isn't
> > > relevant to any work going forward; as far as anyone in chromium needs to
be
> > > concerned now, this condition is always false and I intend to permanently
> > remove
> > > them all from the sources soon.
> > 
> > Sounds good, I reverted this part
> 
> Can you mention which JNI_OnLoad you are updating in the commit message? It
> reads a bit oddly at the moment :)
>

Description updated.
 
> >
>
https://codereview.chromium.org/975253002/diff/20001/android_webview/lib/main...
> > File android_webview/lib/main/webview_jni_onload.cc (right):
> > 
> >
>
https://codereview.chromium.org/975253002/diff/20001/android_webview/lib/main...
> > android_webview/lib/main/webview_jni_onload.cc:57:
> > base::android::DisableManualJniRegistration();
> > On 2015/03/05 18:56:09, Torne wrote:
> > > I think we should leave this in JNI_OnLoad, since whether it's safe to do
> this
> > > or not is a property of the final linked .so. It is not automatically the
> case
> > > that any binary which includes webview_jni_onload.cc is able to use native
> JNI
> > > exports.
> > 
> > Will WebView always disable JNI Registration no matter what shared library
it
> is
> > in?
> > If the shared library doesn't support native JNI export, it shouldn't
include
> > WebView.
> 
> It just seems like we should keep the disabling of JNI registration in the
same
> place that actually makes it possible, namely the actual shared library target
> (gyp/gn settings are required on the actual target to make it work). If this
> code gets linked into another binary which doesn't set the right settings then
> it will make everything crash.

I think we only need JNI registration for instrumentation tests, right?
If so, I'd like have a dedicated JNI_OnLoad for it, and call
android_webview::OnJNIOnLoadRegisterJNI(), and current JNI_OnLoad in
webview_entry_point.cc only call android_webview::OnJNIOnLoadInit(), then we can
remove the base::android::DisableManualJniRegistration(), sounds good?

[Torne, 2015-03-05 21:07:18]

On 2015/03/05 20:33:16, michaelbai wrote:
> On 2015/03/05 20:12:48, Torne wrote:
> > It just seems like we should keep the disabling of JNI registration in the
> same
> > place that actually makes it possible, namely the actual shared library
target
> > (gyp/gn settings are required on the actual target to make it work). If this
> > code gets linked into another binary which doesn't set the right settings
then
> > it will make everything crash.
> 
> I think we only need JNI registration for instrumentation tests, right?

I'm not sure what you mean by this; we need JNI registration for any binary
which hasn't explicitly opted in to using exports. Currently the only binary
that has done so is libwebviewchromium. I don't think we should just assume that
nothing else is going to depend on this static library ever. This is one line of
code, it doesn't seem like it should be a problem to repeat it in entrypoints
that want it. 

> If so, I'd like have a dedicated JNI_OnLoad for it, and call
> android_webview::OnJNIOnLoadRegisterJNI(), and current JNI_OnLoad in
> webview_entry_point.cc only call android_webview::OnJNIOnLoadInit(), then we
can
> remove the base::android::DisableManualJniRegistration(), sounds good?

I wasn't planning to remove DisableManualJniRegistration. Even when your
refactoring is done nothing will be preventing people from accidentally
regressing it, and we're unlikely to notice quickly as the incremental startup
cost of registering a few JNI functions is pretty small. We could make it
DCHECK() or something to prevent regressions instead of just having it silently
return at runtime, but we would still need the flag to be set for the DCHECK to
test.

We currently build an identical binary for the instrumentation tests and the
real webview binary (although we still link it twice..), and both use JNI
exports.

[michaelbai, 2015-03-05 21:25:39]

On 2015/03/05 21:07:18, Torne wrote:
> On 2015/03/05 20:33:16, michaelbai wrote:
> > On 2015/03/05 20:12:48, Torne wrote:
> > > It just seems like we should keep the disabling of JNI registration in the
> > same
> > > place that actually makes it possible, namely the actual shared library
> target
> > > (gyp/gn settings are required on the actual target to make it work). If
this
> > > code gets linked into another binary which doesn't set the right settings
> then
> > > it will make everything crash.
> > 
> > I think we only need JNI registration for instrumentation tests, right?
> 
> I'm not sure what you mean by this; we need JNI registration for any binary
> which hasn't explicitly opted in to using exports. Currently the only binary
> that has done so is libwebviewchromium. I don't think we should just assume
that
> nothing else is going to depend on this static library ever. This is one line
of
> code, it doesn't seem like it should be a problem to repeat it in entrypoints
> that want it. 
> 

I meant android webview here.


> > If so, I'd like have a dedicated JNI_OnLoad for it, and call
> > android_webview::OnJNIOnLoadRegisterJNI(), and current JNI_OnLoad in
> > webview_entry_point.cc only call android_webview::OnJNIOnLoadInit(), then we
> can
> > remove the base::android::DisableManualJniRegistration(), sounds good?
> 
> I wasn't planning to remove DisableManualJniRegistration. Even when your
> refactoring is done nothing will be preventing people from accidentally
> regressing it, and we're unlikely to notice quickly as the incremental startup
> cost of registering a few JNI functions is pretty small. We could make it
> DCHECK() or something to prevent regressions instead of just having it
silently
> return at runtime, but we would still need the flag to be set for the DCHECK
to
> test.
> 
> We currently build an identical binary for the instrumentation tests and the
> real webview binary (although we still link it twice..), and both use JNI
> exports.


OK, so we don't need android_webview::OnJNIOnLoadRegisterJNI() at all, right?
It should be safe to remove all the registration code in android_webview now?

[Torne, 2015-03-05 21:30:00]

On 2015/03/05 21:25:39, michaelbai wrote:
> On 2015/03/05 21:07:18, Torne wrote:
> > On 2015/03/05 20:33:16, michaelbai wrote:
> > > On 2015/03/05 20:12:48, Torne wrote:
> > > > It just seems like we should keep the disabling of JNI registration in
the
> > > same
> > > > place that actually makes it possible, namely the actual shared library
> > target
> > > > (gyp/gn settings are required on the actual target to make it work). If
> this
> > > > code gets linked into another binary which doesn't set the right
settings
> > then
> > > > it will make everything crash.
> > > 
> > > I think we only need JNI registration for instrumentation tests, right?
> > 
> > I'm not sure what you mean by this; we need JNI registration for any binary
> > which hasn't explicitly opted in to using exports. Currently the only binary
> > that has done so is libwebviewchromium. I don't think we should just assume
> that
> > nothing else is going to depend on this static library ever. This is one
line
> of
> > code, it doesn't seem like it should be a problem to repeat it in
entrypoints
> > that want it. 
> > 
> 
> I meant android webview here.
> 
> 
> > > If so, I'd like have a dedicated JNI_OnLoad for it, and call
> > > android_webview::OnJNIOnLoadRegisterJNI(), and current JNI_OnLoad in
> > > webview_entry_point.cc only call android_webview::OnJNIOnLoadInit(), then
we
> > can
> > > remove the base::android::DisableManualJniRegistration(), sounds good?
> > 
> > I wasn't planning to remove DisableManualJniRegistration. Even when your
> > refactoring is done nothing will be preventing people from accidentally
> > regressing it, and we're unlikely to notice quickly as the incremental
startup
> > cost of registering a few JNI functions is pretty small. We could make it
> > DCHECK() or something to prevent regressions instead of just having it
> silently
> > return at runtime, but we would still need the flag to be set for the DCHECK
> to
> > test.
> > 
> > We currently build an identical binary for the instrumentation tests and the
> > real webview binary (although we still link it twice..), and both use JNI
> > exports.
> 
> 
> OK, so we don't need android_webview::OnJNIOnLoadRegisterJNI() at all, right?
> It should be safe to remove all the registration code in android_webview now?

I'd prefer not to because it means we can't, for example, build with manual
registration to compare performance. We don't ever run with it any more, though,
so it might well regress and not actually work..

[michaelbai, 2015-03-05 21:51:09]

On 2015/03/05 21:30:00, Torne wrote:
> On 2015/03/05 21:25:39, michaelbai wrote:
> > On 2015/03/05 21:07:18, Torne wrote:
> > > On 2015/03/05 20:33:16, michaelbai wrote:
> > > > On 2015/03/05 20:12:48, Torne wrote:
> > > > > It just seems like we should keep the disabling of JNI registration in
> the
> > > > same
> > > > > place that actually makes it possible, namely the actual shared
library
> > > target
> > > > > (gyp/gn settings are required on the actual target to make it work).
If
> > this
> > > > > code gets linked into another binary which doesn't set the right
> settings
> > > then
> > > > > it will make everything crash.
> > > > 
> > > > I think we only need JNI registration for instrumentation tests, right?
> > > 
> > > I'm not sure what you mean by this; we need JNI registration for any
binary
> > > which hasn't explicitly opted in to using exports. Currently the only
binary
> > > that has done so is libwebviewchromium. I don't think we should just
assume
> > that
> > > nothing else is going to depend on this static library ever. This is one
> line
> > of
> > > code, it doesn't seem like it should be a problem to repeat it in
> entrypoints
> > > that want it. 
> > > 
> > 
> > I meant android webview here.
> > 
> > 
> > > > If so, I'd like have a dedicated JNI_OnLoad for it, and call
> > > > android_webview::OnJNIOnLoadRegisterJNI(), and current JNI_OnLoad in
> > > > webview_entry_point.cc only call android_webview::OnJNIOnLoadInit(),
then
> we
> > > can
> > > > remove the base::android::DisableManualJniRegistration(), sounds good?
> > > 
> > > I wasn't planning to remove DisableManualJniRegistration. Even when your
> > > refactoring is done nothing will be preventing people from accidentally
> > > regressing it, and we're unlikely to notice quickly as the incremental
> startup
> > > cost of registering a few JNI functions is pretty small. We could make it
> > > DCHECK() or something to prevent regressions instead of just having it
> > silently
> > > return at runtime, but we would still need the flag to be set for the
DCHECK
> > to
> > > test.
> > > 
> > > We currently build an identical binary for the instrumentation tests and
the
> > > real webview binary (although we still link it twice..), and both use JNI
> > > exports.
> > 
> > 
> > OK, so we don't need android_webview::OnJNIOnLoadRegisterJNI() at all,
right?
> > It should be safe to remove all the registration code in android_webview
now?
> 
> I'd prefer not to because it means we can't, for example, build with manual
> registration to compare performance. We don't ever run with it any more,
though,
> so it might well regress and not actually work..

OK, then, I think current change (putting the DisableManualJniRegistration() in
OnJNIOnLoadRegisterJNI()) is reasonable, we could add comments to explain why
disable registration before registration.

[michaelbai, 2015-03-05 23:39:15]

On 2015/03/05 21:51:09, michaelbai wrote:
> On 2015/03/05 21:30:00, Torne wrote:
> > On 2015/03/05 21:25:39, michaelbai wrote:
> > > On 2015/03/05 21:07:18, Torne wrote:
> > > > On 2015/03/05 20:33:16, michaelbai wrote:
> > > > > On 2015/03/05 20:12:48, Torne wrote:
> > > > > > It just seems like we should keep the disabling of JNI registration
in
> > the
> > > > > same
> > > > > > place that actually makes it possible, namely the actual shared
> library
> > > > target
> > > > > > (gyp/gn settings are required on the actual target to make it work).
> If
> > > this
> > > > > > code gets linked into another binary which doesn't set the right
> > settings
> > > > then
> > > > > > it will make everything crash.
> > > > > 
> > > > > I think we only need JNI registration for instrumentation tests,
right?
> > > > 
> > > > I'm not sure what you mean by this; we need JNI registration for any
> binary
> > > > which hasn't explicitly opted in to using exports. Currently the only
> binary
> > > > that has done so is libwebviewchromium. I don't think we should just
> assume
> > > that
> > > > nothing else is going to depend on this static library ever. This is one
> > line
> > > of
> > > > code, it doesn't seem like it should be a problem to repeat it in
> > entrypoints
> > > > that want it. 
> > > > 
> > > 
> > > I meant android webview here.
> > > 
> > > 
> > > > > If so, I'd like have a dedicated JNI_OnLoad for it, and call
> > > > > android_webview::OnJNIOnLoadRegisterJNI(), and current JNI_OnLoad in
> > > > > webview_entry_point.cc only call android_webview::OnJNIOnLoadInit(),
> then
> > we
> > > > can
> > > > > remove the base::android::DisableManualJniRegistration(), sounds good?
> > > > 
> > > > I wasn't planning to remove DisableManualJniRegistration. Even when your
> > > > refactoring is done nothing will be preventing people from accidentally
> > > > regressing it, and we're unlikely to notice quickly as the incremental
> > startup
> > > > cost of registering a few JNI functions is pretty small. We could make
it
> > > > DCHECK() or something to prevent regressions instead of just having it
> > > silently
> > > > return at runtime, but we would still need the flag to be set for the
> DCHECK
> > > to
> > > > test.
> > > > 
> > > > We currently build an identical binary for the instrumentation tests and
> the
> > > > real webview binary (although we still link it twice..), and both use
JNI
> > > > exports.
> > > 
> > > 
> > > OK, so we don't need android_webview::OnJNIOnLoadRegisterJNI() at all,
> right?
> > > It should be safe to remove all the registration code in android_webview
> now?
> > 
> > I'd prefer not to because it means we can't, for example, build with manual
> > registration to compare performance. We don't ever run with it any more,
> though,
> > so it might well regress and not actually work..
> 
> OK, then, I think current change (putting the DisableManualJniRegistration()
in
> OnJNIOnLoadRegisterJNI()) is reasonable, we could add comments to explain why
> disable registration before registration.

I doubt we should keep android_webview::OnJNIOnLoadRegisterJNI(), as WebView
never uses it, maybe no one will ever add JNI registration code in
android_webview in the future, this will be broken sooner or later.

[Torne, 2015-03-06 11:36:05]

On 2015/03/05 23:39:15, michaelbai wrote:
> On 2015/03/05 21:51:09, michaelbai wrote:
> > On 2015/03/05 21:30:00, Torne wrote:
> > > On 2015/03/05 21:25:39, michaelbai wrote:
> > > > On 2015/03/05 21:07:18, Torne wrote:
> > > > > On 2015/03/05 20:33:16, michaelbai wrote:
> > > > > > On 2015/03/05 20:12:48, Torne wrote:
> > > > > > > It just seems like we should keep the disabling of JNI
registration
> in
> > > the
> > > > > > same
> > > > > > > place that actually makes it possible, namely the actual shared
> > library
> > > > > target
> > > > > > > (gyp/gn settings are required on the actual target to make it
work).
> > If
> > > > this
> > > > > > > code gets linked into another binary which doesn't set the right
> > > settings
> > > > > then
> > > > > > > it will make everything crash.
> > > > > > 
> > > > > > I think we only need JNI registration for instrumentation tests,
> right?
> > > > > 
> > > > > I'm not sure what you mean by this; we need JNI registration for any
> > binary
> > > > > which hasn't explicitly opted in to using exports. Currently the only
> > binary
> > > > > that has done so is libwebviewchromium. I don't think we should just
> > assume
> > > > that
> > > > > nothing else is going to depend on this static library ever. This is
one
> > > line
> > > > of
> > > > > code, it doesn't seem like it should be a problem to repeat it in
> > > entrypoints
> > > > > that want it. 
> > > > > 
> > > > 
> > > > I meant android webview here.
> > > > 
> > > > 
> > > > > > If so, I'd like have a dedicated JNI_OnLoad for it, and call
> > > > > > android_webview::OnJNIOnLoadRegisterJNI(), and current JNI_OnLoad in
> > > > > > webview_entry_point.cc only call android_webview::OnJNIOnLoadInit(),
> > then
> > > we
> > > > > can
> > > > > > remove the base::android::DisableManualJniRegistration(), sounds
good?
> > > > > 
> > > > > I wasn't planning to remove DisableManualJniRegistration. Even when
your
> > > > > refactoring is done nothing will be preventing people from
accidentally
> > > > > regressing it, and we're unlikely to notice quickly as the incremental
> > > startup
> > > > > cost of registering a few JNI functions is pretty small. We could make
> it
> > > > > DCHECK() or something to prevent regressions instead of just having it
> > > > silently
> > > > > return at runtime, but we would still need the flag to be set for the
> > DCHECK
> > > > to
> > > > > test.
> > > > > 
> > > > > We currently build an identical binary for the instrumentation tests
and
> > the
> > > > > real webview binary (although we still link it twice..), and both use
> JNI
> > > > > exports.
> > > > 
> > > > 
> > > > OK, so we don't need android_webview::OnJNIOnLoadRegisterJNI() at all,
> > right?
> > > > It should be safe to remove all the registration code in android_webview
> > now?
> > > 
> > > I'd prefer not to because it means we can't, for example, build with
manual
> > > registration to compare performance. We don't ever run with it any more,
> > though,
> > > so it might well regress and not actually work..
> > 
> > OK, then, I think current change (putting the DisableManualJniRegistration()
> in
> > OnJNIOnLoadRegisterJNI()) is reasonable, we could add comments to explain
why
> > disable registration before registration.
> 
> I doubt we should keep android_webview::OnJNIOnLoadRegisterJNI(), as WebView
> never uses it, maybe no one will ever add JNI registration code in
> android_webview in the future, this will be broken sooner or later.

Yes, it will probably be broken eventually, but it's not broken now. We are
intending to do perf analysis on this in the near future, so please do not
remove this now. Can you just do the refactoring in this CL and not change any
behaviour, and we will revisit this once we're done with startup time testing?

[michaelbai, 2015-03-06 16:33:54]

Add it back, PTAL

[Torne, 2015-03-06 16:56:24]

On 2015/03/06 16:33:54, michaelbai wrote:
> Add it back, PTAL

This is still a functional change (it's still not calling it from JNI_OnLoad).
Please make it just behave identically to before until we are done verifying the
JNI work.

[michaelbai, 2015-03-09 21:04:30]

Did you mean I should call android_webview::OnJNIOnLoadRegisterJNI()? If so, I
don't think we should call it, this will hide the potential issues when we
refactoring code, like the AtExitManager shown in the latest patch.

If we want to testing the JNI registration later, we could just add
android_webview::OnJNIOnLoadRegisterJNI() back.

[Torne, 2015-03-11 12:02:17]

On 2015/03/09 21:04:30, michaelbai wrote:
> Did you mean I should call android_webview::OnJNIOnLoadRegisterJNI()? If so, I
> don't think we should call it, this will hide the potential issues when we
> refactoring code, like the AtExitManager shown in the latest patch.

The AtExitManager changes here don't seem like the ideal way to do it. Surely
every entry point needs to create AtExitManager at the same point? We should not
handle that specially for WebView, we should fix all the cases together.

> If we want to testing the JNI registration later, we could just add
> android_webview::OnJNIOnLoadRegisterJNI() back.

This is not about testing it later, this is about testing it *now*, as in, the
next couple of days. We have not yet confirmed that we are definitely happy with
the lazy JNI registration approach here. Please do not remove the code to do
manual registration. If you don't want to land a plain refactoring that doesn't
change behaviour, then please hold off on this CL entirely until we are done
verifying things.

[michaelbai, 2015-03-11 16:33:16]

On 2015/03/11 12:02:17, Torne wrote:
> On 2015/03/09 21:04:30, michaelbai wrote:
> > Did you mean I should call android_webview::OnJNIOnLoadRegisterJNI()? If so,
I
> > don't think we should call it, this will hide the potential issues when we
> > refactoring code, like the AtExitManager shown in the latest patch.
> 
> The AtExitManager changes here don't seem like the ideal way to do it. Surely
> every entry point needs to create AtExitManager at the same point? We should
not
> handle that specially for WebView, we should fix all the cases together.
> 

I am refactoring other components which create AtExitManager by itself,
afterward, we could remove AtExitManager here.

> > If we want to testing the JNI registration later, we could just add
> > android_webview::OnJNIOnLoadRegisterJNI() back.
> 
> This is not about testing it later, this is about testing it *now*, as in, the
> next couple of days. We have not yet confirmed that we are definitely happy
with
> the lazy JNI registration approach here. Please do not remove the code to do
> manual registration. If you don't want to land a plain refactoring that
doesn't
> change behaviour, then please hold off on this CL entirely until we are done
> verifying things.

[michaelbai, 2015-03-11 16:33:40]

As we discussed, I updated the patch, PTAL

[Torne, 2015-03-11 18:08:29]

https://codereview.chromium.org/975253002/diff/140001/android_webview/lib/mai...
File android_webview/lib/main/webview_jni_onload.h (right):

https://codereview.chromium.org/975253002/diff/140001/android_webview/lib/mai...
android_webview/lib/main/webview_jni_onload.h:13: // We didn't remove the JNI
registration for comparing the performance between
Replace this whole comment with "TODO(<you or me>): remove this once we no
longer need to be able to run webview with manual JNI registration". There's not
much point in explaining exactly what we are doing (especially as that's not all
we are doing)

https://codereview.chromium.org/975253002/diff/140001/base/android/library_lo...
File base/android/library_loader/library_loader_hooks.cc (right):

https://codereview.chromium.org/975253002/diff/140001/base/android/library_lo...
base/android/library_loader/library_loader_hooks.cc:146: void
InitAtExitManager() {
Why do we need to InitAtExitManager in this way at all? Can't it be done during
the regular init for base? I assume something that's happening during
registerjni requires it; what is that? SHould it be there?

[michaelbai, 2015-03-11 18:29:10]

PTAL

https://codereview.chromium.org/975253002/diff/140001/android_webview/lib/mai...
File android_webview/lib/main/webview_jni_onload.h (right):

https://codereview.chromium.org/975253002/diff/140001/android_webview/lib/mai...
android_webview/lib/main/webview_jni_onload.h:13: // We didn't remove the JNI
registration for comparing the performance between
On 2015/03/11 18:08:29, Torne wrote:
> Replace this whole comment with "TODO(<you or me>): remove this once we no
> longer need to be able to run webview with manual JNI registration". There's
not
> much point in explaining exactly what we are doing (especially as that's not
all
> we are doing)

Done, but you told me that you still want it because you want to comparing the
performance. why it becomes "not all we are doing now"!

https://codereview.chromium.org/975253002/diff/140001/base/android/library_lo...
File base/android/library_loader/library_loader_hooks.cc (right):

https://codereview.chromium.org/975253002/diff/140001/base/android/library_lo...
base/android/library_loader/library_loader_hooks.cc:146: void
InitAtExitManager() {
On 2015/03/11 18:08:29, Torne wrote:
> Why do we need to InitAtExitManager in this way at all? Can't it be done
during
> the regular init for base? I assume something that's happening during
> registerjni requires it; what is that? SHould it be there?

As I said in my previous comment, I am refactoring other components which
initialized AtExitManager by itself, once it is done, we no longer need to call
InitAtExitManager in android_webview, even better, it might be done before
landing this patch

[michaelbai, 2015-03-11 18:41:50]

Removed AtExitManager, PTAL

[Torne, 2015-03-11 19:43:47]

lgtm

[michaelbai, 2015-03-11 22:26:49]

The CQ bit was checked by michaelbai@chromium.org

[commit-bot: I haz the power, 2015-03-11 22:27:31]

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

[commit-bot: I haz the power, 2015-03-11 22:56:26]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2015-03-11 22:57:06]

Patchset 10 (id:??) landed as
https://crrev.com/0fb835f016fa4c63bdb3a2bf363d67eb8525a121
Cr-Commit-Position: refs/heads/master@{#320174}