-
Notifications
You must be signed in to change notification settings - Fork 3.3k
[url_launcher] Android API 34 support #4660
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] Android API 34 support #4660
Conversation
…equired by target api 34" (flutter#4027)" This reverts commit baeca88.
packages/url_launcher/url_launcher_android/example/integration_test/url_launcher_test.dart
Outdated
Show resolved
Hide resolved
also fixes https://b.corp.google.com/issues/294558650 |
await Future<void>.delayed(const Duration(seconds: 3)); | ||
await launcher.closeWebView(); | ||
// Delay required to catch android side crashes in onDestroy | ||
await Future<void>.delayed(const Duration(seconds: 3)); |
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 we would need flutter/flutter#20034 to remove this; a TODO referencing that issue wouldn't hurt.
In the meantime, do we really need 3 seconds to reliably catch closing the page? That seems crazy long.
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.
3s was the shortest I could make it and have the test reliability catch a crash that can happen in onDestroy on my physical phone. I ended up verifying on an emulator and actually needed to bump the delay to 5s. 3 would not reliably catch the potential crash and 4s was flakey in catching the crash.
I think we need to wait the full 5s otherwise this test does not actually catch a regression in the code that was fixed.
packages/url_launcher/url_launcher_android/example/integration_test/url_launcher_test.dart
Outdated
Show resolved
Hide resolved
packages/url_launcher/url_launcher_android/example/integration_test/url_launcher_test.dart
Outdated
Show resolved
Hide resolved
…esting on emulators, update documentation with an explination of why the delay
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 with nits.
Thanks for investigating more on the delay. It's not ideal, but at least this is an integration test where taking longer is expected.
packages/url_launcher/url_launcher_android/example/integration_test/url_launcher_test.dart
Outdated
Show resolved
Hide resolved
packages/url_launcher/url_launcher_android/example/integration_test/url_launcher_test.dart
Outdated
Show resolved
Hide resolved
flutter/packages@ac41376...881c1f5 2023-08-09 engine-flutter-autoroll@skia.org Roll Flutter from 436df69 to f4c25bb (28 revisions) (flutter/packages#4666) 2023-08-09 reidbaker@google.com [url_launcher] Android API 34 support (flutter/packages#4660) 2023-08-08 67326251+Franreno@users.noreply.github.com Migrating styleFrom API to new version. (flutter/packages#4540) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages-flutter-autoroll Please CC flutter-ecosystem@google.com,rmistry@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
fixed #flutter/flutter/issues/126460
Reland of #3973 which incorrectly used application context when registering and not when unregistering. This pr uses activity context for both which is aligned with what the behavior was before.
Tested with manual test and new integration test.
Pre-launch Checklist
dart format
.)[shared_preferences]
pubspec.yaml
with an appropriate new version according to the [pub versioning philosophy], or this PR is [exempt from version changes].CHANGELOG.md
to add a description of the change, [following repository CHANGELOG style].///
).