Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
977843003:
Hook-up OnJNIOnLoadRegisterJNI for JNI registration in mojo network service. (Closed)
|
Created:
5 years, 9 months ago by ppi Modified:
5 years, 9 months ago CC:
chromium-reviews, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org, abarth-chromium, Aaron Boodman, darin (slow to review), ben+mojo_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionHook-up OnJNIOnLoadRegisterJNI for JNI registration in mojo network service.
This patch uses the unified JNI_OnLoad mechanism introduced in
http://crbug.com/447393 in mojo network service.
BUG=447393
Committed: https://crrev.com/860688338d1173c3dae471a4cc17b3809d6cfb36
Cr-Commit-Position: refs/heads/master@{#319152}
Patch Set 1 #Patch Set 2 : Address Ben's comment. #
Total comments: 2
Messages
Total messages: 19 (5 generated)
ppi@chromium.org changed reviewers: + qsr@chromium.org
ppi@chromium.org changed reviewers: + michaelbai@chromium.org
Hi Ben, Tao, ptal.
On 2015/03/04 14:55:30, ppi wrote: > Hi Ben, Tao, > > ptal. LGTM if it runs. I remember an issue with an at_exit manager registered twice that I needed to handle in the mojo repo.
lgtm
ppi@chromium.org changed reviewers: + ben@chromium.org, jamesr@chromium.org
Yup, it was crashing in Debug, thanks for catching this, Ben. I carried the fix we used in java_handler in patch set 2. ptal. +James, Ben - could you review or stamp?
stampy stamp stamp lgtm
https://codereview.chromium.org/977843003/diff/20001/mojo/services/network/an... File mojo/services/network/android_hooks.cc (right): https://codereview.chromium.org/977843003/diff/20001/mojo/services/network/an... mojo/services/network/android_hooks.cc:39: base::android::LibraryLoaderExitHook(); Why can't you use the AtExitManager created in base? If you have to do this, it should be in Init().
LGTM to unblock. Michael -> if you are ready to optionally not call LibraryLoadedHook in Init, we could definitively use that for mojo. https://codereview.chromium.org/977843003/diff/20001/mojo/services/network/an... File mojo/services/network/android_hooks.cc (right): https://codereview.chromium.org/977843003/diff/20001/mojo/services/network/an... mojo/services/network/android_hooks.cc:39: base::android::LibraryLoaderExitHook(); On 2015/03/04 17:25:52, michaelbai wrote: > Why can't you use the AtExitManager created in base? If you have to do this, it > should be in Init(). We use the one in base, but we create it later on as part of the normal flow for all mojo application.
On 2015/03/04 17:28:59, qsr wrote: > LGTM to unblock. Michael -> if you are ready to optionally not call > LibraryLoadedHook in Init, we could definitively use that for mojo. > > https://codereview.chromium.org/977843003/diff/20001/mojo/services/network/an... > File mojo/services/network/android_hooks.cc (right): > > https://codereview.chromium.org/977843003/diff/20001/mojo/services/network/an... > mojo/services/network/android_hooks.cc:39: > base::android::LibraryLoaderExitHook(); > On 2015/03/04 17:25:52, michaelbai wrote: > > Why can't you use the AtExitManager created in base? If you have to do this, > it > > should be in Init(). > > We use the one in base, but we create it later on as part of the normal flow for > all mojo application. I am ok with land this CL, and wonder why mojo creates the AtExitManager instead of using the existing one which is created in JNI_OnLoad?
On 2015/03/04 17:50:51, michaelbai wrote: > On 2015/03/04 17:28:59, qsr wrote: > > LGTM to unblock. Michael -> if you are ready to optionally not call > > LibraryLoadedHook in Init, we could definitively use that for mojo. > > > > > https://codereview.chromium.org/977843003/diff/20001/mojo/services/network/an... > > File mojo/services/network/android_hooks.cc (right): > > > > > https://codereview.chromium.org/977843003/diff/20001/mojo/services/network/an... > > mojo/services/network/android_hooks.cc:39: > > base::android::LibraryLoaderExitHook(); > > On 2015/03/04 17:25:52, michaelbai wrote: > > > Why can't you use the AtExitManager created in base? If you have to do this, > > it > > > should be in Init(). > > > > We use the one in base, but we create it later on as part of the normal flow > for > > all mojo application. > > I am ok with land this CL, and wonder why mojo creates the AtExitManager instead > of using the existing one which is created in JNI_OnLoad? Because the code that create the AtExitManager is shared code.
On 2015/03/04 17:51:50, qsr wrote: > On 2015/03/04 17:50:51, michaelbai wrote: > > On 2015/03/04 17:28:59, qsr wrote: > > > LGTM to unblock. Michael -> if you are ready to optionally not call > > > LibraryLoadedHook in Init, we could definitively use that for mojo. > > > > > > > > > https://codereview.chromium.org/977843003/diff/20001/mojo/services/network/an... > > > File mojo/services/network/android_hooks.cc (right): > > > > > > > > > https://codereview.chromium.org/977843003/diff/20001/mojo/services/network/an... > > > mojo/services/network/android_hooks.cc:39: > > > base::android::LibraryLoaderExitHook(); > > > On 2015/03/04 17:25:52, michaelbai wrote: > > > > Why can't you use the AtExitManager created in base? If you have to do > this, > > > it > > > > should be in Init(). > > > > > > We use the one in base, but we create it later on as part of the normal flow > > for > > > all mojo application. > > > > I am ok with land this CL, and wonder why mojo creates the AtExitManager > instead > > of using the existing one which is created in JNI_OnLoad? > > Because the code that create the AtExitManager is shared code. Sorry, might not have been clear enough. The code run after this is common to all platform, and it is creating the AtExitManager and handling its lifetime.
On 2015/03/04 17:53:01, qsr wrote: > On 2015/03/04 17:51:50, qsr wrote: > > On 2015/03/04 17:50:51, michaelbai wrote: > > > On 2015/03/04 17:28:59, qsr wrote: > > > > LGTM to unblock. Michael -> if you are ready to optionally not call > > > > LibraryLoadedHook in Init, we could definitively use that for mojo. > > > > > > > > > > > > > > https://codereview.chromium.org/977843003/diff/20001/mojo/services/network/an... > > > > File mojo/services/network/android_hooks.cc (right): > > > > > > > > > > > > > > https://codereview.chromium.org/977843003/diff/20001/mojo/services/network/an... > > > > mojo/services/network/android_hooks.cc:39: > > > > base::android::LibraryLoaderExitHook(); > > > > On 2015/03/04 17:25:52, michaelbai wrote: > > > > > Why can't you use the AtExitManager created in base? If you have to do > > this, > > > > it > > > > > should be in Init(). > > > > > > > > We use the one in base, but we create it later on as part of the normal > flow > > > for > > > > all mojo application. > > > > > > I am ok with land this CL, and wonder why mojo creates the AtExitManager > > instead > > > of using the existing one which is created in JNI_OnLoad? > > > > Because the code that create the AtExitManager is shared code. > > Sorry, might not have been clear enough. The code run after this is common to > all platform, and it is creating the AtExitManager and handling its lifetime. Thanks for the clarifying!
The CQ bit was checked by ppi@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from michaelbai@chromium.org Link to the patchset: https://codereview.chromium.org/977843003/#ps20001 (title: "Address Ben's comment.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/977843003/20001
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/860688338d1173c3dae471a4cc17b3809d6cfb36 Cr-Commit-Position: refs/heads/master@{#319152} |
