Create a WeakPtrFactory for use on the UI thread in VsyncWaiter #13781
Conversation
|
can we write a test for this? |
|
It should be covered by the existing Shell unittests, we just don't run them yet on Fuchsia (currently on the roadmap) |
|
Oh my bad, Shell unittests use their own implementation of VsyncWaiter for the test harness. I'll have a think about how best to do this but it may end up being put on the tail end of a long list of Fuchsia testing-related items as it'll likely need other work to be completed first (I'm thinking we can probably piggyback off the flutter_runner test harness work). |
|
@gw280 given that you have some subset of unittests already working on Fuchsia target device. How feasible is it to add a test to a new test suite that leverages this behavior. |
The unittests I'm currently looking at are only exercising the cross-platform code with stub implementations of things like VsyncWaiter so they unfortunately won't cover them. Luckily though, @dnfield already did some work to add a11y tests at the flutter_runner layer (which this test would need to live in) and it does actually build the Fuchsia VsyncWaiter so it might not be too hard to add the test to that harness. I'll look into that now. |
|
@iskakaushik unit test written and seems to work locally! @chinmaygarde I wasn't thinking straight yesterday, I've updated the changes today and fixed a couple of issues (notably: i was constructing the UI thread's WeakPtrFactory inside a closure instead of the ctor itself, and I was dereffing the WeakPtrFactory on the platform thread in the lambda capture instead of on the UI thread). This has now been resolved and the test passes. |
| // thread so we have ordering guarantees (see ::AwaitVSync()) | ||
| fml::TaskRunner::RunNowOrPostTask( | ||
| task_runners_.GetUITaskRunner(), fml::MakeCopyable([this]() mutable { | ||
| this->weak_factory_ui_ = |
There was a problem hiding this comment.
@chinmaygarde should we block this call. I think its ok to not block but jut want to confirm.
There was a problem hiding this comment.
I think this is fine because of the reason mentioned in the comment.
| // thread so we have ordering guarantees (see ::AwaitVSync()) | ||
| fml::TaskRunner::RunNowOrPostTask( | ||
| task_runners_.GetUITaskRunner(), fml::MakeCopyable([this]() mutable { | ||
| this->weak_factory_ui_ = |
There was a problem hiding this comment.
I think this is fine because of the reason mentioned in the comment.
4 participants
This fixes the failed assertion that the WeakPtr is being used on the wrong thread.
Issue: flutter/flutter#44480