The expectation is that all shell methods are called on the platform task runner. So let's post the task to the correct task runner. I don't think anyone ever uses the run status to log an error. So, we can return void here. Or, better still, take a callback that returns the result.

Fixed. I've also "fixed" the problems I was having with Flutter tester by just exposing the things it needs through the shell. We may want to remove those but that will be an effort I'm less familiar with the internals of at this point.

Additionally, I'm adding back the getEngine but marking ti deprecated to make the eventual roll to Fuchsia less painful.

I think it would be better to have the result callback be executed on the calling task runner (the platform task runner in this case). Since doing the re-threading at the each possible call-site is tedious, you can wrap the same in and closure that does the re-threading and then pass that around as normal. See the image decoder for a use of this pattern.

On a related note, since forgetting to invoke the closure in the error case is possible, maybe we should have an RAII wrapper for the same. I have a prototype for this in a different patch that is not in yet.

That way, you can wrap the callback in a closure that does the re-threading and put it in that RAII wrapper. You could then invoke the same only in the success case and ensure that all erroneous cases invoke the callback and on the right thread.

Something like the following should work for now:

auto result = [runner = task_runner_.GetPlatformTaskRunner(), result_callback]
    (Engine::RunStatus status){
  if (!result_callback) {
    return;
  }
  runner->PostTask([result_callback, status](){ result_callback(status); });
};

// Pass result around and invoke it directly (no null check is necessary as it is performed in the wrapper).
// In the future, result can be stuffed inside an AutoFireClosure.

Done

std::move on closures is not necessary.

Done

Can we make these enums instead now?

Using a weak pointer on the non-originating thread is not thread safe. See the comment in weak_ptr.h that describes safe use. I have pasted the relevant bits below.

Now in this particular case, the engine provides a guarantee that shell subcomponents will be destroyed before its destructor finishes execution on the platform task runner. So, it happens to work out. But I would still be more comfortable if code didn't break safe threading rules because of the lifecycle assumptions. It is not immediately clear from reading just this bit of code about why this is safe even though it breaks rules of safe weak pointer use.

You can use the same pattern used right above in the RunEngine call.

// Weak pointers are not in general thread-safe. They may only be *used* on a
// single thread, namely the same thread as the "originating" |WeakPtrFactory|
// (which can invalidate the weak pointers that it generates).
//
// However, weak pointers may be passed to other threads, reset on other
// threads, or destroyed on other threads. They may also be reassigned on other
// threads (in which case they should then only be used on the thread
// corresponding to the new "originating" |WeakPtrFactory|).

Going with the unique_ptr and a comment, as discussed offline

Ditto on the comment in Shell::GetUIIsolateLastError

0 is actually EXIT_SUCCESS. So we will end up saying an unknown failure is no failure. This seems dodgy. Lets use kErrorExitCode as the default.

I was under the impression the engine method returned 0 if ther ewas no error. Is that not the case?

The tester_main was defaulting this case to 0 as well AFAICT

It seems to be tonic::kNoError. You can return 0 in that case instead of just defaulting to success.

Done

Done

Going with the unique_ptr and a comment, as discussed offline

Much clearer. Thanks.