note: we intentionally pass a copy of compositionOrder by value here since the first thing we do after tossing this in the task queue is clear it. Passing by reference would result in a loop over nothing.

The review bot seems to really want to mechanically tell you to pass self as weakSelf, and do a check in the closure to capture it strongly. Fortunately it didn't on this pass. That would introduce a serious bug. The whole point of the -reset method is to remove any platform views from their superviews. We need that to happen whether or not FlutterPlatformViewsController is around (it almost always will be) or not (race condition during Flutter teardown).

The correct solution is to pass the set of platformviews to remove by value, as I've done here.

I guess it worked before because of merged raster/platform thread, so that runNowOrPostTask runs immediately, before executing self.platformViews.clear()

See below; this isn't a change -- I've just saved a line by doing fml::MakeCopyable here instead of below.

The strong self-capture in this block is safe (and required); this code is effectively synchronous. Notice that right below this we call latch->Wait() to block.

We don't retain this block and it finishes executing right below with the latch, so no extra retention.

Can you put this in a comment? (In case someone "fix" the retain cycle into a wrong state.

Consolidating this in a helper class isn't strictly-speaking required for this but this code was duplicated in basically every test case, so I think worth doing.

Yes, I hate that we're using OCMock too. I have a followup that migrates us off this nonsense onto real objects (and a couple small fakes).

Not a change; just fixing a typo.

This is a semantic change, but a reasonable one based on @hellohuanlin's feedback on a previous patch. There are a couple potential sources of error with this sort of thing:

1. If delayTime is not an inverse power of two, there's floating point truncation error.
2. In the insanely rare case where an NTP clock sync happens during the delay, the wall clock difference might be different from the actual delay interval.

Cleanup that should have been here before.

This will go away in the future.

Eventually this will be the only initializer.

Moved to FlutterVSyncClientTestHelper.

I've added a bunch of comments with rationale because the behaviour of CADisplayLink and retention here is more complex than was initially obvious to me. Happy to remove them if they seem extraneous but this was kind of painful to work out, so seemed useful to leave breadcrumbs for others.

nice

maybe add a comment since the it reads a bit odd. what is this trying to verify?

This is checking stable identity -- since these are wrappers, in theory we could create new throwaway wrapper each time we get it, but it's better to just do a single allocation per runner and keep em identical at the Obj-C level too.

FlutterOwned sounds a bit unclear to me when I first read it. How about FakeFlutterFMLThreadTaskRunner?

What about FlutterFMLThreadTaskRunner since it's a real task runner implemented around fml::Thread?

I suggested Fake because it's used in test only, so maybe TestFlutterFMLTaskRunner? Naming is hard, up to you.

good renaming

I guess it worked before because of merged raster/platform thread, so that runNowOrPostTask runs immediately, before executing self.platformViews.clear()

love to see these C++ closures gone. ObjC syntax looks so much more familiar to an iOS engineer!

Can you put this in a comment? (In case someone "fix" the retain cycle into a wrong state.

the first 2 funcs seems to be sharable to non-platform view tests in the future? Maybe move them to a separate file?

Seems reasonable; unless any objection, I'll do that in a followup. Probably a good fit for the FlutterFMLTaskRunnerTestHelpers.h except they're C++ and that file should be pure Obj-C. Will find a spot for them and move them.

why cast here?

Good point -- totally unnecessary. I was following the same approach as on line 609 where it was necessary. Removed.

added by accident?

Thanks for spotting -- all these patches were carved out of one big mega-patch and this was probably leftover copied from the main (Obj-C header) which includes DisplayLinkManager (that holds a CADisplayLink). Removed.