1593381 - Have APZ gtests exercise WebRender codepaths (in preparation for removing Layers codepaths)

Status: RESOLVED FIXED

(Core :: Panning and Zooming, task, P3)

Description

[Botond Ballo [:botond]]

APZ gtests that use APZCTreeManagerTester and derived fixture types currently all create layer trees and exercise the codepath where UpdateHitTestingTree is instantiated with ScrollNode=LayerMetricsWrapper (i.e. the non-WebRender codepath).

To provide better test coverage, and to prepare the gtests for a future where layers no longer exist (i.e. everything uses WebRender), it would be better if the test cases built some sort of generic "scroll node" structure that could be turned into either layers, or WebRenderScrollData.

Comment 1

[Hiroyuki Ikezoe (:hiro)]

Attachment: A very early state of the change to exercise gtest with webrender codepath

For future references, I am attaching a patch that I did locally, it's not working at all, it's far away from its completion.

I started from tweaking ScopedLayerTreeRegistration to initialize a bunch of things which are necessary for the WebRender stuff. It turns out it's quite difficult without creating corresponding dedicate threads (i.e. compositor thread, webrender renderer thread etc.) or bypassing a bunch of assertions something like MOZ_ASSERT(CompositorThreadHolder::IsInCompositorThread()).

Comment 2

[Botond Ballo [:botond]]

I'm going to re-title this bug to make it a bit more general: to make APZ gtests exercise WebRender codepaths as much as possible, because Layers codepaths are slated to be removed in the near future (potentially 2021 H2).

There are two main issues that I'm aware of:

1. Using WebRender data structures (WebRenderScrollData) instead of Layers data structures as the test inputs (or possibly something generic that can be converted to either the WebRender data structures or the Layers data structures, as originally suggested in comment 0).

2. Figure out what to do about hit-testing. A few potential options here:

   a. Have the gtests spin up WebRender itself and perform WebRender hit-testing. This sounds difficult as it requires creating various threads and components (see comment 1), and also because WebRender needs additional inputs (basically, a WebRender display list) beyond just the WebRenderScrollData equivalent of what we currently provide.

   b. Keep the non-WebRender (HitTestingTreeNode-based) hit-testing codepath around for gtests. This can be done while removing everything else that's Layers-specific (i.e. the hit-testing tree can be built from WebRenderScrollData, no Layers needed). This has the downside that we're maintaining an extra codepath that's not used in production, and that's what we're exercising during tests. It may also inhibit some code cleanups that would have e.g. removed fields of HitTestingTreeNode that were only used by the non-WR hit-testing codepath.

   c. Mock out the hit-testing implementation, and have the tests provide their own. This can range from a full HitTestingTreeNode-based hit-test (which would make this a variant of (b), but with the impl. living in test code and thus more explicitly test-only), to test-specific mocks that just pick the APZC we know we want for the test.

   d. Abandon all gtests that operate at the APZCTreeManager level and port them to mochitest instead. I'm mentioning this for completeness only, as I don't like this option :) I find gtests super useful for testing APZ code that doesn't depend on main-thread Gecko stuff.

For (b) and (c), tests that specifically want to test hit-testing behaviour would still ideally be ported to mochitest, but there are only a handful of those (mostly the ones in TestHitTesting.cpp, I believe).

Comment 3

[Kartikaya Gupta (email:[e-mail])]

For item 2, I'd go with option c. I concur that for tests that specifically want to exercise hit-testing behaviour, they should be ported to mochitests instead. The machinery for exercising the hit-test codepaths is relatively robust in the APZ mochitests, and in most cases it's actually easier to set up the test scenario using HTML than it is to figure out the right combination of WebRenderScrollData that you would need to replicate it at a gtest level.

Setting up the WR machinery you'd need to actually exercise the production hit-testing code in a gtest seems unreasonably hard. As you pointed out, that involves setting up a whole bunch of threads and other stuff that is hard to get right. I'd discount option a for that reason. Keeping the HTTN-based hit-testing codepath around I think should be avoided for exactly the reasons you stated in point b.

Assuming the gtests whose sole purpose is to do hit-testing tests get migrated to mochitests, you'd still need to deal with the remaining gtests which might incidentally run through the hit-testing codepath (e.g. if you're gtesting a touch-drag, APZ would need to hit-test to see where the touch lands). For those cases I'd go with option c, but without the full HitTestingTreeNode stuff - just have the test explicitly indicate which things would get hit for each hit-test operation. It's a little tedious at a mechanical level (i.e. entails some boilerplate code) but since most gtests only have one or two scrollable frames anyway it's not a huge mental burden to figure out which one is going to be the hit target. With the right abstractions I think this could be done in a fairly ergonomic manner.

As for item 1, yeah, you'd want to create a WebRenderScrollData instance that would be equivalent to what would get created by a gecko display list. Then you can use that to construct the APZ tree. I think ideally you'd want to avoid having to create an APZUpdater instance entirely and just call apzctm->UpdateHitTestingTree directly. WebRenderScrollDataWrapper doesn't really use the APZUpdater unless you have a reflayer-type setup, and even then you can probably insert an interface there with a separate test implementation.

Comment 4

[Botond Ballo [:botond]]

Attachment: Bug 1593381 - Add a WR replacement for CreateLayerTree(). r=hiro

The replacement TestWRScrollData::Create(), which creates a
TestWRScrollData object, which extends WebRenderScrollData with
some functions useful for tests.

The patch also ports the existing tests of LayerMetricsWrapper
to test WebRenderScrollDataWrapper instead and exercise the
new function.

Comment 5

[Botond Ballo [:botond]]

Attachment: Bug 1593381 - Extend TestWRScrollData::Create() with parameters to set visible regions and transforms. r=hiro

Depends on D124002

Comment 6

[Botond Ballo [:botond]]

Attachment: Bug 1593381 - Port TestBasic to use WR inputs. r=hiro

Depends on D124288

Comment 7

[Botond Ballo [:botond]]

Attachment: Bug 1593381 - Introduce a more generic mechanism for APZ test code to access private members of APZ types. r=hiro

Depends on D124289

Comment 8

[Botond Ballo [:botond]]

Attachment: Bug 1593381 - Add some new test-only APIs to WebRender[Layer]ScrollData. r=hiro

Depends on D124290

Comment 9

[Botond Ballo [:botond]]

Attachment: Bug 1593381 - Support WR inputs in APZCTreeManagerTester. r=hiro

For now, the class also retains the ability to use Layer inputs.
This allows porting its subclasses incrementally.

Depends on D124291

Comment 10

[Botond Ballo [:botond]]

Attachment: Bug 1593381 - Port CreateSimpleScrollingLayer(), and the tests that use it, to use WR inputs. r=hiro

Depends on D124292

Comment 11

[Botond Ballo [:botond]]

Attachment: Bug 1593381 - Add a mechanism to allow accessing layers using 'layers[i]' syntax in WR tests. r=hiro

For now, using the mechanism requires a local variable declaration
in the test:

auto& layers = wrLayerAccessor;

This is so that existing Layers tests can continue to compile.

Once all test are ported, these declarations can be removed
and the member 'wrLayerAccessor' itself renamed to 'layers'.

Depends on D124294

Comment 12

[Botond Ballo [:botond]]

Attachment: Bug 1593381 - Port TestFlingAcceleration to use WR inputs. r=hiro

Depends on D124295

Comment 13

[Botond Ballo [:botond]]

Attachment: Bug 1593381 - Port APZCTreeManagerGenericTester.ScrollablePaintedLayers to use WR inputs. r=hiro

Part of this test had to be removed because it performs an operation,
"change a FrameMetrics which is shared with another Layer", which
the WR data structures do not support, and which does not need
test coverage (since production code cannot do it either).

Depends on D124296

Comment 14

[Botond Ballo [:botond]]

Status update: a lot of the infrastructure is now in place, and I've ported a subset of the tests to use WR inputs.

The sticking point for porting many of the remaining tests is that WebRenderScrollData does not store EventRegions, and many tests need the EventRegions for the internal hit-test to produce a correct result.

The refactoring of the internal hit-test (task 2 from comment 2) is going to solve this, but to unblock task 1 (and the Layers removal) in the meantime, I'm going to put in place some sort of test-only EventRegions storage for WebRenderScrollData.

Comment 15

[Botond Ballo [:botond]]

Attachment: Bug 1593381 - Make WebRenderLayerScrollData move-only. r=hiro

Comment 16

[Botond Ballo [:botond]]

Attachment: Bug 1593381 - Allow tests to store EventRegions on WebRenderLayerScrollData. r=hiro

Use this ability to set event regions corresponding to the visible region
in TestWRScrollData::Create().

Depends on D124520

Comment 17

[Botond Ballo [:botond]]

Attachment: Bug 1593381 - Set EventRegions on WR scroll data in APZCTreeManagerTester. r=hiro

Depends on D124521

Comment 18

[Botond Ballo [:botond]]

Attachment: Bug 1593381 - Provide a minimal implementation of WebRenderScrollDataWrapper::GetClipRect() which is sufficient to get the tests to pass. r=hiro

Depends on D124522

Comment 19

[Botond Ballo [:botond]]

Attachment: Bug 1593381 - Additional WR support in APZCTreeManagerTester. r=hiro

Depends on D124523

Comment 20

[Botond Ballo [:botond]]

Attachment: Bug 1593381 - Port the remaining tests in TestTreeManager to use WR inputs. r=hiro

Depends on D124524

Comment 21

[Botond Ballo [:botond]]

Attachment: Bug 1593381 - Port TestSnapping to use WR inputs. r=hiro

Depends on D124525

Comment 22

[Botond Ballo [:botond]]

Attachment: Bug 1593381 - Port TestSnappingOnMomentum to use WR inputs. r=hiro

Depends on D124526

Comment 23

[Botond Ballo [:botond]]

Attachment: Bug 1593381 - Port TestScrollHandoff to use WR inputs. r=hiro

Depends on D124527

Comment 24

[Botond Ballo [:botond]]

Attachment: Bug 1593381 - Port TestEventRegions to use WR inputs. r=hiro

Depends on D124528

Comment 25

[Botond Ballo [:botond]]

Attachment: Bug 1593381 - Port TestEventResult to use WR inputs. r=hiro

Depends on D124529

Comment 26

[Botond Ballo [:botond]]

Attachment: Bug 1593381 - Port TestOverscroll to use WR inputs. r=hiro

Depends on D124530

Comment 27

[Botond Ballo [:botond]]

Attachment: Bug 1593381 - Port TestHitTesting to use WR inputs. r=hiro

Depends on D124531

Comment 28

[Botond Ballo [:botond]]

Attachment: Bug 1593381 - Remove Layers support from APZCTreeManagerTester. r=hiro

Depends on D124532

Comment 29

[Botond Ballo [:botond]]

I spun out task 2 from comment 2 to bug 1729118. This bug now tracks task 1 only.

I have a couple of more cleanup patches to post here to round out task 1, but what I've posted so far should be enough to unblock the removal of LayerMetricsWrapper (which I plan to do in bug 1729117), which in turn should unblock the removal of Layer / LayerManager as far as APZ dependencies are concerned.

Comment 30

[Botond Ballo [:botond]]

Attachment: Bug 1593381 - Rename APZCTreeManagerTester::scrollData to 'layers'. r=hiro

Also add a |WebRenderLayerScrollData* root| member for convenience.

Comment 31

[Botond Ballo [:botond]]

Attachment: Bug 1593381 - Replace APZCTreeManager::mIsUsingWebRender with a HitTestKind enum. r=hiro

Now that WebRender is on in all configurations, this flag only
needs to be set to true in 'LayersOnly' gtests fixtures (which
are also renamed in this patch to 'Internal').

Depends on D124541

Comment 32

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/69e985516ea6
Add a WR replacement for CreateLayerTree(). r=hiro
https://hg.mozilla.org/integration/autoland/rev/635f8255ae1d
Extend TestWRScrollData::Create() with parameters to set visible regions and transforms. r=hiro
https://hg.mozilla.org/integration/autoland/rev/9785fb8d9df5
Port TestBasic to use WR inputs. r=hiro
https://hg.mozilla.org/integration/autoland/rev/cb624b4f7ae4
Introduce a more generic mechanism for APZ test code to access private members of APZ types. r=hiro
https://hg.mozilla.org/integration/autoland/rev/323485a94225
Add some new test-only APIs to WebRender[Layer]ScrollData. r=hiro
https://hg.mozilla.org/integration/autoland/rev/2f61fe33e110
Support WR inputs in APZCTreeManagerTester. r=hiro
https://hg.mozilla.org/integration/autoland/rev/0a6645ff5fba
Port CreateSimpleScrollingLayer(), and the tests that use it, to use WR inputs. r=hiro
https://hg.mozilla.org/integration/autoland/rev/ef84e24b7e13
Port TestFlingAcceleration to use WR inputs. r=hiro
https://hg.mozilla.org/integration/autoland/rev/5c6a4f9f5135
Port APZCTreeManagerGenericTester.ScrollablePaintedLayers to use WR inputs. r=hiro
https://hg.mozilla.org/integration/autoland/rev/682b48d591bc
Make WebRenderLayerScrollData move-only. r=hiro
https://hg.mozilla.org/integration/autoland/rev/3c565c33dbdf
Allow tests to store EventRegions on WebRenderLayerScrollData. r=hiro
https://hg.mozilla.org/integration/autoland/rev/c22d6fd1882c
Set EventRegions on WR scroll data in APZCTreeManagerTester. r=hiro
https://hg.mozilla.org/integration/autoland/rev/a4a6d43f1b97
Provide a minimal implementation of WebRenderScrollDataWrapper::GetClipRect() which is sufficient to get the tests to pass. r=hiro
https://hg.mozilla.org/integration/autoland/rev/3d4dac0081c5
Additional WR support in APZCTreeManagerTester. r=hiro
https://hg.mozilla.org/integration/autoland/rev/3e0dc03188e3
Port the remaining tests in TestTreeManager to use WR inputs. r=hiro
https://hg.mozilla.org/integration/autoland/rev/fab337ac9f49
Port TestSnapping to use WR inputs. r=hiro
https://hg.mozilla.org/integration/autoland/rev/b691c41cab73
Port TestSnappingOnMomentum to use WR inputs. r=hiro
https://hg.mozilla.org/integration/autoland/rev/955a8b96933a
Port TestScrollHandoff to use WR inputs. r=hiro
https://hg.mozilla.org/integration/autoland/rev/32770ecde6db
Port TestEventRegions to use WR inputs. r=hiro
https://hg.mozilla.org/integration/autoland/rev/7ccab346d65b
Port TestEventResult to use WR inputs. r=hiro
https://hg.mozilla.org/integration/autoland/rev/70dd06eae57e
Port TestOverscroll to use WR inputs. r=hiro
https://hg.mozilla.org/integration/autoland/rev/12be9c5f1224
Port TestHitTesting to use WR inputs. r=hiro
https://hg.mozilla.org/integration/autoland/rev/9493a4f78224
Remove Layers support from APZCTreeManagerTester. r=hiro
https://hg.mozilla.org/integration/autoland/rev/43d0b002b6f1
Rename APZCTreeManagerTester::scrollData to 'layers'. r=hiro
https://hg.mozilla.org/integration/autoland/rev/cd98dfc8d45d
Replace APZCTreeManager::mIsUsingWebRender with a HitTestKind enum. r=hiro

Comment 33

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/69e985516ea6
https://hg.mozilla.org/mozilla-central/rev/635f8255ae1d
https://hg.mozilla.org/mozilla-central/rev/9785fb8d9df5
https://hg.mozilla.org/mozilla-central/rev/cb624b4f7ae4
https://hg.mozilla.org/mozilla-central/rev/323485a94225
https://hg.mozilla.org/mozilla-central/rev/2f61fe33e110
https://hg.mozilla.org/mozilla-central/rev/0a6645ff5fba
https://hg.mozilla.org/mozilla-central/rev/ef84e24b7e13
https://hg.mozilla.org/mozilla-central/rev/5c6a4f9f5135
https://hg.mozilla.org/mozilla-central/rev/682b48d591bc
https://hg.mozilla.org/mozilla-central/rev/3c565c33dbdf
https://hg.mozilla.org/mozilla-central/rev/c22d6fd1882c
https://hg.mozilla.org/mozilla-central/rev/a4a6d43f1b97
https://hg.mozilla.org/mozilla-central/rev/3d4dac0081c5
https://hg.mozilla.org/mozilla-central/rev/3e0dc03188e3
https://hg.mozilla.org/mozilla-central/rev/fab337ac9f49
https://hg.mozilla.org/mozilla-central/rev/b691c41cab73
https://hg.mozilla.org/mozilla-central/rev/955a8b96933a
https://hg.mozilla.org/mozilla-central/rev/32770ecde6db
https://hg.mozilla.org/mozilla-central/rev/7ccab346d65b
https://hg.mozilla.org/mozilla-central/rev/70dd06eae57e
https://hg.mozilla.org/mozilla-central/rev/12be9c5f1224
https://hg.mozilla.org/mozilla-central/rev/9493a4f78224
https://hg.mozilla.org/mozilla-central/rev/43d0b002b6f1
https://hg.mozilla.org/mozilla-central/rev/cd98dfc8d45d

Comment 34

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #2)

For (b) and (c), tests that specifically want to test hit-testing behaviour would still ideally be ported to mochitest, but there are only a handful of those (mostly the ones in TestHitTesting.cpp, I believe).

Filed bug 1730606 for this.