Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
922283002:
Use @NativeClassQualifiedName in CronetUrlRequestContext.java (Closed)
DescriptionUse @NativeClassQualifiedName in CronetUrlRequestContext.java
This CL uses @NativeClassQualifiedName in CronetUrlRequestContext.java to reduce
JNI code. After the refactoring, cronet_url_request_context.cc is merged
with cronet_url_request_context_adapter.cc, since the former mostly contains
JNI code that will be generated by @NativeClassQualifiedName.
BUG=458276
Committed: https://crrev.com/8ee9d324f5a59c3af69e2fdc17ad4421c1df2ea2
Cr-Commit-Position: refs/heads/master@{#317068}
Patch Set 1 : Self review #
Total comments: 2
Patch Set 2 : Addressed Misha's comments #
Total comments: 18
Patch Set 3 : Matt's comments #
Total comments: 3
Patch Set 4 : Misha's comments #
Messages
Total messages: 20 (4 generated)
Patchset #1 (id:1) has been deleted
xunjieli@chromium.org changed reviewers: + mef@chromium.org, mmenke@chromium.org, pauljensen@chromium.org
Per Paul's suggestion, I have added @NativeClassQualifiedName in CronetUrlRequestContext.java. This is mostly just a refactoring CL, but it affects quite a bit of code. PTAL.
Looks pretty good! https://codereview.chromium.org/922283002/diff/20001/components/cronet/androi... File components/cronet/android/cronet_url_request_context_adapter.cc (right): https://codereview.chromium.org/922283002/diff/20001/components/cronet/androi... components/cronet/android/cronet_url_request_context_adapter.cc:318: jobject jcaller, jcaller doesn't seem to be used here and below. Does it make sense to make these methods static member functions of CronetURLRequestContextAdapter?
https://codereview.chromium.org/922283002/diff/20001/components/cronet/androi... File components/cronet/android/cronet_url_request_context_adapter.cc (right): https://codereview.chromium.org/922283002/diff/20001/components/cronet/androi... components/cronet/android/cronet_url_request_context_adapter.cc:318: jobject jcaller, On 2015/02/17 10:02:43, mef wrote: > jcaller doesn't seem to be used here and below. Does it make sense to make these > methods static member functions of CronetURLRequestContextAdapter? Done. Good catch! We should probably make them static. They look like they should be static methods.
https://codereview.chromium.org/922283002/diff/40001/components/cronet/androi... File components/cronet/android/cronet_url_request_context_adapter.cc (right): https://codereview.chromium.org/922283002/diff/40001/components/cronet/androi... components/cronet/android/cronet_url_request_context_adapter.cc:15: #include "jni/CronetUrlRequestContext_jni.h" While you're fixing these, should include base/logging.h for DCHECK. https://codereview.chromium.org/922283002/diff/40001/components/cronet/androi... components/cronet/android/cronet_url_request_context_adapter.cc:23: #include "net/url_request/url_request_context_builder.h" Thanks for cleaning these up! https://codereview.chromium.org/922283002/diff/40001/components/cronet/androi... components/cronet/android/cronet_url_request_context_adapter.cc:115: void initJavaNetworkThread( nit: This should be called InitJavaNetworkThread https://codereview.chromium.org/922283002/diff/40001/components/cronet/androi... components/cronet/android/cronet_url_request_context_adapter.cc:119: } nit: This should go in the anonymous namespace up top. https://codereview.chromium.org/922283002/diff/40001/components/cronet/androi... components/cronet/android/cronet_url_request_context_adapter.cc:142: jcaller_ref); Suggest just inlining this. https://codereview.chromium.org/922283002/diff/40001/components/cronet/androi... components/cronet/android/cronet_url_request_context_adapter.cc:162: context_builder.set_proxy_config_service(proxy_config_service_.release()); Was the old code here a bug? If so, seems like it should be fixed in a separate CL...Ideally with tests, if we can manage it. https://codereview.chromium.org/922283002/diff/40001/components/cronet/androi... components/cronet/android/cronet_url_request_context_adapter.cc:274: base::android::ConvertJavaStringToUTF8(env, jfile_name); Suggest just inlining this. https://codereview.chromium.org/922283002/diff/40001/components/cronet/androi... File components/cronet/android/cronet_url_request_context_adapter.h (right): https://codereview.chromium.org/922283002/diff/40001/components/cronet/androi... components/cronet/android/cronet_url_request_context_adapter.h:8: #include <jni.h> nit: Blank line between C and C++ headers.
https://codereview.chromium.org/922283002/diff/40001/components/cronet/androi... File components/cronet/android/cronet_url_request_context_adapter.cc (right): https://codereview.chromium.org/922283002/diff/40001/components/cronet/androi... components/cronet/android/cronet_url_request_context_adapter.cc:15: #include "jni/CronetUrlRequestContext_jni.h" On 2015/02/17 16:30:17, mmenke wrote: > While you're fixing these, should include base/logging.h for DCHECK. Done. https://codereview.chromium.org/922283002/diff/40001/components/cronet/androi... components/cronet/android/cronet_url_request_context_adapter.cc:23: #include "net/url_request/url_request_context_builder.h" On 2015/02/17 16:30:17, mmenke wrote: > Thanks for cleaning these up! Sure thing! https://codereview.chromium.org/922283002/diff/40001/components/cronet/androi... components/cronet/android/cronet_url_request_context_adapter.cc:115: void initJavaNetworkThread( On 2015/02/17 16:30:17, mmenke wrote: > nit: This should be called InitJavaNetworkThread Done. https://codereview.chromium.org/922283002/diff/40001/components/cronet/androi... components/cronet/android/cronet_url_request_context_adapter.cc:119: } On 2015/02/17 16:30:17, mmenke wrote: > nit: This should go in the anonymous namespace up top. Done. https://codereview.chromium.org/922283002/diff/40001/components/cronet/androi... components/cronet/android/cronet_url_request_context_adapter.cc:142: jcaller_ref); On 2015/02/17 16:30:17, mmenke wrote: > Suggest just inlining this. Done. I inlined the entire method. Let me know if you'd like me to revert. https://codereview.chromium.org/922283002/diff/40001/components/cronet/androi... components/cronet/android/cronet_url_request_context_adapter.cc:162: context_builder.set_proxy_config_service(proxy_config_service_.release()); On 2015/02/17 16:30:17, mmenke wrote: > Was the old code here a bug? If so, seems like it should be fixed in a separate > CL...Ideally with tests, if we can manage it. Done. Reverted. Will do it in a separate CL. https://codereview.chromium.org/922283002/diff/40001/components/cronet/androi... components/cronet/android/cronet_url_request_context_adapter.cc:274: base::android::ConvertJavaStringToUTF8(env, jfile_name); On 2015/02/17 16:30:17, mmenke wrote: > Suggest just inlining this. Done. https://codereview.chromium.org/922283002/diff/40001/components/cronet/androi... File components/cronet/android/cronet_url_request_context_adapter.h (right): https://codereview.chromium.org/922283002/diff/40001/components/cronet/androi... components/cronet/android/cronet_url_request_context_adapter.h:8: #include <jni.h> On 2015/02/17 16:30:17, mmenke wrote: > nit: Blank line between C and C++ headers. Done.
LGTM
https://codereview.chromium.org/922283002/diff/40001/components/cronet/androi... File components/cronet/android/cronet_url_request_context_adapter.h (right): https://codereview.chromium.org/922283002/diff/40001/components/cronet/androi... components/cronet/android/cronet_url_request_context_adapter.h:30: bool CronetUrlRequestContextAdapterRegisterJni(JNIEnv* env); nit: I'm not sure, but I think this should go below forward declaration of struct URLRequestContextConfig; https://codereview.chromium.org/922283002/diff/60001/components/cronet/androi... File components/cronet/android/java/src/org/chromium/net/CronetUrlRequestContext.java (right): https://codereview.chromium.org/922283002/diff/60001/components/cronet/androi... components/cronet/android/java/src/org/chromium/net/CronetUrlRequestContext.java:198: private static native long nativeCreateRequestContextAdapter( What would happen if you add a @NativeClassQualifiedName("CronetURLRequestContextAdapter") to this and setMinLogLevel?
New patchsets have been uploaded after l-g-t-m from mmenke@chromium.org
https://codereview.chromium.org/922283002/diff/40001/components/cronet/androi... File components/cronet/android/cronet_url_request_context_adapter.h (right): https://codereview.chromium.org/922283002/diff/40001/components/cronet/androi... components/cronet/android/cronet_url_request_context_adapter.h:30: bool CronetUrlRequestContextAdapterRegisterJni(JNIEnv* env); On 2015/02/19 14:45:07, mef wrote: > nit: I'm not sure, but I think this should go below forward declaration of > struct URLRequestContextConfig; Done. Didn't realize struct URLRequestContextConfig was a forward declaration :) https://codereview.chromium.org/922283002/diff/60001/components/cronet/androi... File components/cronet/android/java/src/org/chromium/net/CronetUrlRequestContext.java (right): https://codereview.chromium.org/922283002/diff/60001/components/cronet/androi... components/cronet/android/java/src/org/chromium/net/CronetUrlRequestContext.java:198: private static native long nativeCreateRequestContextAdapter( On 2015/02/19 14:45:07, mef wrote: > What would happen if you add a > @NativeClassQualifiedName("CronetURLRequestContextAdapter") to this and > setMinLogLevel? The @NativeClassQualifiedName will generate JNI code to call class methods of CronetURLRequestContextAdapter. But CreateRequestContextAdapter and SetMinLogLevel are static bound, so the JNI code generated won't be able to find the method definitions (Since there isn't a, eg., cronet::CronetURLRequesContextAdapter::setMinLogLevel).
https://codereview.chromium.org/922283002/diff/60001/components/cronet/androi... File components/cronet/android/java/src/org/chromium/net/CronetUrlRequestContext.java (right): https://codereview.chromium.org/922283002/diff/60001/components/cronet/androi... components/cronet/android/java/src/org/chromium/net/CronetUrlRequestContext.java:198: private static native long nativeCreateRequestContextAdapter( On 2015/02/19 15:19:34, xunjieli wrote: > On 2015/02/19 14:45:07, mef wrote: > > What would happen if you add a > > @NativeClassQualifiedName("CronetURLRequestContextAdapter") to this and > > setMinLogLevel? > > The @NativeClassQualifiedName will generate JNI code to call class methods of > CronetURLRequestContextAdapter. But CreateRequestContextAdapter and > SetMinLogLevel are static bound, so the JNI code generated won't be able to find > the method definitions (Since there isn't a, eg., > cronet::CronetURLRequesContextAdapter::setMinLogLevel). Realized that I was a little loose in the terminology. If we add the notation, the @NativeClassQualifiedName will generate JNI headers for cronet::CronetURLRequesContextAdapter::SetMinLogLevel, but we have no definition for cronet::CronetURLRequestContextAdapter::SetMinLogLevel, instead we have a static method cronet::SetMinLevel. So the compiler complains that we have declared cronet::CronetURLRequesContextAdapter::SetMinLogLevel, but we never defined it. Hope that makes sense :)
On 2015/02/19 15:24:29, xunjieli wrote: > https://codereview.chromium.org/922283002/diff/60001/components/cronet/androi... > File > components/cronet/android/java/src/org/chromium/net/CronetUrlRequestContext.java > (right): > > https://codereview.chromium.org/922283002/diff/60001/components/cronet/androi... > components/cronet/android/java/src/org/chromium/net/CronetUrlRequestContext.java:198: > private static native long nativeCreateRequestContextAdapter( > On 2015/02/19 15:19:34, xunjieli wrote: > > On 2015/02/19 14:45:07, mef wrote: > > > What would happen if you add a > > > @NativeClassQualifiedName("CronetURLRequestContextAdapter") to this and > > > setMinLogLevel? > > > > The @NativeClassQualifiedName will generate JNI code to call class methods of > > CronetURLRequestContextAdapter. But CreateRequestContextAdapter and > > SetMinLogLevel are static bound, so the JNI code generated won't be able to > find > > the method definitions (Since there isn't a, eg., > > cronet::CronetURLRequesContextAdapter::setMinLogLevel). > > Realized that I was a little loose in the terminology. If we add the notation, > the @NativeClassQualifiedName will generate JNI headers for > cronet::CronetURLRequesContextAdapter::SetMinLogLevel, but we have no definition > for cronet::CronetURLRequestContextAdapter::SetMinLogLevel, instead we have a > static method cronet::SetMinLevel. So the compiler > complains that we have declared > cronet::CronetURLRequesContextAdapter::SetMinLogLevel, but we never defined it. > Hope that makes sense :) Thanks, that makes sense! If generated cronet::CronetURLRequesContextAdapter::SetMinLogLevel is a static method of the class, then would it make sense to define that instead of static method cronet::SetMinLevel? That way all JNI-generated native methods will belong to the class (some being static members).
On 2015/02/19 15:51:05, mef wrote: > On 2015/02/19 15:24:29, xunjieli wrote: > > > https://codereview.chromium.org/922283002/diff/60001/components/cronet/androi... > > File > > > components/cronet/android/java/src/org/chromium/net/CronetUrlRequestContext.java > > (right): > > > > > https://codereview.chromium.org/922283002/diff/60001/components/cronet/androi... > > > components/cronet/android/java/src/org/chromium/net/CronetUrlRequestContext.java:198: > > private static native long nativeCreateRequestContextAdapter( > > On 2015/02/19 15:19:34, xunjieli wrote: > > > On 2015/02/19 14:45:07, mef wrote: > > > > What would happen if you add a > > > > @NativeClassQualifiedName("CronetURLRequestContextAdapter") to this and > > > > setMinLogLevel? > > > > > > The @NativeClassQualifiedName will generate JNI code to call class methods > of > > > CronetURLRequestContextAdapter. But CreateRequestContextAdapter and > > > SetMinLogLevel are static bound, so the JNI code generated won't be able to > > find > > > the method definitions (Since there isn't a, eg., > > > cronet::CronetURLRequesContextAdapter::setMinLogLevel). > > > > Realized that I was a little loose in the terminology. If we add the notation, > > the @NativeClassQualifiedName will generate JNI headers for > > cronet::CronetURLRequesContextAdapter::SetMinLogLevel, but we have no > definition > > for cronet::CronetURLRequestContextAdapter::SetMinLogLevel, instead we have a > > static method cronet::SetMinLevel. So the compiler > > complains that we have declared > > cronet::CronetURLRequesContextAdapter::SetMinLogLevel, but we never defined > it. > > Hope that makes sense :) > > Thanks, that makes sense! > If generated cronet::CronetURLRequesContextAdapter::SetMinLogLevel is a static > method of the class, > then would it make sense to define that instead of static method > cronet::SetMinLevel? > > That way all JNI-generated native methods will belong to the class (some being > static members). Sorry, I forgot to mention the requirement for @NativeClassQualifiedName. According to https://code.google.com/p/chromium/codesearch#chromium/src/base/android/java/..., the first argument to the native function must be a native pointer of an instance of that class. So unfortunately this cannot be applied to these two methods.
On 2015/02/19 15:59:16, xunjieli wrote: > On 2015/02/19 15:51:05, mef wrote: > > On 2015/02/19 15:24:29, xunjieli wrote: > > > > > > https://codereview.chromium.org/922283002/diff/60001/components/cronet/androi... > > > File > > > > > > components/cronet/android/java/src/org/chromium/net/CronetUrlRequestContext.java > > > (right): > > > > > > > > > https://codereview.chromium.org/922283002/diff/60001/components/cronet/androi... > > > > > > components/cronet/android/java/src/org/chromium/net/CronetUrlRequestContext.java:198: > > > private static native long nativeCreateRequestContextAdapter( > > > On 2015/02/19 15:19:34, xunjieli wrote: > > > > On 2015/02/19 14:45:07, mef wrote: > > > > > What would happen if you add a > > > > > @NativeClassQualifiedName("CronetURLRequestContextAdapter") to this and > > > > > setMinLogLevel? > > > > > > > > The @NativeClassQualifiedName will generate JNI code to call class methods > > of > > > > CronetURLRequestContextAdapter. But CreateRequestContextAdapter and > > > > SetMinLogLevel are static bound, so the JNI code generated won't be able > to > > > find > > > > the method definitions (Since there isn't a, eg., > > > > cronet::CronetURLRequesContextAdapter::setMinLogLevel). > > > > > > Realized that I was a little loose in the terminology. If we add the > notation, > > > the @NativeClassQualifiedName will generate JNI headers for > > > cronet::CronetURLRequesContextAdapter::SetMinLogLevel, but we have no > > definition > > > for cronet::CronetURLRequestContextAdapter::SetMinLogLevel, instead we have > a > > > static method cronet::SetMinLevel. So the compiler > > > complains that we have declared > > > cronet::CronetURLRequesContextAdapter::SetMinLogLevel, but we never defined > > it. > > > Hope that makes sense :) > > > > Thanks, that makes sense! > > If generated cronet::CronetURLRequesContextAdapter::SetMinLogLevel is a static > > method of the class, > > then would it make sense to define that instead of static method > > cronet::SetMinLevel? > > > > That way all JNI-generated native methods will belong to the class (some being > > static members). > > Sorry, I forgot to mention the requirement for @NativeClassQualifiedName. > According to > https://code.google.com/p/chromium/codesearch#chromium/src/base/android/java/..., > the first argument to the native function must be a native pointer of an > instance of that class. So unfortunately this cannot be applied to these two > methods. Ah, that's what I was missing! Thanks for the explanation, lgtm.
On 2015/02/19 16:07:35, mef wrote: > On 2015/02/19 15:59:16, xunjieli wrote: > > On 2015/02/19 15:51:05, mef wrote: > > > On 2015/02/19 15:24:29, xunjieli wrote: > > > > > > > > > > https://codereview.chromium.org/922283002/diff/60001/components/cronet/androi... > > > > File > > > > > > > > > > components/cronet/android/java/src/org/chromium/net/CronetUrlRequestContext.java > > > > (right): > > > > > > > > > > > > > > https://codereview.chromium.org/922283002/diff/60001/components/cronet/androi... > > > > > > > > > > components/cronet/android/java/src/org/chromium/net/CronetUrlRequestContext.java:198: > > > > private static native long nativeCreateRequestContextAdapter( > > > > On 2015/02/19 15:19:34, xunjieli wrote: > > > > > On 2015/02/19 14:45:07, mef wrote: > > > > > > What would happen if you add a > > > > > > @NativeClassQualifiedName("CronetURLRequestContextAdapter") to this > and > > > > > > setMinLogLevel? > > > > > > > > > > The @NativeClassQualifiedName will generate JNI code to call class > methods > > > of > > > > > CronetURLRequestContextAdapter. But CreateRequestContextAdapter and > > > > > SetMinLogLevel are static bound, so the JNI code generated won't be able > > to > > > > find > > > > > the method definitions (Since there isn't a, eg., > > > > > cronet::CronetURLRequesContextAdapter::setMinLogLevel). > > > > > > > > Realized that I was a little loose in the terminology. If we add the > > notation, > > > > the @NativeClassQualifiedName will generate JNI headers for > > > > cronet::CronetURLRequesContextAdapter::SetMinLogLevel, but we have no > > > definition > > > > for cronet::CronetURLRequestContextAdapter::SetMinLogLevel, instead we > have > > a > > > > static method cronet::SetMinLevel. So the compiler > > > > complains that we have declared > > > > cronet::CronetURLRequesContextAdapter::SetMinLogLevel, but we never > defined > > > it. > > > > Hope that makes sense :) > > > > > > Thanks, that makes sense! > > > If generated cronet::CronetURLRequesContextAdapter::SetMinLogLevel is a > static > > > method of the class, > > > then would it make sense to define that instead of static method > > > cronet::SetMinLevel? > > > > > > That way all JNI-generated native methods will belong to the class (some > being > > > static members). > > > > Sorry, I forgot to mention the requirement for @NativeClassQualifiedName. > > According to > > > https://code.google.com/p/chromium/codesearch#chromium/src/base/android/java/..., > > the first argument to the native function must be a native pointer of an > > instance of that class. So unfortunately this cannot be applied to these two > > methods. > > Ah, that's what I was missing! Thanks for the explanation, lgtm. Haha yea, somehow I forgot about it. Thanks for bringing it up!
The CQ bit was checked by xunjieli@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/922283002/80001
Message was sent while issue was closed.
Committed patchset #4 (id:80001)
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/8ee9d324f5a59c3af69e2fdc17ad4421c1df2ea2 Cr-Commit-Position: refs/heads/master@{#317068} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
