New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Android][WebView] onShouldStartLoadWithRequest callback #6478

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
@pglotov
Contributor

pglotov commented Mar 16, 2016

This PR implements onShouldStartLoadWithRequest callback for WebView on Android. Similar to iOS approach in PR#3643.

@facebook-github-bot

This comment has been minimized.

facebook-github-bot commented Mar 16, 2016

By analyzing the blame information on this pull request, we identified @mkonicek, @dmmiller and @korDen to be potential reviewers.

@facebook-github-bot

This comment has been minimized.

facebook-github-bot commented Mar 16, 2016

@pglotov updated the pull request.

@jordansexton

This comment has been minimized.

jordansexton commented Mar 16, 2016

I love you so much right now.

@ide

This comment has been minimized.

Collaborator

ide commented Mar 16, 2016

cc @astreet this PR introduces synchronous JS execution by allowing the UI thread to invoke JSC. CCing you since you wrote the Worker implementation. Based on my experience with multicore JSC, calling into the same JSContext from two threads is going to introduce race conditions within JS itself since JSC will try to concurrently interleave two separate JS calls.

@pglotov I don't think this approach is going to work. If the WebView method needs to be synchronous, you should queue up the JS command to be run on the JS thread and then block the UI thread until the JS command is processed.

UIManager.dispatchViewManagerCommandSync(
this.getWebViewHandle(),
UIManager.RCTWebView.Commands.shouldOverrideWithResult,
[shouldOverride]

This comment has been minimized.

@eslint-bot

eslint-bot Mar 16, 2016

no-extra-bind: The function binding is unnecessary.

This comment has been minimized.

@eslint-bot

eslint-bot Mar 16, 2016

no-extra-bind: The function binding is unnecessary.

@pglotov

This comment has been minimized.

Contributor

pglotov commented Mar 16, 2016

@ide Actually it synchronously calls Java from JS thread. UI thread sends an (async) event to JS and blocks. JS processes the event and posts result into Java object (synchronously), unblocking UI thread. Race conditions in Java world can be dealt with.

@dmmiller

This comment has been minimized.

Contributor

dmmiller commented Mar 16, 2016

Can you look at how iOS does this in WebView.ios.js? Also, would be nice if it had the same property name as in ios as well. Have you checked how this works for back/forward and history? Also shouldOverrideUrlLoading is not called in all instances. For instance, not on the initial load, nor when you explicitly tell the WebView to load a URL, only on link clicks which may be good enough.

@pglotov

This comment has been minimized.

Contributor

pglotov commented Mar 16, 2016

@dmmiller Yes I looked at iOS onShouldStartLoadWithRequest before implementing this, and tried to follow that approach. No I didn't check back/forward. I need it to intercept link clicks in webview and open them in browser instead. Makes sense to have a single property name for both platforms.

@pglotov pglotov force-pushed the pglotov:android-shouldOverrideUrlLoading branch from d9e336b to f497da8 Mar 16, 2016

@pglotov pglotov changed the title from [Android][WebView] shouldOverrideUrlLoading callback to [Android][WebView] onShouldStartLoadWithRequest callback Mar 16, 2016

@facebook-github-bot

This comment has been minimized.

facebook-github-bot commented Mar 16, 2016

@pglotov updated the pull request.

UIManager.dispatchViewManagerCommandSync(
this.getWebViewHandle(),
UIManager.RCTWebView.Commands.shouldOverrideWithResult,
[shouldOverride]

This comment has been minimized.

@eslint-bot

eslint-bot Mar 16, 2016

no-extra-bind: The function binding is unnecessary.

@@ -195,6 +198,16 @@ var WebView = React.createClass({
console.warn('WebView: `source.body` is not supported when using GET.');
}
var onShouldOverrideUrlLoading = this.props.onShouldStartLoadWithRequest && ((event: Event) => {

This comment has been minimized.

@davidaurelio

davidaurelio Mar 16, 2016

Contributor

just some formal rants (aka nits): The formatting here makes this particularly hard to read. Please add some line breaks, and white space. Thank you.

@@ -195,6 +198,16 @@ var WebView = React.createClass({
console.warn('WebView: `source.body` is not supported when using GET.');
}
var onShouldOverrideUrlLoading = this.props.onShouldStartLoadWithRequest && ((event: Event) => {
var shouldOverride = this.props.onShouldStartLoadWithRequest &&

This comment has been minimized.

@davidaurelio

davidaurelio Mar 16, 2016

Contributor

How can this.props.onShouldStartLoadWithRequest go away in the meantime?

UIManager.RCTWebView.Commands.shouldOverrideWithResult,
[shouldOverride]
);
}).bind(this);

This comment has been minimized.

@davidaurelio

davidaurelio Mar 16, 2016

Contributor

@eslint-bot tells the truth

@ide

This comment has been minimized.

Collaborator

ide commented Mar 16, 2016

@pglotov ah sorry I must have misread what is going on

@pglotov pglotov force-pushed the pglotov:android-shouldOverrideUrlLoading branch from f497da8 to 11385ba Mar 17, 2016

@facebook-github-bot

This comment has been minimized.

facebook-github-bot commented Mar 17, 2016

@pglotov updated the pull request.

@pglotov pglotov force-pushed the pglotov:android-shouldOverrideUrlLoading branch from 11385ba to 39a04bb Mar 17, 2016

@facebook-github-bot

This comment has been minimized.

facebook-github-bot commented Mar 17, 2016

@pglotov updated the pull request.

@pglotov pglotov force-pushed the pglotov:android-shouldOverrideUrlLoading branch from 39a04bb to 9c4132a Mar 17, 2016

@facebook-github-bot

This comment has been minimized.

facebook-github-bot commented Mar 17, 2016

@pglotov updated the pull request.

@pglotov

This comment has been minimized.

Contributor

pglotov commented Mar 17, 2016

@davidaurelio Thanks, made changes.

@davidaurelio

This comment has been minimized.

Contributor

davidaurelio commented Mar 17, 2016

great, thank you!

@@ -7,7 +7,7 @@
* of patent rights can be found in the PATENTS file in the same directory.
*/
package com.facebook.catalyst.views.webview.events;
package com.facebook.react.views.webview.events;

This comment has been minimized.

@dmmiller

dmmiller Mar 17, 2016

Contributor

Thanks for cleaning this, but this is really orthagonal. Want to put up a quick PR to fix these and then I can accept and then this one will be easier to read?

This comment has been minimized.

@pglotov

pglotov Mar 17, 2016

Contributor

Sure, will do it later today.

This comment has been minimized.

@pglotov

pglotov Mar 18, 2016

Contributor

Created PR for path changes here.

@dmmiller

This comment has been minimized.

Contributor

dmmiller commented Mar 17, 2016

This question of whether we want to allow synchronous communication this way is pretty big. I'd like @andreicoman11 and @astreet to weigh in on it.

@younthu

This comment has been minimized.

Contributor

younthu commented Jan 3, 2017

Strong need this.

@evanidul

This comment has been minimized.

evanidul commented Feb 1, 2017

+1

@astreet

This comment has been minimized.

Contributor

astreet commented Feb 1, 2017

Like I mentioned above, I think a reasonable solution for the majority of cases would be to add a prop to WebView which is an array of regex's of urls to block synchronously. We would also want to add a new event for WebViews to be sent to JS when a URL is blocked this way so that JS can respond appropriately. Unfortunately, this isn't something we have a need for at FB so it's going to be up to an external contributor working on it, I can help review the code and offer advice.

@younthu

This comment has been minimized.

Contributor

younthu commented Feb 2, 2017

@astreet , i just implemented it in our project with the regex prop way you described, i will try to extract those code out and send a pr to the FB, hope it help others. Please help to review it then, Thanks.

@mattiashalldin

This comment has been minimized.

mattiashalldin commented May 15, 2017

When will onShouldStartLoadWithRequest be available for Android?

@zheng-liu-seattle

This comment has been minimized.

zheng-liu-seattle commented May 22, 2017

+1 need this.

@coyer

This comment has been minimized.

coyer commented May 27, 2017

+1 need this

2 similar comments
@caigehui

This comment has been minimized.

caigehui commented Jul 17, 2017

+1 need this

@Livyli

This comment has been minimized.

Livyli commented Aug 15, 2017

+1 need this

@ybolaris

This comment has been minimized.

ybolaris commented Sep 26, 2017

+1

4 similar comments
@miguelocarvajal

This comment has been minimized.

miguelocarvajal commented Oct 3, 2017

+1

@tourze

This comment has been minimized.

tourze commented Oct 16, 2017

+1

@kyangy

This comment has been minimized.

kyangy commented Nov 10, 2017

+1

@stanleycyang

This comment has been minimized.

stanleycyang commented Nov 21, 2017

+1

@0x5e

This comment has been minimized.

0x5e commented Nov 29, 2017

Without onShouldStartLoadWithRequest implemented on Android, the same method on iOS is useless. :-(

@wesleymooiman

This comment has been minimized.

wesleymooiman commented Dec 4, 2017

+1 we need this

@MikePodgorniy

This comment has been minimized.

Contributor

MikePodgorniy commented Dec 7, 2017

+1

11 similar comments
@punksta

This comment has been minimized.

punksta commented Jan 8, 2018

+1

@anuraginwadi

This comment has been minimized.

anuraginwadi commented Jan 29, 2018

+1

@shashankwadi

This comment has been minimized.

shashankwadi commented Jan 29, 2018

+1

@pulkitsharma99

This comment has been minimized.

pulkitsharma99 commented Jan 29, 2018

+1

@pramodinwadi

This comment has been minimized.

pramodinwadi commented Jan 29, 2018

+1

@akhilwadi

This comment has been minimized.

akhilwadi commented Jan 29, 2018

+1

@manjeetwadi

This comment has been minimized.

manjeetwadi commented Jan 29, 2018

+1

@simranjeet-sawhney

This comment has been minimized.

simranjeet-sawhney commented Jan 29, 2018

+1

@rschef

This comment has been minimized.

rschef commented Mar 26, 2018

+1

@prademak

This comment has been minimized.

prademak commented Mar 27, 2018

+1

@SanlinBlackball

This comment has been minimized.

SanlinBlackball commented Apr 19, 2018

+1

@braydo25

This comment has been minimized.

braydo25 commented May 8, 2018

In the meantime, there is a pending PR on the react-native-webview-android module with a working implementation of onShouldStartLoadWithRequest for Android here: lucasferreira/react-native-webview-android#98

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment