[webview_flutter_wkwebview] Add support for WKNavigationDelegate#4893
[webview_flutter_wkwebview] Add support for WKNavigationDelegate#4893bparrishMines merged 11 commits intoflutter:mainfrom
Conversation
github-actions
bot
added
p: webview_flutter
bparrishMines
changed the title
bparrishMines
changed the title
| errorType: WebResourceErrorType.webContentProcessTerminated, | ||
| )); | ||
| }; | ||
|
|
There was a problem hiding this comment.
Relevant code from current implementation: https://github.com/flutter/plugins/blob/main/packages/webview_flutter/webview_flutter_wkwebview/ios/Classes/FLTWKNavigationDelegate.m
| WKUserContentController userContentController, | ||
| WKScriptMessage message, | ||
| ) | ||
| )? |
There was a problem hiding this comment.
These can be nullable now since null parameters with pigeon are on their way.
| webViewProxy.createNavigationDelegate() | ||
| ..didStartProvisionalNavigation = (WKWebView webView) { | ||
| webView.url.then<void>( | ||
| (String? url) => callbacksHandler.onPageStarted(url ?? ''), |
There was a problem hiding this comment.
An alternative to needing to make another method call to retrieve the url would be to create a custom method in WKNavigationDelegate that also includes the url.
There was a problem hiding this comment.
I think this is a case where we are going to actually need to flex the boundary away from the direct wrapping slightly, because of the async calls. This won't actually give correct results in all cases, because If there's a sequence of redirects there's no guarantee that the URL that is retrieved asynchronously is actually the one that the delegate callback was actually about.
There was a problem hiding this comment.
I agree. I updated the method
packages/webview_flutter/webview_flutter_wkwebview/lib/src/web_kit/web_kit.dart
Show resolved
Hide resolved
packages/webview_flutter/webview_flutter_wkwebview/lib/src/web_kit/web_kit.dart
Outdated
Show resolved
Hide resolved
| /// | ||
| /// Provide a delegate object when you want to manage or restrict navigation | ||
| /// in your web content, track the progress of navigation requests, and handle | ||
| /// authentication challenges for any new content. |
There was a problem hiding this comment.
Per previous discussion, we should be linking to docs rather that duplicating them for wrappers.
There was a problem hiding this comment.
My intention was to include basic information, so someone wouldn't have to open a browser for every class/method. I removed this paragraph and just kept the summary above.
| webViewProxy.createNavigationDelegate() | ||
| ..didStartProvisionalNavigation = (WKWebView webView) { | ||
| webView.url.then<void>( | ||
| (String? url) => callbacksHandler.onPageStarted(url ?? ''), |
There was a problem hiding this comment.
I think this is a case where we are going to actually need to flex the boundary away from the direct wrapping slightly, because of the async calls. This won't actually give correct results in all cases, because If there's a sequence of redirects there's no guarantee that the URL that is retrieved asynchronously is actually the one that the delegate callback was actually about.
packages/webview_flutter/webview_flutter_wkwebview/lib/src/web_kit_webview_widget.dart
Outdated
Show resolved
Hide resolved
|
@stuartmorgan This should be ready for another review. |
stuartmorgan-g
left a comment
stuartmorgan-g
left a comment
There was a problem hiding this comment.
LGTM with nit, sorry for the delay.
| @@ -1,5 +1,5 @@ | |||
| // Mocks generated by Mockito 5.0.17 from annotations | |||
| // in webview_flutter_wkwebview/test/src/web_kit_webview_widget_test.dart. | |||
| // in webview_flutter_wkwebview/example/ios/.symlinks/plugins/webview_flutter_wkwebview/test/src/web_kit_webview_widget_test.dart. | |||
There was a problem hiding this comment.
This seems wrong; any idea how this path got messed up?
There was a problem hiding this comment.
I'm not sure, but I upgraded Flutter and mockito to the latest versions and it still outputs the symlink. I created an issue for the dart team: dart-lang/mockito#521
bparrishMines
added
the
waiting for tree to go green
2 participants
No version change:
Part of flutter/flutter#93732 and doesn't make any changes to the current implementation.
No CHANGELOG change: Incremental unused code doesn't need to be noted in the CHANGELOG.
Pre-launch Checklist
dart format.)[shared_preferences]pubspec.yamlwith an appropriate new version according to the pub versioning philosophy, or this PR is exempt from version changes.CHANGELOG.mdto add a description of the change, following repository CHANGELOG style.///).If you need help, consider asking for advice on the #hackers-new channel on Discord.