-
Notifications
You must be signed in to change notification settings - Fork 9.7k
Initial (Android only) implementation of a WebView widget. #752
Conversation
The focus of this CL is the overall widget/controller structure, and test plumbing. The API surface is just the bare minimum at this point (only loadUrl).
3640b9a to
c5a3f05
Compare
bparrishMines
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
| class WebViewController { | ||
| WebViewController._(int id) { | ||
| _channel = new MethodChannel( | ||
| 'plugins.flutter.io/webview_$id', const StandardMethodCodec()); |
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: StandardMethodCodec() is the default value
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
| 'plugins.flutter.io/webview_$id', const StandardMethodCodec()); | ||
| } | ||
|
|
||
| MethodChannel _channel; |
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.
I think you can make this final MethodChannel _channel; by changing the constructor to:
WebViewController._(int id) : _channel = new MethodChannel(
'plugins.flutter.io/webview_$id', const StandardMethodCodec());
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.
good catch
| }); | ||
| } | ||
|
|
||
| class FakePlatformWebView { |
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 way of testing works too, but you could avoid making a Mock class by doing something similar in firebase_performance.
We started testing plugins by making the MethodChannel public and using the @visibleForTesting annotation. It lets us test the actual class instead of having to always make a Fake one for testing.
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 fake class is faking the Java implementation, the test is testing the real Dart WebView implementation.
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.
I was thinking of something along the lines of:
final List<MethodCall> log = <MethodCall>[];
WebViewController controller;
Future<dynamic> methodCallHandler(MethodCall methodCall) async {
log.add(methodCall);
switch (methodCall.method) {
case 'loadUrl':
return null;
default:
return null;
}
}
testWidgets('loadUrl', (WidgetTester tester) async {
await tester.pumpWidget(
WebView(
onWebViewCreated: (WebViewController webViewController) {
controller = webViewController;
},
),
);
expect(controller, isNotNull);
controller.channel.setMethodCallHandler(methodCallHandler);
log.clear();
controller.loadUrl('https://flutter.io');
expect(log, <Matcher>[
isMethodCall(
'loadUrl',
arguments: 'https://flutter.io',
)
]);
});
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.
Following offline discussion we decided we're happy with the fake approach here.
amirh
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.
Thanks!
| class WebViewController { | ||
| WebViewController._(int id) { | ||
| _channel = new MethodChannel( | ||
| 'plugins.flutter.io/webview_$id', const StandardMethodCodec()); |
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
| 'plugins.flutter.io/webview_$id', const StandardMethodCodec()); | ||
| } | ||
|
|
||
| MethodChannel _channel; |
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.
good catch
| }); | ||
| } | ||
|
|
||
| class FakePlatformWebView { |
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 fake class is faking the Java implementation, the test is testing the real Dart WebView implementation.
The focus of this CL is the overall widget/controller structure, and test plumbing. The API surface is just the bare minimum at this point (only loadUrl).
The focus of this CL is the overall widget/controller structure, and test plumbing. The API surface is just the bare minimum at this point (only loadUrl).
…lutter#752)" This reverts commit 4bc38d7.
The focus of this CL is the overall widget/controller structure, and
test plumbing.
The API surface is just the bare minimum at this point (only loadUrl).