[android] add support to reuse instance with webViewKey #11

nealmanaktola · 2022-08-29T21:45:30Z

Adds support to re-use the same native webview

this adds basic support to re-use the native webview on android with a webViewKey
renames old RNCWebView to InternalWebView and the name for the wrapper class is RNCWebView
implementation is fairly similar to iOS

android-reuse-webview.mov

android/src/main/java/com/reactnativecommunity/webview/RNCWebViewManager.java

android/src/main/java/com/reactnativecommunity/webview/RNCWebView.java

nealmanaktola · 2022-08-29T21:49:13Z

android/src/main/java/com/reactnativecommunity/webview/RNCWebViewManager.java

+  }
+
+  @ReactProp(name = "webViewKey")
+  public void setWebViewKey(RNCWebView view, String webViewKey) {


unlike iOS, where there is a didMoveToWindow function, if there is a webViewKey, we rettach the view at this point

android/src/main/java/com/reactnativecommunity/webview/RNCWebViewManager.java

android/src/main/java/com/reactnativecommunity/webview/RNCWebView.java

android/src/main/java/com/reactnativecommunity/webview/RNCWebViewManager.java

android/src/main/java/com/reactnativecommunity/webview/RNCWebViewMapManager.kt

donaldchen · 2022-08-30T00:17:33Z

android/src/main/java/com/reactnativecommunity/webview/RNCWebViewManager.java

+    if (rncWebViewMap.containsKey(webViewKey)) {
+      RNCWebView existingView = rncWebViewMap.get(webViewKey);
+
+      InternalWebView existingWebView = existingView.detachWebView();


https://github.com/discord/react-native-webview/blob/11.18.1-discord-1/apple/RNCWebViewMapManager.m
https://github.com/discord/react-native-webview/blob/11.18.1-discord-1/apple/RNCWKWebViewMapManager.m

In the iOS implementation, we have a map for both the outer view group RNCWebView and also the inner WebView WKWebView. If I'm remembering correctly, depending on how the JS for the react components was implemented, it was possible for the old RNCWebView to get destroyed before the new one got created. In that case, we wouldn't be able to use the old RNCWebView to get the old InternalWebView via detachWebView. Instead, we would have to grab the old InternalWebView / WKWebView from a singleton map.

Is that possible on Android, too? In the test app, I think you can test this with the "toggle pages" button. That should let you test destroying the first RNCWebVIew without immediately getting a second RNCWebView. Then when you navigate away from the web view page to a different page and then back to the web view page, you should be able to still restore the old InternalWebView. For that, you might need a map for InternalWebView, but let me know what you find.

In the test app, I think you can test this with the "toggle pages" button. That should let you test destroying the first RNCWebVIew without immediately getting a second RNCWebView. Then when you navigate away from the web view page to a different page and then back to the web view page, you should be able to still restore the old InternalWebView. For that, you might need a map for InternalWebView, but let me know what you find.

I am able to restore the old InternalWebView with Toggle Pages.

if I'm remembering correctly, depending on how the JS for the react components was implemented, it was possible for the old RNCWebView to get destroyed before the new one got created.

I am not sure how this is possible because this method is called
https://github.com/discord/react-native-webview/pull/11/files/de8f049d35b3911134285a514add9418eceec7ff#diff-e9700631fc9fa5600d2e1ee70eb2d28aa2a1fbc695b85f99981972d3904ca485R807
when the view is being destroyed. We are also maintaining a reference to this view inside the hashmap so it shouldn't be destroyed

I'll double check on iOS

We are also maintaining a reference to this view inside the hashmap so it shouldn't be destroyed

I guess I was thinking that when the react component unmounts, we wouldn't hold onto the wrapper / RNCWebView, and instead, we'd only hold onto the InternalWebView / WKWebView. That way, we'd be holding on to only what's necessary without using too much memory (since it's fine if the RNCWebView gets destroyed when the react component unmounts as long as the InternalWebView stays alive).

In this code in the iOS RNCWebView.m,

- (void)removeFromSuperview { bool keepWebViewInstance = _keepWebViewInstanceAfterUnmount && _webViewKey != nil; if (!keepWebViewInstance) { [self cleanUpWebView]; } if (_webViewKey != nil) { NSMutableDictionary *sharedRNCWebViewDictionary= [[RNCWebViewMapManager sharedManager] sharedRNCWebViewDictionary]; RNCWebView *rncWebView = sharedRNCWebViewDictionary[_webViewKey]; // When this view is being unmounted, only remove the WKWebView from the superview // if this RNCWebView is the "active" view. if (rncWebView == self) { [self removeWKWebViewFromSuperView:self]; } } [super removeFromSuperview]; }

, we remove the RNCWebView from the map when the react component unmounts. I think that part is different from the current Android implementation because it seems like we're keeping the Android RNCWebView in the map when the component unmounts.

I'll double check on iOS

Sounds good! Let me know if you find anything different from what I described above

That way, we'd be holding on to only what's necessary without using too much memory (since it's fine if the RNCWebView gets destroyed when the react component unmounts as long as the InternalWebView stays alive).

I think the added memory here is marginal since it can be at most one wrapper view for each webViewKey. The wrapper view is also a single FrameLayout and I don't expect to see any reasonable performance gains by using another map.

I think the bigger question is consistency between iOS and Android - if there is no real need to maintain another map, I think it's easier to to maintain one map. iOS is slightly different in the sense that the RNCWebView is more complex so it might justify another map even though it may not be technically required.

I think the bigger question is consistency between iOS and Android - if there is no real need to maintain another map, I think it's easier to to maintain one map. iOS is slightly different in the sense that the RNCWebView is more complex so it might justify another map even though it may not be technically required.

Makes sense. The consistency is valuable to me to make it easier for us to apply similar changes to both platforms. I'm leaning toward keeping the iOS and Android implementations more consistent. I think the easiest way to do that today would be to make Android have two maps (to match the current iOS implementation): one for the outer webview and one for the inner webview. Alternatively, I'm also open to us making iOS only have one map to match what this Android PR is doing if that doesn't create other issues on iOS.

What do you think?

I agree for consistency, given that iOS is somewhat battle-tested I'd leave that alone for now. I'll update Android and leave a comment that this is to remain consistent with iOS. We can do a pass later to remove it from both at some point (if needed?)

Sounds good!

android/src/main/java/com/reactnativecommunity/webview/RNCWebView.java

android/src/main/java/com/reactnativecommunity/webview/RNCWebViewManager.java

nealmanaktola · 2022-08-30T22:38:09Z

android/src/main/java/com/reactnativecommunity/webview/RNCWebView.java

+    if (internalWebView != null) {
+      action.apply(internalWebView);
+    } else {
+      FLog.e(TAG, new Throwable(), "Internal WebView is null");


I've added an empty throwable to get the stacktrace - not sure if this is the best way. I'd like the stacktrace so I can see what method was originally being called

This seems appropriate to me

nealmanaktola · 2022-08-30T22:39:21Z

android/src/main/java/com/reactnativecommunity/webview/RNCWebViewMapManager.kt

+
+object RNCWebViewMapManager {
+  val rncWebViewMap = mutableMapOf<String, RNCWebView>()
+  val internalWebViewMap = mutableMapOf<String, WebView>()


I've used the type WebView since Kotlin was complaining that the InternalWebView is a protected class and shouldn't be referenced here

nealmanaktola · 2022-08-30T22:43:27Z

@donaldchen I think most of your comments have been addressed - let me know if you find something new!

donaldchen · 2022-08-30T23:46:19Z

android/src/main/java/com/reactnativecommunity/webview/RNCWebView.java

+    if (internalWebView != null) {
+      action.apply(internalWebView);
+    } else {
+      FLog.e(TAG, new Throwable(), "Internal WebView is null");


This seems appropriate to me

donaldchen · 2022-08-30T23:52:41Z

android/src/main/java/com/reactnativecommunity/webview/RNCWebViewManager.java

+    super.onDropViewInstance(view);
+
+    // The internal webview can be null since the view may have been already reattached
+    if (view.getWebView() == null) {


nit: Is this null-check and early-return necessary? view.ifHasInternalWebView already does the null-check and then does nothing if it's null, right?

It's not necessary, it's more for explicitness i guess. if it's confusing I can remove it

Either way works for me! I'll leave it up to your preference

android/src/main/java/com/reactnativecommunity/webview/RNCWebViewModule.java

donaldchen

Awesome work! The high level changes look good to me 🎉 .

Before merging, it's worth double checking these small details:
#11 (comment)
and
#11 (comment)

This PR adds messaging passing. This was much simpler to do than the iOS counter part. This is the analogue to the following PR: #6. I've followed the same test plan. The sample app was really nice to use when testing. In the previous PR #11, I broke all native->react-native communication since events were being passed by the `InternalWebView` viewId not the `RNCWebView` viewId. This is now fixed.

…bView in view hierarchy * add support to reuse instance with webViewKey * change up * address comments related to layout params * add nullability checks * fix * get full stack trace * add secondary map * rename to ifHasInternalWebView * additional fixes * fix * fix boolean [android] add messaging passing without a react tag (#12) This PR adds messaging passing. This was much simpler to do than the iOS counter part. This is the analogue to the following PR: #6. I've followed the same test plan. The sample app was really nice to use when testing. In the previous PR #11, I broke all native->react-native communication since events were being passed by the `InternalWebView` viewId not the `RNCWebView` viewId. This is now fixed. [android] check source prop if changed (#14) * [android] check source prop if changed * detect if new source is null too

nealmanaktola added 2 commits August 29, 2022 17:35

add support to reuse instance with webViewKey

e657c66

change up

de8f049

nealmanaktola commented Aug 29, 2022

View reviewed changes

android/src/main/java/com/reactnativecommunity/webview/RNCWebViewManager.java Outdated Show resolved Hide resolved

nealmanaktola commented Aug 29, 2022

View reviewed changes

android/src/main/java/com/reactnativecommunity/webview/RNCWebView.java Show resolved Hide resolved

nealmanaktola commented Aug 29, 2022

View reviewed changes

android/src/main/java/com/reactnativecommunity/webview/RNCWebViewManager.java Outdated Show resolved Hide resolved

nealmanaktola requested a review from donaldchen August 29, 2022 21:58

donaldchen reviewed Aug 30, 2022

View reviewed changes

nealmanaktola added 2 commits August 30, 2022 13:16

address comments related to layout params

9267e2d

add nullability checks

89d1835

nealmanaktola commented Aug 30, 2022

View reviewed changes

android/src/main/java/com/reactnativecommunity/webview/RNCWebView.java Outdated Show resolved Hide resolved

nealmanaktola commented Aug 30, 2022

View reviewed changes

android/src/main/java/com/reactnativecommunity/webview/RNCWebViewManager.java Outdated Show resolved Hide resolved

nealmanaktola added 4 commits August 30, 2022 15:47

fix

96ded98

get full stack trace

bc2612d

add secondary map

b4795d0

rename to ifHasInternalWebView

c6011d1

nealmanaktola commented Aug 30, 2022

View reviewed changes

additional fixes

957c74f

donaldchen reviewed Aug 31, 2022

View reviewed changes

donaldchen approved these changes Aug 31, 2022

View reviewed changes

nealmanaktola added 2 commits August 31, 2022 11:11

fix

bc21c22

fix boolean

56fd883

nealmanaktola merged commit f30880b into 11.18.1-discord-1 Aug 31, 2022

nealmanaktola mentioned this pull request Sep 2, 2022

[android] add messaging passing without a react tag #12

Merged

This was referenced Sep 6, 2022

support message passing and keeping native WebView in memory when react component is unmounted react-native-webview/react-native-webview#2469

Open

WIP reuse webview instance on Android #2

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[android] add support to reuse instance with webViewKey #11

[android] add support to reuse instance with webViewKey #11

nealmanaktola commented Aug 29, 2022 •

edited

nealmanaktola Aug 29, 2022

donaldchen Aug 30, 2022

nealmanaktola Aug 30, 2022

donaldchen Aug 30, 2022

nealmanaktola Aug 30, 2022 •

edited

donaldchen Aug 30, 2022

nealmanaktola Aug 30, 2022 •

edited

donaldchen Aug 30, 2022

nealmanaktola Aug 30, 2022

nealmanaktola Aug 30, 2022 •

edited

donaldchen Aug 30, 2022

nealmanaktola Aug 30, 2022

nealmanaktola commented Aug 30, 2022

donaldchen Aug 30, 2022

donaldchen Aug 30, 2022

nealmanaktola Aug 31, 2022

donaldchen Aug 31, 2022

donaldchen left a comment •

edited

[android] add support to reuse instance with webViewKey #11

[android] add support to reuse instance with webViewKey #11

Conversation

nealmanaktola commented Aug 29, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nealmanaktola Aug 30, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nealmanaktola Aug 30, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nealmanaktola Aug 30, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nealmanaktola commented Aug 30, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

donaldchen left a comment • edited

Choose a reason for hiding this comment

nealmanaktola commented Aug 29, 2022 •

edited

nealmanaktola Aug 30, 2022 •

edited

nealmanaktola Aug 30, 2022 •

edited

nealmanaktola Aug 30, 2022 •

edited

donaldchen left a comment •

edited