Skip to content
This repository was archived by the owner on Feb 22, 2023. It is now read-only.

Allow clearing cookies for FlutterWebView #1149

Merged
iskakaushik merged 22 commits into from
Feb 7, 2019
Merged

Allow clearing cookies for FlutterWebView #1149

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
02f74bb
Add android version for incognito web view
Jan 31, 2019
e44c7eb
Added the feature to remove cookies from webviews
Feb 1, 2019
3cb4073
Merge branch 'master' of github.com:flutter/plugins into feature/inco…
Feb 1, 2019
0499934
revert no-op changes
Feb 1, 2019
1652b45
fix ios prefix
Feb 2, 2019
656a442
make it return so we can test
Feb 2, 2019
ffa8007
example to remove cookies
Feb 2, 2019
fe27c42
add a test too!
Feb 2, 2019
fe02c64
fix analyzer errors
Feb 2, 2019
e3afcc4
format objc stuff
Feb 2, 2019
ed66d5c
Merge branch 'master' of github.com:flutter/plugins into feature/inco…
Feb 5, 2019
b8922ee
Address CR comments
Feb 5, 2019
5257258
Semver changes and Changelog
Feb 5, 2019
ac23c4a
Fix cookie handling in IOS
Feb 7, 2019
11dadf9
Add issue
Feb 7, 2019
9609895
make factory method
Feb 7, 2019
85b9f7e
Typed invokeMethod is not in stable yet
Feb 7, 2019
53ae5ed
don't name stuff private if it's not visible for testing
Feb 7, 2019
7a20956
move method channel to const
Feb 7, 2019
02da0fb
Separate list cookies from clear cookies
Feb 7, 2019
2dd3756
Use shorthand initialization
Feb 7, 2019
be3851f
fix typos
Feb 7, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions packages/webview_flutter/example/lib/main.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,7 @@ class SampleMenu extends StatelessWidget {
SampleMenu(this.controller);

final Future<WebViewController> controller;
final CookieManager cookieManager = CookieManager();

@override
Widget build(BuildContext context) {
Expand Down
20 changes: 12 additions & 8 deletions packages/webview_flutter/lib/webview_flutter.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -471,10 +471,19 @@ class WebViewController {

/// Manages cookies pertaining to all [WebView]s.
class CookieManager {
const CookieManager._();
factory CookieManager() {
Copy link
Contributor

@amirh amirh Feb 7, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@amirh amirh Feb 7, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if (_instance == null) {
final MethodChannel methodChannel =
Copy link
Contributor

@jonahwilliams jonahwilliams Feb 7, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@iskakaushik iskakaushik Feb 7, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@jonahwilliams jonahwilliams Feb 7, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consistency is good to a point, but correctness is better

Copy link
Contributor Author

@iskakaushik iskakaushik Feb 7, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@jonahwilliams jonahwilliams Feb 7, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not necessary at all

Copy link
Contributor Author

@iskakaushik iskakaushik Feb 7, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

@jonahwilliams jonahwilliams Feb 7, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;

const MethodChannel('plugins.flutter.io/cookie_manager');
_instance = CookieManager.private(methodChannel);
}
return _instance;
}

static const MethodChannel _channel =
MethodChannel('plugins.flutter.io/cookie_manager');
CookieManager.private(this._channel);
Copy link
Contributor

@jonahwilliams jonahwilliams Feb 7, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@amirh amirh Feb 7, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


static CookieManager _instance;
final MethodChannel _channel;

/// Clears all cookies.
///
Expand All @@ -496,8 +505,3 @@ void _validateUrlString(String url) {
throw ArgumentError(e);
}
}

/// Singleton [CookieManager].
///
/// Manages cookies pertaining to all [WebView]s.
final CookieManager cookieManager = const CookieManager._();
2 changes: 2 additions & 0 deletions packages/webview_flutter/test/webview_flutter_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -367,6 +367,7 @@ void main() {
initialUrl: 'https://flutter.io',
),
);
final CookieManager cookieManager = CookieManager();
final bool hasCookies = await cookieManager.clearCookies();
expect(hasCookies, true);
});
Expand All @@ -378,6 +379,7 @@ void main() {
initialUrl: 'https://flutter.io',
),
);
final CookieManager cookieManager = CookieManager();
final bool hasCookies = await cookieManager.clearCookies();
expect(hasCookies, true);
final bool hasCookiesSecond = await cookieManager.clearCookies();
Expand Down