Closed Bug 1218618 Opened 4 years ago Closed 4 years ago

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

Categories

(Core :: Panning and Zooming, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla44
Tracking Status
firefox44 --- fixed

People

(Reporter: botond, Assigned: botond)

Details

Attachments

(1 file)

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
Kats, any thoughts/objections?
Flags: needinfo?(bugmail.mozilla)
Sounds good to me!
Flags: needinfo?(bugmail.mozilla)
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.
Assignee: nobody → botond
Bug 1218618 - Assert that the hit-testing tree's root node doesn't have siblings. r=kats
Attachment #8680173 - Flags: review?(bugmail.mozilla)
Attachment #8680173 - Flags: review?(bugmail.mozilla) → review+
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.
Added null check and landed.
https://hg.mozilla.org/mozilla-central/rev/086aa469aa84
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
You need to log in before you can comment on or make changes to this bug.