Android sync java on should start load with request #8
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
trcoffman
commented
Aug 19, 2020
android/src/main/java/com/reactnativecommunity/webview/RNCWebViewModule.java
Outdated
Show resolved
Hide resolved
android/src/main/java/com/reactnativecommunity/webview/RNCWebViewManager.java
Outdated
Show resolved
Hide resolved
|
|
||
| if (jsContext.get() != 0 && rncWebView.mCatalystInstance != null) { | ||
| final Pair<Integer, AtomicReference<RNCWebViewModule.ShouldOverrideUrlLoadingLock.ShouldOverrideCallbackState>> lock = RNCWebViewModule.shouldOverrideUrlLoadingLock.getNewLock(); |
There was a problem hiding this comment.
I'd break out all this logic inside this if statement to another method so that the overall structure of shouldOverrideUrlLoading is very easy to understand.
There was a problem hiding this comment.
I don't agree that this would help much (not a fan of extraction into a function just cause... especially if it's not reusable), maybe let's see where the other changes take it first.
android/src/main/java/com/reactnativecommunity/webview/RNCWebViewManager.java
Outdated
Show resolved
Hide resolved
android/src/main/java/com/reactnativecommunity/webview/RNCWebViewManager.java
Outdated
Show resolved
Hide resolved
android/src/main/java/com/reactnativecommunity/webview/RNCWebViewManager.java
Outdated
Show resolved
Hide resolved
| @@ -76,6 +76,7 @@ class WebView extends React.Component<AndroidWebViewProps, State> { | |||
| lastErrorEvent: null, | |||
| }; | |||
|
|
|||
| onShouldStartLoadWithRequest: ReturnType<typeof createOnShouldStartLoadWithRequest> | null = null; | |||
There was a problem hiding this comment.
Perhaps rename things in this file to make it more clear to the reader that this is distinct? It's kind of confusing the way things are written now.
There was a problem hiding this comment.
I'll play with it and see how much better I can make it.
Karthz
reviewed
Aug 19, 2020
android/src/main/java/com/reactnativecommunity/webview/RNCWebViewManager.java
Outdated
Show resolved
Hide resolved
Karthz
reviewed
Aug 19, 2020
android/src/main/java/com/reactnativecommunity/webview/RNCWebViewManager.java
Outdated
Show resolved
Hide resolved
Karthz
reviewed
Aug 19, 2020
android/src/main/java/com/reactnativecommunity/webview/RNCWebViewManager.java
Outdated
Show resolved
Hide resolved
Karthz
reviewed
Aug 21, 2020
|
|
||
| if (jsContext.get() != 0 && rncWebView.mCatalystInstance != null) { | ||
| final Pair<Integer, AtomicReference<RNCWebViewModule.ShouldOverrideUrlLoadingLock.ShouldOverrideCallbackState>> lock = RNCWebViewModule.shouldOverrideUrlLoadingLock.getNewLock(); | ||
| if (! isJsDebugging && rncWebView.mCatalystInstance != null) { |
There was a problem hiding this comment.
nit: in Java, we don't usually leave a space between ! and variable.
Karthz
approved these changes
Aug 21, 2020
* Add androidLayerType as a prop * Deprecate "androidHardwareAccelerationDisabled" prop * Update reference Co-authored-by: Olivia Caraiman <olcaraim@microsoft.com>
# [10.8.0](react-native-webview/react-native-webview@v10.7.0...v10.8.0) (2020-08-24) ### Features * **android:** Add androidLayerType as prop ([react-native-webview#1588](react-native-webview#1588)) ([9ffca8f](react-native-webview@9ffca8f))
This solves a class of issues when the WebView loses "context" that a subsequent page load is the same as what was attempted to be loaded previously. This solves a bug where a HTTP redirect in combination with history manipulations causes a user to be stuck and prevented from going back. Since WebView requests are allowed to happen normally, debugging the WebView and tracking redirects and page load initiators is more accurate and easier. This will also bypass bridge latency and provide a faster navigation. To do this, we must lock in the shouldOverrideUrlLoading callback and send an event to JS. Currently, this callback is ran on the main UI thread, of which we have no control over. This is problematic as using the bridge in most ways seems to require the main UI thread, which will cause a deadlock. However, using BatchedBridge for Java->JS and a synchronous method for JS->Java doesn't cause any problems. Additionally, it's been designed so that if WebView suddenly runs the callback on a different thread allowing for concurrency, it will continue to work.
07efafd to
773a3a4
Compare
|
Hello 👋, this PR has been opened for more than 2 months with no activity on it. If you think this is a mistake please comment and ping a maintainer to get this merged ASAP! Thanks for contributing! You have 7 days until this gets closed automatically |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.