-
Notifications
You must be signed in to change notification settings - Fork 9.8k
[webview_flutter] Add supporting interfaces for setting cookies to platform interface. #4555
[webview_flutter] Add supporting interfaces for setting cookies to platform interface. #4555
Conversation
@@ -59,6 +59,7 @@ abstract class WebViewPlatform { | |||
/// Clears all cookies for all [WebView] instances. | |||
/// | |||
/// Returns true if cookies were present before clearing, else false. | |||
@Deprecated('Use `WebViewCookieManagerPlatform.clearCookies` instead.') |
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.
@ reviewer: Just want to double check if it's ok to deprecate platform interface methods like this?
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.
You can't deprecate methods we're still using; it breaks the tree. (I should have tooling soon that will catch this kind of thing in CI.)
They can be commented as deprecated, and then actually deprecated later once the uses have been removed.
Unless you can fix the call sites right away, we need to revert this part, as this will close not only our tree, but the flutter/flutter tree as well.
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.
@stuartmorgan Alright, I'll quickly submit a PR that changes this annotation to 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.
This is looking good, I just added some comments regarding some of the DartDocs.
packages/webview_flutter/webview_flutter_platform_interface/lib/src/types/webview_cookie.dart
Outdated
Show resolved
Hide resolved
packages/webview_flutter/webview_flutter_platform_interface/lib/src/types/webview_cookie.dart
Outdated
Show resolved
Hide resolved
packages/webview_flutter/webview_flutter_platform_interface/lib/src/types/webview_cookie.dart
Outdated
Show resolved
Hide resolved
packages/webview_flutter/webview_flutter_platform_interface/lib/src/types/webview_cookie.dart
Outdated
Show resolved
Hide resolved
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
…es to platform interface. (flutter/plugins#4555)
…es to platform interface. (flutter/plugins#4555)
…atform interface. (flutter#4555)
…atform interface. (flutter#4555)
This PR adds the following:
clearCookies
was moved here, andsetCookie
was added.WebViewPlatform
causes breaking changes due to [webview_flutter] Update to standard platform interface structure flutter#94051. Having a separate cookie manager should prevent this.clearCookies
made this rather difficult.CreationParams
has been expanded to also contain initial cookies to be set upon creation of the webview.clearCookies
inWebViewPlatform
has been deprecated in favour ofWebViewCookieManager.clearCookies
.Relevant issue:
Pre-launch Checklist
dart format
.)[shared_preferences]
pubspec.yaml
with an appropriate new version according to the pub versioning philosophy, or this PR is exempt from version changes.CHANGELOG.md
to 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.