redux of a change to use new engine APIs for Flutter.Frame events #34365
Conversation
goderbauer
added
the
framework
| if (!kReleaseMode) { | ||
| int frameNumber = 0; | ||
|
|
||
| window.onReportTimings = (List<FrameTiming> timings) { |
There was a problem hiding this comment.
If the app has already set a window.onReportTimings listener, will this make it not work? Maybe we should also call the original onReportTimings if it's not null.
There was a problem hiding this comment.
This is called pretty early in app initialization; I don't know how likely it is that we'll lose the race condition with user code.
If a user of the API is responsible for delegating to the last person who set onReportTimings, how would that user know to stop calling the previous closure when that closure was no longer interested in events?
If we're concerned about this, I think an option here is to adjust the API to use a broadcast stream based approach (Stream<FrameTiming> get onFrameTimings). Clients could add themselves as a listener to the stream, and remove themselves, without having to worry about other potential users of the API interfering with them.
There was a problem hiding this comment.
Right, I think I'll keep the current API for a simple engine-to-framework communication, and add a broadcast stream for flexibility later as a pure framework pull request.
There was a problem hiding this comment.
OK, makes sense.
| if (!kReleaseMode) { | ||
| int frameNumber = 0; | ||
|
|
||
| window.onReportTimings = (List<FrameTiming> timings) { |
There was a problem hiding this comment.
Right, I think I'll keep the current API for a simple engine-to-framework communication, and add a broadcast stream for flexibility later as a pure framework pull request.
* master: retry on HttpException (flutter#34526) 308c113 Roll src/third_party/skia 057a32d9a2c8..5fe7429babe2 (1 commits) (flutter/engine#9342) (flutter#34583) Add back ability to override the local engine in Gradle (flutter#34460) Added customizable padding for the segmented controll (flutter#34555) 466a1d8 Roll src/third_party/skia 3721688f64a5..057a32d9a2c8 (1 commits) (flutter/engine#9340) (flutter#34558) Handles parsing APK manifests with additional namespaces or attributes (flutter#34535) add route information to Flutter.Navigation events (flutter#34508) Separate web and io implementations of network image (flutter#34112) pass .packages path to snapshot invocation (flutter#34517) [Material] Fix TextDirection and selected thumb for RangeSliderThumbShape and RangeSliderValueIndicatorShape (flutter#34501) redux of a change to use new engine APIs for Flutter.Frame events (flutter#34521) Roll engine b0757e6..f44b7b5 (4 commits) (flutter#34533) Roll engine 2589785..b0757e6 (6 commits) (flutter#34522) Revert "Revert "redux of a change to use new engine APIs for Flutter.Frame events (flutter#34365)" (flutter#34514)" (flutter#34530) Make sure fab semantics end up on top (flutter#34512) Revert "redux of a change to use new engine APIs for Flutter.Frame events (flutter#34365)" (flutter#34514) redux of a change to use new engine APIs for Flutter.Frame events (flutter#34365)
4 participants
Description
Redux of #34243
Flutter.Frameevent - sent over the VM service protocol - to use newly available engine apis. This will make the data match that reported via the Timeline events, and allows us to include timing info for both the dart and gpu portions of the frameRelated Issues
none
Tests
I added the following tests:
Flutter.Frameevent in the binding testsJSON-RPC error 103: Stream already subscribed)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.///).flutter analyze --flutter-repo) does not report any problems on my PR.Breaking Change
Does your PR require Flutter developers to manually update their apps to accommodate your change?