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

[url_launcher] Convert iOS to Pigeon #3481

Merged
merged 5 commits into from
Mar 18, 2023

Conversation

stuartmorgan
Copy link
Contributor

Replaces the manual method channel code with Pigeon. Includes minor adjustments to the boundary API:

  • Eliminates unused parameters (the old code was cloned directly from the old shared implementation, so included parameters that are only supported on Android).
  • Splits SVC and external launch paths on the Dart side rather than the native side, since it was trivial to do and it made the methods match the current native implementation structure that the method channel handler was calling out to.

I also added explicit handling of invalid URLs, which previously would just throw when we tried to use the nil NSURL and bubble up to the Dart side. The resulting wrapped-raw-OS-exception PlatformException was pretty low-information, and has caused a lot of confusion in the past (although now that the primary entry point to url_launcher is Uri based rather than String based, that's much less common). The exception strings match the macOS implementation.

  • While this is arguably breaking, I think it's extremely unlikely that anyone is relying on the specific values of a PlatformException that can only result from invalid input, and to the extent that they are they could have been broken at any time by minor changes to the exception message coming from iOS.
  • Long term we should be turning this into an ArgumentError in all implementations, but that really is a breaking change, so will require a concerted cross-implementation push.

Since there were no real native unit tests, I did some backfilling to help fill in coverage. I didn't try to be comprehensive (there's still no testing of the SVC paths), but it's substantially more than was there. I used a protocol and manual fake, rather than injecting an OCMock, so that the tests will be easy to convert to Swift later.

Part of flutter/flutter#117915

Pre-launch Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I read and followed the relevant style guides and ran the auto-formatter. (Unlike the flutter/flutter repo, the flutter/packages repo does use dart format.)
  • I signed the CLA.
  • The title of the PR starts with the name of the package surrounded by square brackets, e.g. [shared_preferences]
  • I listed at least one issue that this PR fixes in the description above.
  • I updated pubspec.yaml with an appropriate new version according to the pub versioning philosophy, or this PR is exempt from version changes.
  • I updated CHANGELOG.md to add a description of the change, following repository CHANGELOG style.
  • I updated/added relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making, or this PR is test-exempt.
  • All existing and new tests are passing.

@stuartmorgan
Copy link
Contributor Author

I used FUL for new classes, per flutter/flutter#102601, but didn't change the existing class. Let me know if you want me to fold that into this PR.

@stuartmorgan stuartmorgan added the autosubmit Merge PR when tree becomes green via auto submit App label Mar 17, 2023
@auto-submit auto-submit bot merged commit 3b3a09d into flutter:main Mar 18, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Mar 20, 2023
tarrinneal pushed a commit to flutter/flutter that referenced this pull request Mar 20, 2023
* 7636eb7 [go_router_builder] Support default value for `Set`, `List` and `Iterable` route parameters (flutter/packages#3391)

* 26c95da [image_picker] Move HashSet construction within if-statement (flutter/packages#3448)

* f5687b2 [image_picker] fix typos in comments (flutter/packages#3413)

* 84afba7 [image_picker] Migrate Android to Pigeon (flutter/packages#3476)

* 42927fc [image_picker]: Bump androidx.exifinterface:exifinterface from 1.3.3 to 1.3.6 in /packages/image_picker/image_picker_android/android (flutter/packages#3238)

* 9a44bdf Require Dart SDK 2.14, because of using APIs. (flutter/packages#3468)

* 12609a2 [ci] Clean up iOS simulators (flutter/packages#3458)

* 9b136a9 [ci/tool] Add external dependency validation (flutter/packages#3466)

* 11aab47 Manual roll Flutter from fb7e828 to 67e5f66 (8 revisions) (flutter/packages#3482)

* 784291b Update goldens (flutter/packages#3442)

* 43a42d1 [webview_flutter_android] Updates Dart and Java InstanceManagers (flutter/packages#3282)

* d0de136 [camera] Reland android flip/change camera while recording (flutter/packages#3460)

* 74fd094 [image_picker_android] Adjust file extension in FileUtils when it does not match its mime type (flutter/packages#3409)

* d2f1d2d [flutter_adaptive_scaffold] : 🐛 [FIX] : Issue: 121135. (flutter/packages#3253)

* 3d078b5 Roll Flutter from 67e5f66 to 53dfd23 (39 revisions) (flutter/packages#3484)

* 3b3a09d [url_launcher] Convert iOS to Pigeon (flutter/packages#3481)

* 80cd50a Roll Flutter from 53dfd23 to 6bd2b3c (17 revisions) (flutter/packages#3486)

* 998bb29 [webview_flutter] Updates the README with the migration of `WebView.initialCookies` and Hybrid Composition on (flutter/packages#3470)

* bbf86a7 Roll Flutter from 6bd2b3c to 3e4e092 (7 revisions) (flutter/packages#3490)
nploi pushed a commit to nploi/packages that referenced this pull request Jul 16, 2023
[url_launcher] Convert iOS to Pigeon
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
autosubmit Merge PR when tree becomes green via auto submit App p: url_launcher platform-ios
Projects
None yet
2 participants