Skip to content

Commit 9db3c51

Browse files
axe-fbkelset
authored andcommittedAug 13, 2018
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
1 parent 095b8f0 commit 9db3c51

File tree

5 files changed

+13
-55
lines changed

5 files changed

+13
-55
lines changed
 

‎ReactAndroid/src/main/java/com/facebook/react/NativeModuleRegistryBuilder.java

+1-8
Original file line numberDiff line numberDiff line change
@@ -122,14 +122,7 @@ private void putModuleTypeAndHolderToModuleMaps(
122122
}
123123

124124
public NativeModuleRegistry build() {
125-
ArrayList<ModuleHolder> batchCompleteListenerModules = new ArrayList<>();
126-
for (Map.Entry<String, ModuleHolder> entry : mModules.entrySet()) {
127-
if (entry.getValue().hasOnBatchCompleteListener()) {
128-
batchCompleteListenerModules.add(entry.getValue());
129-
}
130-
}
131-
132125
return new NativeModuleRegistry(
133-
mReactApplicationContext, mModules, batchCompleteListenerModules);
126+
mReactApplicationContext, mModules);
134127
}
135128
}

‎ReactAndroid/src/main/java/com/facebook/react/bridge/ModuleHolder.java

-5
Original file line numberDiff line numberDiff line change
@@ -41,7 +41,6 @@ public class ModuleHolder {
4141
private final boolean mCanOverrideExistingModule;
4242
private final boolean mHasConstants;
4343
private final boolean mIsCxxModule;
44-
private final boolean mHasOnBatchCompleteListener;
4544

4645
private @Nullable Provider<? extends NativeModule> mProvider;
4746
// Outside of the constructur, these should only be checked or set when synchronized on this
@@ -57,7 +56,6 @@ public ModuleHolder(ReactModuleInfo moduleInfo, Provider<? extends NativeModule>
5756
mCanOverrideExistingModule = moduleInfo.canOverrideExistingModule();
5857
mHasConstants = moduleInfo.hasConstants();
5958
mProvider = provider;
60-
mHasOnBatchCompleteListener = moduleInfo.hasOnBatchCompleteListener();
6159
mIsCxxModule = moduleInfo.isCxxModule();
6260
if (moduleInfo.needsEagerInit()) {
6361
mModule = create();
@@ -69,7 +67,6 @@ public ModuleHolder(NativeModule nativeModule) {
6967
mCanOverrideExistingModule = nativeModule.canOverrideExistingModule();
7068
mHasConstants = true;
7169
mIsCxxModule = CxxModuleWrapper.class.isAssignableFrom(nativeModule.getClass());
72-
mHasOnBatchCompleteListener = OnBatchCompleteListener.class.isAssignableFrom(nativeModule.getClass());
7370
mModule = nativeModule;
7471
PrinterHolder.getPrinter()
7572
.logMessage(ReactDebugOverlayTags.NATIVE_MODULE, "NativeModule init: %s", mName);
@@ -121,8 +118,6 @@ public boolean getHasConstants() {
121118

122119
public boolean isCxxModule() {return mIsCxxModule; }
123120

124-
public boolean hasOnBatchCompleteListener() {return mHasOnBatchCompleteListener; }
125-
126121
@DoNotStrip
127122
public NativeModule getModule() {
128123
NativeModule module;

‎ReactAndroid/src/main/java/com/facebook/react/bridge/NativeModuleRegistry.java

+10-20
Original file line numberDiff line numberDiff line change
@@ -7,14 +7,12 @@
77

88
package com.facebook.react.bridge;
99

10+
import com.facebook.infer.annotation.Assertions;
11+
import com.facebook.systrace.Systrace;
1012
import java.util.ArrayList;
1113
import java.util.Collection;
1214
import java.util.List;
1315
import java.util.Map;
14-
import java.util.HashMap;
15-
16-
import com.facebook.infer.annotation.Assertions;
17-
import com.facebook.systrace.Systrace;
1816

1917
/**
2018
* A set of Java APIs to expose to a particular JavaScript instance.
@@ -23,15 +21,12 @@ public class NativeModuleRegistry {
2321

2422
private final ReactApplicationContext mReactApplicationContext;
2523
private final Map<String, ModuleHolder> mModules;
26-
private final ArrayList<ModuleHolder> mBatchCompleteListenerModules;
2724

2825
public NativeModuleRegistry(
2926
ReactApplicationContext reactApplicationContext,
30-
Map<String, ModuleHolder> modules,
31-
ArrayList<ModuleHolder> batchCompleteListenerModules) {
27+
Map<String, ModuleHolder> modules) {
3228
mReactApplicationContext = reactApplicationContext;
3329
mModules = modules;
34-
mBatchCompleteListenerModules = batchCompleteListenerModules;
3530
}
3631

3732
/**
@@ -45,10 +40,6 @@ private ReactApplicationContext getReactApplicationContext() {
4540
return mReactApplicationContext;
4641
}
4742

48-
private ArrayList<ModuleHolder> getBatchCompleteListenerModules() {
49-
return mBatchCompleteListenerModules;
50-
}
51-
5243
/* package */ Collection<JavaModuleWrapper> getJavaModules(
5344
JSInstance jsInstance) {
5445
ArrayList<JavaModuleWrapper> javaModules = new ArrayList<>();
@@ -81,15 +72,11 @@ private ArrayList<ModuleHolder> getBatchCompleteListenerModules() {
8172
"Extending native modules with non-matching application contexts.");
8273

8374
Map<String, ModuleHolder> newModules = newRegister.getModuleMap();
84-
ArrayList<ModuleHolder> batchCompleteListeners = newRegister.getBatchCompleteListenerModules();
8575

8676
for (Map.Entry<String, ModuleHolder> entry : newModules.entrySet()) {
8777
String key = entry.getKey();
8878
if (!mModules.containsKey(key)) {
8979
ModuleHolder value = entry.getValue();
90-
if (batchCompleteListeners.contains(value)) {
91-
mBatchCompleteListenerModules.add(value);
92-
}
9380
mModules.put(key, value);
9481
}
9582
}
@@ -129,10 +116,13 @@ private ArrayList<ModuleHolder> getBatchCompleteListenerModules() {
129116
}
130117

131118
public void onBatchComplete() {
132-
for (ModuleHolder moduleHolder : mBatchCompleteListenerModules) {
133-
if (moduleHolder.hasInstance()) {
134-
((OnBatchCompleteListener) moduleHolder.getModule()).onBatchComplete();
135-
}
119+
// The only native module that uses the onBatchComplete is the UI Manager. Hence, instead of
120+
// iterating over all the modules for find this one instance, and then calling it, we short-circuit
121+
// the search, and simply call OnBatchComplete on the UI Manager.
122+
// With Fabric, UIManager would no longer be a NativeModule, so this call would simply go away
123+
ModuleHolder moduleHolder = mModules.get("com.facebook.react.uimanager.UIManagerModule");
124+
if (moduleHolder != null && moduleHolder.hasInstance()) {
125+
((OnBatchCompleteListener) moduleHolder.getModule()).onBatchComplete();
136126
}
137127
}
138128

‎ReactAndroid/src/main/java/com/facebook/react/module/model/ReactModuleInfo.java

+1-7
Original file line numberDiff line numberDiff line change
@@ -16,21 +16,18 @@ public class ReactModuleInfo {
1616
private final boolean mNeedsEagerInit;
1717
private final boolean mHasConstants;
1818
private final boolean mIsCxxModule;
19-
private final boolean mHasOnBatchCompleteListener;
2019

2120
public ReactModuleInfo(
2221
String name,
2322
boolean canOverrideExistingModule,
2423
boolean needsEagerInit,
2524
boolean hasConstants,
26-
boolean isCxxModule,
27-
boolean hasOnBatchCompleteListener) {
25+
boolean isCxxModule) {
2826
mName = name;
2927
mCanOverrideExistingModule = canOverrideExistingModule;
3028
mNeedsEagerInit = needsEagerInit;
3129
mHasConstants = hasConstants;
3230
mIsCxxModule = isCxxModule;
33-
mHasOnBatchCompleteListener = hasOnBatchCompleteListener;
3431
}
3532

3633
public String name() {
@@ -51,7 +48,4 @@ public boolean hasConstants() {
5148

5249
public boolean isCxxModule() {return mIsCxxModule; }
5350

54-
public boolean hasOnBatchCompleteListener() {
55-
return mHasOnBatchCompleteListener;
56-
}
5751
}

‎ReactAndroid/src/main/java/com/facebook/react/module/processing/ReactModuleSpecProcessor.java

+1-15
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,6 @@
66
package com.facebook.react.module.processing;
77

88
import com.facebook.react.bridge.CxxModuleWrapper;
9-
import com.facebook.react.bridge.OnBatchCompleteListener;
109
import javax.annotation.processing.AbstractProcessor;
1110
import javax.annotation.processing.Filer;
1211
import javax.annotation.processing.Messager;
@@ -160,7 +159,6 @@ private CodeBlock getCodeBlockForReactModuleInfos(List<String> nativeModules)
160159
builder.addStatement("$T map = new $T()", MAP_TYPE, INSTANTIATED_MAP_TYPE);
161160

162161
TypeMirror cxxModuleWrapperTypeMirror = mElements.getTypeElement(CxxModuleWrapper.class.getName()).asType();
163-
TypeMirror onBatchCompleteListenerTypeMirror = mElements.getTypeElement(OnBatchCompleteListener.class.getName()).asType();
164162

165163
for (String nativeModule : nativeModules) {
166164
String keyString = nativeModule;
@@ -192,26 +190,14 @@ private CodeBlock getCodeBlockForReactModuleInfos(List<String> nativeModules)
192190
}
193191

194192
boolean isCxxModule = mTypes.isAssignable(typeElement.asType(), cxxModuleWrapperTypeMirror);
195-
boolean hasOnBatchCompleteListener = false;
196-
try {
197-
hasOnBatchCompleteListener = mTypes.isAssignable(typeElement.asType(), onBatchCompleteListenerTypeMirror);
198-
} catch (RuntimeException e) {
199-
// This is SUPER ugly, but we need to do this, especially for AsyncStorageModule which implements ModuleDataCleaner
200-
// In the case of that specific class, we get the exception
201-
// com.sun.tools.javac.code.Symbol$CompletionFailure: class file for ModuleDataCleaner not found.
202-
// The exception is caused because the class is not loaded the first time. However, catching it and
203-
// running it again the second time loads the class and does what the following statement originally intended
204-
hasOnBatchCompleteListener = mTypes.isAssignable(typeElement.asType(), onBatchCompleteListenerTypeMirror);
205-
}
206193

207194
String valueString = new StringBuilder()
208195
.append("new ReactModuleInfo(")
209196
.append("\"").append(reactModule.name()).append("\"").append(", ")
210197
.append(reactModule.canOverrideExistingModule()).append(", ")
211198
.append(reactModule.needsEagerInit()).append(", ")
212199
.append(hasConstants).append(", ")
213-
.append(isCxxModule).append(", ")
214-
.append(hasOnBatchCompleteListener)
200+
.append(isCxxModule)
215201
.append(")")
216202
.toString();
217203

0 commit comments

Comments
 (0)
Please sign in to comment.