[webview_flutter] Adds platform implementations for onHttpError callback #3695

HugoOlthof · 2023-04-12T15:38:26Z

Reference #3278

Pre-launch Checklist

If you need help, consider asking for advice on the #hackers-new channel on Discord.

stuartmorgan · 2023-04-12T16:18:31Z

We should make sure #3675 gets the baseline file updates and then lands, and then this has that merged in, before this lands, so that we can better see what the baseline changes here are and if any new warnings are being introduced.

This doesn't have to block initial review, but I'm flagging it now so that we don't forget.

bparrishMines · 2023-04-15T21:55:49Z

#3675 has been merged, so this PR will need to pull from main and handle the Android lint warnings.

bparrishMines

LGTM with last few nits

packages/webview_flutter/webview_flutter_wkwebview/ios/Classes/FWFDataConverters.h

packages/webview_flutter/webview_flutter_wkwebview/CHANGELOG.md

stuartmorgan · 2023-04-25T15:12:32Z

...s/webview_flutter/webview_flutter_android/example/integration_test/webview_flutter_test.dart

+            }),
+        )
+        ..loadRequest(
+          LoadRequestParams(uri: Uri.parse('https://www.google.com')),


This needs to be primaryUrl, not an actual site. Using real sites in integration tests makes tests much more prone to flake.

stuartmorgan · 2023-04-25T15:13:21Z

...webview_flutter/webview_flutter_wkwebview/example/integration_test/webview_flutter_test.dart

+        ..loadRequest(
+          LoadRequestParams(
+            uri: Uri.parse(
+              'https://www.google.nl',


primaryUrl

stuartmorgan · 2023-04-25T15:21:28Z

packages/webview_flutter/webview_flutter_wkwebview/ios/Classes/FWFNavigationDelegateHostApi.m

+                                                     FlutterError *error) {
+                                          NSAssert(!error, @"%@", error);
+                                          decisionHandler(
+                                              FWFNativeWKNavigationResponsePolicyFromEnumData(


This has the some problem as the code discussed recently in the permission callback PR; it will unconditionally call the callback with an invalid value if Apple ever adds a new enum value. @bparrishMines Are you going to mass-fix this in a fast follow? I'm concerned about replicating a pattern that we know is dangerous.

This is fixed now with #3812.

This also only affects enums that we receive from Objective-C. The Dart code shouldn't be able to return an enum the native side doesn't recognize.

stuartmorgan · 2023-04-25T15:42:05Z

packages/webview_flutter/webview_flutter_android/lib/src/android_webview_controller.dart

+      ) {
+        if (weakThis.target?._onHttpError != null) {
+          weakThis.target!._onHttpError!(
+              HttpResponseError(statusCode: response.statusCode));


I'm confused; how is this callback intended to work in practice? You are throwing away the request object before it gets to the client, which means they will have absolutely no way of knowing whether the error is coming from a navigation (and if so to where) or a resource on the page (and if so, what). I'm not clear on what non-trivial use case a client could satisfy without having the request.

Am I missing something here?

/cc @bparrishMines who reviewed the overall PR, since it's core to the overall design. (It looks like the main PR only had one review before sub-parts started landing? Ideally that should have been two.)

@stuartmorgan It makes sense to include the request in the HttpResponseError object. Is it better to create a platform specific implementation for HttpResponseError and include the request in there, like AndroidWebResourceError, or include it in the HttpResponseError object itself?

Since it seems core to using this correctly, I would include it in the base object; it could be nullable in case there are implementations or cases where it's not available.

@stuartmorgan Sorry about that. I've been reviewing all of the WebView PRs as the owner of both the Android and iOS implementation, so I assumed it was fine to be the only approval for the aggregate PR. I'll start getting secondary reviews for those as well.

I agree with creating adding a request object to HttpResponseError.

It actually looks like Android and iOS have a request object and a response object.

Android:
https://developer.android.com/reference/android/webkit/WebResourceRequest
https://developer.android.com/reference/android/webkit/WebResourceResponse

iOS:
https://developer.apple.com/documentation/foundation/nsurlrequest?language=objc
https://developer.apple.com/documentation/foundation/nshttpurlresponse

In their respective methods,
https://developer.apple.com/documentation/webkit/wknavigationdelegate/1455643-webview?language=objc
https://developer.android.com/reference/android/webkit/WebViewClient#onReceivedHttpError(android.webkit.WebView,%20android.webkit.WebResourceRequest,%20android.webkit.WebResourceResponse)

Android provides the request and response and iOS provides the response. I think we could add a nullable LoadRequestParams to HttpResponseError AND a new nullable WebResourceResponse field.

HugoOlthof · 2023-05-17T08:36:33Z

@stuartmorgan @bparrishMines I've added a new PR #4025 which adds the request to the HttpResponseError object.

stuartmorgan · 2023-06-13T19:55:42Z

Update from triage: the response object PR, which is blocking this, is still in progress.

stuartmorgan · 2023-07-13T19:01:48Z

Update from triage: still waiting for #4025, then this can proceed.

Hixie · 2023-10-24T22:59:01Z

This is blocked on #4025 which is currently in review.

stuartmorgan · 2024-01-09T20:52:13Z

Update from triage: #5790 has been created as a copy of the blocking PR, to work on unblocking this.

stuartmorgan · 2024-01-30T21:00:58Z

Update from triage: the blocker has been landed, so this is ready to be updated.

stuartmorgan · 2024-03-05T20:48:35Z

Closing in favor of the PR cross-linked just above, as this one isn't editable by the Flutter team.

…plementations for onHttpError (#6149) Copy of #3695 since it doesn't contain permission to edit from contributors. Part of flutter/flutter#39502 Full PR #3278

…plementations for onHttpError (flutter#6149) Copy of flutter#3695 since it doesn't contain permission to edit from contributors. Part of flutter/flutter#39502 Full PR flutter#3278

…plementations for onHttpError (#6149) Copy of flutter/packages#3695 since it doesn't contain permission to edit from contributors. Part of flutter/flutter#39502 Full PR flutter/packages#3278

HugoOlthof requested review from cyanglaz and bparrishMines as code owners April 12, 2023 15:38

github-actions bot added p: webview_flutter platform-android platform-ios labels Apr 12, 2023

HugoOlthof mentioned this pull request Apr 12, 2023

[webview_flutter] Adds onHttpError callback to NavigationDelegate to catch HTTP error status codes #3278

Closed

11 tasks

ghost force-pushed the on-http-error-platform-implementations branch 2 times, most recently from c550592 to d62dfc3 Compare April 18, 2023 11:01

stuartmorgan self-requested a review April 18, 2023 20:05

ghost force-pushed the on-http-error-platform-implementations branch from d62dfc3 to 7a6086a Compare April 20, 2023 07:08

bparrishMines approved these changes Apr 20, 2023

View reviewed changes

bparrishMines mentioned this pull request Apr 20, 2023

PlatformException(sign_in_failed, com.google.android.gms.common.api.ApiException: 10: , null, null) #3766

Closed

HugoOlthof added 4 commits April 21, 2023 09:01

Adds platform implementations for onHttpError callback

931f2ef

Fixes Android lint errors

301a0e7

Bumps versions

6f4a864

Fixes feedback

59b12f2

ghost force-pushed the on-http-error-platform-implementations branch from 7a6086a to 59b12f2 Compare April 21, 2023 07:01

stuartmorgan changed the title ~~[webview_flutter_android] [webview_flutter_wkwebview] Adds platform implementations for onHttpError callback~~ [webview_flutter] Adds platform implementations for onHttpError callback Apr 25, 2023

stuartmorgan requested changes Apr 25, 2023

View reviewed changes

HugoOlthof mentioned this pull request May 17, 2023

[webview_flutter_platform_interface] Adds WebResourceRequest to HttpResponseError #4025

Closed

11 tasks

bparrishMines added the federated: partial_changes PR that contains changes for only a single package of a federated plugin change label Jul 17, 2023

bparrishMines mentioned this pull request Feb 16, 2024

[webview_flutter_android][webview_flutter_wkwebview] Adds platform implementations for onHttpError #6149

Merged

11 tasks

stuartmorgan closed this Mar 5, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[webview_flutter] Adds platform implementations for onHttpError callback #3695

[webview_flutter] Adds platform implementations for onHttpError callback #3695

HugoOlthof commented Apr 12, 2023

stuartmorgan commented Apr 12, 2023

bparrishMines commented Apr 15, 2023

bparrishMines left a comment

stuartmorgan Apr 25, 2023

stuartmorgan Apr 25, 2023

stuartmorgan Apr 25, 2023

bparrishMines May 19, 2023

stuartmorgan Apr 25, 2023

stuartmorgan Apr 25, 2023

HugoOlthof May 2, 2023 •

edited

Loading

stuartmorgan May 10, 2023

bparrishMines May 19, 2023

bparrishMines May 19, 2023

HugoOlthof commented May 17, 2023

stuartmorgan commented Jun 13, 2023

stuartmorgan commented Jul 13, 2023

Hixie commented Oct 24, 2023

stuartmorgan commented Jan 9, 2024

stuartmorgan commented Jan 30, 2024

stuartmorgan commented Mar 5, 2024

[webview_flutter] Adds platform implementations for onHttpError callback #3695

[webview_flutter] Adds platform implementations for onHttpError callback #3695

Conversation

HugoOlthof commented Apr 12, 2023

Pre-launch Checklist

stuartmorgan commented Apr 12, 2023

bparrishMines commented Apr 15, 2023

bparrishMines left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HugoOlthof May 2, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HugoOlthof commented May 17, 2023

stuartmorgan commented Jun 13, 2023

stuartmorgan commented Jul 13, 2023

Hixie commented Oct 24, 2023

stuartmorgan commented Jan 9, 2024

stuartmorgan commented Jan 30, 2024

stuartmorgan commented Mar 5, 2024

HugoOlthof May 2, 2023 •

edited

Loading