Allow clearing cookies for FlutterWebView #1149

iskakaushik · 2019-02-02T01:53:44Z

As a first step to address flutter/flutter#27321

…gnito-web-view

amirh

Thanks! Looks great overall, left a few nits (mainly on documentation)

amirh · 2019-02-05T04:37:19Z

packages/webview_flutter/lib/webview_flutter.dart

+  /// Removes all cookies, and returns true if cookies were
+  /// present before clearing, else false.
+  Future<bool> clearCookies() async {
+    print(defaultTargetPlatform);


amirh · 2019-02-05T04:38:56Z

packages/webview_flutter/lib/webview_flutter.dart

+  static const MethodChannel _channel =
+      MethodChannel('plugins.flutter.io/cookie_manager');
+
+  /// Removes all cookies, and returns true if cookies were


The first paragraph will be used in the Dartdoc's table of contents. better to keep it short. I'd just have the summary paragraph say "Clears all cookies."
And then in the next paragraph expand on which webview instances are affected, and in a following paragraph describe the return value.

amirh · 2019-02-05T04:40:09Z

packages/webview_flutter/lib/webview_flutter.dart

@@ -319,3 +334,6 @@ void _validateUrlString(String url) {
    throw ArgumentError(e);
  }
 }
+
+/// Singleton [CookieManager]


Describe what it is.
also nit: end sentences with period.

amirh · 2019-02-05T04:42:23Z

packages/webview_flutter/ios/Classes/WebViewFlutterPlugin.m

@@ -7,6 +8,7 @@ + (void)registerWithRegistrar:(NSObject<FlutterPluginRegistrar>*)registrar {
  FLTWebViewFactory* webviewFactory =
      [[FLTWebViewFactory alloc] initWithMessenger:registrar.messenger];
  [registrar registerViewFactory:webviewFactory withId:@"plugins.flutter.io/webview"];
+  FLTCookieManager* cookieManager = [[FLTCookieManager alloc] init:registrar.messenger];


What's keeping a reference to this instance?

the instance is not being used, but the init method needs to be called. Removing the return reference.

If we don't keep a reference what prevents FLTCookieManager from being disposed by the time we got a method channel invocation?
(we are running this with ARC)

amirh · 2019-02-05T04:45:52Z

packages/webview_flutter/example/lib/main.dart

+
+  void _onClearCookies(BuildContext context) async {
+    final bool hadCookies = await cookieManager.clearCookies();
+    String message = "There were cookies. Now, they are gone!";


How do we know that it worked?
Maybe we could make a JS call to get the list of cookies present it?

Good point, for now i verified that flutter.io was setting and deleting cookies. Will add a method to list all cookies.

amirh · 2019-02-05T04:46:55Z

...ew_flutter/android/src/main/java/io/flutter/plugins/webviewflutter/FlutterCookieManager.java

+import io.flutter.plugin.common.MethodChannel.MethodCallHandler;
+import io.flutter.plugin.common.MethodChannel.Result;
+
+public class FlutterCookieManager implements MethodCallHandler {


nit: can we make this package private?

amirh · 2019-02-05T04:47:05Z

...ew_flutter/android/src/main/java/io/flutter/plugins/webviewflutter/FlutterCookieManager.java

+public class FlutterCookieManager implements MethodCallHandler {
+
+  private FlutterCookieManager() {
+    // Do not instantiate.


explain why

amirh · 2019-02-05T04:50:23Z

Please also update the change log, and bump the plugin version in pubspec.yaml.
Versioning guidelines are here: https://www.dartlang.org/tools/pub/versioning#semantic-versions

…gnito-web-view

jonahwilliams · 2019-02-07T18:17:50Z

packages/webview_flutter/lib/webview_flutter.dart

+  /// This is supported for >= IOS 9 and Android api level >= 16.
+  /// returns true if cookies were present before clearing, else false.
+  Future<bool> clearCookies() async {
+    return await _channel.invokeMethod<bool>('clearCookies');


Suggested change

return await _channel.invokeMethod<bool>('clearCookies');

return _channel.invokeMethod<bool>('clearCookies');

jonahwilliams · 2019-02-07T18:18:39Z

packages/webview_flutter/lib/webview_flutter.dart

+  /// This is supported for >= IOS 9 and Android api level >= 16.
+  /// returns true if cookies were present before clearing, else false.
+  Future<bool> clearCookies() async {
+    return await _channel.invokeMethod<bool>('clearCookies');


Using the type parameter will make this incompatible with stable, see the other plugin repos

good catch, these are risky days for missing these on code reviews. 😨
flutter/flutter#27110 would have been helpful here.

Ouch, fixed this. Thanks for the flag!

jonahwilliams · 2019-02-07T18:19:05Z

packages/webview_flutter/lib/webview_flutter.dart

+/// Singleton [CookieManager].
+///
+/// Manages cookies pertaining to all [WebView]s.
+final CookieManager cookieManager = const CookieManager._();


Suggested change

final CookieManager cookieManager = const CookieManager._();

const CookieManager cookieManager = CookieManager._();

jonahwilliams · 2019-02-07T18:28:43Z

packages/webview_flutter/lib/webview_flutter.dart


-  static const MethodChannel _channel =
-      MethodChannel('plugins.flutter.io/cookie_manager');
+  CookieManager.private(this._channel);


Don't name a constructor .private, either its private _ or its not

jonahwilliams · 2019-02-07T18:33:59Z

packages/webview_flutter/lib/webview_flutter.dart

+class CookieManager {
+  factory CookieManager() {
+    if (_instance == null) {
+      final MethodChannel methodChannel =


Since we can set mockMethodCallHandlers for testing, what is the value of providing the channel this way?

My original thought was to make this a const field on the class. I wanted to be consistent with Battery plugin, i should maybe look through the test code there.

Consistency is good to a point, but correctness is better

curious, does this approach expose a way of injecting the wrong method channel?

It's not necessary at all

i agree that passing in a method channel is not needed for testing, but passing it though this way -- creating it on initialization vs having it static, should be equivalent?

Generally making const fields is frowned upon. since the const is already canonicalized we are only adding overhead to the class. Furthermore, removing the initialization from the factory lets us cut down the code a bit with ??=:

factory Foo() { return _instance ??= Foo._(); } Foo._(); static Foo _instance;

amirh · 2019-02-07T18:31:44Z

packages/webview_flutter/lib/webview_flutter.dart

+    return _instance;
+  }
+
+  CookieManager.private(this._channel);


the name private has no special meaning.
this should be named _ to make sure it is really private.

amirh · 2019-02-07T18:33:11Z

packages/webview_flutter/lib/webview_flutter.dart

@@ -469,6 +469,32 @@ class WebViewController {
  }
 }

+/// Manages cookies pertaining to all [WebView]s.
+class CookieManager {
+  factory CookieManager() {


This needs a Dartdoc, it should mention that it returns or created the singleton instance.

nit: not sure if '--' is common in Flutter dartdocs (of if it is grammatically correct), I'd just say:

Returns the singleton cookie manager instance. The instance is created the first time this factory is called.

amirh · 2019-02-07T18:33:44Z

packages/webview_flutter/lib/webview_flutter.dart

+
+  /// Clears all cookies.
+  ///
+  /// This is supported for >= IOS 9 and Android api level >= 16.


no need to mention Android API level (16 is the minimum level supported by Flutter anyway)

amirh · 2019-02-07T18:34:17Z

packages/webview_flutter/lib/webview_flutter.dart

+  /// Clears all cookies.
+  ///
+  /// This is supported for >= IOS 9 and Android api level >= 16.
+  /// returns true if cookies were present before clearing, else false.


nit: separate the paragraph, start sentence with a capital letter.

amirh · 2019-02-07T18:34:51Z

packages/webview_flutter/lib/webview_flutter.dart

+  /// This is supported for >= IOS 9 and Android api level >= 16.
+  /// returns true if cookies were present before clearing, else false.
+  Future<bool> clearCookies() => _channel
+      // ignore: strong_mode_implicit_dynamic_method


Add the same comment we have everywhere(minus the typo 😄 ) so it's easy to find all of these at once (and we need a TODO here anyway)

keep it TODO(amirh), most likely someone will end up searching for all these TODO(amirh): occurrences at once.

amirh · 2019-02-07T18:36:34Z

packages/webview_flutter/ios/Classes/FLTCookieManager.m

+
+- (void)clearCookies:(FlutterResult)result {
+  NSOperatingSystemVersion ios9 = (NSOperatingSystemVersion){9, 0};
+  if ([[NSProcessInfo processInfo] isOperatingSystemAtLeastVersion:ios9]) {


if (@available(iOS 9.0, *)) {

amirh · 2019-02-07T18:37:17Z

packages/webview_flutter/ios/Classes/FLTCookieManager.m

+    [self clearCookiesIos9AndLater:result];
+  } else {
+    // support for IOS-8 tracked in https://github.com/flutter/flutter/issues/27624.
+    NSLog(@"Clearing cookies is not supported for Flutter WebViews prior to iOS9.");


nit: s/iOS9/iOS 9/

amirh · 2019-02-07T18:40:04Z

packages/webview_flutter/example/lib/main.dart

+  void _onClearCookies(
+      WebViewController controller, BuildContext context) async {
+    final String cookies =
+        await controller.evaluateJavascript('document.cookie');


What do you think about adding a menu entry that just shows a toast with the cookies?
I'd feel more confident to verify that it's working if the button that's showing me the cookies is independent of the button that's deleting the cookies.

Makes sense.

- Add a more detailed TODO for invokeMethod generic - Fix Dart doc comments.

amirh

LGTM

This reverts commit 75b959f.

Kaushik Iska added 8 commits January 31, 2019 13:26

Add android version for incognito web view

02f74bb

Added the feature to remove cookies from webviews

e44c7eb

Merge branch 'master' of github.com:flutter/plugins into feature/inco…

3cb4073

…gnito-web-view

revert no-op changes

0499934

fix ios prefix

1652b45

make it return so we can test

656a442

example to remove cookies

ffa8007

add a test too!

fe27c42

iskakaushik requested a review from amirh February 2, 2019 01:53

googlebot added the cla: yes label Feb 2, 2019

Kaushik Iska added 2 commits February 1, 2019 18:18

fix analyzer errors

fe02c64

format objc stuff

e3afcc4

amirh reviewed Feb 5, 2019

View reviewed changes

Kaushik Iska added 5 commits February 5, 2019 11:08

Merge branch 'master' of github.com:flutter/plugins into feature/inco…

ed66d5c

…gnito-web-view

Address CR comments

b8922ee

Semver changes and Changelog

5257258

Fix cookie handling in IOS

ac23c4a

Add issue

11dadf9

jonahwilliams reviewed Feb 7, 2019

View reviewed changes

make factory method

9609895

jonahwilliams reviewed Feb 7, 2019

View reviewed changes

Kaushik Iska added 2 commits February 7, 2019 10:31

Typed invokeMethod is not in stable yet

85b9f7e

don't name stuff private if it's not visible for testing

53ae5ed

jonahwilliams reviewed Feb 7, 2019

View reviewed changes

move method channel to const

7a20956

amirh reviewed Feb 7, 2019

View reviewed changes

Separate list cookies from clear cookies

02da0fb

- Add a more detailed TODO for invokeMethod generic - Fix Dart doc comments.

Kaushik Iska added 2 commits February 7, 2019 10:56

Use shorthand initialization

2dd3756

fix typos

be3851f

amirh approved these changes Feb 7, 2019

View reviewed changes

iskakaushik merged commit 38f8198 into flutter:master Feb 7, 2019

iskakaushik deleted the feature/incognito-web-view branch February 7, 2019 20:43

andreidiaconu pushed a commit to andreidiaconu/plugins that referenced this pull request Feb 17, 2019

Allow clearing cookies for FlutterWebView (flutter#1149)

75b959f

andreidiaconu added a commit to andreidiaconu/plugins that referenced this pull request Feb 17, 2019

Revert "Allow clearing cookies for FlutterWebView (flutter#1149)"

074ecea

This reverts commit 75b959f.

karantanwar pushed a commit to karantanwar/plugins that referenced this pull request Feb 19, 2019

Allow clearing cookies for FlutterWebView (flutter#1149)

12a63ab

karantanwar pushed a commit to karantanwar/plugins that referenced this pull request Feb 19, 2019

Allow clearing cookies for FlutterWebView (flutter#1149)

a7d49ea

YukiOya mentioned this pull request Mar 27, 2019

[webview_flutter]Support remove cookies setting for webview_flutter #990

Closed

	return await _channel.invokeMethod<bool>('clearCookies');
	return _channel.invokeMethod<bool>('clearCookies');

	final CookieManager cookieManager = const CookieManager._();
	const CookieManager cookieManager = CookieManager._();

Allow clearing cookies for FlutterWebView #1149

Allow clearing cookies for FlutterWebView #1149

Uh oh!

Conversation

iskakaushik commented Feb 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

amirh left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amirh commented Feb 5, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iskakaushik commented Feb 2, 2019 •

edited

Loading