Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2626523003:
Cronet: a framework for providing alternative Cronet implementations (Closed)
|
Created:
3 years, 11 months ago by kapishnikov Modified:
3 years, 10 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org, agrieve+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionCronet: a framework to provide alternative Cronet implementations
1. Added CronetProvider interface (abstract class) that can be subclassed
in order to add an alternative CronetEngineBuilder implementation.
The interface is implemented by the new built-in Java and Native
Cronet engine providers.
2. Provided functionality that can be used by an embedder to explicitly
select the desired provider from the list of available ones. If the
embedder doesn't select one. The system select the the best available
one based on the provider versions and types.
3. Deprecated enableLegacyMode(). The embedder can now explicitly choose
what implementation to use.
4. Changed the tests to use the new approach.
BUG=686774
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester
Review-Url: https://codereview.chromium.org/2626523003
Cr-Commit-Position: refs/heads/master@{#445561}
Committed: https://chromium.googlesource.com/chromium/src/+/640977d34b03a64f019b3b9f25fb6e15820914a6
Patch Set 1 #Patch Set 2 : Added context to getName() & getVersion(). #
Total comments: 26
Patch Set 3 : Make Providers to return real builders & get rid of legacy mode #Patch Set 4 : Resolved rebase conflicts #Patch Set 5 : Addressed comments and fixed tests #Patch Set 6 : Changed the version templates to expose static methods instead of static fields #Patch Set 7 : Updated api.txt #
Total comments: 57
Patch Set 8 : Default provider selection based on version and type + addressed comments #Patch Set 9 : Resolved rebase conflicts #Patch Set 10 : Fixed JavaDoc links #
Total comments: 74
Patch Set 11 : Addressed Misha's comments, class renaming and extra javadoc #Patch Set 12 : Addressed Paul's comments #Patch Set 13 : Fixed api.txt #
Total comments: 16
Patch Set 14 : Addressed Paul's comments #
Total comments: 22
Patch Set 15 : Changed ordering of final fields #Patch Set 16 : Proguard changes and javadoc #
Total comments: 2
Patch Set 17 : Javadoc fixes #Messages
Total messages: 38 (14 generated)
Description was changed from ========== Cronet: a framework to provide alternative Cronet implementations 1. Added CronetImplProvider interface (abstract class) that should be implemented by all Cronet implementation (built-in and alternative). 2. Added CronetImplProviderSelector that is responsible for selecting a Cronet implementation from the list of available implementations. 3. Added internal CronetImplProviderFinder that looks for available Cronet implementations. One place it searches is the app resources that list the alternative implementation. ========== to ========== Cronet: a framework to provide alternative Cronet implementations 1. Added CronetImplProvider interface (abstract class) that should be implemented by all Cronet implementation (built-in and alternative). 2. Added CronetImplProviderSelector that is responsible for selecting a Cronet implementation from the list of available implementations. 3. Added internal CronetImplProviderFinder that looks for available Cronet implementations. One place it searches is the app resources that list the alternative implementation. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester ==========
kapishnikov@chromium.org changed reviewers: + mef@chromium.org, pauljensen@chromium.org
PTAL
https://codereview.chromium.org/2626523003/diff/20001/components/cronet/andro... File components/cronet/android/api/src/org/chromium/net/CronetEngine.java (right): https://codereview.chromium.org/2626523003/diff/20001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:513: CronetImplProvider provider; Should it call addCronetProviderImplByClassName(className) at this point? https://codereview.chromium.org/2626523003/diff/20001/components/cronet/andro... File components/cronet/android/api/src/org/chromium/net/CronetImplProvider.java (right): https://codereview.chromium.org/2626523003/diff/20001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/CronetImplProvider.java:14: public abstract class CronetImplProvider { nit: Maybe CronetProvider or CronetLoader? https://codereview.chromium.org/2626523003/diff/20001/components/cronet/andro... File components/cronet/android/api/src/org/chromium/net/CronetImplProviderSelector.java (right): https://codereview.chromium.org/2626523003/diff/20001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/CronetImplProviderSelector.java:12: interface CronetImplProviderSelector { can we make it abstract class?
https://codereview.chromium.org/2626523003/diff/20001/components/cronet/andro... File components/cronet/android/api/src/org/chromium/net/CronetEngine.java (right): https://codereview.chromium.org/2626523003/diff/20001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:29: static final String TAG = "CronetApi"; private https://codereview.chromium.org/2626523003/diff/20001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:29: static final String TAG = "CronetApi"; "CronetApi" -> CronetEngine.class.getSimpleName() https://codereview.chromium.org/2626523003/diff/20001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:472: return providers; We need to filter out providers that implement a version older than the API. Unfortunately using the Chromium version (e.g. 57.0.2977.0) doesn't tell you what version of the API they support, which could cause problems, for example: Today we branch M60 and trunk becomes M61. Let's call them 60.0 and 61.0. Tomorrow we update the API version and for whatever reason merge the change into M60. Let's call tomorrow's versions 60.1 and 61.1. Now we can't use the Chromium version to tell which API is newer, for example 61.0 implements an API version older than 60.1. Perhaps we could do this in a follow-on CL where we allow users to find out if they want to update the impl (by getting an Intent to throw back). https://codereview.chromium.org/2626523003/diff/20001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:485: Class<? extends CronetImplProvider> providerClass; move inside try block (variables should always have the smallest scope possible), unless you go with my next comment https://codereview.chromium.org/2626523003/diff/20001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:488: providers.add(providerClass.newInstance()); how about moving this line outside the try block? https://codereview.chromium.org/2626523003/diff/20001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:490: } catch (Exception ex) { catching all exceptions is not recommended, how about just ClassNotFound? https://codereview.chromium.org/2626523003/diff/20001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:506: RES_KEY_CRONET_IMPL_CLASS, "string", packageName); packageName->mContext.getPackageName() https://codereview.chromium.org/2626523003/diff/20001/components/cronet/andro... File components/cronet/android/api/src/org/chromium/net/CronetImplProvider.java (right): https://codereview.chromium.org/2626523003/diff/20001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/CronetImplProvider.java:21: protected abstract ICronetEngineBuilder load(Context context); rather than having every call take Context, can we add a constructor that takes a Context and a mContext protected member? https://codereview.chromium.org/2626523003/diff/20001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/CronetImplProvider.java:29: protected abstract String getName(Context context); What's the name used for? can we remove it? we'll log the name for the selected engine later using CronetEngine.getVersionString(). https://codereview.chromium.org/2626523003/diff/20001/components/cronet/andro... File components/cronet/android/api_version.txt (right): https://codereview.chromium.org/2626523003/diff/20001/components/cronet/andro... components/cronet/android/api_version.txt:1: 5 how about 3?
Thanks for the review! I have addressed the comments. PTAL. The next PS will contain the logic that selects the default provider based on the type and version. https://codereview.chromium.org/2626523003/diff/20001/components/cronet/andro... File components/cronet/android/api/src/org/chromium/net/CronetEngine.java (right): https://codereview.chromium.org/2626523003/diff/20001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:29: static final String TAG = "CronetApi"; On 2017/01/11 16:48:12, pauljensen wrote: > private Gone in the latest CL. https://codereview.chromium.org/2626523003/diff/20001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:29: static final String TAG = "CronetApi"; On 2017/01/11 16:48:12, pauljensen wrote: > "CronetApi" -> CronetEngine.class.getSimpleName() Gone in the latest CL. https://codereview.chromium.org/2626523003/diff/20001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:472: return providers; On 2017/01/11 16:48:13, pauljensen wrote: > We need to filter out providers that implement a version older than the API. > Unfortunately using the Chromium version (e.g. 57.0.2977.0) doesn't tell you > what version of the API they support, which could cause problems, for example: > > Today we branch M60 and trunk becomes M61. Let's call them 60.0 and 61.0. > Tomorrow we update the API version and for whatever reason merge the change into > M60. Let's call tomorrow's versions 60.1 and 61.1. > Now we can't use the Chromium version to tell which API is newer, for example > 61.0 implements an API version older than 60.1. > > Perhaps we could do this in a follow-on CL where we allow users to find out if > they want to update the impl (by getting an Intent to throw back). We are going to use API_LEVEL that is introduced in this CL. https://codereview.chromium.org/2626523003/diff/20001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:485: Class<? extends CronetImplProvider> providerClass; On 2017/01/11 16:48:12, pauljensen wrote: > move inside try block (variables should always have the smallest scope > possible), unless you go with my next comment Done. See CronetProviders.java https://codereview.chromium.org/2626523003/diff/20001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:488: providers.add(providerClass.newInstance()); On 2017/01/11 16:48:13, pauljensen wrote: > how about moving this line outside the try block? Kept it here for readability. https://codereview.chromium.org/2626523003/diff/20001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:490: } catch (Exception ex) { On 2017/01/11 16:48:12, pauljensen wrote: > catching all exceptions is not recommended, how about just ClassNotFound? Changed to multiple catches. We cannot catch multiple exceptions at once since it is only available in API Level 19 but we currently support API Level 16. https://codereview.chromium.org/2626523003/diff/20001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:506: RES_KEY_CRONET_IMPL_CLASS, "string", packageName); On 2017/01/11 16:48:13, pauljensen wrote: > packageName->mContext.getPackageName() Done. https://codereview.chromium.org/2626523003/diff/20001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:513: CronetImplProvider provider; On 2017/01/10 23:30:03, mef wrote: > Should it call addCronetProviderImplByClassName(className) at this point? Done. See CronetProviders.java https://codereview.chromium.org/2626523003/diff/20001/components/cronet/andro... File components/cronet/android/api/src/org/chromium/net/CronetImplProvider.java (right): https://codereview.chromium.org/2626523003/diff/20001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/CronetImplProvider.java:14: public abstract class CronetImplProvider { On 2017/01/10 23:30:03, mef wrote: > nit: Maybe CronetProvider or CronetLoader? Done. https://codereview.chromium.org/2626523003/diff/20001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/CronetImplProvider.java:21: protected abstract ICronetEngineBuilder load(Context context); On 2017/01/11 16:48:13, pauljensen wrote: > rather than having every call take Context, can we add a constructor that takes > a Context and a mContext protected member? Done. https://codereview.chromium.org/2626523003/diff/20001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/CronetImplProvider.java:29: protected abstract String getName(Context context); On 2017/01/11 16:48:13, pauljensen wrote: > What's the name used for? can we remove it? we'll log the name for the > selected engine later using CronetEngine.getVersionString(). The name can be used by embedders to select the provider that should be used. https://codereview.chromium.org/2626523003/diff/20001/components/cronet/andro... File components/cronet/android/api/src/org/chromium/net/CronetImplProviderSelector.java (right): https://codereview.chromium.org/2626523003/diff/20001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/CronetImplProviderSelector.java:12: interface CronetImplProviderSelector { On 2017/01/10 23:30:03, mef wrote: > can we make it abstract class? This interface was removed in the latest CL. https://codereview.chromium.org/2626523003/diff/20001/components/cronet/andro... File components/cronet/android/api_version.txt (right): https://codereview.chromium.org/2626523003/diff/20001/components/cronet/andro... components/cronet/android/api_version.txt:1: 5 On 2017/01/11 16:48:13, pauljensen wrote: > how about 3? Done.
https://codereview.chromium.org/2626523003/diff/120001/components/cronet/andr... File components/cronet/android/api/src/org/chromium/net/CronetEngine.java (right): https://codereview.chromium.org/2626523003/diff/120001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:74: } nit: how about we get rid of this if statement, the if statement on line 87 should catch this and the exception message includes the provider list? https://codereview.chromium.org/2626523003/diff/120001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:92: mBuilderDelegate = builderDelegate; nit: perhaps change the body of this function to this(createBuilderDelegate(context)) https://codereview.chromium.org/2626523003/diff/120001/components/cronet/andr... File components/cronet/android/api/src/org/chromium/net/CronetProvider.java (right): https://codereview.chromium.org/2626523003/diff/120001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetProvider.java:11: * must have a public constructor that accepts a single {@link Context} parameter. what about prefacing this with something like: <b>NOTE:</b> This class is for advanced users that want to select a particular Cronet implementation. Most users should simply use {@code new} {@link CronetEngine.Builder#Builder}. <p> https://codereview.chromium.org/2626523003/diff/120001/components/cronet/andr... File components/cronet/android/api/src/org/chromium/net/CronetProviders.java (right): https://codereview.chromium.org/2626523003/diff/120001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetProviders.java:17: */ what about prefacing this with something like: <b>NOTE:</b> This class is for advanced users that want to select a particular Cronet implementation. Most users should simply use {@code new} {@link CronetEngine.Builder#Builder}. <p> https://codereview.chromium.org/2626523003/diff/120001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetProviders.java:27: private static CronetProviders sSingleton = new CronetProviders(); nit: rename to sInstance https://codereview.chromium.org/2626523003/diff/120001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetProviders.java:31: public static CronetProviders instance() { I think this kind of function is normally called getInstance https://codereview.chromium.org/2626523003/diff/120001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetProviders.java:65: return providers; how slow is this? should we cache the list? https://codereview.chromium.org/2626523003/diff/120001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetProviders.java:75: private boolean addCronetProviderImplByClassName( nit: prefix with "maybe" as it can silently fail. https://codereview.chromium.org/2626523003/diff/120001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetProviders.java:103: private void logReflectiveOperationException(String className, boolean logError, Exception e) { nit: for this and all other private functions, can they be static? https://codereview.chromium.org/2626523003/diff/120001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetProviders.java:118: private boolean addCronetProviderFromResourceFile( nit: prefix with "maybe" as it can silently fail. https://codereview.chromium.org/2626523003/diff/120001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetProviders.java:134: + RES_KEY_CRONET_IMPL_CLASS + " key"); I think this should throw, not just log. I think this represents a serious misconfiguration. https://codereview.chromium.org/2626523003/diff/120001/components/cronet/andr... File components/cronet/android/cronet_impl_common_proguard.cfg (right): https://codereview.chromium.org/2626523003/diff/120001/components/cronet/andr... components/cronet/android/cronet_impl_common_proguard.cfg:9: -keep class org.chromium.net.impl.JavaCronetEngine Our production proguard files shouldn't have testing references if possible. Can we change our tests to simply use "instanceof JavaCronetEngine"? https://codereview.chromium.org/2626523003/diff/120001/components/cronet/andr... File components/cronet/android/java/src/org/chromium/net/impl/JavaCronetProvider.java (right): https://codereview.chromium.org/2626523003/diff/120001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/impl/JavaCronetProvider.java:19: public static final String PROVIDER_NAME = "Platform-Cronet-Provider"; Can we make it clearer that this is not the full-fledged native impl with the performance everyone expects, perhaps "FallbackPlatformBased" or "LowerPerformancePlatformBased". I don't think we need to include "Cronet-Provider" in the name; it seems redundant with the class name. https://codereview.chromium.org/2626523003/diff/120001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/impl/JavaCronetProvider.java:19: public static final String PROVIDER_NAME = "Platform-Cronet-Provider"; private? https://codereview.chromium.org/2626523003/diff/120001/components/cronet/andr... File components/cronet/android/java/src/org/chromium/net/impl/NativeCronetProvider.java (right): https://codereview.chromium.org/2626523003/diff/120001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/impl/NativeCronetProvider.java:19: public static final String PROVIDER_NAME = "Native-Cronet-Provider"; perhaps we should differentiate this from other native impls, perhaps "Prepackaged-Native" or "AppLocalNative" https://codereview.chromium.org/2626523003/diff/120001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/impl/NativeCronetProvider.java:19: public static final String PROVIDER_NAME = "Native-Cronet-Provider"; private? https://codereview.chromium.org/2626523003/diff/120001/components/cronet/andr... File components/cronet/android/test/javatests/src/org/chromium/net/CronetTestBase.java (right): https://codereview.chromium.org/2626523003/diff/120001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/CronetTestBase.java:163: return (ExperimentalCronetEngine.Builder) provider.createBuilder(); why not just make this "return new JavaCronetProvider().createBuilder();"? https://codereview.chromium.org/2626523003/diff/120001/components/cronet/andr... File components/cronet/android/test/smoketests/src/org/chromium/net/smoke/MissingNativeLibraryTest.java (right): https://codereview.chromium.org/2626523003/diff/120001/components/cronet/andr... components/cronet/android/test/smoketests/src/org/chromium/net/smoke/MissingNativeLibraryTest.java:53: for (CronetProvider provider : availableProviders) { assertTrue(provider.isEnabled());
looks pretty good to me. https://codereview.chromium.org/2626523003/diff/120001/components/cronet/andr... File components/cronet/android/api/src/org/chromium/net/CronetEngine.java (right): https://codereview.chromium.org/2626523003/diff/120001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:89: + " The list of available providers: " + providerList); CronetProvider probably needs a meaningful toString() method for this. https://codereview.chromium.org/2626523003/diff/120001/components/cronet/andr... File components/cronet/android/api/src/org/chromium/net/CronetProvider.java (right): https://codereview.chromium.org/2626523003/diff/120001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetProvider.java:44: * @return true if the provider is available. nit: s/available/enabled/. https://codereview.chromium.org/2626523003/diff/120001/components/cronet/andr... File components/cronet/android/api/src/org/chromium/net/CronetProviders.java (right): https://codereview.chromium.org/2626523003/diff/120001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetProviders.java:65: return providers; On 2017/01/18 17:03:04, pauljensen wrote: > how slow is this? should we cache the list? Can it change? For example if ProviderFromResourceFile is updated to match api version? https://codereview.chromium.org/2626523003/diff/120001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetProviders.java:71: * @param className the class name of the provider that shoul dbe instantiated. nit: s/shoul dbe/should be/ https://codereview.chromium.org/2626523003/diff/120001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetProviders.java:134: + RES_KEY_CRONET_IMPL_CLASS + " key"); On 2017/01/18 17:03:04, pauljensen wrote: > I think this should throw, not just log. I think this represents a serious > misconfiguration. I disagree, this could represent that provider isn't available on the device. https://codereview.chromium.org/2626523003/diff/120001/components/cronet/andr... File components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestContextTest.java (right): https://codereview.chromium.org/2626523003/diff/120001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestContextTest.java:1371: CronetEngineBuilderImpl builder = nit: Can we make |builder| be a CronetEngine.Builder and do CronetUrlRequestContext.createNativeUrlRequestContextConfig((CronetEngineBuilderImpl) builder.mDelegate) down below? https://codereview.chromium.org/2626523003/diff/120001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestContextTest.java:1411: CronetEngineBuilderImpl builder = nit: Can we make |builder| be a CronetEngine.Builder?
Thanks for the another review. I have addressed the comments and added the logic to selects the default provider based on the type and version. Also added some tests. https://codereview.chromium.org/2626523003/diff/120001/components/cronet/andr... File components/cronet/android/api/src/org/chromium/net/CronetEngine.java (right): https://codereview.chromium.org/2626523003/diff/120001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:74: } On 2017/01/18 17:03:04, pauljensen wrote: > nit: how about we get rid of this if statement, the if statement on line 87 > should catch this and the exception message includes the provider list? I added more error messages in a new PS ad some extra logic of filtering out disabled providers from the list, so the final list is different from the original one. PTAL. Some messages can be compacted into a single message but I wanted to be more verbose. https://codereview.chromium.org/2626523003/diff/120001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:89: + " The list of available providers: " + providerList); On 2017/01/18 20:57:54, mef wrote: > CronetProvider probably needs a meaningful toString() method for this. Good point. It will give the name of the class but having toString() with more info is better. Done. https://codereview.chromium.org/2626523003/diff/120001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:92: mBuilderDelegate = builderDelegate; On 2017/01/18 17:03:04, pauljensen wrote: > nit: perhaps change the body of this function to > this(createBuilderDelegate(context)) Good idea. Done. https://codereview.chromium.org/2626523003/diff/120001/components/cronet/andr... File components/cronet/android/api/src/org/chromium/net/CronetProvider.java (right): https://codereview.chromium.org/2626523003/diff/120001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetProvider.java:11: * must have a public constructor that accepts a single {@link Context} parameter. On 2017/01/18 17:03:04, pauljensen wrote: > what about prefacing this with something like: > > <b>NOTE:</b> This class is for advanced users that want to select a particular > Cronet implementation. Most users should simply use {@code new} {@link > CronetEngine.Builder#Builder}. > <p> Done. https://codereview.chromium.org/2626523003/diff/120001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetProvider.java:44: * @return true if the provider is available. On 2017/01/18 20:57:54, mef wrote: > nit: s/available/enabled/. Done. https://codereview.chromium.org/2626523003/diff/120001/components/cronet/andr... File components/cronet/android/api/src/org/chromium/net/CronetProviders.java (right): https://codereview.chromium.org/2626523003/diff/120001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetProviders.java:17: */ On 2017/01/18 17:03:04, pauljensen wrote: > what about prefacing this with something like: > > <b>NOTE:</b> This class is for advanced users that want to select a particular > Cronet implementation. Most users should simply use {@code new} {@link > CronetEngine.Builder#Builder}. > <p> Done. After the description; otherwise, it will not look good in JavaDocs. https://codereview.chromium.org/2626523003/diff/120001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetProviders.java:27: private static CronetProviders sSingleton = new CronetProviders(); On 2017/01/18 17:03:04, pauljensen wrote: > nit: rename to sInstance Done. https://codereview.chromium.org/2626523003/diff/120001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetProviders.java:31: public static CronetProviders instance() { On 2017/01/18 17:03:04, pauljensen wrote: > I think this kind of function is normally called getInstance Done. https://codereview.chromium.org/2626523003/diff/120001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetProviders.java:65: return providers; On 2017/01/18 17:03:04, pauljensen wrote: > how slow is this? should we cache the list? It should not be slow and normally it will be called only once. I think it is better to create new instances of the providers since they may have some logic in the constructor that depends on the current environment state. It can potentially have other negative implications. E.g. if a provider is not thread-safe and two different threads obtain the same provider instance, there can be threading issues. https://codereview.chromium.org/2626523003/diff/120001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetProviders.java:71: * @param className the class name of the provider that shoul dbe instantiated. On 2017/01/18 20:57:54, mef wrote: > nit: s/shoul dbe/should be/ Done. https://codereview.chromium.org/2626523003/diff/120001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetProviders.java:75: private boolean addCronetProviderImplByClassName( On 2017/01/18 17:03:04, pauljensen wrote: > nit: prefix with "maybe" as it can silently fail. The return value implies that the provider may not be added in some circumstances. I improved the description. https://codereview.chromium.org/2626523003/diff/120001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetProviders.java:103: private void logReflectiveOperationException(String className, boolean logError, Exception e) { On 2017/01/18 17:03:04, pauljensen wrote: > nit: for this and all other private functions, can they be static? Done. https://codereview.chromium.org/2626523003/diff/120001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetProviders.java:118: private boolean addCronetProviderFromResourceFile( On 2017/01/18 17:03:04, pauljensen wrote: > nit: prefix with "maybe" as it can silently fail. Added extra documentation to the @return. https://codereview.chromium.org/2626523003/diff/120001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetProviders.java:134: + RES_KEY_CRONET_IMPL_CLASS + " key"); On 2017/01/18 20:57:54, mef wrote: > On 2017/01/18 17:03:04, pauljensen wrote: > > I think this should throw, not just log. I think this represents a serious > > misconfiguration. > > I disagree, this could represent that provider isn't available on the device. Actually, the provider should be packaged inside the app apk, so we can throw an exception since it is wrong to reference a provider that cannot be found. Changed. https://codereview.chromium.org/2626523003/diff/120001/components/cronet/andr... File components/cronet/android/cronet_impl_common_proguard.cfg (right): https://codereview.chromium.org/2626523003/diff/120001/components/cronet/andr... components/cronet/android/cronet_impl_common_proguard.cfg:9: -keep class org.chromium.net.impl.JavaCronetEngine On 2017/01/18 17:03:04, pauljensen wrote: > Our production proguard files shouldn't have testing references if possible. > Can we change our tests to simply use "instanceof JavaCronetEngine"? Agree. I actually wanted to add it to the test proguard. One concern is that the test will not check whether removing this "keep" will break anything in production. https://codereview.chromium.org/2626523003/diff/120001/components/cronet/andr... File components/cronet/android/java/src/org/chromium/net/impl/JavaCronetProvider.java (right): https://codereview.chromium.org/2626523003/diff/120001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/impl/JavaCronetProvider.java:19: public static final String PROVIDER_NAME = "Platform-Cronet-Provider"; On 2017/01/18 17:03:05, pauljensen wrote: > private? Done. https://codereview.chromium.org/2626523003/diff/120001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/impl/JavaCronetProvider.java:19: public static final String PROVIDER_NAME = "Platform-Cronet-Provider"; On 2017/01/18 17:03:05, pauljensen wrote: > Can we make it clearer that this is not the full-fledged native impl with the > performance everyone expects, perhaps "FallbackPlatformBased" or > "LowerPerformancePlatformBased". I don't think we need to include > "Cronet-Provider" in the name; it seems redundant with the class name. I think the embedder should use the name of the provider instead of the class name when choosing providers. I added the well-known names to the CornetProviders class as public constants. Not sure it should be called FallbackPlatformBased since some apps/libraries may use it as the primary provider due to the size. It shouldn't matter much since the embedder should use public constants. This is an internal class. We should document somewhere else regarding the benefits and disadvantages of including one or other provider. https://codereview.chromium.org/2626523003/diff/120001/components/cronet/andr... File components/cronet/android/java/src/org/chromium/net/impl/NativeCronetProvider.java (right): https://codereview.chromium.org/2626523003/diff/120001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/impl/NativeCronetProvider.java:19: public static final String PROVIDER_NAME = "Native-Cronet-Provider"; On 2017/01/18 17:03:05, pauljensen wrote: > private? Done. https://codereview.chromium.org/2626523003/diff/120001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/impl/NativeCronetProvider.java:19: public static final String PROVIDER_NAME = "Native-Cronet-Provider"; On 2017/01/18 17:03:05, pauljensen wrote: > perhaps we should differentiate this from other native impls, perhaps > "Prepackaged-Native" or "AppLocalNative" We don't have any other native providers. If one appears, perhaps, it should have some branded name. https://codereview.chromium.org/2626523003/diff/120001/components/cronet/andr... File components/cronet/android/test/javatests/src/org/chromium/net/CronetTestBase.java (right): https://codereview.chromium.org/2626523003/diff/120001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/CronetTestBase.java:163: return (ExperimentalCronetEngine.Builder) provider.createBuilder(); On 2017/01/18 17:03:05, pauljensen wrote: > why not just make this "return new JavaCronetProvider().createBuilder();"? Some tests rely on features that are available in the experimental builder only. This method can be called from a test. We can explicitly cast everywhere an experimental feature is needed and return CronetEngine.Builder here. https://codereview.chromium.org/2626523003/diff/120001/components/cronet/andr... File components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestContextTest.java (right): https://codereview.chromium.org/2626523003/diff/120001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestContextTest.java:1371: CronetEngineBuilderImpl builder = On 2017/01/18 20:57:54, mef wrote: > nit: Can we make |builder| be a CronetEngine.Builder and do > CronetUrlRequestContext.createNativeUrlRequestContextConfig((CronetEngineBuilderImpl) > builder.mDelegate) down below? Good idea. Done! https://codereview.chromium.org/2626523003/diff/120001/components/cronet/andr... File components/cronet/android/test/smoketests/src/org/chromium/net/smoke/MissingNativeLibraryTest.java (right): https://codereview.chromium.org/2626523003/diff/120001/components/cronet/andr... components/cronet/android/test/smoketests/src/org/chromium/net/smoke/MissingNativeLibraryTest.java:53: for (CronetProvider provider : availableProviders) { On 2017/01/18 17:03:05, pauljensen wrote: > assertTrue(provider.isEnabled()); Done.
https://codereview.chromium.org/2626523003/diff/120001/components/cronet/andr... File components/cronet/android/api/src/org/chromium/net/CronetProviders.java (right): https://codereview.chromium.org/2626523003/diff/120001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetProviders.java:65: return providers; On 2017/01/19 01:25:24, kapishnikov wrote: > On 2017/01/18 17:03:04, pauljensen wrote: > > how slow is this? should we cache the list? > > It should not be slow and normally it will be called only once. I think it is > better to create new instances of the providers since they may have some logic > in the constructor that depends on the current environment state. It can > potentially have other negative implications. E.g. if a provider is not > thread-safe and two different threads obtain the same provider instance, there > can be threading issues. In this case what's the point of mInstance and getInstance()? It seems like this method could also be static. https://codereview.chromium.org/2626523003/diff/180001/components/cronet/andr... File components/cronet/android/api/src/org/chromium/net/CronetEngine.java (right): https://codereview.chromium.org/2626523003/diff/180001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:317: // Try to load implementation using available providers. Assuming that nit: Assuming -> Assume https://codereview.chromium.org/2626523003/diff/180001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:349: static List<CronetProvider> getEnabledCronetProviders( Should this and below be moved into CronetProviders? Then all sorting / enabled / available logic is located in one place? Per offline discussion I don't feel strongly and can be convinced otherwise. Paul, WDYT? https://codereview.chromium.org/2626523003/diff/180001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:368: throw new RuntimeException("All found Cronet providers are disabled." nit: found -> available https://codereview.chromium.org/2626523003/diff/180001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:376: // The platform provider should always be at the end of the list. Can PlatformProvider be just added to the end of the list if it is available? https://codereview.chromium.org/2626523003/diff/180001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:384: return -compareVersions(p1.getVersion(), p2.getVersion()); Is it ok if compareVersions throws an exception? https://codereview.chromium.org/2626523003/diff/180001/components/cronet/andr... File components/cronet/android/api/src/org/chromium/net/CronetProviders.java (right): https://codereview.chromium.org/2626523003/diff/180001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetProviders.java:32: * Name of the native platform provider. nit: s/platform/Cronet/ or remove. https://codereview.chromium.org/2626523003/diff/180001/components/cronet/andr... File components/cronet/android/test/javatests/src/org/chromium/net/CronetEngineBuilderTest.java (right): https://codereview.chromium.org/2626523003/diff/180001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/CronetEngineBuilderTest.java:61: for (int i = 0; i < availableProviders.length; i++) { Can this just be assertEquals(PROVIDER_NAME_NATIVE, orderedProviders.get(0).getName()), etc? Same way as in the next test? https://codereview.chromium.org/2626523003/diff/180001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/CronetEngineBuilderTest.java:75: @CronetTestBase.OnlyRunNativeCronet Why is it marked OnlyRunNativeCronet? Would running it twice break? https://codereview.chromium.org/2626523003/diff/180001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/CronetEngineBuilderTest.java:114: // TODO(kapishnikov): Replace with a mock when mockito is supported. FWIW it seems that mockito is already supported: https://cs.chromium.org/search/?q=org.mockito.Mockito&sq=package:chromium&typ... Feel free to do it in followup CL though. https://codereview.chromium.org/2626523003/diff/180001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/CronetEngineBuilderTest.java:153: // TODO(kapishnikov): Replace with a mock when mockito is supported. unused?
https://codereview.chromium.org/2626523003/diff/120001/components/cronet/andr... File components/cronet/android/api/src/org/chromium/net/CronetProviders.java (right): https://codereview.chromium.org/2626523003/diff/120001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetProviders.java:65: return providers; On 2017/01/19 22:57:46, mef wrote: > On 2017/01/19 01:25:24, kapishnikov wrote: > > On 2017/01/18 17:03:04, pauljensen wrote: > > > how slow is this? should we cache the list? > > > > It should not be slow and normally it will be called only once. I think it is > > better to create new instances of the providers since they may have some logic > > in the constructor that depends on the current environment state. It can > > potentially have other negative implications. E.g. if a provider is not > > thread-safe and two different threads obtain the same provider instance, there > > can be threading issues. > In this case what's the point of mInstance and getInstance()? It seems like this > method could also be static. A singleton provides more flexibility since it can act as a normal class. E.g., it can be mocked and extended. That is how the CronetEngineBuilderTest extends CronetProviders and injects it into CronetEngine.Builder.getEnabledCronetProviders(getContext(). That would be difficult if the class contained only static methods and fields. https://codereview.chromium.org/2626523003/diff/180001/components/cronet/andr... File components/cronet/android/api/src/org/chromium/net/CronetEngine.java (right): https://codereview.chromium.org/2626523003/diff/180001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:317: // Try to load implementation using available providers. Assuming that On 2017/01/19 22:57:46, mef wrote: > nit: Assuming -> Assume Done. https://codereview.chromium.org/2626523003/diff/180001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:349: static List<CronetProvider> getEnabledCronetProviders( On 2017/01/19 22:57:46, mef wrote: > Should this and below be moved into CronetProviders? > Then all sorting / enabled / available logic is located in one place? > > Per offline discussion I don't feel strongly and can be convinced otherwise. > Paul, WDYT? I have a strong opinion that CronetProviders should only be responsible for providing the list of all Cronet providers available by the app. It is responsibility of the client of the class to select the right one. https://codereview.chromium.org/2626523003/diff/180001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:368: throw new RuntimeException("All found Cronet providers are disabled." On 2017/01/19 22:57:46, mef wrote: > nit: found -> available Done. https://codereview.chromium.org/2626523003/diff/180001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:376: // The platform provider should always be at the end of the list. On 2017/01/19 22:57:46, mef wrote: > Can PlatformProvider be just added to the end of the list if it is available? We could extract PlatformProvider from the list, sort it and add it to the end of the list. I don't think it is easier way to do the sorting; unless we change CronetProviders interface to add some PlatformProvider specific logic. https://codereview.chromium.org/2626523003/diff/180001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:384: return -compareVersions(p1.getVersion(), p2.getVersion()); On 2017/01/19 22:57:46, mef wrote: > Is it ok if compareVersions throws an exception? It is a good question. Currently it will cause the app stop with the exception. I think that is what we should do since if a provider has an invalid version something is wrong. https://codereview.chromium.org/2626523003/diff/180001/components/cronet/andr... File components/cronet/android/api/src/org/chromium/net/CronetProviders.java (right): https://codereview.chromium.org/2626523003/diff/180001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetProviders.java:32: * Name of the native platform provider. On 2017/01/19 22:57:46, mef wrote: > nit: s/platform/Cronet/ or remove. Done. https://codereview.chromium.org/2626523003/diff/180001/components/cronet/andr... File components/cronet/android/test/javatests/src/org/chromium/net/CronetEngineBuilderTest.java (right): https://codereview.chromium.org/2626523003/diff/180001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/CronetEngineBuilderTest.java:61: for (int i = 0; i < availableProviders.length; i++) { On 2017/01/19 22:57:46, mef wrote: > Can this just be assertEquals(PROVIDER_NAME_NATIVE, > orderedProviders.get(0).getName()), etc? > Same way as in the next test? Good point. Done. https://codereview.chromium.org/2626523003/diff/180001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/CronetEngineBuilderTest.java:75: @CronetTestBase.OnlyRunNativeCronet On 2017/01/19 22:57:46, mef wrote: > Why is it marked OnlyRunNativeCronet? Would running it twice break? Done. https://codereview.chromium.org/2626523003/diff/180001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/CronetEngineBuilderTest.java:114: // TODO(kapishnikov): Replace with a mock when mockito is supported. On 2017/01/19 22:57:46, mef wrote: > FWIW it seems that mockito is already supported: > https://cs.chromium.org/search/?q=org.mockito.Mockito&sq=package:chromium&typ... > Feel free to do it in followup CL though. The components that use mockito run junit tests (i.e. compared to instrumentation tests that Cronet has). I tried to include the mockito library directly but got a message that some of the dependencies do not support android. We should address it in a follow up CL. https://codereview.chromium.org/2626523003/diff/180001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/CronetEngineBuilderTest.java:153: // TODO(kapishnikov): Replace with a mock when mockito is supported. On 2017/01/19 22:57:46, mef wrote: > unused? The class is used in testProviderOrdering() and testThatDisabledProvidersAreExcluded().
https://codereview.chromium.org/2626523003/diff/120001/components/cronet/andr... File components/cronet/android/api/src/org/chromium/net/CronetProviders.java (right): https://codereview.chromium.org/2626523003/diff/120001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetProviders.java:65: return providers; On 2017/01/19 01:25:24, kapishnikov wrote: > On 2017/01/18 17:03:04, pauljensen wrote: > > how slow is this? should we cache the list? > > It should not be slow Can you quantify that? > and normally it will be called only once. Won't it be called every time we want to create a CronetEngine? https://codereview.chromium.org/2626523003/diff/120001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetProviders.java:75: private boolean addCronetProviderImplByClassName( On 2017/01/19 01:25:24, kapishnikov wrote: > On 2017/01/18 17:03:04, pauljensen wrote: > > nit: prefix with "maybe" as it can silently fail. > > The return value implies that the provider may not be added in some > circumstances. I improved the description. Change the javadoc to start with "Attempts to add" https://codereview.chromium.org/2626523003/diff/120001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetProviders.java:118: private boolean addCronetProviderFromResourceFile( On 2017/01/19 01:25:24, kapishnikov wrote: > On 2017/01/18 17:03:04, pauljensen wrote: > > nit: prefix with "maybe" as it can silently fail. > > Added extra documentation to the @return. Change the javadoc to start with "Attempts to add" https://codereview.chromium.org/2626523003/diff/120001/components/cronet/andr... File components/cronet/android/test/javatests/src/org/chromium/net/CronetTestBase.java (right): https://codereview.chromium.org/2626523003/diff/120001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/CronetTestBase.java:163: return (ExperimentalCronetEngine.Builder) provider.createBuilder(); On 2017/01/19 01:25:25, kapishnikov wrote: > On 2017/01/18 17:03:05, pauljensen wrote: > > why not just make this "return new JavaCronetProvider().createBuilder();"? > > Some tests rely on features that are available in the experimental builder only. > This method can be called from a test. We can explicitly cast everywhere an > experimental feature is needed and return CronetEngine.Builder here. How about: (ExperimentalCronetEngine.Builder) new JavaCronetProvider().createBuilder() https://codereview.chromium.org/2626523003/diff/180001/components/cronet/andr... File components/cronet/android/api/src/org/chromium/net/CronetEngine.java (right): https://codereview.chromium.org/2626523003/diff/180001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:307: * with the highest priority. Please explain "highest priority" https://codereview.chromium.org/2626523003/diff/180001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:313: ICronetEngineBuilder mBuilderDelegate; local variables should not have "m" prefixes, remove this variable. https://codereview.chromium.org/2626523003/diff/180001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:322: if (builder != null) { We should use either isEnabled() or createBuilder() returning non-null to signal an enabled provider, not both. I vote for isEnabled(), so we can get rid of this null check and just write: builderDelegate = provider.createBuilder().mBuilderDelegate; https://codereview.chromium.org/2626523003/diff/180001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:346: * {@hide} I don't think package private appears in javadocs, so no {@hide} needed. https://codereview.chromium.org/2626523003/diff/180001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:402: * {@hide} I don't think package private appears in javadocs, so no {@hide} needed. https://codereview.chromium.org/2626523003/diff/180001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:412: while (s1segments.length > index && s2segments.length > index) { how about: for (int i = 0; i < s1segments.length && i < s2segments.length; i++) { https://codereview.chromium.org/2626523003/diff/180001/components/cronet/andr... File components/cronet/android/api/src/org/chromium/net/CronetProvider.java (right): https://codereview.chromium.org/2626523003/diff/180001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetProvider.java:10: * empty line https://codereview.chromium.org/2626523003/diff/180001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetProvider.java:11: * A Cronet implementation provider. Note: every implementation of CronetProvider I don't understand the "Note"...there is only one constructor, remove? https://codereview.chromium.org/2626523003/diff/180001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetProvider.java:11: * A Cronet implementation provider. Note: every implementation of CronetProvider I think we need to add more description here. Like: A Cronet implementation provider. This class can be used to select specific Cronet implementations to create {@link CronetEngine.Builder} instances. To get the list of available {@link CronetProvider}s call {@link CronetProviders#getAvailableProviders} https://codereview.chromium.org/2626523003/diff/180001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetProvider.java:16: * CronetEngine.Builder#CronetEngine.Builder(android.content.Context)}. Might want to put this NOTE at the top. https://codereview.chromium.org/2626523003/diff/180001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetProvider.java:26: * Creates and returns {@link CronetEngine.Builder}. returns->returns an instance of https://codereview.chromium.org/2626523003/diff/180001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetProvider.java:33: * Returns the provider name. Include @links to statically defined names. https://codereview.chromium.org/2626523003/diff/180001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetProvider.java:40: * Returns the provider version. Mention that this can be used to select newest (i.e. highest performing, most secure) provider https://codereview.chromium.org/2626523003/diff/180001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetProvider.java:47: * Returns whether the provider is enabled and can be used to instantiate the Cronet engine. Mention that if isEnabled() returns false there are other APIs available to enable the engine. They should refer to the documentation for the particular provider. https://codereview.chromium.org/2626523003/diff/180001/components/cronet/andr... File components/cronet/android/api/src/org/chromium/net/CronetProviders.java (right): https://codereview.chromium.org/2626523003/diff/180001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetProviders.java:21: * CronetEngine.Builder#CronetEngine.Builder(android.content.Context)}. Might want to put this NOTE at the top. https://codereview.chromium.org/2626523003/diff/180001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetProviders.java:27: * Name of the platform Cronet provider. I don't understand this. I don't know what "platform" refers to. I think this should be moved to CronetProvider. How about: /** * String returned by {@link CronetProvider#getName} for {@link CronetProvider} that provides * Cronet implementation based on the system's {@link java.net.HttpURLConnection} implementation. * This implementation offers significantly degraded performance relative to native Cronet * implementations (see {@link #NATIVE_PROVIDER_NAME}). */ public static final String LOW_PERFORMANCE_PROVIDER_NAME = "Low-Performance"; https://codereview.chromium.org/2626523003/diff/180001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetProviders.java:32: * Name of the native platform provider. I don't understand this. I don't know what "platform" refers to. I think this should be moved to CronetProvider. I think we need to differentiate this from other native providers. How about: /** * String returned by {@link CronetProvider#getName} for {@link CronetProvider} that provides * high performance Cronet native implementation distributed with the application. */ public static final String APP_PACKAGED_NATIVE_PROVIDER_NAME = "App-Packaged-Native"; https://codereview.chromium.org/2626523003/diff/180001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetProviders.java:47: public static CronetProviders getInstance() { needs javadoc, or preferably just remove if we instead just make getAllProviders() a static member of CronetProvider https://codereview.chromium.org/2626523003/diff/180001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetProviders.java:76: public List<CronetProvider> getAvailableProviders(Context context) { "Available" is confusing because we also use the term "Enabled". How about just getAllProviders? https://codereview.chromium.org/2626523003/diff/180001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetProviders.java:76: public List<CronetProvider> getAvailableProviders(Context context) { How about moving this to CronetProvider (after making it static like Misha recommended)? I think that would simplify things significantly https://codereview.chromium.org/2626523003/diff/180001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetProviders.java:94: ClassLoader loader = CronetProviders.class.getClassLoader(); perhaps this should use context.getClassLoader()? https://codereview.chromium.org/2626523003/diff/180001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetProviders.java:136: * @ spurious @ https://codereview.chromium.org/2626523003/diff/180001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetProviders.java:151: if (!added) { now that added is used once you can combine these lines into "if (!addCronet..." https://codereview.chromium.org/2626523003/diff/180001/components/cronet/andr... File components/cronet/android/java/src/org/chromium/net/impl/JavaCronetProvider.java (right): https://codereview.chromium.org/2626523003/diff/180001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/impl/JavaCronetProvider.java:20: private static final String PROVIDER_NAME = CronetProviders.PROVIDER_NAME_PLATFORM; Remove this variable and just use CronetProviders.PROVIDER_NAME_PLATFORM https://codereview.chromium.org/2626523003/diff/180001/components/cronet/andr... File components/cronet/android/java/src/org/chromium/net/impl/NativeCronetProvider.java (right): https://codereview.chromium.org/2626523003/diff/180001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/impl/NativeCronetProvider.java:20: private static final String PROVIDER_NAME = CronetProviders.PROVIDER_NAME_NATIVE; Remove this variable and just use CronetPRoviders.PROVIDER_NAME_NATIVE https://codereview.chromium.org/2626523003/diff/180001/components/cronet/andr... File components/cronet/android/test/proguard.cfg (right): https://codereview.chromium.org/2626523003/diff/180001/components/cronet/andr... components/cronet/android/test/proguard.cfg:7: -keep class org.chromium.net.smoke.ChromiumPlatformOnlyTestSupport why are these two lines needed? can you add a comment? https://codereview.chromium.org/2626523003/diff/180001/components/cronet/andr... components/cronet/android/test/proguard.cfg:16: -keep class org.chromium.net.impl.JavaCronetEngine Instead of checking the name, why not just do "instanceof JavaCronetEngine"? Then we can remove this proguard line...which kinda scares me.
Paul, thanks for the comments. I addressed most of them but some require further discussion. https://codereview.chromium.org/2626523003/diff/120001/components/cronet/andr... File components/cronet/android/api/src/org/chromium/net/CronetProviders.java (right): https://codereview.chromium.org/2626523003/diff/120001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetProviders.java:65: return providers; On 2017/01/20 17:13:34, pauljensen wrote: > On 2017/01/19 01:25:24, kapishnikov wrote: > > On 2017/01/18 17:03:04, pauljensen wrote: > > > how slow is this? should we cache the list? > > > > It should not be slow > Can you quantify that? > > > and normally it will be called only once. > Won't it be called every time we want to create a CronetEngine? Yes, it will be called every time the default CronetEngine.Bulder is created. It will not be called multiple times if the same builder is used to create multiple engines. But I agree, it is better to cache the providers and specify that they should be thread-safe & that they should not rely on the environment state that was in place when the provider constructor was called. https://codereview.chromium.org/2626523003/diff/120001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetProviders.java:75: private boolean addCronetProviderImplByClassName( On 2017/01/20 17:13:34, pauljensen wrote: > On 2017/01/19 01:25:24, kapishnikov wrote: > > On 2017/01/18 17:03:04, pauljensen wrote: > > > nit: prefix with "maybe" as it can silently fail. > > > > The return value implies that the provider may not be added in some > > circumstances. I improved the description. > > Change the javadoc to start with "Attempts to add" Done. https://codereview.chromium.org/2626523003/diff/120001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetProviders.java:118: private boolean addCronetProviderFromResourceFile( On 2017/01/20 17:13:34, pauljensen wrote: > On 2017/01/19 01:25:24, kapishnikov wrote: > > On 2017/01/18 17:03:04, pauljensen wrote: > > > nit: prefix with "maybe" as it can silently fail. > > > > Added extra documentation to the @return. > > Change the javadoc to start with "Attempts to add" Done. Also added @throws RuntimeException https://codereview.chromium.org/2626523003/diff/120001/components/cronet/andr... File components/cronet/android/test/javatests/src/org/chromium/net/CronetTestBase.java (right): https://codereview.chromium.org/2626523003/diff/120001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/CronetTestBase.java:163: return (ExperimentalCronetEngine.Builder) provider.createBuilder(); On 2017/01/20 17:13:34, pauljensen wrote: > On 2017/01/19 01:25:25, kapishnikov wrote: > > On 2017/01/18 17:03:05, pauljensen wrote: > > > why not just make this "return new JavaCronetProvider().createBuilder();"? > > > > Some tests rely on features that are available in the experimental builder > only. > > This method can be called from a test. We can explicitly cast everywhere an > > experimental feature is needed and return CronetEngine.Builder here. > > How about: > (ExperimentalCronetEngine.Builder) new JavaCronetProvider().createBuilder() That is a great suggestion. I tried to avoid referencing the internal classes in our tests to improve code coverage but I think in this case it is justified. We already have a smoke test that uses Providers to create the Java engine. Done. https://codereview.chromium.org/2626523003/diff/180001/components/cronet/andr... File components/cronet/android/api/src/org/chromium/net/CronetEngine.java (right): https://codereview.chromium.org/2626523003/diff/180001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:307: * with the highest priority. On 2017/01/20 17:13:35, pauljensen wrote: > Please explain "highest priority" Since an enabled provider should never return null. This method implementation has been changed to consider only the first element of the list. Changed the javadoc accordingly. https://codereview.chromium.org/2626523003/diff/180001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:313: ICronetEngineBuilder mBuilderDelegate; On 2017/01/20 17:13:35, pauljensen wrote: > local variables should not have "m" prefixes, remove this variable. Deleted. https://codereview.chromium.org/2626523003/diff/180001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:322: if (builder != null) { On 2017/01/20 17:13:35, pauljensen wrote: > We should use either isEnabled() or createBuilder() returning non-null to signal > an enabled provider, not both. I vote for isEnabled(), so we can get rid of > this null check and just write: > builderDelegate = provider.createBuilder().mBuilderDelegate; Agree. createBuilder() now throws an exception that should be propagated to the embedder. https://codereview.chromium.org/2626523003/diff/180001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:346: * {@hide} On 2017/01/20 17:13:35, pauljensen wrote: > I don't think package private appears in javadocs, so no {@hide} needed. Removed. https://codereview.chromium.org/2626523003/diff/180001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:402: * {@hide} On 2017/01/20 17:13:35, pauljensen wrote: > I don't think package private appears in javadocs, so no {@hide} needed. Removed. https://codereview.chromium.org/2626523003/diff/180001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:412: while (s1segments.length > index && s2segments.length > index) { On 2017/01/20 17:13:35, pauljensen wrote: > how about: > for (int i = 0; i < s1segments.length && i < s2segments.length; i++) { Neat. Done. https://codereview.chromium.org/2626523003/diff/180001/components/cronet/andr... File components/cronet/android/api/src/org/chromium/net/CronetProvider.java (right): https://codereview.chromium.org/2626523003/diff/180001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetProvider.java:10: * On 2017/01/20 17:13:35, pauljensen wrote: > empty line Done. https://codereview.chromium.org/2626523003/diff/180001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetProvider.java:11: * A Cronet implementation provider. Note: every implementation of CronetProvider On 2017/01/20 17:13:35, pauljensen wrote: > I don't understand the "Note"...there is only one constructor, remove? A subclass can have a different constructor that accepts a second parameter in addition to Context. That is not allowed since the provider instantiation logic relies on the fact that the subclass is always going to have a constructor that accepts single Context parameter. Added comments. https://codereview.chromium.org/2626523003/diff/180001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetProvider.java:16: * CronetEngine.Builder#CronetEngine.Builder(android.content.Context)}. On 2017/01/20 17:13:35, pauljensen wrote: > Might want to put this NOTE at the top. Javadoc uses the first sentence to create the short description of the class in the package content table. That is why I added it after the description of the class. https://codereview.chromium.org/2626523003/diff/180001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetProvider.java:26: * Creates and returns {@link CronetEngine.Builder}. On 2017/01/20 17:13:35, pauljensen wrote: > returns->returns an instance of Done. https://codereview.chromium.org/2626523003/diff/180001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetProvider.java:33: * Returns the provider name. On 2017/01/20 17:13:35, pauljensen wrote: > Include @links to statically defined names. Done. https://codereview.chromium.org/2626523003/diff/180001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetProvider.java:40: * Returns the provider version. On 2017/01/20 17:13:35, pauljensen wrote: > Mention that this can be used to select newest (i.e. highest performing, most > secure) provider Done. https://codereview.chromium.org/2626523003/diff/180001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetProvider.java:47: * Returns whether the provider is enabled and can be used to instantiate the Cronet engine. On 2017/01/20 17:13:35, pauljensen wrote: > Mention that if isEnabled() returns false there are other APIs available to > enable the engine. They should refer to the documentation for the particular > provider. Done. https://codereview.chromium.org/2626523003/diff/180001/components/cronet/andr... File components/cronet/android/api/src/org/chromium/net/CronetProviders.java (right): https://codereview.chromium.org/2626523003/diff/180001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetProviders.java:21: * CronetEngine.Builder#CronetEngine.Builder(android.content.Context)}. On 2017/01/20 17:13:35, pauljensen wrote: > Might want to put this NOTE at the top. See the answer to the similar comment. https://codereview.chromium.org/2626523003/diff/180001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetProviders.java:27: * Name of the platform Cronet provider. On 2017/01/20 17:13:36, pauljensen wrote: > I don't understand this. I don't know what "platform" refers to. I think this > should be moved to CronetProvider. How about: > /** > * String returned by {@link CronetProvider#getName} for {@link CronetProvider} > that provides > * Cronet implementation based on the system's {@link > java.net.HttpURLConnection} implementation. > * This implementation offers significantly degraded performance relative to > native Cronet > * implementations (see {@link #NATIVE_PROVIDER_NAME}). > */ > public static final String LOW_PERFORMANCE_PROVIDER_NAME = "Low-Performance"; Nice. Changed the docs. Regarding the naming, if the providers are named based on performance it is not clear how to name other providers. https://codereview.chromium.org/2626523003/diff/180001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetProviders.java:32: * Name of the native platform provider. On 2017/01/20 17:13:36, pauljensen wrote: > I don't understand this. I don't know what "platform" refers to. I think this > should be moved to CronetProvider. I think we need to differentiate this from > other native providers. How about: > /** > * String returned by {@link CronetProvider#getName} for {@link CronetProvider} > that provides > * high performance Cronet native implementation distributed with the > application. > */ > public static final String APP_PACKAGED_NATIVE_PROVIDER_NAME = > "App-Packaged-Native"; Changed the docs similar to PROVIDER_NAME_PLATFORM. Needs more discussion. https://codereview.chromium.org/2626523003/diff/180001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetProviders.java:47: public static CronetProviders getInstance() { On 2017/01/20 17:13:36, pauljensen wrote: > needs javadoc, or preferably just remove if we instead just make > getAllProviders() a static member of CronetProvider If we make it static, it will be difficult to mock it and difficult to test classes that use it. We should see if there is a workaround. Added javadoc. https://codereview.chromium.org/2626523003/diff/180001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetProviders.java:76: public List<CronetProvider> getAvailableProviders(Context context) { On 2017/01/20 17:13:36, pauljensen wrote: > "Available" is confusing because we also use the term "Enabled". How about just > getAllProviders? Done. https://codereview.chromium.org/2626523003/diff/180001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetProviders.java:94: ClassLoader loader = CronetProviders.class.getClassLoader(); On 2017/01/20 17:13:35, pauljensen wrote: > perhaps this should use context.getClassLoader()? This is a good point. I am not sure in what circumstances it can be different from the one that loaded this class. https://codereview.chromium.org/2626523003/diff/180001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetProviders.java:136: * @ On 2017/01/20 17:13:36, pauljensen wrote: > spurious @ Forgot to add @throws RuntimeException. Done. https://codereview.chromium.org/2626523003/diff/180001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetProviders.java:151: if (!added) { On 2017/01/20 17:13:36, pauljensen wrote: > now that added is used once you can combine these lines into "if (!addCronet..." Done. https://codereview.chromium.org/2626523003/diff/180001/components/cronet/andr... File components/cronet/android/java/src/org/chromium/net/impl/JavaCronetProvider.java (right): https://codereview.chromium.org/2626523003/diff/180001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/impl/JavaCronetProvider.java:20: private static final String PROVIDER_NAME = CronetProviders.PROVIDER_NAME_PLATFORM; On 2017/01/20 17:13:36, pauljensen wrote: > Remove this variable and just use CronetProviders.PROVIDER_NAME_PLATFORM Done. https://codereview.chromium.org/2626523003/diff/180001/components/cronet/andr... File components/cronet/android/java/src/org/chromium/net/impl/NativeCronetProvider.java (right): https://codereview.chromium.org/2626523003/diff/180001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/impl/NativeCronetProvider.java:20: private static final String PROVIDER_NAME = CronetProviders.PROVIDER_NAME_NATIVE; On 2017/01/20 17:13:36, pauljensen wrote: > Remove this variable and just use CronetPRoviders.PROVIDER_NAME_NATIVE Done. https://codereview.chromium.org/2626523003/diff/180001/components/cronet/andr... File components/cronet/android/test/proguard.cfg (right): https://codereview.chromium.org/2626523003/diff/180001/components/cronet/andr... components/cronet/android/test/proguard.cfg:7: -keep class org.chromium.net.smoke.ChromiumPlatformOnlyTestSupport On 2017/01/20 17:13:36, pauljensen wrote: > why are these two lines needed? can you add a comment? Added comment; # Keep ChromiumNativeTestSupport & ChromiumPlatformOnlyTestSupport since they are # instantiated through Reflection by the smoke tests. https://codereview.chromium.org/2626523003/diff/180001/components/cronet/andr... components/cronet/android/test/proguard.cfg:16: -keep class org.chromium.net.impl.JavaCronetEngine On 2017/01/20 17:13:36, pauljensen wrote: > Instead of checking the name, why not just do "instanceof JavaCronetEngine"? > Then we can remove this proguard line...which kinda scares me. This is a good point. Currently smoke tests only rely on the API, i.e. they don't require the implementation part to compile. The implementation part is included when the app is linked. That is the realization of the idea that the client code can compile without including any implementation jars and at the app packaging stage the embedder can decide what implementations to package. I changed "-keep" to "-keepnames".
Should we @hide CronetProvider(s)? https://codereview.chromium.org/2626523003/diff/180001/components/cronet/andr... File components/cronet/android/api/src/org/chromium/net/CronetProviders.java (right): https://codereview.chromium.org/2626523003/diff/180001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetProviders.java:27: * Name of the platform Cronet provider. On 2017/01/20 21:48:38, kapishnikov wrote: > On 2017/01/20 17:13:36, pauljensen wrote: > > I don't understand this. I don't know what "platform" refers to. I think > this > > should be moved to CronetProvider. How about: > > /** > > * String returned by {@link CronetProvider#getName} for {@link > CronetProvider} > > that provides > > * Cronet implementation based on the system's {@link > > java.net.HttpURLConnection} implementation. > > * This implementation offers significantly degraded performance relative to > > native Cronet > > * implementations (see {@link #NATIVE_PROVIDER_NAME}). > > */ > > public static final String LOW_PERFORMANCE_PROVIDER_NAME = "Low-Performance"; > > Nice. Changed the docs. Regarding the naming, if the providers are named based > on performance it is not clear how to name other providers. You didn't address my comment about "platform". I still don't understand what "platform" refers to. I still think we need a new name and value for this variable. We really don't want embedders using this by accident, they'll get performance worse than Android's HttpURLConnection, and think Cronet is a waste. You didn't address my request to move this to CronetProvider. I think it makes more sense there as it can be beside the getName() function where it is returned from. https://codereview.chromium.org/2626523003/diff/240001/components/cronet/andr... File components/cronet/android/api/src/org/chromium/net/CronetEngine.java (right): https://codereview.chromium.org/2626523003/diff/240001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:317: return builder.mBuilderDelegate; how about combining these two lines together? return providerList.get(0).createBuilder().mBuilderDelegate; https://codereview.chromium.org/2626523003/diff/240001/components/cronet/andr... File components/cronet/android/api/src/org/chromium/net/CronetProvider.java (right): https://codereview.chromium.org/2626523003/diff/240001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetProvider.java:16: * constructor that accepts a single {@link Context} parameter. I don't think embedders should generally be extending this class, so I don't think we need this comment. I'd vote for removing this comment as it detracts from the very important NOTE below. If someone extends this class they either: 1. will be calling it directly, in which case they don't care about how "new CronetEngine.Builder" calls it, or 2. they'll figure out very quickly how it gets called from "new CronetEngine.Builder" If you really really want to keep this comment, can we move it to the constructor (so it doesn't detract from the very important NOTE? https://codereview.chromium.org/2626523003/diff/240001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetProvider.java:19: * threads, all subclasses of this class should be thread-safe. I don't think this is necessary. I think APIs are generally supposed to be thread-safe unless restrictions are explicitly stated and enforced. I'd recommend removing, but if you really want to keep it, I'd say move it below the NOTE. I don't want to detract from the fact that nearly all users shouldn't be interacting with this class. https://codereview.chromium.org/2626523003/diff/240001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetProvider.java:33: * Creates and returns an instance of {@link CronetEngine.Builder}. Perhaps we should restate the NOTE here: This method is for advanced users that want to select a particular Cronet implementation. Most users should simply use {@code new} {@link CronetEngine.Builder#CronetEngine.Builder(android.content.Context)}. https://codereview.chromium.org/2626523003/diff/240001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetProvider.java:36: * @throws IllegalStateException if the provider is not enabled. enabled -> enabled (see {@link #isEnabled} https://codereview.chromium.org/2626523003/diff/240001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetProvider.java:61: * Please read the provider documentation for the instructions how to enable it. for the instructions how to enable it-> for enablement procedure. https://codereview.chromium.org/2626523003/diff/240001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetProvider.java:63: * @return true if the provider is enabled. true -> {@code true} https://codereview.chromium.org/2626523003/diff/240001/components/cronet/andr... File components/cronet/android/api/src/org/chromium/net/CronetProviders.java (right): https://codereview.chromium.org/2626523003/diff/240001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetProviders.java:40: * implementations (see {@link #PROVIDER_NAME_PLATFORM}). I don't understand "system Cronet implementation", this sounds like the Java-only one is the default Cronet implementation. Can we use my previously suggested rewording, that doesn't mention the Java-only one (I really don't want people mistakenly using it; I fear they'll assume Cronet always has performance worse than Android's HttpURLConnection impl and never use Cronet again). https://codereview.chromium.org/2626523003/diff/240001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetProviders.java:52: private final Object mLock = new Object(); swap these two lines and add @GuardedBy https://codereview.chromium.org/2626523003/diff/240001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetProviders.java:86: public List<CronetProvider> getAllProviders(Context context) { I really think we should move this to CronetProvider as a static function. I know it makes testing harder, but it cuts the number of API classes related to this advanced feature in half, which I think is a good thing as it doesn't draw as much attention to it, which I think is a good thing because I think only a very small number of consumers will need it, and I don't want it misleading others into thinking this is the typical way of getting a CronetEngine.Builder.
1. Renamed provider names to ROVIDER_NAME_FALLBACK & PROVIDER_NAME_APP_PACKAGED. 2. Got rid of CronetProviders class and moved all the logic to CronetProvider making the getAppProviders() method static. 3. Removed caching of the provider list. 4. Javadoc fixes. https://codereview.chromium.org/2626523003/diff/180001/components/cronet/andr... File components/cronet/android/api/src/org/chromium/net/CronetProviders.java (right): https://codereview.chromium.org/2626523003/diff/180001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetProviders.java:27: * Name of the platform Cronet provider. On 2017/01/23 14:44:28, pauljensen wrote: > On 2017/01/20 21:48:38, kapishnikov wrote: > > On 2017/01/20 17:13:36, pauljensen wrote: > > > I don't understand this. I don't know what "platform" refers to. I think > > this > > > should be moved to CronetProvider. How about: > > > /** > > > * String returned by {@link CronetProvider#getName} for {@link > > CronetProvider} > > > that provides > > > * Cronet implementation based on the system's {@link > > > java.net.HttpURLConnection} implementation. > > > * This implementation offers significantly degraded performance relative to > > > native Cronet > > > * implementations (see {@link #NATIVE_PROVIDER_NAME}). > > > */ > > > public static final String LOW_PERFORMANCE_PROVIDER_NAME = > "Low-Performance"; > > > > Nice. Changed the docs. Regarding the naming, if the providers are named based > > on performance it is not clear how to name other providers. > > You didn't address my comment about "platform". I still don't understand what > "platform" refers to. I still think we need a new name and value for this > variable. We really don't want embedders using this by accident, they'll get > performance worse than Android's HttpURLConnection, and think Cronet is a waste. > > You didn't address my request to move this to CronetProvider. I think it makes > more sense there as it can be beside the getName() function where it is returned > from. Renamed to PROVIDER_NAME_FALLBACK & PROVIDER_NAME_APP_PACKAGED and moved them to CronetProvider. https://codereview.chromium.org/2626523003/diff/240001/components/cronet/andr... File components/cronet/android/api/src/org/chromium/net/CronetEngine.java (right): https://codereview.chromium.org/2626523003/diff/240001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:317: return builder.mBuilderDelegate; On 2017/01/23 14:44:28, pauljensen wrote: > how about combining these two lines together? > return providerList.get(0).createBuilder().mBuilderDelegate; Done. https://codereview.chromium.org/2626523003/diff/240001/components/cronet/andr... File components/cronet/android/api/src/org/chromium/net/CronetProvider.java (right): https://codereview.chromium.org/2626523003/diff/240001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetProvider.java:16: * constructor that accepts a single {@link Context} parameter. On 2017/01/23 14:44:29, pauljensen wrote: > I don't think embedders should generally be extending this class, so I don't > think we need this comment. I'd vote for removing this comment as it detracts > from the very important NOTE below. If someone extends this class they either: > 1. will be calling it directly, in which case they don't care about how "new > CronetEngine.Builder" calls it, or > 2. they'll figure out very quickly how it gets called from "new > CronetEngine.Builder" > If you really really want to keep this comment, can we move it to the > constructor (so it doesn't detract from the very important NOTE? Removed. Also, this class is hidden now. https://codereview.chromium.org/2626523003/diff/240001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetProvider.java:19: * threads, all subclasses of this class should be thread-safe. On 2017/01/23 14:44:28, pauljensen wrote: > I don't think this is necessary. I think APIs are generally supposed to be > thread-safe unless restrictions are explicitly stated and enforced. I'd > recommend removing, but if you really want to keep it, I'd say move it below the > NOTE. I don't want to detract from the fact that nearly all users shouldn't be > interacting with this class. Removed. https://codereview.chromium.org/2626523003/diff/240001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetProvider.java:33: * Creates and returns an instance of {@link CronetEngine.Builder}. On 2017/01/23 14:44:28, pauljensen wrote: > Perhaps we should restate the NOTE here: > This method is for advanced users that want to select a particular Cronet > implementation. Most users should simply use {@code new} {@link > CronetEngine.Builder#CronetEngine.Builder(android.content.Context)}. Done. https://codereview.chromium.org/2626523003/diff/240001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetProvider.java:36: * @throws IllegalStateException if the provider is not enabled. On 2017/01/23 14:44:29, pauljensen wrote: > enabled -> enabled (see {@link #isEnabled} Done. https://codereview.chromium.org/2626523003/diff/240001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetProvider.java:61: * Please read the provider documentation for the instructions how to enable it. On 2017/01/23 14:44:28, pauljensen wrote: > for the instructions how to enable it-> for enablement procedure. Done.
https://codereview.chromium.org/2626523003/diff/260001/components/cronet/andr... File components/cronet/android/api/src/org/chromium/net/CronetProvider.java (right): https://codereview.chromium.org/2626523003/diff/260001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetProvider.java:43: public static final String PROVIDER_NAME_APP_PACKAGED = "App-Packaged-Cronet-Provider"; can we swap this with PROVIDER_NAME_FALLBACK? Seems to better map to priorities and alphabet.
https://codereview.chromium.org/2626523003/diff/260001/components/cronet/andr... File components/cronet/android/api/src/org/chromium/net/CronetProvider.java (right): https://codereview.chromium.org/2626523003/diff/260001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetProvider.java:43: public static final String PROVIDER_NAME_APP_PACKAGED = "App-Packaged-Cronet-Provider"; On 2017/01/23 18:28:57, pauljensen wrote: > can we swap this with PROVIDER_NAME_FALLBACK? Seems to better map to priorities > and alphabet. Done.
LGTM modulo comments. specify BUG=629299 https://codereview.chromium.org/2626523003/diff/260001/components/cronet/andr... File components/cronet/android/api/src/org/chromium/net/CronetEngine.java (right): https://codereview.chromium.org/2626523003/diff/260001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:354: // The platform provider should always be at the end of the list. platform->fallback https://codereview.chromium.org/2626523003/diff/260001/components/cronet/andr... File components/cronet/android/api/src/org/chromium/net/CronetProvider.java (right): https://codereview.chromium.org/2626523003/diff/260001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetProvider.java:39: * that provides native Cronet implementation. This implementation .->packaged inside an application. https://codereview.chromium.org/2626523003/diff/260001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetProvider.java:40: * offers significantly higher performance relative to the system Cronet system->fallback https://codereview.chromium.org/2626523003/diff/260001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetProvider.java:47: * for an alternative implementation. implementation->implementation CronetProvider class name. https://codereview.chromium.org/2626523003/diff/260001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetProvider.java:91: * Returns whether the provider is enabled and can be used to instantiate the Cronet engine. We could mention something like: "A provider being out-of-date (older than the API) and needing updating is one potential reason it could be disabled." https://codereview.chromium.org/2626523003/diff/260001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetProvider.java:204: + className + " that is listed as in the app string resource file under" add space before last quote https://codereview.chromium.org/2626523003/diff/260001/components/cronet/andr... File components/cronet/android/api_version.txt (right): https://codereview.chromium.org/2626523003/diff/260001/components/cronet/andr... components/cronet/android/api_version.txt:1: 4 3? https://codereview.chromium.org/2626523003/diff/260001/components/cronet/andr... File components/cronet/android/cronet_impl_common_proguard.cfg (right): https://codereview.chromium.org/2626523003/diff/260001/components/cronet/andr... components/cronet/android/cronet_impl_common_proguard.cfg:5: -keep class org.chromium.net.impl.ImplVersion Perhaps not the right question for this CL, but what is this for? I don't think this class is referened by reflection or anything not explicit. https://codereview.chromium.org/2626523003/diff/260001/components/cronet/andr... File components/cronet/android/cronet_impl_native_proguard.cfg (right): https://codereview.chromium.org/2626523003/diff/260001/components/cronet/andr... components/cronet/android/cronet_impl_native_proguard.cfg:6: } nit: can we instead do this via @UsedByReflection in the source? https://codereview.chromium.org/2626523003/diff/260001/components/cronet/andr... File components/cronet/android/cronet_impl_platform_proguard.cfg (right): https://codereview.chromium.org/2626523003/diff/260001/components/cronet/andr... components/cronet/android/cronet_impl_platform_proguard.cfg:6: } nit: can we instead do this via @UsedByReflection in the source?
The CQ bit was checked by kapishnikov@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
AFAIK this needs to be committed (i.e. all the way through CQ) by 7pm to get in tonight's build.
https://codereview.chromium.org/2626523003/diff/260001/components/cronet/andr... File components/cronet/android/api/src/org/chromium/net/CronetEngine.java (right): https://codereview.chromium.org/2626523003/diff/260001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:354: // The platform provider should always be at the end of the list. On 2017/01/23 19:42:21, pauljensen wrote: > platform->fallback Done. https://codereview.chromium.org/2626523003/diff/260001/components/cronet/andr... File components/cronet/android/api/src/org/chromium/net/CronetProvider.java (right): https://codereview.chromium.org/2626523003/diff/260001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetProvider.java:39: * that provides native Cronet implementation. This implementation On 2017/01/23 19:42:21, pauljensen wrote: > .->packaged inside an application. Done. https://codereview.chromium.org/2626523003/diff/260001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetProvider.java:40: * offers significantly higher performance relative to the system Cronet On 2017/01/23 19:42:21, pauljensen wrote: > system->fallback Done. https://codereview.chromium.org/2626523003/diff/260001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetProvider.java:47: * for an alternative implementation. On 2017/01/23 19:42:21, pauljensen wrote: > implementation->implementation CronetProvider class name. Changed it to: /** * The name of an optional key in the app string resource file that contains the class name of * an alternative {@code CronetProvider} implementation. */ https://codereview.chromium.org/2626523003/diff/260001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetProvider.java:91: * Returns whether the provider is enabled and can be used to instantiate the Cronet engine. On 2017/01/23 19:42:21, pauljensen wrote: > We could mention something like: "A provider being out-of-date (older than the > API) and needing updating is one potential reason it could be disabled." Done. https://codereview.chromium.org/2626523003/diff/260001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetProvider.java:204: + className + " that is listed as in the app string resource file under" On 2017/01/23 19:42:21, pauljensen wrote: > add space before last quote Done. https://codereview.chromium.org/2626523003/diff/260001/components/cronet/andr... File components/cronet/android/api_version.txt (right): https://codereview.chromium.org/2626523003/diff/260001/components/cronet/andr... components/cronet/android/api_version.txt:1: 4 On 2017/01/23 19:42:21, pauljensen wrote: > 3? Done. https://codereview.chromium.org/2626523003/diff/260001/components/cronet/andr... File components/cronet/android/cronet_impl_common_proguard.cfg (right): https://codereview.chromium.org/2626523003/diff/260001/components/cronet/andr... components/cronet/android/cronet_impl_common_proguard.cfg:5: -keep class org.chromium.net.impl.ImplVersion On 2017/01/23 19:42:21, pauljensen wrote: > Perhaps not the right question for this CL, but what is this for? I don't think > this class is referened by reflection or anything not explicit. That is a good point. Before, it failed to access fields of the class if class/merging/horizontal proguard optimization was enabled. Since this CL added getters and made the fields private, the tests don't fail anymore. I deleted the "keep" but kept the file for the time being. https://codereview.chromium.org/2626523003/diff/260001/components/cronet/andr... File components/cronet/android/cronet_impl_native_proguard.cfg (right): https://codereview.chromium.org/2626523003/diff/260001/components/cronet/andr... components/cronet/android/cronet_impl_native_proguard.cfg:6: } On 2017/01/23 19:42:22, pauljensen wrote: > nit: can we instead do this via @UsedByReflection in the source? Good suggestion. Done. https://codereview.chromium.org/2626523003/diff/260001/components/cronet/andr... File components/cronet/android/cronet_impl_platform_proguard.cfg (right): https://codereview.chromium.org/2626523003/diff/260001/components/cronet/andr... components/cronet/android/cronet_impl_platform_proguard.cfg:6: } On 2017/01/23 19:42:22, pauljensen wrote: > nit: can we instead do this via @UsedByReflection in the source? Only cronet_impl_native_proguard.cfg contains "-keep" for classes annotated by @UsedByReflection. Also, the annotation lives in org.chromium.base.annotations.UsedByReflection and we do not want to introduce that dependency to the Java engine.
On 2017/01/23 21:59:08, pauljensen wrote: > AFAIK this needs to be committed (i.e. all the way through CQ) by 7pm to get in > tonight's build. Good point. Looking.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2626523003/diff/300001/components/cronet/andr... File components/cronet/android/java/src/org/chromium/net/impl/JavaCronetProvider.java (right): https://codereview.chromium.org/2626523003/diff/300001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/impl/JavaCronetProvider.java:16: * for building the Java-base implementation of {@link CronetEngine}. nit: Java-base => Java-based
Submitting. https://codereview.chromium.org/2626523003/diff/300001/components/cronet/andr... File components/cronet/android/java/src/org/chromium/net/impl/JavaCronetProvider.java (right): https://codereview.chromium.org/2626523003/diff/300001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/impl/JavaCronetProvider.java:16: * for building the Java-base implementation of {@link CronetEngine}. On 2017/01/23 22:38:56, mef wrote: > nit: Java-base => Java-based Done.
The CQ bit was checked by kapishnikov@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pauljensen@chromium.org, mef@chromium.org Link to the patchset: https://codereview.chromium.org/2626523003/#ps320001 (title: "Javadoc fixes")
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 kapishnikov@chromium.org
Description was changed from ========== Cronet: a framework to provide alternative Cronet implementations 1. Added CronetImplProvider interface (abstract class) that should be implemented by all Cronet implementation (built-in and alternative). 2. Added CronetImplProviderSelector that is responsible for selecting a Cronet implementation from the list of available implementations. 3. Added internal CronetImplProviderFinder that looks for available Cronet implementations. One place it searches is the app resources that list the alternative implementation. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester ========== to ========== Cronet: a framework to provide alternative Cronet implementations 1. Added CronetProvider interface (abstract class) that can be subclassed in order to add an alternative CronetEngineBuilder implementation. The interface is implemented by the new built-in Java and Native Cronet engine providers. 2. Provided functionality that can be used by an embedder to explicitly select the desired provider from the list of available ones. If the embedder doesn't select one. The system select the the best available one based on the provider versions and types. 3. Deprecated enableLegacyMode(). The embedder can now explicitly choose what implementation to use. 4. Changed the tests to use the new approach. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester ==========
The CQ bit was checked by kapishnikov@chromium.org
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": 320001, "attempt_start_ts": 1485212038094540,
"parent_rev": "702ade7cb3ec7ab79f482008239ae396c6767e22", "commit_rev":
"640977d34b03a64f019b3b9f25fb6e15820914a6"}
Message was sent while issue was closed.
Description was changed from ========== Cronet: a framework to provide alternative Cronet implementations 1. Added CronetProvider interface (abstract class) that can be subclassed in order to add an alternative CronetEngineBuilder implementation. The interface is implemented by the new built-in Java and Native Cronet engine providers. 2. Provided functionality that can be used by an embedder to explicitly select the desired provider from the list of available ones. If the embedder doesn't select one. The system select the the best available one based on the provider versions and types. 3. Deprecated enableLegacyMode(). The embedder can now explicitly choose what implementation to use. 4. Changed the tests to use the new approach. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester ========== to ========== Cronet: a framework to provide alternative Cronet implementations 1. Added CronetProvider interface (abstract class) that can be subclassed in order to add an alternative CronetEngineBuilder implementation. The interface is implemented by the new built-in Java and Native Cronet engine providers. 2. Provided functionality that can be used by an embedder to explicitly select the desired provider from the list of available ones. If the embedder doesn't select one. The system select the the best available one based on the provider versions and types. 3. Deprecated enableLegacyMode(). The embedder can now explicitly choose what implementation to use. 4. Changed the tests to use the new approach. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester Review-Url: https://codereview.chromium.org/2626523003 Cr-Commit-Position: refs/heads/master@{#445561} Committed: https://chromium.googlesource.com/chromium/src/+/640977d34b03a64f019b3b9f25fb... ==========
Message was sent while issue was closed.
Committed patchset #17 (id:320001) as https://chromium.googlesource.com/chromium/src/+/640977d34b03a64f019b3b9f25fb...
Message was sent while issue was closed.
Description was changed from ========== Cronet: a framework to provide alternative Cronet implementations 1. Added CronetProvider interface (abstract class) that can be subclassed in order to add an alternative CronetEngineBuilder implementation. The interface is implemented by the new built-in Java and Native Cronet engine providers. 2. Provided functionality that can be used by an embedder to explicitly select the desired provider from the list of available ones. If the embedder doesn't select one. The system select the the best available one based on the provider versions and types. 3. Deprecated enableLegacyMode(). The embedder can now explicitly choose what implementation to use. 4. Changed the tests to use the new approach. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester Review-Url: https://codereview.chromium.org/2626523003 Cr-Commit-Position: refs/heads/master@{#445561} Committed: https://chromium.googlesource.com/chromium/src/+/640977d34b03a64f019b3b9f25fb... ========== to ========== Cronet: a framework to provide alternative Cronet implementations 1. Added CronetProvider interface (abstract class) that can be subclassed in order to add an alternative CronetEngineBuilder implementation. The interface is implemented by the new built-in Java and Native Cronet engine providers. 2. Provided functionality that can be used by an embedder to explicitly select the desired provider from the list of available ones. If the embedder doesn't select one. The system select the the best available one based on the provider versions and types. 3. Deprecated enableLegacyMode(). The embedder can now explicitly choose what implementation to use. 4. Changed the tests to use the new approach. BUG=686774 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester Review-Url: https://codereview.chromium.org/2626523003 Cr-Commit-Position: refs/heads/master@{#445561} Committed: https://chromium.googlesource.com/chromium/src/+/640977d34b03a64f019b3b9f25fb... ========== |
