[Metal] Refactor Shell unit-tests to allow runtime selection of client rendering API. #14023
Conversation
|
Fixes flutter/flutter#41930 |
| @@ -0,0 +1,37 @@ | |||
| // Copyright 2013 The Flutter Authors. All rights reserved. | |||
There was a problem hiding this comment.
Is it possible to remove this class (and thus shell_test_surface.h/cc) and just merge the CreateTestSurface/CreateRenderingSurface into ShellTestPlatformView?
There was a problem hiding this comment.
This is the common interface used by ShellTestSurfaceGL and ShellTestSurfaceMetal. Without this common interface, the ShellTestPlatformView will have to own two unique pointers to both ShellTestSurfaceGL and ShellTestSurfacePlatform view and then conditionally check for the availability of either on each call. Now, the base can own just one member and call CreateRenderingSurface on it without null checks everywhere.
There was a problem hiding this comment.
Can ShellTestPlatformView only own a unique pointer to GPUSurfaceDelegate (the parent class of both ShellTestSurfaceMetalImpl and ShellTestSurfaceGL), and directly implement CreateRenderingSurface based on that? We can discuss offline about this as I feel that Github comments don't have the bandwidth to resolve this efficiently.
| @@ -0,0 +1,36 @@ | |||
| // Copyright 2013 The Flutter Authors. All rights reserved. | |||
There was a problem hiding this comment.
Could we just have a single shell_test_surface_metal.h (no shell_test_surface_metal_impl.h), and just add shell_test_surface_metal.cc or shell_test_surface_metal_impl.mm to build depending on the GN settings?
There was a problem hiding this comment.
We want the tests to be able to compile on both Darwin and non-Darwin platforms. Only that on non-Darwin platforms, the test will be skipped via GTEST_SKIP if Metal is not available and attempting to create a Metal surface will fail. shell_test_surface_metal.cc contains the no-op implementation that is used on non-Darwin platforms and shell_test_surface_metal_impl.mm contains the actual implementation on Darwin. Having just shell_test_surface_metal_impl.mm will cause linker errors on Linux and we cannot put ObjC code in a C++ in shell_test_surface_metal.cc.
There was a problem hiding this comment.
Discussed offline. The difference of those two approaches seems to be: shell_test_surface_metal_impl.h makes it easier to add metal only methods (such methods may be able to be just defined in the .mm file), while removing shell_test_surface_metal_impl.h makes it easier to add methods to shell_test_surface.h.
…ing API. Adds support for Metal rasterizers in the unit-test harness.
chinmaygarde
force-pushed
the
metal_shell_unittests
branch
from
c241538 to
82b3b8b
Compare
|
Rebased to ToT. PTAL. I don't remember if @liyuqian had a preference based on his last comment. If this looks fine, can we land this. It is a rather large patch is hard to rebase to ToT. cc @iskakaushik who was fiddling with the test vsync waiter than I just moved too. |
liyuqian
left a comment
liyuqian
left a comment
There was a problem hiding this comment.
RSLGTM. I do have a preference to remove shell_test_surface_metal_impl.h as we discussed last time. But I'm Ok to land this as is to avoid more merge conflict headaches.
Too late already :( I'll rebase and remove the metal_impl. |
3 participants
Adds support for Metal rasterizers in the unit-test harness. Tests that explicitly need metal can select the same by specifying
ClientRenderingAPI::kClientRenderingAPIMetalto theCreateShellfixture method on Darwin platforms (GTEST_SKIPotherwise should work fine on all other platforms as no-op implementations are present for the linker).