This repository has been archived by the owner on Feb 22, 2023. It is now read-only.
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.
[camera] Run iOS methods on UI thread by default #4140
[camera] Run iOS methods on UI thread by default #4140
Changes from 15 commits
dea50fc
da83b51
277d923
9bd75bd
a079af9
bd7d8eb
f0a2bb0
4125244
71e321e
bdfde1a
e6b848c
d59a80d
0f35095
0b63dea
8bc557f
b53ab3e
b6acf82
9f31156
a4cf79e
2b84db2
92c2d00
2722cf4
4a1155a
04566ae
57c63ae
5c69728
e2ba654
17d1f5d
a0be148
ee453bc
cd48ddd
98fe08a
a4e5262
39d1438
3a12cbf
8e8bdfc
3e86910
9093887
1d51203
f0b9f53
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't there be a thread assertion in this code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you mean with the thread assertion? That we verify that it is not the main thread?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't remember what I meant by having that comment in this location unfortunately; maybe I was forgetting the flow and thinking this was after the bounce back to the main thread.
However, the more general questions is still valid: the goal of this PR is to ensure that methods are getting callbacks on the main thread, so why isn't there any code in the test that calls into the camera handler with a result object controlled by the test, and actually asserts that the callback was on the main thread? Like this: https://github.com/flutter/plugins/blob/master/packages/local_auth/example/ios/RunnerTests/FLTLocalAuthPluginTests.m#L62-L64
It is not clear to me what in this test would deterministically fail without the fix from this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@stuartmorgan I have now added ThreadSafeFlutterResultTests that test that the result is always called on the main thread. These tests in CameraMethodChannelTests just verify that ThreadSafeFlutterResult is used, not how that is working.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Per the comment in the other file: please remove these and make them local to the test. (Having the object under test be part of the fixture state is always a major red flag to me unless there's a very compelling reason.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.