GitLab

Reland of https://chromium-review.googlesource.com/c/chromium/src/+/1066987

Currently there is a HitTestDataProviderSurfaceLayer class in
components/viz/client, which receives a cc::LayerTreeHostImpl* from cc/
and then calls back into cc::LayerTreeHostImpl* to build the
HitTestRegionList from the active tree. This is problematic for these
reasons:

1. cc is-a viz client, so it could depend on viz/client/ code. But the
viz/client/ should not depend back on cc/. The use of the
cc::LayerTreeHostImpl pointer breaks this.

2. The LayerTreeFrameSink implementation can call back to
LayerTreeHostImpl, but it should not do so with the actual LTHI pointer
type. Instead it should use the LayerTreeFrameSinkClient interface,
which LayerTreeHostImpl implements.

3. The order of exection is that cc::LayerTreeHostImpl calls
ClientLayerTreeFrameSink::SubmitCompositorFrame() (through the
LayerTreeFrameSink interface), which then calls back to the
cc::LayerTreeHostImpl class to generate the HitTestRegionList,
creating the A-B-A anti-pattern. We can resolve this by passing
the HitTestRegionList directly to SubmitCompositorFrame(), but to do
so we need ClientLayerTreeFrameSink to not get it from the virtual
HitTestDataProvider interface in this case.

In order to satisfy these things we:

A. Move the implementation of HitTestDataProviderSurfaceLayer into
LayerTreeHostImpl, and move the unit test there.

B. Add a BuildHitTestData() method to LayerTreeFrameSinkClient, which
is implemented in LayerTreeHostImpl as per (A).

C. Move the branching out of the HitTestDataProvider virtuality.
If the HitTestDataProvider in ClientLayerTreeFrameSink is null, then
it uses the LayerTreeFrameSinkClient to ask the LayerTreeHostImpl to
build the HitTestRegionList. This does not break behaviour as the
HitTestDataProvider is created as a HitTestDataProviderDrawQuad
preferably, falling back to HitTestDataProviderSurfaceLayer.
Similarly the HitTestDataProviderDrawQuad would be preferred if the
pointer is not null, falling back to using LayerTreeFrameSinkClient.
(We also leave a TODO that we can just submit the HitTestRegionList
directly to SubmitCompositorFrame.)

This patch would be pretty small.. except..

Because cc/ is building the HitTestRegionList, it can not depend
on services/viz/ which depends back on cc/. So we implement the
viz::mojom::HitTestRegionList and viz::mojom::HitTestRegion as
standard c++ types viz::HitTestRegionList and viz::HitTestRegion
in components/viz/common/hit_test/, which cc/ can make use of. And
we add struct traits to pass these types to mojo and receive them
from mojo.

Then all plumbing code must be changed from the mojom-defined type
to the standard-c++-defined type.

The mojom type was viz::mojom::HitTestRegionListPtr (aka
mojo::StructPtr<viz::mojom::HitTestRegionList>), which is an
optional field in the CompositorFrame, so we must preserve its
ability to be null. We may use a struct traits directly to
viz::HitTestRegionList, and base::Optional<viz::HitTestRegionList>
throughout the plumbing, or we can use a struct traits to a
std::unique_ptr<viz::HitTestRegionList> (which is itself considered
nullable by mojo) and plumb that through directly through
everything. I don't actually know which is preferable here, but
settled on making viz::HitTestRegionList move-only, and using the
former option. This makes most of this patch just type renames.

Lastly, we need LayerTreeHostImpl to not return a HitTestRegionList
in cases where no HitTestDataProvider would have been given to the
ClientLayerTreeFrameSink previously. This is in the case where
features::IsVizHitTestingSurfaceLayerEnabled() returns false. So we
add a field to LayerTreeSettings, which we set to the value of that
feature flag, and have LayerTreeHostImpl return an empty optional
value when the setting is false.

TBR=gklassen@chromium.org, sadrul@chromium.org, reveman@chromium.org, avi@chromium.org, dcheng@chromium.org

Bug: 722935
Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel;luci.chromium.try:win_optional_gpu_tests_rel;master.tryserver.blink:linux_trusty_blink_rel
Change-Id: Iba36b0cb12e2e45b80211956fb932c26693d1cd3
Reviewed-on: https://chromium-review.googlesource.com/1072128Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: danakj <danakj@chromium.org>
Commit-Queue: danakj <danakj@chromium.org>
Cr-Commit-Position: refs/heads/master@{#561697}