fuchsia: Fix Flatland opacity #29844
Conversation
| @@ -257,27 +257,37 @@ void FakeFlatland::SetClipBounds( | |||
| transform->clip_bounds = clip_bounds; | |||
| } | |||
|
|
|||
| void FakeFlatland::SetOpacity( | |||
There was a problem hiding this comment.
Won't this break the existing build? I feel like we need to do a manual SDK roll with the change to fake_flatland included.
There was a problem hiding this comment.
Nothing uses SetOpacity right now, so commenting it out won't break anything. If it broke the build, CQ would fail. :)
The FIDL base class which FakeFlatland derives from implements every method already with a LOG(FATAL). So this change just means that SetOpacity calls would CHECK -- but nothing calls SetOpacity in flutter right now.
Once the SDK rolls I'll land a 2nd PR to re-enable this - notice I already changed the name and logic in the method, so all I'll need to do is uncomment everything.
There was a problem hiding this comment.
I see, I thought the methods in Flatland_TestBase were pure virtual and needed to be linked, instead of stubs. Sorry for the confusion.
There was a problem hiding this comment.
Do the C++ bindings have the equivalent of the dart package:fidl_foo/fidl_test.dart files which protect against breakages like this?
There was a problem hiding this comment.
Yeah, thats what the Flatland_TestBase is, its the C++ version of the auto-generated FIDL test stubs.
However these FIDL stubs only protect against Fuchsia adding a new method. That way the new method becomes a stub instead of a linker error when we generate the C++ code from it.
In the case of this CL Fuchsia removed the SetOpacity method (well, they renamed it). None of the FIDL stubs in any language protect against that.
It's not their fault, the engineer who renamed it didn't know about this FakeFlatland. That's why it needs to move into the SDK so the Scenic team can maintain it.
| @@ -257,27 +257,37 @@ void FakeFlatland::SetClipBounds( | |||
| transform->clip_bounds = clip_bounds; | |||
| } | |||
|
|
|||
| void FakeFlatland::SetOpacity( | |||
There was a problem hiding this comment.
I see, I thought the methods in Flatland_TestBase were pure virtual and needed to be linked, instead of stubs. Sorry for the confusion.
https://fuchsia-review.googlesource.com/c/fuchsia/+/608181 changed this API, so update the test Fake to match
Bug: https://bugs.fuchsia.dev/p/fuchsia/issues/detail?id=89111
Test: Modified existing test