Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[webview_flutter] add events: onPageStarted, onReceivedError (with some simple error handling) and action: stopLoading. #1389

Open
wants to merge 6 commits into
base: master
from

Conversation

@hpoul
Copy link
Contributor

hpoul commented Mar 23, 2019

Description

  • Adds new events to WebView for onPageStarted and onReceivedError
    • Tries to combine error handling between iOS/Android
  • add stopLoading of webview.

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.

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • My PR includes unit or integration tests for all changed/updated/fixed behaviors (See Contributor Guide).
  • All existing and new tests are passing.
  • I updated/added relevant documentation (doc comments with ///).
  • The analyzer (flutter analyze) does not report any problems on my PR.
  • I read and followed the Flutter Style Guide.
  • The title of the PR starts with the name of the plugin surrounded by square brackets, e.g. [shared_preferences]
  • I updated pubspec.yaml with an appropriate new version according to the pub versioning philosophy.
  • I updated CHANGELOG.md to add a description of the change.
  • I signed the CLA.
  • I am willing to follow-up on review comments in a timely manner.

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.
@googlebot googlebot added the cla: yes label Mar 23, 2019
@hpoul hpoul force-pushed the hpoul:webview_flutter_events branch from 9aa3d6c to 0dc72b8 Mar 24, 2019
…me simple error handling) and action: stopLoading.
@hpoul hpoul force-pushed the hpoul:webview_flutter_events branch from 0dc72b8 to 74e0450 Mar 24, 2019
@amirh amirh added this to Awaiting initial triage in [experimental] Plugins PR tracking Mar 29, 2019
@amirh

This comment has been minimized.

Copy link
Contributor

amirh commented Mar 29, 2019

@hpoul please remove WIP label when the PR is ready for review.

@hpoul

This comment has been minimized.

Copy link
Contributor Author

hpoul commented Mar 29, 2019

@amirh there are just two things I am unsure before it's ready imo:

  1. unifying android/iOS - WKWebView and android's WebView present errors quite differently (and the iOS documentation is pretty sparse about which error callback is called when) - I have tried my best to unify the logic. The question is if that is preferable to just passing iOS and Android specifics back to flutter (like e.g. device_info would pass an iosInfo and androidInfo depending on platform.. I hope I chose a useful subset of errors which can occur, but there might be edge cases where it could be useful to have specific errors.. (although I guess it should be easy to add more error cases later in a backward compatible way)
  2. tests: I have added a simple test for onPageStarted .. maybe you could give me a hint on how to add e2e tests? will take a look tomorrow if there are any examples..
@amirh amirh moved this from Awaiting initial triage to Waiting for a collaborator to review in [experimental] Plugins PR tracking Mar 29, 2019
@hpoul hpoul changed the title [webview_flutter] WIP add events: onPageStarted, onReceivedError (with some simple error handling) and action: stopLoading. [webview_flutter] add events: onPageStarted, onReceivedError (with some simple error handling) and action: stopLoading. Mar 30, 2019
@hpoul hpoul marked this pull request as ready for review Mar 30, 2019
@hpoul

This comment has been minimized.

Copy link
Contributor Author

hpoul commented Apr 9, 2019

@amirh maybe you could give a short comment if this implementation could fit into the project, then i could start working on basing basic http auth support based on this PR.. (because supporting authentication requires error handling callbacks and I wouldn't want to completely refactor it again..) thanks. :)

(note: ups, had that comment posted in the wrong PR previously, sorry about that)

Copy link
Contributor

collinjackson left a comment

Thanks fo the contribution.

Can you add a CHANGELOG entry and bump the pubspec.yaml for release?

});
}

Future<void> testWebViewError(

This comment has been minimized.

Copy link
@collinjackson

collinjackson Apr 10, 2019

Contributor

I think this should be a Future<WebViewError> instead of taking a Completer as an argument if it is going to be a helper method. However, right now we are leaning towards not having helper methods in favor of more verbose tests that show exactly what's going on in each test.

This comment has been minimized.

Copy link
@hpoul

hpoul Apr 10, 2019

Author Contributor

okay, i have inlined the helper method: 05444fa
if you figure it's easier to figure out whats going on here, it's fine with me.. my personal taste would be to have the helper method, because this seems so verbose that it's actually harder to grasp whats going on (although "whats going on" for me, is knowing where the differences are between inputs vs. outputs, with so much boiler plate it's actually hard to see the different cases being tested..)
if it's okay this way, okay.. otherwise i can also revert it and change it to Future<WebViewError> whatever fits better..

@hpoul hpoul requested a review from amirh as a code owner Apr 10, 2019
@hpoul

This comment has been minimized.

Copy link
Contributor Author

hpoul commented Apr 10, 2019

@collinjackson thanks for the feedback 👍 i've bumped minor version number and added basic CHANGELOG and hopefully implemented your comments waiting for tests

Copy link

luohao8023 left a comment

@hpoul Hi there,how can i get the plugin with 'onPageStarted'?The latest version can't find this method.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
[experimental] Plugins PR tracking
  
Waiting for a collaborator to review
6 participants
You can’t perform that action at this time.