-
Notifications
You must be signed in to change notification settings - Fork 52
[webview_flutter] Update webview_flutter_tizen to v0.7.0 #531
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
[webview_flutter] Update webview_flutter_tizen to v0.7.0 #531
Conversation
78b77b0 to
86b92e4
Compare
bbrto21
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.
Generally, Looks good to me but I leave a few comments.
And I would appreciate it if you could briefly add the reason why Pigeon was not used or why it was implemented using MethodChannel in the PR description.
|
I can't run the integration tests, am I missing something? |
bbrto21
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
| Future<T?> _invokeChannelMethod<T>(String method, [dynamic arguments]) async { | ||
| if (!_isCreated) { | ||
| _pendingMethodCalls[method] = arguments; | ||
| return Future<T?>(() => 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.
Maybe just
| return Future<T?>(() => null); | |
| return 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.
done thank you
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.
Just be aware that adding a method call that is expected to return a value to a pending list is actually a bit pointless (since the method will be executed later and the caller will not have an access to the return value).
| _webview.loadRequest(params.uri.toString()); | ||
| return; | ||
| case LoadRequestMethod.post: | ||
| break; |
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.
Post requests are 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.
The webview_flutter_tizen(ewk) did not support post request.
I've found an api related to this, but haven't yet checked that it works well.
After this patch is merged, I will make a separate PR.
packages/webview_flutter/lib/src/tizen_webview_cookie_manager.dart
Outdated
Show resolved
Hide resolved
9eecdd5 to
1234085
Compare
swift-kim
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.
Looks good to me except for the following. Thanks for all your hard work!
Apply the changed interface from webview_flutter v4.0.2.
- Remove unnecessary try-catch - Change channel name
4096cc3 to
fa53b24
Compare
|
Will webview_flutter_lwe be updated as well or should we deprecate the package? |
@swift-kim I will prepare a PR to upgrade webview_flutter_lwe. |
Apply the changed interface from webview_flutter v4.0.2.
Other platforms use pigeon to create APIs and call the associated native APIs.
webview_flutter_tizen support the new interface while maintaining the existing method channel calls.
This change minimize changes to the tizen code in webview.cc while keeping method channel.