From 59137c367618b66ebda04170cb058343a6705d63 Mon Sep 17 00:00:00 2001 From: Thote Date: Sun, 21 May 2017 16:28:00 +0530 Subject: [PATCH] Incorporate review feedback points --- .../network/DefaultOkHttpProvider.java | 16 +++++++-------- .../modules/network/NetworkingModule.java | 20 +++++++++---------- .../modules/network/OkHttpClientProvider.java | 2 +- .../react/shell/MainPackageConfig.java | 17 ++++++++-------- .../react/shell/MainReactPackage.java | 8 ++++++-- .../modules/network/NetworkingModuleTest.java | 1 - 6 files changed, 34 insertions(+), 30 deletions(-) diff --git a/ReactAndroid/src/main/java/com/facebook/react/modules/network/DefaultOkHttpProvider.java b/ReactAndroid/src/main/java/com/facebook/react/modules/network/DefaultOkHttpProvider.java index 6d84ceb0ea3af7..aa2fa230ce7e06 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/modules/network/DefaultOkHttpProvider.java +++ b/ReactAndroid/src/main/java/com/facebook/react/modules/network/DefaultOkHttpProvider.java @@ -12,6 +12,7 @@ import android.os.Build; import com.facebook.common.logging.FLog; +import com.facebook.react.common.ReactConstants; import java.util.ArrayList; import java.util.List; @@ -34,12 +35,12 @@ public OkHttpClient get() { return enableTls12OnPreLollipop(client).build(); } - /* - On Android 4.1-4.4 (API level 16 to 19) TLS 1.1 and 1.2 are - available but not enabled by default. The following method - enables it. + /** + * On Android 4.1-4.4 (API level 16 to 19) TLS 1.1 and 1.2 are + * available but not enabled by default. The following method + * enables it. */ - public static OkHttpClient.Builder enableTls12OnPreLollipop(OkHttpClient.Builder client) { + private static OkHttpClient.Builder enableTls12OnPreLollipop(OkHttpClient.Builder client) { if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.JELLY_BEAN && Build.VERSION.SDK_INT <= Build.VERSION_CODES.KITKAT) { try { client.sslSocketFactory(new TLSSocketFactory()); @@ -54,11 +55,10 @@ public static OkHttpClient.Builder enableTls12OnPreLollipop(OkHttpClient.Builder specs.add(ConnectionSpec.CLEARTEXT); client.connectionSpecs(specs); - } catch (Exception exc) { - FLog.e("OkHttpClientProvider", "Error while enabling TLS 1.2", exc); + } catch (Exception e) { + FLog.e(ReactConstants.TAG, "Error while enabling TLS 1.2", e); } } - return client; } } diff --git a/ReactAndroid/src/main/java/com/facebook/react/modules/network/NetworkingModule.java b/ReactAndroid/src/main/java/com/facebook/react/modules/network/NetworkingModule.java index e8f4ef6795ecdb..63e879e04976f0 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/modules/network/NetworkingModule.java +++ b/ReactAndroid/src/main/java/com/facebook/react/modules/network/NetworkingModule.java @@ -76,11 +76,11 @@ public final class NetworkingModule extends ReactContextBaseJavaModule { /* package */ NetworkingModule( ReactApplicationContext reactContext, @Nullable String defaultUserAgent, - OkHttpClientProvider okHttpClientProvider, + OkHttpClientProvider httpClientProvider, @Nullable List networkInterceptorCreators) { super(reactContext); - mClient = createOkHttpClient(okHttpClientProvider, networkInterceptorCreators); + mClient = createOkHttpClient(httpClientProvider, networkInterceptorCreators); mCookieHandler = new ForwardingCookieHandler(reactContext); mCookieJarContainer = (CookieJarContainer) mClient.cookieJar(); mShuttingDown = false; @@ -89,9 +89,9 @@ public final class NetworkingModule extends ReactContextBaseJavaModule { } private OkHttpClient createOkHttpClient( - OkHttpClientProvider okHttpClientProvider, + @Nullable OkHttpClientProvider httpClientProvider, @Nullable List networkInterceptorCreators) { - OkHttpClient client = okHttpClientProvider == null ? new DefaultOkHttpProvider().get() : okHttpClientProvider.get(); + OkHttpClient client = httpClientProvider == null ? new DefaultOkHttpProvider().get() : httpClientProvider.get(); if (networkInterceptorCreators != null) { OkHttpClient.Builder clientBuilder = client.newBuilder(); for (NetworkInterceptorCreator networkInterceptorCreator : networkInterceptorCreators) { @@ -106,13 +106,13 @@ private OkHttpClient createOkHttpClient( * @param context the ReactContext of the application * @param defaultUserAgent the User-Agent header that will be set for all requests where the * caller does not provide one explicitly - * @param okHttpClientProvider the {@link OkHttpClient} provider to be used for networking + * @param httpClientProvider {@link OkHttpClientProvider} to provider {@link OkHttpClient} */ /* package */ NetworkingModule( ReactApplicationContext context, @Nullable String defaultUserAgent, - OkHttpClientProvider okHttpClientProvider) { - this(context, defaultUserAgent, okHttpClientProvider, null); + @Nullable OkHttpClientProvider httpClientProvider) { + this(context, defaultUserAgent, httpClientProvider, null); } /** @@ -124,10 +124,10 @@ public NetworkingModule(final ReactApplicationContext context) { /** * @param context the ReactContext of the application - * @param okHttpClientProvider to provide OkHttpInstance with different settings + * @param httpClientProvider {@link OkHttpClientProvider} to provide {@link OkHttpClient} */ - public NetworkingModule(final ReactApplicationContext context, OkHttpClientProvider okHttpClientProvider) { - this(context, null, okHttpClientProvider, null); + public NetworkingModule(final ReactApplicationContext context, @Nullable OkHttpClientProvider httpClientProvider) { + this(context, null, httpClientProvider, null); } /** diff --git a/ReactAndroid/src/main/java/com/facebook/react/modules/network/OkHttpClientProvider.java b/ReactAndroid/src/main/java/com/facebook/react/modules/network/OkHttpClientProvider.java index 97aa13e9cf5123..5c8b3012deb5f9 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/modules/network/OkHttpClientProvider.java +++ b/ReactAndroid/src/main/java/com/facebook/react/modules/network/OkHttpClientProvider.java @@ -12,7 +12,7 @@ import okhttp3.OkHttpClient; /** - * Interface that provides the OkHttpClient instance, that will be used for all networking requests. + * Interface to provide OkHttpClient instance that will be used for all networking requests. */ public interface OkHttpClientProvider { OkHttpClient get(); diff --git a/ReactAndroid/src/main/java/com/facebook/react/shell/MainPackageConfig.java b/ReactAndroid/src/main/java/com/facebook/react/shell/MainPackageConfig.java index f5b84e70f231c9..666bdb8770dfcc 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/shell/MainPackageConfig.java +++ b/ReactAndroid/src/main/java/com/facebook/react/shell/MainPackageConfig.java @@ -13,19 +13,20 @@ import com.facebook.react.modules.network.DefaultOkHttpProvider; import com.facebook.react.modules.network.OkHttpClientProvider; +import javax.annotation.Nullable; + /** * Configuration for {@link MainReactPackage} */ public class MainPackageConfig { - - private final OkHttpClientProvider mOkHttpClientProvider; + private final @Nullable OkHttpClientProvider mHttpClientProvider; private final ImagePipelineConfig mFrescoConfig; private MainPackageConfig(Builder builder) { mFrescoConfig = builder.mFrescoConfig; - mOkHttpClientProvider = builder.mOkHttpClientProvider; + mHttpClientProvider = builder.mHttpClientProvider; } public ImagePipelineConfig getFrescoConfig() { @@ -33,13 +34,13 @@ public ImagePipelineConfig getFrescoConfig() { } public OkHttpClientProvider getOkHttpClientProvider() { - return mOkHttpClientProvider != null ? mOkHttpClientProvider : new DefaultOkHttpProvider(); + return mHttpClientProvider != null ? mHttpClientProvider : new DefaultOkHttpProvider(); } public static class Builder { private ImagePipelineConfig mFrescoConfig; - private OkHttpClientProvider mOkHttpClientProvider; + private OkHttpClientProvider mHttpClientProvider; public Builder setFrescoConfig(ImagePipelineConfig frescoConfig) { mFrescoConfig = frescoConfig; @@ -47,11 +48,11 @@ public Builder setFrescoConfig(ImagePipelineConfig frescoConfig) { } /** - * Ability to inject configured OkHttpClient provider to react-native networking subsystem. - * This will be very useful, especially in brownfield applications. + * Option to inject configured OkHttpClient to react-native networking subsystem. + * This will be used only by the fetch networking calls (Fresco library, for eg, will not use this client). */ public Builder setOkHttpClientProvider(OkHttpClientProvider okHttpClientProvider) { - this.mOkHttpClientProvider = okHttpClientProvider; + this.mHttpClientProvider = okHttpClientProvider; return this; } diff --git a/ReactAndroid/src/main/java/com/facebook/react/shell/MainReactPackage.java b/ReactAndroid/src/main/java/com/facebook/react/shell/MainReactPackage.java index 691b370cdaebe7..40c3eea9f539fc 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/shell/MainReactPackage.java +++ b/ReactAndroid/src/main/java/com/facebook/react/shell/MainReactPackage.java @@ -11,6 +11,7 @@ import android.content.SharedPreferences; import android.preference.PreferenceManager; + import com.facebook.react.LazyReactPackage; import com.facebook.react.animated.NativeAnimatedModule; import com.facebook.react.bridge.JavaScriptModule; @@ -75,12 +76,13 @@ import com.facebook.react.views.viewpager.ReactViewPagerManager; import com.facebook.react.views.webview.ReactWebViewManager; -import javax.inject.Provider; import java.util.ArrayList; import java.util.Arrays; import java.util.Collections; import java.util.List; +import javax.inject.Provider; + /** * Package defining basic modules and view managers. */ @@ -194,7 +196,9 @@ public NativeModule get() { new ModuleSpec(NetworkingModule.class, new Provider() { @Override public NativeModule get() { - return mConfig == null ? new NetworkingModule(context) : new NetworkingModule(context, mConfig.getOkHttpClientProvider()); + return mConfig == null ? + new NetworkingModule(context) : + new NetworkingModule(context, mConfig.getOkHttpClientProvider()); } }), new ModuleSpec(NetInfoModule.class, new Provider() { diff --git a/ReactAndroid/src/test/java/com/facebook/react/modules/network/NetworkingModuleTest.java b/ReactAndroid/src/test/java/com/facebook/react/modules/network/NetworkingModuleTest.java index 1756d589a0f36a..8924d200ec3548 100644 --- a/ReactAndroid/src/test/java/com/facebook/react/modules/network/NetworkingModuleTest.java +++ b/ReactAndroid/src/test/java/com/facebook/react/modules/network/NetworkingModuleTest.java @@ -78,7 +78,6 @@ public class NetworkingModuleTest { @Test public void testGetWithoutHeaders() throws Exception { OkHttpClient httpClient = mock(OkHttpClient.class); - when(httpClient.newCall(any(Request.class))).thenAnswer(new Answer() { @Override public Object answer(InvocationOnMock invocation) throws Throwable {