This repository was archived by the owner on Feb 25, 2025. It is now read-only.
Add callback to Embedder API to respond to new channel listeners, and use for Windows lifecycle #44827
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
yaakovschectman
added
affects: desktop
platform-windows
embedder
loic-sharma
reviewed
Aug 18, 2023
loic-sharma
reviewed
Aug 18, 2023
loic-sharma
reviewed
Aug 18, 2023
yaakovschectman
requested review from
cbracken and
chinmaygarde
as code owners
loic-sharma
reviewed
Aug 18, 2023
loic-sharma
reviewed
Aug 18, 2023
loic-sharma
reviewed
Aug 28, 2023
loic-sharma
reviewed
Aug 28, 2023
loic-sharma
approved these changes
Aug 28, 2023
There was a problem hiding this comment.
Looks great, excellent work! Please get an approval from @cbracken or @chinmaygarde on the changes in //flutter/shell/platform/embedder/embedder.h before merging.
cbracken
reviewed
Aug 28, 2023
| const char* channel; | ||
| /// True if a listener has been set, false if one has been cleared. | ||
| bool listening; | ||
| } FlutterChannelUpdate; |
There was a problem hiding this comment.
Are we 100% certain that we'll never want to add more members to this struct? If not, add a size_t struct_size member as the first field in the struct and use safe dereference macros when reading from it.
There was a problem hiding this comment.
Oh derp I completely forgot this 🤦. Yup let's add a struct_size to be safe
cbracken
reviewed
Aug 28, 2023
cbracken
reviewed
Aug 28, 2023
| /// @param[in] listening Whether the listener has been set (true) or | ||
| /// cleared (false). | ||
| /// | ||
| virtual void SendChannelUpdate(std::string name, bool listening) = 0; |
There was a problem hiding this comment.
This signature doesn't appear to match the implementation. std::string vs const std::string& name. Based on @loic-sharma's comment, looks like maybe this is the correct signature but the one lower down static void SendChannelUpdate(const std::string& name, bool listening); should be corrected?
There was a problem hiding this comment.
What non-matching implementation to this signature are you looking at? It is implemented in runtime_controller.cc:405, where the signatures match.
cbracken
reviewed
Aug 28, 2023
cbracken
reviewed
Aug 28, 2023
cbracken
reviewed
Aug 28, 2023
Co-authored-by: Chris Bracken <chris@bracken.jp>
Co-authored-by: Chris Bracken <chris@bracken.jp>
|
Looks great -- the lint would prefer const ref for the strings, which seems fine to me. I don't think we're mutating them anywhere. |
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/flutter
that referenced
this pull request
Aug 29, 2023
…eners, and use for Windows lifecycle (flutter/engine#44827)
auto-submit bot
pushed a commit
to flutter/flutter
that referenced
this pull request
Aug 29, 2023
…133604) flutter/engine@01a1579...1feb930 2023-08-29 109111084+yaakovschectman@users.noreply.github.com Add callback to Embedder API to respond to new channel listeners, and use for Windows lifecycle (flutter/engine#44827) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC aaclarke@google.com,rmistry@google.com,zra@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
gaaclarke
pushed a commit
to gaaclarke/engine
that referenced
this pull request
Aug 30, 2023
… use for Windows lifecycle (flutter#44827) Supplant the temporary solution flutter#44238 with a more elegant solution with am embedder API callback. The windows engine provides a callback that enables graceful exit and app lifecycle when the platform and lifecycle channels are listened to, respectively. flutter/flutter#131616 ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide] and the [C++, Objective-C, Java style guides]. - [x] I listed at least one issue that this PR fixes in the description above. - [x] I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test-exempt. See [testing the engine] for instructions on writing and running engine tests. - [x] I updated/added relevant documentation (doc comments with `///`). - [ ] I signed the [CLA]. - [x] All existing and new tests are passing. If you need help, consider asking for advice on the #hackers-new channel on [Discord]. <!-- Links --> [Contributor Guide]: https://github.com/flutter/flutter/wiki/Tree-hygiene#overview [Tree Hygiene]: https://github.com/flutter/flutter/wiki/Tree-hygiene [Flutter Style Guide]: https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style [testing the engine]: https://github.com/flutter/flutter/wiki/Testing-the-engine [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/wiki/Tree-hygiene#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/wiki/Chat --------- Co-authored-by: Chris Bracken <chris@bracken.jp>
2 tasks
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Supplant the temporary solution #44238 with a more elegant solution with am embedder API callback. The windows engine provides a callback that enables graceful exit and app lifecycle when the platform and lifecycle channels are listened to, respectively.
flutter/flutter#131616
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.