-
Notifications
You must be signed in to change notification settings - Fork 5.9k
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
iOS Background Platform Channels #29665
iOS Background Platform Channels #29665
Conversation
0e26a3a
to
e96fac1
Compare
8f9bdc2
to
0ba85eb
Compare
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 nit
codec:codec | ||
taskQueue:taskQueue]; | ||
FlutterMessageHandler handler = ^(id _Nullable message, FlutterReply callback) { | ||
NSLog(@"hey"); |
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.
nit: avoid extra logs even in tests
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.
done
// Called from any thread. | ||
// TODO(gaaclarke: This vestigal from the Android implementation, find a way | ||
// to migrate this to PlatformMessageHandlerAndroid. |
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.
Maybe just make it not purely virtual and have a no-op implementation in the base class?
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.
That just puts wallpaper over the issue. I'd like this to stick out like a sore thumb since it should be fixed. It's just a pain given how things are divided up.
...once CI is happy |
The luci link is missing but the Mac Unopt failure is https://ci.chromium.org/p/flutter/builders/try/Mac%20Unopt/6227. |
15b6415
to
02293bb
Compare
I'm landing this with luci-engine on red. It's not red if you actually check the status and autoroller PRs are landing and that check is not showing up red for them. |
* iOS Background Platform Channels (flutter#29665) * added test that passes before this change, and fails after it * started supporting backwards compatible usage of setting handlers
issue: flutter/flutter#91635
Pre-launch Checklist
writing and running engine tests.
///
).If you need help, consider asking for advice on the #hackers-new channel on Discord.