From 9db3c51d11d3d4d696126de1366160586e66552c Mon Sep 17 00:00:00 2001 From: Ram N Date: Wed, 8 Aug 2018 14:03:14 -0700 Subject: [PATCH] Explicitly make UIManagerModule use OnBatchComplete on Android Summary: Currently, we scan all native modules to see if they implement the OnBatchCompleteListerner. If they do, we add those modules to a list, and when C++ calls OnBactchComplete is called, we execute the callback on each of the modules. The only native module implementing this callback today is the UIManagerModule. With Fabric, UIManager will also not be a native module anymore. This diff removes all the work done for creating the list and assumes that UIManagerModule is the only place that is interested in OnBatchComplete call - and calls it directly. Reviewed By: achen1 Differential Revision: D9186651 fbshipit-source-id: 473586b37c2465ccd041985dcdd56132026f34f1 --- .../react/NativeModuleRegistryBuilder.java | 9 +----- .../facebook/react/bridge/ModuleHolder.java | 5 ---- .../react/bridge/NativeModuleRegistry.java | 30 +++++++------------ .../react/module/model/ReactModuleInfo.java | 8 +---- .../processing/ReactModuleSpecProcessor.java | 16 +--------- 5 files changed, 13 insertions(+), 55 deletions(-) diff --git a/ReactAndroid/src/main/java/com/facebook/react/NativeModuleRegistryBuilder.java b/ReactAndroid/src/main/java/com/facebook/react/NativeModuleRegistryBuilder.java index b8898dc10b993a..ba107f1a638dd2 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/NativeModuleRegistryBuilder.java +++ b/ReactAndroid/src/main/java/com/facebook/react/NativeModuleRegistryBuilder.java @@ -122,14 +122,7 @@ private void putModuleTypeAndHolderToModuleMaps( } public NativeModuleRegistry build() { - ArrayList batchCompleteListenerModules = new ArrayList<>(); - for (Map.Entry entry : mModules.entrySet()) { - if (entry.getValue().hasOnBatchCompleteListener()) { - batchCompleteListenerModules.add(entry.getValue()); - } - } - return new NativeModuleRegistry( - mReactApplicationContext, mModules, batchCompleteListenerModules); + mReactApplicationContext, mModules); } } diff --git a/ReactAndroid/src/main/java/com/facebook/react/bridge/ModuleHolder.java b/ReactAndroid/src/main/java/com/facebook/react/bridge/ModuleHolder.java index f1b67e7a81ef7e..4693775435b22f 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/bridge/ModuleHolder.java +++ b/ReactAndroid/src/main/java/com/facebook/react/bridge/ModuleHolder.java @@ -41,7 +41,6 @@ public class ModuleHolder { private final boolean mCanOverrideExistingModule; private final boolean mHasConstants; private final boolean mIsCxxModule; - private final boolean mHasOnBatchCompleteListener; private @Nullable Provider mProvider; // Outside of the constructur, these should only be checked or set when synchronized on this @@ -57,7 +56,6 @@ public ModuleHolder(ReactModuleInfo moduleInfo, Provider mCanOverrideExistingModule = moduleInfo.canOverrideExistingModule(); mHasConstants = moduleInfo.hasConstants(); mProvider = provider; - mHasOnBatchCompleteListener = moduleInfo.hasOnBatchCompleteListener(); mIsCxxModule = moduleInfo.isCxxModule(); if (moduleInfo.needsEagerInit()) { mModule = create(); @@ -69,7 +67,6 @@ public ModuleHolder(NativeModule nativeModule) { mCanOverrideExistingModule = nativeModule.canOverrideExistingModule(); mHasConstants = true; mIsCxxModule = CxxModuleWrapper.class.isAssignableFrom(nativeModule.getClass()); - mHasOnBatchCompleteListener = OnBatchCompleteListener.class.isAssignableFrom(nativeModule.getClass()); mModule = nativeModule; PrinterHolder.getPrinter() .logMessage(ReactDebugOverlayTags.NATIVE_MODULE, "NativeModule init: %s", mName); @@ -121,8 +118,6 @@ public boolean getHasConstants() { public boolean isCxxModule() {return mIsCxxModule; } - public boolean hasOnBatchCompleteListener() {return mHasOnBatchCompleteListener; } - @DoNotStrip public NativeModule getModule() { NativeModule module; diff --git a/ReactAndroid/src/main/java/com/facebook/react/bridge/NativeModuleRegistry.java b/ReactAndroid/src/main/java/com/facebook/react/bridge/NativeModuleRegistry.java index ffd2e8ecfd63fe..f13f88fbb90915 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/bridge/NativeModuleRegistry.java +++ b/ReactAndroid/src/main/java/com/facebook/react/bridge/NativeModuleRegistry.java @@ -7,14 +7,12 @@ package com.facebook.react.bridge; +import com.facebook.infer.annotation.Assertions; +import com.facebook.systrace.Systrace; import java.util.ArrayList; import java.util.Collection; import java.util.List; import java.util.Map; -import java.util.HashMap; - -import com.facebook.infer.annotation.Assertions; -import com.facebook.systrace.Systrace; /** * A set of Java APIs to expose to a particular JavaScript instance. @@ -23,15 +21,12 @@ public class NativeModuleRegistry { private final ReactApplicationContext mReactApplicationContext; private final Map mModules; - private final ArrayList mBatchCompleteListenerModules; public NativeModuleRegistry( ReactApplicationContext reactApplicationContext, - Map modules, - ArrayList batchCompleteListenerModules) { + Map modules) { mReactApplicationContext = reactApplicationContext; mModules = modules; - mBatchCompleteListenerModules = batchCompleteListenerModules; } /** @@ -45,10 +40,6 @@ private ReactApplicationContext getReactApplicationContext() { return mReactApplicationContext; } - private ArrayList getBatchCompleteListenerModules() { - return mBatchCompleteListenerModules; - } - /* package */ Collection getJavaModules( JSInstance jsInstance) { ArrayList javaModules = new ArrayList<>(); @@ -81,15 +72,11 @@ private ArrayList getBatchCompleteListenerModules() { "Extending native modules with non-matching application contexts."); Map newModules = newRegister.getModuleMap(); - ArrayList batchCompleteListeners = newRegister.getBatchCompleteListenerModules(); for (Map.Entry entry : newModules.entrySet()) { String key = entry.getKey(); if (!mModules.containsKey(key)) { ModuleHolder value = entry.getValue(); - if (batchCompleteListeners.contains(value)) { - mBatchCompleteListenerModules.add(value); - } mModules.put(key, value); } } @@ -129,10 +116,13 @@ private ArrayList getBatchCompleteListenerModules() { } public void onBatchComplete() { - for (ModuleHolder moduleHolder : mBatchCompleteListenerModules) { - if (moduleHolder.hasInstance()) { - ((OnBatchCompleteListener) moduleHolder.getModule()).onBatchComplete(); - } + // The only native module that uses the onBatchComplete is the UI Manager. Hence, instead of + // iterating over all the modules for find this one instance, and then calling it, we short-circuit + // the search, and simply call OnBatchComplete on the UI Manager. + // With Fabric, UIManager would no longer be a NativeModule, so this call would simply go away + ModuleHolder moduleHolder = mModules.get("com.facebook.react.uimanager.UIManagerModule"); + if (moduleHolder != null && moduleHolder.hasInstance()) { + ((OnBatchCompleteListener) moduleHolder.getModule()).onBatchComplete(); } } diff --git a/ReactAndroid/src/main/java/com/facebook/react/module/model/ReactModuleInfo.java b/ReactAndroid/src/main/java/com/facebook/react/module/model/ReactModuleInfo.java index 5cafdc44edd37f..58b416df3a4c7e 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/module/model/ReactModuleInfo.java +++ b/ReactAndroid/src/main/java/com/facebook/react/module/model/ReactModuleInfo.java @@ -16,21 +16,18 @@ public class ReactModuleInfo { private final boolean mNeedsEagerInit; private final boolean mHasConstants; private final boolean mIsCxxModule; - private final boolean mHasOnBatchCompleteListener; public ReactModuleInfo( String name, boolean canOverrideExistingModule, boolean needsEagerInit, boolean hasConstants, - boolean isCxxModule, - boolean hasOnBatchCompleteListener) { + boolean isCxxModule) { mName = name; mCanOverrideExistingModule = canOverrideExistingModule; mNeedsEagerInit = needsEagerInit; mHasConstants = hasConstants; mIsCxxModule = isCxxModule; - mHasOnBatchCompleteListener = hasOnBatchCompleteListener; } public String name() { @@ -51,7 +48,4 @@ public boolean hasConstants() { public boolean isCxxModule() {return mIsCxxModule; } - public boolean hasOnBatchCompleteListener() { - return mHasOnBatchCompleteListener; - } } diff --git a/ReactAndroid/src/main/java/com/facebook/react/module/processing/ReactModuleSpecProcessor.java b/ReactAndroid/src/main/java/com/facebook/react/module/processing/ReactModuleSpecProcessor.java index ab5dbe85e96e05..7137e3944dd240 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/module/processing/ReactModuleSpecProcessor.java +++ b/ReactAndroid/src/main/java/com/facebook/react/module/processing/ReactModuleSpecProcessor.java @@ -6,7 +6,6 @@ package com.facebook.react.module.processing; import com.facebook.react.bridge.CxxModuleWrapper; -import com.facebook.react.bridge.OnBatchCompleteListener; import javax.annotation.processing.AbstractProcessor; import javax.annotation.processing.Filer; import javax.annotation.processing.Messager; @@ -160,7 +159,6 @@ private CodeBlock getCodeBlockForReactModuleInfos(List nativeModules) builder.addStatement("$T map = new $T()", MAP_TYPE, INSTANTIATED_MAP_TYPE); TypeMirror cxxModuleWrapperTypeMirror = mElements.getTypeElement(CxxModuleWrapper.class.getName()).asType(); - TypeMirror onBatchCompleteListenerTypeMirror = mElements.getTypeElement(OnBatchCompleteListener.class.getName()).asType(); for (String nativeModule : nativeModules) { String keyString = nativeModule; @@ -192,17 +190,6 @@ private CodeBlock getCodeBlockForReactModuleInfos(List nativeModules) } boolean isCxxModule = mTypes.isAssignable(typeElement.asType(), cxxModuleWrapperTypeMirror); - boolean hasOnBatchCompleteListener = false; - try { - hasOnBatchCompleteListener = mTypes.isAssignable(typeElement.asType(), onBatchCompleteListenerTypeMirror); - } catch (RuntimeException e) { - // This is SUPER ugly, but we need to do this, especially for AsyncStorageModule which implements ModuleDataCleaner - // In the case of that specific class, we get the exception - // com.sun.tools.javac.code.Symbol$CompletionFailure: class file for ModuleDataCleaner not found. - // The exception is caused because the class is not loaded the first time. However, catching it and - // running it again the second time loads the class and does what the following statement originally intended - hasOnBatchCompleteListener = mTypes.isAssignable(typeElement.asType(), onBatchCompleteListenerTypeMirror); - } String valueString = new StringBuilder() .append("new ReactModuleInfo(") @@ -210,8 +197,7 @@ private CodeBlock getCodeBlockForReactModuleInfos(List nativeModules) .append(reactModule.canOverrideExistingModule()).append(", ") .append(reactModule.needsEagerInit()).append(", ") .append(hasConstants).append(", ") - .append(isCxxModule).append(", ") - .append(hasOnBatchCompleteListener) + .append(isCxxModule) .append(")") .toString();