[webview_flutter] Add onReceivedError callback #2639

bparrishMines · 2020-04-07T02:32:50Z

Description

This adds the feature where the webview can report an error when loading page resources.

After looking for error reporting in the Webview implementation of each platform I found Android uses onReceivedError in its WebviewClient and iOS uses webView:didFailNavigation:withError: and webView:didFailProvisionalNavigation:withError: in its WKNavigationDelegate.

However, each platform handles error codes differently. Android provides a WebResourceError which has an int code that is defined as a constant in WebViewClient. iOS uses a generic NSError and doesn't specify the possible values for the error code.

Related Issues

Fixes flutter/flutter#53023

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

Breaking Change

Does your PR require plugin users to manually update their apps to accommodate your change?

Yes, this is a breaking change (please indicate a breaking change in CHANGELOG.md and increment major revision).
No, this is not a breaking change.

amirh

Thanks!
Overall looks good, left a few nits and questions.

amirh · 2020-04-07T07:12:56Z

packages/webview_flutter/example/test_driver/webview_flutter_e2e.dart

+      final Map<String, String> errorResult = await errorCompleter.future;
+      expect(errorResult['errorCode'], isNotNull);
+      expect(errorResult['description'], isNotNull);
+    });


Let's add a test with a valid url that verifies the error callback is not invoked on a valid url. I'd use a data url like:

plugins/packages/webview_flutter/example/test_driver/webview_flutter_e2e.dart

Line 116 in c03166b

'data:text/html;charset=utf-8;base64,PCFET0NUWVBFIGh0bWw+',

amirh · 2020-04-07T07:18:57Z

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

+    }
+
+    final String message =
+        String.format(Locale.getDefault(), "Could not find a string for errorCode: %d", errorCode);


nit:

Is %d formatting affected by the locale?
Also seems like you can omit specifying Locale.getDefault() as this is the default for the String.format version that doesn't take a locale.

I only added the Locale.getDefault() to get rid of the lint warning. It does change the digit to whatever locale the device is set to. I don't think it would change the interpretation of the digit though.

Interesting, my read of https://docs.oracle.com/javase/7/docs/api/java/util/Formatter.html#syntax is that %d formatting is not affected by the locale.

amirh · 2020-04-07T07:33:48Z

packages/webview_flutter/lib/webview_flutter.dart

+  /// (e.g. The error code of `WebViewClient.ERROR_AUTHENTICATION` on Android
+  /// translates to `AUTHENTICATION` in Dart.
+  ///
+  /// On iOS, the error code will represent the "domain" and the "code" for an


My preference would be to avoid having different semantic meaning to the same field on different platforms.
Instead I'd have different fields with a single semantic meaning which may not be set on all platforms.

done. I added a domain only for iOS and a errorType that can be null on iOS.

amirh · 2020-04-07T07:35:36Z

packages/webview_flutter/lib/webview_flutter.dart

+  /// translates to `AUTHENTICATION` in Dart.
+  ///
+  /// On iOS, the error code will represent the "domain" and the "code" for an
+  /// `NSError` in Objective-C. The format will be `$domain: $code`. See


Is $code the http status code?

I think it's possible for it to be an http status code, but the documentation doesn't say anything about it:
https://developer.apple.com/documentation/webkit/wknavigationdelegate/1455623-webview?language=objc

However, their https://developer.apple.com/documentation/webkit/wkerrorcode?language=objc states that these are possible values. I can't tell if these are the ONLY possible values or not.

amirh · 2020-04-07T07:38:23Z

packages/webview_flutter/lib/webview_flutter.dart

 typedef void PageFinishedCallback(String url);

+/// Signature for when a [WebView] has failed to load a resource.
+typedef void ReceivedErrorCallback(String errorCode, String description);


When reading this my gut was that we should have an error class though I'm not finding good arguments any way 😄 (slight pro of an object is that it's easier for users to pass the error information around within their app)

I wonder if you had a reason to prefer not introducing an error class.

I was in favor of a data class as well. I chose not to create one to be consistent with the style of the WebViewPlatformCallbacksHandler. Method

void onJavaScriptChannelMessage(String channel, String message);

and

FutureOr<bool> onNavigationRequest({String url, bool isForMainFrame});

don't use them either.

However, since I need more than 2 parameters now, I created a WebResourceError class.

amirh · 2020-04-07T07:40:34Z

packages/webview_flutter/lib/webview_flutter.dart

    this.gestureRecognizers,
    this.onPageStarted,
    this.onPageFinished,
+    this.onReceivedError,


another nit: why onReceivedError and not onError/onResourceLoadError?

It was the name of the method in WebViewClient. Same as onPageStarted and onPageFinished. It was mostly to keep the pattern of method name origins, but I think WebResourceError is more expressive. (or something similar)

… valid url.

amirh

LGTM after addressing remaining comments

amirh · 2020-04-08T05:44:07Z

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

+    }
+
+    final String message =
+        String.format(Locale.getDefault(), "Could not find a string for errorCode: %d", errorCode);


Interesting, my read of https://docs.oracle.com/javase/7/docs/api/java/util/Formatter.html#syntax is that %d formatting is not affected by the locale.

amirh · 2020-04-08T05:44:26Z

packages/webview_flutter/example/test_driver/webview_flutter_e2e.dart

      expect(currentUrl, 'https://www.google.com/');
    });

+    testWidgets('onReceivedError', (WidgetTester tester) async {


nit: onWebResourceError

amirh · 2020-04-08T05:44:39Z

packages/webview_flutter/example/test_driver/webview_flutter_e2e.dart

+      if (Platform.isAndroid) expect(error.errorType, isNotNull);
+    });
+
+    testWidgets('onReceivedError is not called with valid url',


amirh · 2020-04-08T05:45:25Z

packages/webview_flutter/lib/platform_interface.dart

+  javaScriptResultTypeIsUnsupported,
+}
+
+/// On Android, the error code will be a constant from


This class comment probably needs an update as well?

amirh · 2020-04-08T05:46:28Z

packages/webview_flutter/lib/platform_interface.dart

+  /// for more information on error handling on iOS.
+  final String domain;
+
+  /// Describes the error.


Style guide says avoid useless documentation

amirh · 2020-04-08T05:47:28Z

packages/webview_flutter/lib/platform_interface.dart

+
+  /// The type of error.
+  ///
+  /// This is will never be null on Android, but can be null on iOS.


nit: s/is//

amirh · 2020-04-08T05:51:41Z

packages/webview_flutter/lib/src/webview_method_channel.dart

+                ? null
+                : WebResourceErrorType.values.firstWhere(
+                    (WebResourceErrorType type) {
+                      final String enumName = type.toString().replaceFirst(


nit, may be easier to follow as:

return type.toString() == '$WebResourceErrorType.${call.arguments['errorType']}';

amirh · 2020-04-08T05:54:54Z

Don't forget to add a version bump and a changelog entry

This reverts commit 8819b21. # Conflicts: # packages/webview_flutter/CHANGELOG.md # packages/webview_flutter/pubspec.yaml

bparrishMines added 2 commits April 6, 2020 14:18

Start of onReceivedError support

463b84b

Documentation

6964b9b

googlebot added the cla: yes label Apr 7, 2020

bparrishMines changed the title ~~[webview_flutter] Add onReceivedError callback~~ [WIP][webview_flutter] Add onReceivedError callback Apr 7, 2020

bparrishMines added 5 commits April 6, 2020 20:15

Format change and bug fix

38985d6

Android format change

70347cf

Add error test

c8bd541

formatting

8915e9d

We actually do need to support deprecated method

e7e5655

bparrishMines changed the title ~~[WIP][webview_flutter] Add onReceivedError callback~~ [webview_flutter] Add onReceivedError callback Apr 7, 2020

bparrishMines requested a review from amirh April 7, 2020 05:36

amirh reviewed Apr 7, 2020

View reviewed changes

bparrishMines added 5 commits April 7, 2020 13:54

Create WebResourceError. Seperate iOS and Android parts. Add test for…

d0e2ec1

… valid url.

formatting

c10e171

Add ios error types

4fb4b21

Format and change method name

7ebb729

fix flutter driver test

17f6246

amirh approved these changes Apr 8, 2020

View reviewed changes

bparrishMines added 5 commits April 8, 2020 10:26

Better documentation and good code suggestion

28e67e3

version bump

bcc1d45

typa

35fcf43

Remove error code docs and clearer error documetation

a3bfcc6

another documentation fix

e469165

bparrishMines merged commit 8819b21 into flutter:master Apr 9, 2020

bparrishMines deleted the onReceivedError branch April 9, 2020 00:54

jerryzhoujw pushed a commit to jerryzhoujw/plugins that referenced this pull request Apr 10, 2020

[webview_flutter] Add onReceivedError callback (flutter#2639)

3822348

EdwinRomelta pushed a commit to EdwinRomelta/plugins that referenced this pull request Jun 11, 2020

[webview_flutter] Add onReceivedError callback (flutter#2639)

733fe2e

jorgefspereira pushed a commit to jorgefspereira/plugins_flutter that referenced this pull request Oct 10, 2020

[webview_flutter] Add onReceivedError callback (flutter#2639)

9d5f901

FlutterSu pushed a commit to FlutterSu/flutter-plugins that referenced this pull request Nov 20, 2020

[webview_flutter] Add onReceivedError callback (flutter#2639)

5d41542

stuartmorgan-g mentioned this pull request Jul 13, 2021

Feature/web view failure listener #2416

Closed

13 tasks

[webview_flutter] Add onReceivedError callback #2639

[webview_flutter] Add onReceivedError callback #2639

Uh oh!

Conversation

bparrishMines commented Apr 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issues

Checklist

Breaking Change

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!

bparrishMines Apr 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

bparrishMines Apr 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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 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!

amirh commented Apr 8, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

bparrishMines commented Apr 7, 2020 •

edited

Loading

bparrishMines Apr 7, 2020 •

edited

Loading

bparrishMines Apr 7, 2020 •

edited

Loading