Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1619673004:
Add API for custom library loading (Closed)
DescriptionAdd API for custom library loading
Loading the native library can cause a crash on some older versions of
Android. This CL provides a way for embedders to implement custom library
loading logic that works around the issue.
BUG=550584
R=xunjieli@chromium.org
Committed: https://crrev.com/12d01cb35ec0ed7a259c19dd1390b5a486d61a9d
Cr-Commit-Position: refs/heads/master@{#371319}
Committed: https://crrev.com/5a35917b538a7b749ea9da4571f8ed6e4402b70d
Cr-Commit-Position: refs/heads/master@{#371571}
Patch Set 1 #
Total comments: 10
Patch Set 2 : #
Total comments: 8
Patch Set 3 : #
Total comments: 6
Patch Set 4 : #
Total comments: 1
Patch Set 5 : javadoc error #
Total comments: 1
Patch Set 6 : #
Messages
Total messages: 32 (11 generated)
Looking good! Feel free to ping me over chat if you have any question at all. Sorry that I just got back, and we yet to get time to sync :) https://codereview.chromium.org/1619673004/diff/1/components/cronet/android/a... File components/cronet/android/api/src/org/chromium/net/CronetEngine.java (right): https://codereview.chromium.org/1619673004/diff/1/components/cronet/android/a... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:45: * A callback for loading the "cronet" library. Suggest making this into an abstract class, and mLibraryLoader as an optional param. Maybe also rename LibraryLoader to CustomLibraryLoader. That way we can avoid creating an instance of LibraryLoader if it isn't needed. It is slightly clearer that this step is optional. Also document in what scenarios the embedder might want to have a custom LibraryLoader. A few sentences will do. https://codereview.chromium.org/1619673004/diff/1/components/cronet/android/a... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:51: public void loadLibrary(Context context, String name) { I don't think we need the context as an argument. If the embedder needs it, they can have it through other means e.g. supplied through the constructor. Also document what "name" is. Maybe rename it to libName to be clearer. https://codereview.chromium.org/1619673004/diff/1/components/cronet/android/a... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:215: mLibraryLoader.loadLibrary(context, mLibraryName); Suggest do the following: If mLibraryLoader is null, do System.loadLibrary( mLibraryName); Otherwise invoke the loadLibrary on mLibraryLoader. https://codereview.chromium.org/1619673004/diff/1/components/cronet/android/t... File components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestContextTest.java (right): https://codereview.chromium.org/1619673004/diff/1/components/cronet/android/t... components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestContextTest.java:1000: nativeLibraryExists(); How does this work? The Builder.loadLibrary has not been invoked, right? How can we call the native function?
Thanks for the comments! Uploaded a new version. https://codereview.chromium.org/1619673004/diff/1/components/cronet/android/a... File components/cronet/android/api/src/org/chromium/net/CronetEngine.java (right): https://codereview.chromium.org/1619673004/diff/1/components/cronet/android/a... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:45: * A callback for loading the "cronet" library. On 2016/01/21 19:50:53, xunjieli wrote: > Suggest making this into an abstract class, and mLibraryLoader as an optional > param. > Maybe also rename LibraryLoader to CustomLibraryLoader. > > That way we can avoid creating an instance of LibraryLoader if it isn't needed. > It is slightly clearer that this step is optional. > > Also document in what scenarios the embedder might want to have a custom > LibraryLoader. A few sentences will do. I made it into an interface instead, since it looks like it meets all the criteria for that. Let me know if there's a reason for it to be specifically an abstract class. https://codereview.chromium.org/1619673004/diff/1/components/cronet/android/a... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:51: public void loadLibrary(Context context, String name) { On 2016/01/21 19:50:53, xunjieli wrote: > I don't think we need the context as an argument. If the embedder needs it, they > can have it through other means e.g. supplied through the constructor. > > Also document what "name" is. Maybe rename it to libName to be clearer. Done. https://codereview.chromium.org/1619673004/diff/1/components/cronet/android/a... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:215: mLibraryLoader.loadLibrary(context, mLibraryName); On 2016/01/21 19:50:53, xunjieli wrote: > Suggest do the following: > If mLibraryLoader is null, do System.loadLibrary( mLibraryName); > Otherwise invoke the loadLibrary on mLibraryLoader. Done. https://codereview.chromium.org/1619673004/diff/1/components/cronet/android/t... File components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestContextTest.java (right): https://codereview.chromium.org/1619673004/diff/1/components/cronet/android/t... components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestContextTest.java:1000: nativeLibraryExists(); On 2016/01/21 19:50:53, xunjieli wrote: > How does this work? The Builder.loadLibrary has not been invoked, right? How can > we call the native function? It gets invoked during the call to build(), through ensureInitialized(), which is called in the CronetUrlRequestContext constructor, which is used by reflection. Is this not expected behavior?
https://codereview.chromium.org/1619673004/diff/1/components/cronet/android/t... File components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestContextTest.java (right): https://codereview.chromium.org/1619673004/diff/1/components/cronet/android/t... components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestContextTest.java:1000: nativeLibraryExists(); On 2016/01/22 17:41:16, mgersh wrote: > On 2016/01/21 19:50:53, xunjieli wrote: > > How does this work? The Builder.loadLibrary has not been invoked, right? How > can > > we call the native function? > > It gets invoked during the call to build(), through ensureInitialized(), which > is called in the CronetUrlRequestContext constructor, which is used by > reflection. Is this not expected behavior? Ah, yes, you are right. There are plenty of tests where loading the native library is a prerequisite, so I don't think we gain anything extra by having this test. You could add a variable in TestBadLibraryLoader, set it in loadLibrary(). And at the end of testSkipLibraryLoading(), assert it is set. That would serve as a proof that loadLibrary is indeed called (besides the UnsatisfiedLinkError). That one test(testSkipLibraryLoading) should be sufficient. https://codereview.chromium.org/1619673004/diff/20001/components/cronet/andro... File components/cronet/android/api/src/org/chromium/net/CronetEngine.java (right): https://codereview.chromium.org/1619673004/diff/20001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:45: * A callback for loading the cronet native library. Apps needing to implement custom nit: s/A callback/An interface. According to go/android-api-guidelines, we should only use interface if we are certain that this will only contain one method. That was why I suggested abstract class initially. But i think it is as good as of now. https://codereview.chromium.org/1619673004/diff/20001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:51: public static interface CustomLibraryLoader { Gonna retract my earlier comment. I think LibraryLoader is a cleaner naming. Would you mind changing it back? :) https://codereview.chromium.org/1619673004/diff/20001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:53: * Load the native library. nit: s/Load/Loads https://codereview.chromium.org/1619673004/diff/20001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:207: * Overrides the callback to load the native library. Maybe update the comment? something like: Sets a library loader to be used for loading the native library. If it is not set, the library will be load using {@link System.load()}. See {@link LibraryLoader} for more information.
PTAL https://codereview.chromium.org/1619673004/diff/1/components/cronet/android/t... File components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestContextTest.java (right): https://codereview.chromium.org/1619673004/diff/1/components/cronet/android/t... components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestContextTest.java:1000: nativeLibraryExists(); On 2016/01/22 22:53:15, xunjieli wrote: > On 2016/01/22 17:41:16, mgersh wrote: > > On 2016/01/21 19:50:53, xunjieli wrote: > > > How does this work? The Builder.loadLibrary has not been invoked, right? How > > can > > > we call the native function? > > > > It gets invoked during the call to build(), through ensureInitialized(), which > > is called in the CronetUrlRequestContext constructor, which is used by > > reflection. Is this not expected behavior? > > Ah, yes, you are right. There are plenty of tests where loading the native > library is a prerequisite, so I don't think we gain anything extra by having > this test. You could add a variable in TestBadLibraryLoader, set it in > loadLibrary(). And at the end of testSkipLibraryLoading(), assert it is set. > That would serve as a proof that loadLibrary is indeed called (besides the > UnsatisfiedLinkError). That one test(testSkipLibraryLoading) should be > sufficient. Done. https://codereview.chromium.org/1619673004/diff/20001/components/cronet/andro... File components/cronet/android/api/src/org/chromium/net/CronetEngine.java (right): https://codereview.chromium.org/1619673004/diff/20001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:45: * A callback for loading the cronet native library. Apps needing to implement custom On 2016/01/22 22:53:15, xunjieli wrote: > nit: s/A callback/An interface. > > According to go/android-api-guidelines, we should only use interface if we are > certain that this will only contain one method. That was why I suggested > abstract class initially. But i think it is as good as of now. Thanks, that's a useful link. It convinced me to change it back to an abstract class, actually--it might be technically okay to use an interface here but I wasn't thinking of the potential downsides. https://codereview.chromium.org/1619673004/diff/20001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:51: public static interface CustomLibraryLoader { On 2016/01/22 22:53:15, xunjieli wrote: > Gonna retract my earlier comment. I think LibraryLoader is a cleaner naming. > Would you mind changing it back? :) Done. https://codereview.chromium.org/1619673004/diff/20001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:53: * Load the native library. On 2016/01/22 22:53:15, xunjieli wrote: > nit: s/Load/Loads Done. https://codereview.chromium.org/1619673004/diff/20001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:207: * Overrides the callback to load the native library. On 2016/01/22 22:53:15, xunjieli wrote: > Maybe update the comment? something like: Sets a library loader to be used for > loading the native library. If it is not set, the library will be load using > {@link System.load()}. See {@link LibraryLoader} for more information. Done.
lgtm mod nits below. https://codereview.chromium.org/1619673004/diff/40001/components/cronet/andro... File components/cronet/android/api/src/org/chromium/net/CronetEngine.java (right): https://codereview.chromium.org/1619673004/diff/40001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:208: * If not set, the library will be loaded using {@link System.loadLibrary()}. Sorry my bad. The @link should be either {@link System#loadLibrary(String)} or {@link System#loadLibrary}. Notice there is a hash tag instead of the dot. Ideally we should be able to double check with our Javadoc generator to make sure, but the generator script is in a broken state right now. https://codereview.chromium.org/1619673004/diff/40001/components/cronet/andro... File components/cronet/android/test/cronet_url_request_context_config_test.cc (right): https://codereview.chromium.org/1619673004/diff/40001/components/cronet/andro... components/cronet/android/test/cronet_url_request_context_config_test.cc:52: } // namespace cronet Should probably reset this change in this file, as it is not related to the goal of this CL. https://codereview.chromium.org/1619673004/diff/40001/components/cronet/andro... File components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestContextTest.java (right): https://codereview.chromium.org/1619673004/diff/40001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestContextTest.java:965: private int mWasCalled = 0; suggest using a boolean.
Thanks! https://codereview.chromium.org/1619673004/diff/40001/components/cronet/andro... File components/cronet/android/api/src/org/chromium/net/CronetEngine.java (right): https://codereview.chromium.org/1619673004/diff/40001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:208: * If not set, the library will be loaded using {@link System.loadLibrary()}. On 2016/01/25 18:24:09, xunjieli wrote: > Sorry my bad. The @link should be either {@link System#loadLibrary(String)} or > {@link System#loadLibrary}. Notice there is a hash tag instead of the dot. > Ideally we should be able to double check with our Javadoc generator to make > sure, but the generator script is in a broken state right now. Done. https://codereview.chromium.org/1619673004/diff/40001/components/cronet/andro... File components/cronet/android/test/cronet_url_request_context_config_test.cc (right): https://codereview.chromium.org/1619673004/diff/40001/components/cronet/andro... components/cronet/android/test/cronet_url_request_context_config_test.cc:52: } // namespace cronet On 2016/01/25 18:24:09, xunjieli wrote: > Should probably reset this change in this file, as it is not related to the goal > of this CL. Done. https://codereview.chromium.org/1619673004/diff/40001/components/cronet/andro... File components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestContextTest.java (right): https://codereview.chromium.org/1619673004/diff/40001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestContextTest.java:965: private int mWasCalled = 0; On 2016/01/25 18:24:09, xunjieli wrote: > suggest using a boolean. Done.
The CQ bit was checked by mgersh@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from xunjieli@chromium.org Link to the patchset: https://codereview.chromium.org/1619673004/#ps60001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1619673004/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1619673004/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_chromium_gn_compile_rel on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_chro...)
On 2016/01/25 20:23:13, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > android_chromium_gn_compile_rel on tryserver.chromium.android (JOB_FAILED, > https://build.chromium.org/p/tryserver.chromium.android/builders/android_chro...) I just learned that our trybot is online. You could use it (android_cronet_tester) next time.
https://codereview.chromium.org/1619673004/diff/60001/components/cronet/andro... File components/cronet/android/api/src/org/chromium/net/CronetEngine.java (right): https://codereview.chromium.org/1619673004/diff/60001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:47: * to {@link CronetEngine.Builder.setLibraryLoader()}. For example, this might be required Missed this one. This should be {@link CronetEngine.Builder#setLibraryLoader}.
The CQ bit was checked by mgersh@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from xunjieli@chromium.org Link to the patchset: https://codereview.chromium.org/1619673004/#ps80001 (title: "javadoc error")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1619673004/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1619673004/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Add API for custom library loading Loading the native library can cause a crash on some older versions of Android. This CL provides a way for embedders to implement custom library loading logic that works around the issue. BUG=550584 R=xunjieli@chromium.org ========== to ========== Add API for custom library loading Loading the native library can cause a crash on some older versions of Android. This CL provides a way for embedders to implement custom library loading logic that works around the issue. BUG=550584 R=xunjieli@chromium.org Committed: https://crrev.com/12d01cb35ec0ed7a259c19dd1390b5a486d61a9d Cr-Commit-Position: refs/heads/master@{#371319} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/12d01cb35ec0ed7a259c19dd1390b5a486d61a9d Cr-Commit-Position: refs/heads/master@{#371319}
Message was sent while issue was closed.
A revert of this CL (patchset #5 id:80001) has been created in https://codereview.chromium.org/1635033002/ by aberent@chromium.org. The reason for reverting is: New test (CronetUrlRequestContextTest#testSkipLibraryLoading) fails on bots BUG=581309.
Message was sent while issue was closed.
https://codereview.chromium.org/1619673004/diff/80001/components/cronet/andro... File components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestContextTest.java (right): https://codereview.chromium.org/1619673004/diff/80001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestContextTest.java:989: assertTrue(e.getMessage().contains( Looks like this assertion is flaky, and dependent on Android API levels.
Message was sent while issue was closed.
Description was changed from ========== Add API for custom library loading Loading the native library can cause a crash on some older versions of Android. This CL provides a way for embedders to implement custom library loading logic that works around the issue. BUG=550584 R=xunjieli@chromium.org Committed: https://crrev.com/12d01cb35ec0ed7a259c19dd1390b5a486d61a9d Cr-Commit-Position: refs/heads/master@{#371319} ========== to ========== Add API for custom library loading Loading the native library can cause a crash on some older versions of Android. This CL provides a way for embedders to implement custom library loading logic that works around the issue. BUG=550584 R=xunjieli@chromium.org Committed: https://crrev.com/12d01cb35ec0ed7a259c19dd1390b5a486d61a9d Cr-Commit-Position: refs/heads/master@{#371319} ==========
Fixed the test by removing that assert--I think what's there now should be enough.
sgtm On Tue, Jan 26, 2016 at 2:23 PM, <mgersh@chromium.org> wrote: > Fixed the test by removing that assert--I think what's there now should be > enough. > > https://codereview.chromium.org/1619673004/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
The CQ bit was checked by mgersh@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from xunjieli@chromium.org Link to the patchset: https://codereview.chromium.org/1619673004/#ps100001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1619673004/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1619673004/100001
Message was sent while issue was closed.
Description was changed from ========== Add API for custom library loading Loading the native library can cause a crash on some older versions of Android. This CL provides a way for embedders to implement custom library loading logic that works around the issue. BUG=550584 R=xunjieli@chromium.org Committed: https://crrev.com/12d01cb35ec0ed7a259c19dd1390b5a486d61a9d Cr-Commit-Position: refs/heads/master@{#371319} ========== to ========== Add API for custom library loading Loading the native library can cause a crash on some older versions of Android. This CL provides a way for embedders to implement custom library loading logic that works around the issue. BUG=550584 R=xunjieli@chromium.org Committed: https://crrev.com/12d01cb35ec0ed7a259c19dd1390b5a486d61a9d Cr-Commit-Position: refs/heads/master@{#371319} ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Add API for custom library loading Loading the native library can cause a crash on some older versions of Android. This CL provides a way for embedders to implement custom library loading logic that works around the issue. BUG=550584 R=xunjieli@chromium.org Committed: https://crrev.com/12d01cb35ec0ed7a259c19dd1390b5a486d61a9d Cr-Commit-Position: refs/heads/master@{#371319} ========== to ========== Add API for custom library loading Loading the native library can cause a crash on some older versions of Android. This CL provides a way for embedders to implement custom library loading logic that works around the issue. BUG=550584 R=xunjieli@chromium.org Committed: https://crrev.com/12d01cb35ec0ed7a259c19dd1390b5a486d61a9d Cr-Commit-Position: refs/heads/master@{#371319} Committed: https://crrev.com/5a35917b538a7b749ea9da4571f8ed6e4402b70d Cr-Commit-Position: refs/heads/master@{#371571} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/5a35917b538a7b749ea9da4571f8ed6e4402b70d Cr-Commit-Position: refs/heads/master@{#371571} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
