Ensure we use the base CompositorContext's AcquireFrame method when screenshotting #13934
Conversation
screenshotting. This ensures we rasterize into the canvas passed in as subclasses may reimplement AcquireFrame in different ways that don't utilize the canvas object passed in (such as Fuchsia's flutter_runner::CompositorContext).
chinmaygarde
left a comment
chinmaygarde
left a comment
There was a problem hiding this comment.
The shell_unittests target already use the fixtures mechanism. This separates the concerns of how assets are specified to the test vs how they are packaged and deployed to the target when running the test. They are also cross platform and adding support for more platforms is easier in that harness as the executable doesn't have to change at all.
We actively want to migrate the golden tests mechanism to use fixtures for engine unit-tests for these reasons.
shell/common/shell_unittests.cc
Outdated
| } | ||
|
|
||
| TEST_F(ShellTest, Screenshot) { | ||
| #if !OS_LINUX |
There was a problem hiding this comment.
Please don't add platform specific checks for tests that check platform agnostic code. There is no reason this test should not work fine on Mac as well.
There was a problem hiding this comment.
Ah I see later you needed to do this because of goldens, you wouldn't have this problem with fixtures.
There was a problem hiding this comment.
So I did consider using fixtures, but the problem is they don't currently work on Fuchsia because the way it's set up is that there's an assumption that the build machine and the test host are the same machine (the fixtures buildsystem churns out a header file which defines the fixtures path as an absolute path on the build machine).
The goldens mechanism doesn't have this issue as we pass the path in at runtime.
I'm guessing the best way forward here is to use the fixtures path and fix it up to get rid of that assumption but I'm a little wary of adding more blockers in the way of getting tests running on Fuchsia as we have a lot already.
There was a problem hiding this comment.
You will have to account for fixtures on Fuchsia anyway because the shelL_unittests target is not going to work there without fixtures. The fixtures snapshot the Dart stuff too for example. So using goldens here doesn't mean the you can avoid having to work with fixtures to have this test target functional.
Fuchsia because the way it's set up is that there's an assumption that the build machine and the test host are the same machine
That is only how the host targets are set up. It is not a built-in assumption. When you port fixtures to a new platform, one of the efforts is to tell test harness how to find fixtures on the platform.
The goldens mechanism doesn't have this issue as we pass the path in at runtime.
But the assets will still have to be available at runtime on the Fuchsia device right?
There was a problem hiding this comment.
You will have to account for fixtures on Fuchsia anyway because the shelL_unittests target is not going to work there without fixtures. The fixtures snapshot the Dart stuff too for example. So using goldens here doesn't mean the you can avoid having to work with fixtures to have this test target functional.
That's a fair point.
Fuchsia because the way it's set up is that there's an assumption that the build machine and the test host are the same machine
That is only how the host targets are set up. It is not a built-in assumption. When you port fixtures to a new platform, one of the efforts is to tell test harness how to find fixtures on the platform.
The fixtures gn rules seem to be platform agnostic and dumps the build directory: https://github.com/flutter/engine/blob/master/testing/testing.gni#L26
Is the plan here to make this method platform-specific so that we can specify the fixtures paths differently on each platform? I'm ok with that, just want to be clear what path forward we want to take :)
The goldens mechanism doesn't have this issue as we pass the path in at runtime.
But the assets will still have to be available at runtime on the Fuchsia device right?
Yep, I figured we'd either package them in the .far file or push to /tmp in run_tests.py and specify the path accordingly.
This reverts commit 54df916.
- use test fixtures
- comment on why CompositorContext is being called explicitly
| std::promise<Rasterizer::Screenshot> screenshot_promise; | ||
| auto screenshot_future = screenshot_promise.get_future(); |
There was a problem hiding this comment.
Nice, i've never used promises in C++. This is a good use!
…d when screenshotting (flutter/engine#13934)
git@github.com:flutter/engine.git/compare/176f56329631...6bf311f git log 176f563..6bf311f --no-merges --oneline 2019-11-20 bkonyi@google.com Roll src/third_party/dart b188632458..867a6e0e52 (4 commits) (flutter/engine#13948) 2019-11-20 gw280@google.com Ensure we use the base CompositorContext's AcquireFrame method when screenshotting (flutter/engine#13934) 2019-11-20 chinmaygarde@google.com Update SwiftShader to 5d1e854. (flutter/engine#13945) 2019-11-20 ianloic@google.com [flutter_runner] fix a11y tests (flutter/engine#13947) 2019-11-20 bkonyi@google.com Roll src/third_party/dart 8342ec2c71..b188632458 (2 commits) (flutter/engine#13942) 2019-11-20 ferhat@gmail.com Fix edge detection for correct dom_renderer reset (flutter/engine#13940) 2019-11-20 bkonyi@google.com Roll src/third_party/dart 1f933abcee..8342ec2c71 (12 commits) (flutter/engine#13941) 2019-11-20 50856934+nturgut@users.noreply.github.com [web] Flutter for web autocorrect support (flutter/engine#13922) 2019-11-20 skia-flutter-autoroll@skia.org Roll src/third_party/skia e678b79c489d..935a35d972e5 (13 commits) (flutter/engine#13927) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC aaclarke@google.com on the revert to ensure that a human is aware of the problem. To report a problem with the AutoRoller itself, please file a bug: https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+/master/autoroll/README.md
5 participants
This ensures we rasterize into the canvas passed in as subclasses may reimplement AcquireFrame in different ways that don't utilize the canvas object passed in (such as Fuchsia's flutter_runner::CompositorContext).
Also add a unit test.