Fix lint errors in lib/ui

[gspencergoog]

Description

This fixes all of the lint errors in lib/ui, except for a few (three, I think) where it would have changed the API, converting non-const references to pointers. For those, I just did NOLINT on the particular line instead of ignoring the whole file.

[gaaclarke]

These seem like a pretty big oversight if they are necessary. I guess people assummed Dart_ThrowException would roll back the stack?

[gspencergoog]

Yeah, I think so. Basically, I think whoever wrote it assumed they were "real" exceptions.

[gaaclarke]

This would be a safer change if we created the image_callback in the same scope it used to, then used std::move to let the lambda own it.

My concern is that raw_image_callback might be deleted in this case before image_callback is created. DartPersistentValue might be doing some retain count on that value.

[gaaclarke]

fyi: I believe you can say // NOLINT(name_of_lint) It might be easier to track these that way, up to you if you want to change it.

[gaaclarke]

Thanks greg! I thought this would take a while for us to migrate our stuff. I really appreciate it.

[gaaclarke]

I thought about it a bit more and I think we really should make that change I mentioned about where the creation of the tonic::DartPersistentValue

[gspencergoog]

Yeah, given the failing test, I agree with you.

[gspencergoog]

@gaaclarke OK, take a look at picture.cc and let me know if that's what you had in mind.

[gaaclarke]

I'm still a bit uneasy about the change because of the comment on deleting the image_callback and how hard it is to reason about where the shared_ptr is actually being deleted.

If you can explain to me that it is deleting it as expected thats fine. Otherwise I'd try using std::unique_ptr and seeing where that leads you (might be a lot of work). Otherwise I think it's better to disable linting somehow for now.

[gaaclarke]

I think this is good. FWIW I was thinking you could use unique_ptr. Then when you capture it in the lambda you can use std::move. It's slightly more efficient.

auto foo = std::make_unique<Foo>();
auto ui_task = fml::MakeCopyable([bar = std::move(foo)]() mutable {
});

But I don't think that would have worked because ui_task is copied. There would have to be further refactoring to make that work.

[gaaclarke]

The comment above this reset is a bit ominous. With shared_ptr it's a bit harder to reason about whether the object is actually deleted here or not. I'm not sure that it is.

[gspencergoog]

OK, I've switched to a unique_ptr, and it passes lint and compilation. I think that it will be fine (but we'll see if the tests agree). It seems like the main point of MakeCopyable is to allow move-only captures on these lambdas, and they are used that way in other places.

[gaaclarke]

Booyah!