Add a broadcast stream for FrameTiming #38574
Conversation
goderbauer
left a comment
There was a problem hiding this comment.
Maybe add a slightly more detailed PR description for future archeologists: Why this change? Is it just fixing the TODO? Is there an issue where somebody was asking for this to be exposed, etc.
| final TimingsCallback oldCallback = WidgetsBinding.instance.window.onReportTimings; | ||
| WidgetsBinding.instance.window.onReportTimings = (List<FrameTiming> timings) { | ||
| StreamSubscription<FrameTiming> firstFrameSubscription; | ||
| firstFrameSubscription = WidgetsBinding.instance.frameTimingStream.listen((FrameTiming timing) { |
There was a problem hiding this comment.
Would Stream.first be appropriate here since you cancel the stream after you get the first even right away?
https://master-api.flutter.dev/flutter/dart-async/Stream/first.html
There was a problem hiding this comment.
Added more details to the PR description.
Also added test which verifies that the broadcast stream won't be shut down (cancelled) by Stream.first. (Stream.first will only cancel its own subscription, and leave other subscription still running.)
There was a problem hiding this comment.
So, we should be able to use it here as well?
devoncarew
left a comment
There was a problem hiding this comment.
(I thought I'd sent these comments earlier in the day, but noticed they were still pending)
| int _frameTimingListenCount = 0; | ||
|
|
||
| void _onFrameTimingListen() { | ||
| _frameTimingListenCount += 1; |
There was a problem hiding this comment.
I believe that StreamController maintains this count for you - onListen is called when the first listener is added, and onCancel is called when the last listener unsubscribes.
There was a problem hiding this comment.
Thanks! I misread the API doc. Removing my counter.
| _frameTimingListenCount -= 1; | ||
| assert(_frameTimingListenCount >= 0); | ||
| if (_frameTimingListenCount == 0) { | ||
| window.onReportTimings = _oldTimingsCallback; |
There was a problem hiding this comment.
Trying to restore the _oldTimingsCallback is problematic - we don't know if anybody else has changed window.onReportTimings in the meantime, or is similarly trying to manage that field.
A solution w/o that issue would be to have onReportTimings be more of an implementation detail in dart:ui Window, and that that class itself to expose a broadcast stream. Otherwise things get complicated if we have more than one consumer that wants the frame timing info.
There was a problem hiding this comment.
I'd love to do that. Would that be a breaking change as we're removing onReportTimings and only expose this broadcast stream from dart:ui Window?
There was a problem hiding this comment.
It would be, but I think the only use of the API is in package:flutter itself. The breaking change process would be a good way to find any additional usages, and to let people who are using it know how to migrate to the new API (which would be a straightforward migration).
|
Close this in favor of flutter/engine#11041 We'll open a new framework PR once that engine PR lands to re-apply the stream listening and test change in this PR. |
Without this, developers have to override `onReportTimings` to listen for `FrameTiming`. That can potentially break previous `onReportTimings` listeners if they forget to call the old listener in their new callback. This PR replaces the similar RP in the framework: flutter/flutter#38574 Once this PR landed, we'll have to create another framework PR to use the stream to replace `onReportTimings` usages. Once that's done, we can then propose the breaking change of removing the deprecated `onReportTimings`.
This is the continuation of flutter/engine#11041 and #38574 This is not a breaking change as we're not removing `onReportTimings` API. We're simply removing the use of it in our framework.
Without this, developers have to override
onReportTimingsto listen forFrameTiming.That can potentially break previous
onReportTimingslisteners if they forget to callthe old listener in their new callback.
This change would make it safer and more convenient to listen for
FrameTiming(as illustrated in the removed TODO).
The following unit tests (and many other driver tests) covered this change:
setUpAll)