Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2514783002:
[Cronet] Add callback wrapper classes to enforce API version checking. (Closed)
|
Created:
4 years, 1 month ago by pauljensen Modified:
4 years ago Reviewers:
kapishnikov CC:
chromium-reviews, cbentzel+watch_chromium.org, agrieve+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Cronet] Add callback wrapper classes to enforce API version checking.
Wrap Cronet callback/listener classes in wrapper classes whose job it
is to enforce that callbacks added in later versions are not called
when the client API is of an older version. For now all wrapper
classes simply pass through all the callbacks as we're still working
on the first version of the API, which we shall support indefinitly.
A side benefit is this facilitates only passing implementation, not
API, classes to JNI functions like nativeGetStatus. This is
required to support cases where the Cronet API may be repackaged
into another class at the Java layer, but not at the JNI layer.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester
BUG=629299
R=kapishnikov
Committed: https://crrev.com/45280ea947ff1c2db6fbbd2d84b72472f87a7891
Cr-Commit-Position: refs/heads/master@{#434891}
Patch Set 1 #Patch Set 2 : forgot file #Patch Set 3 : make constructors public, adjust copyright date #
Total comments: 20
Patch Set 4 : rebase #Patch Set 5 : delegate equals() and hashCode() to wrapped listeners #Patch Set 6 : rebase #Patch Set 7 : fix delegating equals #
Total comments: 3
Patch Set 8 : rebase #Patch Set 9 : fix JNIAdditionalImport #Messages
Total messages: 26 (6 generated)
Andrei, WDYT? This avoids all calls to Cronet API classes from Cronet implementation with the following exceptions: 1. everything in VersionSafeCallbacks 2. ExperimentalCronetEngine.getVersionString() call from CronetEngineBuilderImpl 3. various calls to API constructors (e.g. from subclassed constructors) 4. calls from impl/urlconnection/ files An alternate to forcing almost everything through VersionSafeCallbacks, would be to white-list everything in version 1 of our API in the static enforcement check. Then we'd just need to add callback wrappers when we added calls to new callbacks down the road.
On 2016/11/18 14:57:41, pauljensen wrote: > Andrei, WDYT? > > This avoids all calls to Cronet API classes from Cronet implementation with the > following exceptions: > > 1. everything in VersionSafeCallbacks > 2. ExperimentalCronetEngine.getVersionString() call from CronetEngineBuilderImpl > 3. various calls to API constructors (e.g. from subclassed constructors) > 4. calls from impl/urlconnection/ files > > An alternate to forcing almost everything through VersionSafeCallbacks, would be > to white-list everything in version 1 of our API in the static enforcement > check. Then we'd just need to add callback wrappers when we added calls to new > callbacks down the road. It looks good. What do you think if we use "abstract factory pattern" here to create family of wrappers for a particular client version? http://www.dofactory.com/net/abstract-factory-design-pattern
On 2016/11/18 15:17:20, kapishnikov wrote: > It looks good. What do you think if we use "abstract factory pattern" here to > create family of wrappers for a particular client version? > http://www.dofactory.com/net/abstract-factory-design-pattern I think using an abstract factory pattern may be a bit overkill for this. I think VersionSafeCallbacks should be very simple, most will be simply call-throughs, and some will be call-throughs conditioned on something like "if (API_VERSION > 4)".
On 2016/11/18 16:28:19, pauljensen wrote: > On 2016/11/18 15:17:20, kapishnikov wrote: > > It looks good. What do you think if we use "abstract factory pattern" here to > > create family of wrappers for a particular client version? > > http://www.dofactory.com/net/abstract-factory-design-pattern > > I think using an abstract factory pattern may be a bit overkill for this. I > think VersionSafeCallbacks should be very simple, most will be simply > call-throughs, and some will be call-throughs conditioned on something like "if > (API_VERSION > 4)". I agree. We still can use some parts of the factory pattern approach to make the code more flexible and testable. The complexity of the code should stay similar. See comments on CronetEngineBuilderImpl.java class.
https://codereview.chromium.org/2514783002/diff/40001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/impl/CronetEngineBuilderImpl.java (right): https://codereview.chromium.org/2514783002/diff/40001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/impl/CronetEngineBuilderImpl.java:90: private VersionSafeCallbacks.LibraryLoader mLibraryLoader; Can we keep the old CronetEngine.Builder.LibraryLoader type here? I think CronetEngineBuilderImpl should only use the interface methods of CronetEngine.Builder.LibraryLoader without being aware that it is actually the wrapper. https://codereview.chromium.org/2514783002/diff/40001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/impl/CronetEngineBuilderImpl.java:177: mLibraryLoader = new VersionSafeCallbacks.LibraryLoader(loader); I think it would be better if we don't create the wrapper directly but instead use some factory that creates the wrapper. Thus, the specifics of how the wrapper is created would be outside of this class and can be changed in the future without changing this class. It can also help with testing. E.g., we can inject (or directly instantiate) something like VersionSafeCallbacksFactory that has a method createLibraryLoader(). This line would be changed to: mLibraryLoader = mVersionSafeCallbacksFactory.createLibraryLoader(loader); https://codereview.chromium.org/2514783002/diff/40001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/impl/CronetEngineBuilderImpl.java:181: VersionSafeCallbacks.LibraryLoader libraryLoader() { Same here. Keep CronetEngine.Builder.LibraryLoader https://codereview.chromium.org/2514783002/diff/40001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/impl/CronetUploadDataStream.java (right): https://codereview.chromium.org/2514783002/diff/40001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/impl/CronetUploadDataStream.java:39: private final VersionSafeCallbacks.UploadDataProviderWrapper mDataProvider; Same here. We should keep UploadDataProvider. Applies to other places. https://codereview.chromium.org/2514783002/diff/40001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/impl/VersionSafeCallbacks.java (right): https://codereview.chromium.org/2514783002/diff/40001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/impl/VersionSafeCallbacks.java:33: public class VersionSafeCallbacks { Instead of list of static classes, we can change this class to VersionSafeCallbacksFactory that returns the callback wrappers. It should not increase the complexity of the code.
https://codereview.chromium.org/2514783002/diff/40001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/impl/CronetEngineBuilderImpl.java (right): https://codereview.chromium.org/2514783002/diff/40001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/impl/CronetEngineBuilderImpl.java:90: private VersionSafeCallbacks.LibraryLoader mLibraryLoader; On 2016/11/18 16:32:57, kapishnikov wrote: > Can we keep the old CronetEngine.Builder.LibraryLoader type here? I think > CronetEngineBuilderImpl should only use the interface methods of > CronetEngine.Builder.LibraryLoader without being aware that it is actually the > wrapper. I think that defeats the purpose of this CL. Users of libraryLoader() would then be calling directly through the API class...which would fail if the API class was too old and was missing new members. https://codereview.chromium.org/2514783002/diff/40001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/impl/CronetEngineBuilderImpl.java:181: VersionSafeCallbacks.LibraryLoader libraryLoader() { On 2016/11/18 16:32:57, kapishnikov wrote: > Same here. Keep CronetEngine.Builder.LibraryLoader I think that defeats the purpose of this CL. Users of libraryLoader() would then be calling directly through the API class...which would fail if the API class was too old and was missing new members. https://codereview.chromium.org/2514783002/diff/40001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/impl/CronetUploadDataStream.java (right): https://codereview.chromium.org/2514783002/diff/40001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/impl/CronetUploadDataStream.java:39: private final VersionSafeCallbacks.UploadDataProviderWrapper mDataProvider; On 2016/11/18 16:32:57, kapishnikov wrote: > Same here. We should keep UploadDataProvider. Applies to other places. I think that defeats the purpose of this CL. Users of mDataProvider would then be calling directly through the API class...which would fail if the API class was too old and was missing new members. https://codereview.chromium.org/2514783002/diff/40001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/impl/VersionSafeCallbacks.java (right): https://codereview.chromium.org/2514783002/diff/40001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/impl/VersionSafeCallbacks.java:33: public class VersionSafeCallbacks { On 2016/11/18 16:32:57, kapishnikov wrote: > Instead of list of static classes, we can change this class to > VersionSafeCallbacksFactory that returns the callback wrappers. It should not > increase the complexity of the code. You just want me to add a bunch of these type of functions: UrlRequestCallback createUrlRequestCallback(UrlRequest.Callback callback) { return new UrlRequestCallback(callback); } Correct?
https://codereview.chromium.org/2514783002/diff/40001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/impl/CronetEngineBuilderImpl.java (right): https://codereview.chromium.org/2514783002/diff/40001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/impl/CronetEngineBuilderImpl.java:90: private VersionSafeCallbacks.LibraryLoader mLibraryLoader; On 2016/11/18 19:18:07, pauljensen wrote: > On 2016/11/18 16:32:57, kapishnikov wrote: > > Can we keep the old CronetEngine.Builder.LibraryLoader type here? I think > > CronetEngineBuilderImpl should only use the interface methods of > > CronetEngine.Builder.LibraryLoader without being aware that it is actually the > > wrapper. > > I think that defeats the purpose of this CL. Users of libraryLoader() would > then be calling directly through the API class...which would fail if the API > class was too old and was missing new members. You are right. We should not reference the API classes directly. It slipped my mind. https://codereview.chromium.org/2514783002/diff/40001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/impl/CronetEngineBuilderImpl.java:181: VersionSafeCallbacks.LibraryLoader libraryLoader() { On 2016/11/18 19:18:07, pauljensen wrote: > On 2016/11/18 16:32:57, kapishnikov wrote: > > Same here. Keep CronetEngine.Builder.LibraryLoader > > I think that defeats the purpose of this CL. Users of libraryLoader() would > then be calling directly through the API class...which would fail if the API > class was too old and was missing new members. Acknowledged. https://codereview.chromium.org/2514783002/diff/40001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/impl/CronetUploadDataStream.java (right): https://codereview.chromium.org/2514783002/diff/40001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/impl/CronetUploadDataStream.java:39: private final VersionSafeCallbacks.UploadDataProviderWrapper mDataProvider; On 2016/11/18 19:18:07, pauljensen wrote: > On 2016/11/18 16:32:57, kapishnikov wrote: > > Same here. We should keep UploadDataProvider. Applies to other places. > > I think that defeats the purpose of this CL. Users of mDataProvider would then > be calling directly through the API class...which would fail if the API class > was too old and was missing new members. Acknowledged. https://codereview.chromium.org/2514783002/diff/40001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/impl/VersionSafeCallbacks.java (right): https://codereview.chromium.org/2514783002/diff/40001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/impl/VersionSafeCallbacks.java:33: public class VersionSafeCallbacks { On 2016/11/18 19:18:07, pauljensen wrote: > On 2016/11/18 16:32:57, kapishnikov wrote: > > Instead of list of static classes, we can change this class to > > VersionSafeCallbacksFactory that returns the callback wrappers. It should not > > increase the complexity of the code. > > You just want me to add a bunch of these type of functions: > > UrlRequestCallback createUrlRequestCallback(UrlRequest.Callback callback) { > return new UrlRequestCallback(callback); > } > > Correct? I was thinking of returning anonymous classes but since we cannot reference the API classes I think we should keep the way you implemented. Other options will increase complexity unnecessary.
Two comments; otherwise the change looks good to me. https://codereview.chromium.org/2514783002/diff/40001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequestContext.java (right): https://codereview.chromium.org/2514783002/diff/40001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequestContext.java:139: private final Map<RequestFinishedInfo.Listener, Can we instead of using the map, implement equals() & hashCode() method on VersionSafeCallbacks.RequestFinishedInfoListener? Objects that are referencing the same RequestFinishedInfo.Listener are equal. This would allow to continue using lists instead of maps. https://codereview.chromium.org/2514783002/diff/40001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/impl/VersionSafeCallbacks.java (right): https://codereview.chromium.org/2514783002/diff/40001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/impl/VersionSafeCallbacks.java:37: public static final class UrlRequestCallback extends UrlRequest.Callback { Would it be safer if we don't extend the underlying base classes (callback)? In that case the developers will always be forced to change both the API & and VersionSafeCallbacks classes when a new method is added to the API. Otherwise, someone can forget to modify this class and the calls will be forwarded directly to the API methods that may be absent in the old client.
https://codereview.chromium.org/2514783002/diff/40001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequestContext.java (right): https://codereview.chromium.org/2514783002/diff/40001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequestContext.java:139: private final Map<RequestFinishedInfo.Listener, On 2016/11/18 20:11:18, kapishnikov wrote: > Can we instead of using the map, implement equals() & hashCode() method on > VersionSafeCallbacks.RequestFinishedInfoListener? Objects that are referencing > the same RequestFinishedInfo.Listener are equal. This would allow to continue > using lists instead of maps. I thought about that but it had a couple downsides: 1. The delegation of equals() and hashcode() are somewhat counterintuitive and kinda abnormal behavior that seems like it could trip someone up in the future (e.g. someone removing the delegating equals() and hashcode() methods, and us not having a test to check that we actually remove items from the list). 2. If you want to delete an element you have to create another wrapping class just for the sake of searching for the pre-existing member and then throw it away which seemed fairly inefficient. I don't feel strongly though, if you prefer this behavior let me know and I'll implement. https://codereview.chromium.org/2514783002/diff/40001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/impl/VersionSafeCallbacks.java (right): https://codereview.chromium.org/2514783002/diff/40001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/impl/VersionSafeCallbacks.java:37: public static final class UrlRequestCallback extends UrlRequest.Callback { On 2016/11/18 20:11:18, kapishnikov wrote: > Would it be safer if we don't extend the underlying base classes (callback)? In > that case the developers will always be forced to change both the API & and > VersionSafeCallbacks classes when a new method is added to the API. Otherwise, > someone can forget to modify this class and the calls will be forwarded directly > to the API methods that may be absent in the old client. I also considered this. I've been working on a presubmit check to verify impl never calls through API classes. I think that should prevent this ever happening in the future, so I'm not too worried about developers messing this up. By not inheriting from the API classes we can't use @Override which I think could get us into trouble. For example imagine the API callback took a long but the wrapper accidentally took an int, there wouldn't be any compile time errors.
https://codereview.chromium.org/2514783002/diff/40001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequestContext.java (right): https://codereview.chromium.org/2514783002/diff/40001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequestContext.java:139: private final Map<RequestFinishedInfo.Listener, On 2016/11/19 01:12:18, pauljensen wrote: > On 2016/11/18 20:11:18, kapishnikov wrote: > > Can we instead of using the map, implement equals() & hashCode() method on > > VersionSafeCallbacks.RequestFinishedInfoListener? Objects that are referencing > > the same RequestFinishedInfo.Listener are equal. This would allow to continue > > using lists instead of maps. > > I thought about that but it had a couple downsides: > 1. The delegation of equals() and hashcode() are somewhat counterintuitive and > kinda abnormal behavior that seems like it could trip someone up in the future > (e.g. someone removing the delegating equals() and hashcode() methods, and us > not having a test to check that we actually remove items from the list). > 2. If you want to delete an element you have to create another wrapping class > just for the sake of searching for the pre-existing member and then throw it > away which seemed fairly inefficient. > > I don't feel strongly though, if you prefer this behavior let me know and I'll > implement. I personally like the idea of implementing equals(). But I agree that some functionality may not be intuitive, e.g. to delete a listener, it will be necessary to create a new object. What about creating a new private nested class that would wrap a list/map and do the necessary logic? At least, the top class will be simplified. One advantage of a list versus a map is that it uses less memory but I don't think it should be a contributing factor here. https://codereview.chromium.org/2514783002/diff/40001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/impl/VersionSafeCallbacks.java (right): https://codereview.chromium.org/2514783002/diff/40001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/impl/VersionSafeCallbacks.java:37: public static final class UrlRequestCallback extends UrlRequest.Callback { On 2016/11/19 01:12:19, pauljensen wrote: > On 2016/11/18 20:11:18, kapishnikov wrote: > > Would it be safer if we don't extend the underlying base classes (callback)? > In > > that case the developers will always be forced to change both the API & and > > VersionSafeCallbacks classes when a new method is added to the API. Otherwise, > > someone can forget to modify this class and the calls will be forwarded > directly > > to the API methods that may be absent in the old client. > > I also considered this. I've been working on a presubmit check to verify impl > never calls through API classes. I think that should prevent this ever > happening in the future, so I'm not too worried about developers messing this > up. > > By not inheriting from the API classes we can't use @Override which I think > could get us into trouble. For example imagine the API callback took a long but > the wrapper accidentally took an int, there wouldn't be any compile time errors. Agree, with the presubmit check it should be safe.
https://codereview.chromium.org/2514783002/diff/40001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequestContext.java (right): https://codereview.chromium.org/2514783002/diff/40001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequestContext.java:139: private final Map<RequestFinishedInfo.Listener, On 2016/11/21 17:35:22, kapishnikov wrote: > On 2016/11/19 01:12:18, pauljensen wrote: > > On 2016/11/18 20:11:18, kapishnikov wrote: > > > Can we instead of using the map, implement equals() & hashCode() method on > > > VersionSafeCallbacks.RequestFinishedInfoListener? Objects that are > referencing > > > the same RequestFinishedInfo.Listener are equal. This would allow to > continue > > > using lists instead of maps. > > > > I thought about that but it had a couple downsides: > > 1. The delegation of equals() and hashcode() are somewhat counterintuitive and > > kinda abnormal behavior that seems like it could trip someone up in the future > > (e.g. someone removing the delegating equals() and hashcode() methods, and us > > not having a test to check that we actually remove items from the list). > > 2. If you want to delete an element you have to create another wrapping class > > just for the sake of searching for the pre-existing member and then throw it > > away which seemed fairly inefficient. > > > > I don't feel strongly though, if you prefer this behavior let me know and I'll > > implement. > > I personally like the idea of implementing equals(). But I agree that some > functionality may not be intuitive, e.g. to delete a listener, it will be > necessary to create a new object. What about creating a new private nested class > that would wrap a list/map and do the necessary logic? At least, the top class > will be simplified. > > One advantage of a list versus a map is that it uses less memory but I don't > think it should be a contributing factor here. I delegated the equals() and hashCode() to the wrapped listener, and documented it as such. I think the is the simplest and cleanest implementation. Let me know what you think.
Friendly ping.
https://codereview.chromium.org/2514783002/diff/120001/components/cronet/andr... File components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequestContext.java (right): https://codereview.chromium.org/2514783002/diff/120001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequestContext.java:130: private final Map<RequestFinishedInfo.Listener, Why can't we use List here as well? I actually think that Set is more appropriate for storing liseners but maybe it is not good from the performance point of view.
https://codereview.chromium.org/2514783002/diff/120001/components/cronet/andr... File components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequestContext.java (right): https://codereview.chromium.org/2514783002/diff/120001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequestContext.java:130: private final Map<RequestFinishedInfo.Listener, On 2016/11/28 17:38:59, kapishnikov wrote: > Why can't we use List here as well? I actually think that Set is more > appropriate for storing liseners but maybe it is not good from the performance > point of view. If the question is List vs Map/Set, I'd always say Map/Set for O(1) removal time. If the question is ObserverList vs Map/Set (where we have to copy all values when issuing callbacks), the tradeoff is performance (ObserverList wins, assuming performance issuing callbacks is more important than list removal) vs correctness (where ObserverList could deadlock when attempting to add/remove on another thread while issuing callbacks). Which List (List or ObserverList) are you proposing?
Thanks for the explanation. LGTM https://codereview.chromium.org/2514783002/diff/120001/components/cronet/andr... File components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequestContext.java (right): https://codereview.chromium.org/2514783002/diff/120001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequestContext.java:130: private final Map<RequestFinishedInfo.Listener, On 2016/11/28 18:23:43, pauljensen wrote: > On 2016/11/28 17:38:59, kapishnikov wrote: > > Why can't we use List here as well? I actually think that Set is more > > appropriate for storing liseners but maybe it is not good from the performance > > point of view. > > If the question is List vs Map/Set, I'd always say Map/Set for O(1) removal > time. > If the question is ObserverList vs Map/Set (where we have to copy all values > when issuing callbacks), the tradeoff is performance (ObserverList wins, > assuming performance issuing callbacks is more important than list removal) vs > correctness (where ObserverList could deadlock when attempting to add/remove on > another thread while issuing callbacks). > Which List (List or ObserverList) are you proposing? I think in a long run we should use same approach for storing and manipulating mRttListenerList, mThroughputListenerList and mFinishedListenerList but it looks that it is out of scope of this CL. I am okay with the current change.
The CQ bit was checked by pauljensen@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by pauljensen@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kapishnikov@chromium.org Link to the patchset: https://codereview.chromium.org/2514783002/#ps160001 (title: "fix JNIAdditionalImport")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 160001, "attempt_start_ts": 1480387772053210,
"parent_rev": "0466e47e778a135cf486ad3fda27232684c2d28b", "commit_rev":
"f86888066cfc251fe14269f748afc648d9f7d02f"}
Message was sent while issue was closed.
Committed patchset #9 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== [Cronet] Add callback wrapper classes to enforce API version checking. Wrap Cronet callback/listener classes in wrapper classes whose job it is to enforce that callbacks added in later versions are not called when the client API is of an older version. For now all wrapper classes simply pass through all the callbacks as we're still working on the first version of the API, which we shall support indefinitly. A side benefit is this facilitates only passing implementation, not API, classes to JNI functions like nativeGetStatus. This is required to support cases where the Cronet API may be repackaged into another class at the Java layer, but not at the JNI layer. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester BUG=629299 R=kapishnikov ========== to ========== [Cronet] Add callback wrapper classes to enforce API version checking. Wrap Cronet callback/listener classes in wrapper classes whose job it is to enforce that callbacks added in later versions are not called when the client API is of an older version. For now all wrapper classes simply pass through all the callbacks as we're still working on the first version of the API, which we shall support indefinitly. A side benefit is this facilitates only passing implementation, not API, classes to JNI functions like nativeGetStatus. This is required to support cases where the Cronet API may be repackaged into another class at the Java layer, but not at the JNI layer. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester BUG=629299 R=kapishnikov Committed: https://crrev.com/45280ea947ff1c2db6fbbd2d84b72472f87a7891 Cr-Commit-Position: refs/heads/master@{#434891} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/45280ea947ff1c2db6fbbd2d84b72472f87a7891 Cr-Commit-Position: refs/heads/master@{#434891} |
