1218618 - Do not support hit-testing trees where the root node has siblings

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

[Botond Ballo [:botond]]

When APZ loops over the hit-testing tree, it handles the case where the root node has siblings. This is a hold-over from the days when those loops were over the APZC tree, in which the root node can indeed have siblings.

The hit-testing tree, however, has the same shape as the LayerMetrics tree, which in turn has the same shape as the layer tree except that layers with multiple FrameMetrics are expanded "vertically" into one node for each metrics. Since the layer tree has the property that the root node has no siblings, the hit-testing tree will have the same property too.

Textbook implementations of tree traversal algorithms (such as the ones in TreeTraversal.h [1]) do not allow for the root node to have siblings. Since we'd like to such algorithms in APZ code (we actually already use them [2], but we want to use them more [3]), we need to either:

  (1) modify the algorithms to support such trees; or

  (2) not support such trees in APZ

As suggested by the bug's title, I propose (2).

[1] https://dxr.mozilla.org/mozilla-central/rev/28068d907290d1f5138a0b9e59ae2233a1c1b7a3/gfx/layers/TreeTraversal.h
[2] https://dxr.mozilla.org/mozilla-central/rev/28068d907290d1f5138a0b9e59ae2233a1c1b7a3/gfx/layers/apz/src/APZCTreeManager.cpp#1730
[3] bug 1199798

Comment 1

[Botond Ballo [:botond]]

Kats, any thoughts/objections?

Comment 2

[Kartikaya Gupta (email:[e-mail]) (away until Feb-2020)]

Sounds good to me!

Comment 3

[Botond Ballo [:botond]]

OK, great!

I think the only code change I'd like to make in this bug is to assert in UpdateHitTestingTree() that the root indeed does not have siblings. Changing the various loops to not handle that case will happen naturally in bug 1199798 as we transition those loops to use the tree traversal algorithms.

Comment 4

[Botond Ballo [:botond]]

Attachment: MozReview Request: Bug 1218618 - Assert that the hit-testing tree's root node doesn't have siblings. r=kats

Bug 1218618 - Assert that the hit-testing tree's root node doesn't have siblings. r=kats

Comment 5

[Kartikaya Gupta (email:[e-mail]) (away until Feb-2020)]

Comment on attachment 8680173 [details]
MozReview Request: Bug 1218618 - Assert that the hit-testing tree's root node doesn't have siblings. r=kats

https://reviewboard.mozilla.org/r/23585/#review21075

r+ with comment addressed

::: gfx/layers/apz/src/APZCTreeManager.cpp:206
(Diff revision 1)
> +  MOZ_ASSERT(!mRootNode->GetPrevSibling());

I think mRootNode could be null here in some circumstances (I think I've run into it before in some testing scenarios at least), so we should guard against that.

Comment 6

[Botond Ballo [:botond]]

Added null check and landed.

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/086aa469aa84

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/086aa469aa84