-
Notifications
You must be signed in to change notification settings - Fork 9.7k
[webview_flutter_wkwebview] Implementation of PlatformNavigationDelegate for WebKit #6151
[webview_flutter_wkwebview] Implementation of PlatformNavigationDelegate for WebKit #6151
Conversation
| void Function(int progress)? _onProgress; | ||
| void Function(WebResourceError error)? _onWebResourceError; | ||
| FutureOr<bool> Function({required String url, required bool isForMainFrame})? | ||
| _onNavigationRequest; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In defense of this pattern:
I wanted setters for the callbacks methods because I wanted a consistent way for a platform to indicate it doesn't support a feature.
For example, if Android doesn't support WebViewController.callSomeMethod(), it throws an UnimplementedError by default. By using setters for callback methods, the default response for a callback method is to throw an UnimplementedError. I didn't know a way to do this with a class that takes the callback methods as constructor parameters.
Also note that the app facing package will probably use fields:
class NavigationDelegate {
final Function(String) onPageFinished;
}But the platform classes will use setters so they can throw when a callback is not supported.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By using setters for callback methods, the default response for a callback method is to throw an
UnimplementedError.
I think I'm missing something about the implementation; where does that happen? It looks in the code above this like the default would be a silent no-op for a null method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's done in the platform interface class: https://github.com/flutter/plugins/blob/main/packages/webview_flutter/webview_flutter_platform_interface/lib/v4/src/platform_navigation_delegate.dart#L63
PlatformNavigationDelegate has to be extended, so when a new callback method is added (e.g. setOnPageRedirected) platform implementations would throw an UnimplementedError by default.
stuartmorgan-g
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
| NSObject object, | ||
| Map<NSKeyValueChangeKey, Object?> change, | ||
| ) { | ||
| if (weakThis.target != null && _onProgress != null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: can't this be weakThis.target?._onProgress != null?
…ationDelegate for WebKit (flutter/plugins#6151)
Part of flutter/flutter#94051
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.