Allow multiple TimingsCallbacks#43676
Conversation
This fixes flutter#39277
liyuqian
force-pushed
the
scheduler_binding
branch
from
5733255 to
1e9582c
Compare
| // TODO(liyuqian): once this is merged, modify the doc of | ||
| // [Window.onReportTimings] inside the engine repo to recommend using this | ||
| // API instead of using [Window.onReportTimings] directly. | ||
| _timingsCallbacksEnabled[callback] = true; |
There was a problem hiding this comment.
If I add the same callback twice it will only get called once? That's probably expected, but might be worth documenting?
There was a problem hiding this comment.
I've modified it to use a list so now it actually gets called twice. Documented such behavior too.
| assert(window.onReportTimings == null); | ||
| window.onReportTimings = (List<FrameTiming> timings) { | ||
| for (TimingsCallback callback in _timingsCallbacks) { | ||
| callback(timings); |
There was a problem hiding this comment.
What if callback removes itself from _timingsCallbacks? You probably want to iterate over a copy of _timingsCallbacks to avoid problems (_timingsCallbacks.toList() should work).
There was a problem hiding this comment.
The original map was for that. But I've changed it the the cloned list approach as you suggested. I was trying to save some copying time but I guess the saving isn't worth the confusion.
| /// Removes a callback that was earlier added by [addTimingsCallback]. | ||
| void removeTimingsCallback(TimingsCallback callback) { | ||
| assert(_timingsCallbacksEnabled.containsKey(callback)); | ||
| _timingsCallbacksEnabled[callback] = false; |
There was a problem hiding this comment.
Why do you keep track of whats enabled/disabled? Why not just remove it?
It also looks like this Map never gets cleaned up and the callback (and all the resources closured in to it) will be held in memory indefinitely. That seems like a leak?
There was a problem hiding this comment.
Changed to list and removed the calllback when needed.
|
|
||
| void _executeTimingsCallbacks(List<FrameTiming> timings) { | ||
| final List<TimingsCallback> clonedCallbacks = | ||
| List<TimingsCallback>.from(_timingsCallbacks); |
There was a problem hiding this comment.
nit: either indent further or move to previous line.
There was a problem hiding this comment.
Thanks for catching this! I thought I indented by somehow it's missing...
| final List<TimingsCallback> clonedCallbacks = | ||
| List<TimingsCallback>.from(_timingsCallbacks); | ||
| for (TimingsCallback callback in clonedCallbacks) { | ||
| callback(timings); |
There was a problem hiding this comment.
In other places where we have similar code we check here if the callback is still in clonedCallbacks, for example:
flutter/packages/flutter/lib/src/foundation/change_notifier.dart
Lines 205 to 206 in b31ca1a
That way if your list is [callbackA, callbackB] and callbackA removes callbackB, callbackB is not called again. This would probably also make sense here?
Also from that link: Should we catch exceptions here that happen while executing callback?
There was a problem hiding this comment.
Done. Also added a test for the error handling.
| } | ||
| } | ||
|
|
||
| final List<TimingsCallback> _timingsCallbacks = <TimingsCallback>[]; |
There was a problem hiding this comment.
nit: I would move this field up above addTimingsCallback, but that's probably up to personal taste.
goderbauer
added
the
framework
Piinks
added
the
c: performance
This fixes flutter#39277 The following tests cover this change: - packages/flutter/test/foundation/service_extensions_test.dart - packages/flutter/test/scheduler/scheduler_test.dart
4 participants
This fixes #39277
The following tests cover this change: