Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Description • 10 years ago

While making the HitTesting2 gtest pass with event-regions enabled I realized that there is in fact a case where we need sub-APZC areas to be included in the hit test region of parent APZCs. This was previously discussed on IRC at [1] and summarized into bugzilla at [2].

The scenario that requires this is when you have an APZC (call it A) with a sub-APZC (call in B). Let's say B is visible inside A, and so is subtracted from A's hit region. Then, A is async-scrolled so that B is no longer visible. At this point, touching A at the point where B used to be should deliver the touch event to A, but if B's area was subtracted (which is what the code currently does) then the touch event misses both A and B and is delivered incorrectly.

This scenario is exercised in the HitTesting2 code, after the first ApzcPanNoFling call.

[1] http://logs.glob.uno/?c=mozilla%23gfx&s=12+Nov+2014&e=12+Nov+2014#c210132
[2] https://bugzilla.mozilla.org/show_bug.cgi?id=1090398#c15

	Botond Ballo [:botond]
	Comment 2 • 10 years ago

Discussed this with Kats on IRC. Summarizing:

There is a related problem that arises while async-scrolling:
  1. Suppose A's event region has a hole where the layer is transparent.
  2. Async-scroll A.
  3. Touch A where the transparent region now is.

In this scenario, we would hit A even though we are touching the transparent region.

This problem can be solved by adjusting the event region by the layer's async transform before using it for hit testing. That same change would solve this bug, rendering this patch unnecessary.

	Botond Ballo [:botond]
	Comment 3 • 10 years ago

Comment on attachment 8534610 [details] [diff] [review]
Patch

Review of attachment 8534610 [details] [diff] [review]:
-----------------------------------------------------------------

Dropping review flag as per comment 2.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 4 • 10 years ago

I started working on changing the way we store hit-test data so that we have separate data per layer in the layer tree rather than accumulating stuff into the APZC instance that is shared. It's turning out to be quite tedious to split this out; this is what I've gotten so far. Stashing it here as a sneak preview. In Part 2 I'm thinking I'll expand the HitTestingTreeNode structure to map 1:1 to layers in the layer tree (so some will have no APZCs), and then in part 3 I'll move over the hit-test data from APZC into the new class. I might reverse those two depending on how things go.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 6 • 10 years ago

Eventually I'll move GestureBehavior in here too, to solve https://bugzilla.mozilla.org/show_bug.cgi?id=1109855#c6 but I don't want to do that in this bug.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 7 • 10 years ago

I tested this on-device and the gtests pass with and without event-regions enabled. The changes I made to the gtests are again to better simulate what layout would send us as an actual layer tree.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 8 • 10 years ago
bad-rebase try

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=acebfe76f77b

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 9 • 10 years ago
green try

I missed a thing rebasing to master, updated try push: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=e21558af4f71

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 10 • 10 years ago

This is rebased to master; just a minor change to the new function dvander added. Try run above is green with this version.

	Botond Ballo [:botond]
	Comment 11 • 10 years ago

I've had an initial look at the Part 1 and Part 2 patches, but I think I need another pass at them to understand the design better before I can evaluate it.

I'm going to post the comments I wrote so far so I don't lose them, but please don't consider them complete reviews.

	Botond Ballo [:botond]
	Comment 12 • 10 years ago

Comment on attachment 8540317 [details] [diff] [review]
Part 1 - Introduce HitTestingTreeNode

Review of attachment 8540317 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/apz/src/APZCTreeManager.cpp
@@ +1363,5 @@
>        }
>      }
>      if (!scrollParent) {
> +      nsRefPtr<HitTestingTreeNode> parentNode = GetTargetNode(parent->GetGuid());
> +      scrollParent = FindTargetAPZC(parentNode, apzc->GetScrollHandoffParentId());

It's unfortunate that we have to do an extra tree walk here.

::: gfx/layers/apz/src/HitTestingTreeNode.h
@@ +18,5 @@
> +/**
> + * This class represents a node in a tree that is used by the APZCTreeManager
> + * to do hit testing. The tree is roughly a copy of the layer tree but only
> + * contains nodes for layers that have scrollable metrics. There will be
> + * exactly one node per layer with scrollable metrics. However, multiple nodes

What happens with multi-FrameMetrics? Would it be more accurate to say that there will be one node per (layer, scrollable metrics) pair?

@@ +19,5 @@
> + * This class represents a node in a tree that is used by the APZCTreeManager
> + * to do hit testing. The tree is roughly a copy of the layer tree but only
> + * contains nodes for layers that have scrollable metrics. There will be
> + * exactly one node per layer with scrollable metrics. However, multiple nodes
> + * may share the same underlying APZC instance if the layers they respresent

represent

@@ +54,5 @@
> +  void SetPrevSibling(HitTestingTreeNode* sibling);
> +  void MakeRoot();
> +
> +  /* Tree walking methods */
> +  HitTestingTreeNode* GetFirstChild() const;

Please document that this function is linear-time in the number of children (while other walking methods are constant-time).

	Botond Ballo [:botond]
	Comment 13 • 10 years ago

Comment on attachment 8539470 [details] [diff] [review]
Part 2 - Move hit-testing data into HitTestingTreeNode

Review of attachment 8539470 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/apz/src/APZCTreeManager.cpp
@@ +582,5 @@
> +      // parent). This obscured region does not move when aLayer is scrolled,
> +      // and so is in the same coordinate space as |aLayer|'s clip rect.
> +      // We also have |obscured| which includes the hit regions of |aLayer|'s
> +      // descendants. However we don't want to use this because those
> +      // descendants (and their their hit regions) move as |aLayer| scrolls.

double 'their'

::: gfx/layers/apz/src/HitTestingTreeNode.cpp
@@ +8,5 @@
>  
>  #include "AsyncPanZoomController.h"
>  #include "LayersLogging.h"
> +#include "mozilla/gfx/Point.h"
> +#include "mozilla/layers/AsyncCompositionManager.h"

Please add a '// for ___' comment here, because the need for this inclusion isn't obvious.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 14 • 10 years ago

While working on bug 920036 I found a bug in the event regions accumulation. Fix and test attached.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 15 • 10 years ago

Comment on attachment 8540882 [details] [diff] [review]
Part 3 - Fix obscuration computation

Moving this to bug 1115111 since it doesn't really belong on this bug.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 16 • 10 years ago

For some reason I didn't bugmail for these comments, so I didn't see them until just now :(

(In reply to Botond Ballo [:botond] from comment #12)
> > +      nsRefPtr<HitTestingTreeNode> parentNode = GetTargetNode(parent->GetGuid());
> > +      scrollParent = FindTargetAPZC(parentNode, apzc->GetScrollHandoffParentId());
> 
> It's unfortunate that we have to do an extra tree walk here.

Yeah, if we want we could just do FindTargetAPZC using the root node which might be better.

> ::: gfx/layers/apz/src/HitTestingTreeNode.h
> @@ +18,5 @@
> > +/**
> > + * This class represents a node in a tree that is used by the APZCTreeManager
> > + * to do hit testing. The tree is roughly a copy of the layer tree but only
> > + * contains nodes for layers that have scrollable metrics. There will be
> > + * exactly one node per layer with scrollable metrics. However, multiple nodes
> 
> What happens with multi-FrameMetrics? Would it be more accurate to say that
> there will be one node per (layer, scrollable metrics) pair?

Yes, that would be more accurate. I'll update the comment accordingly.

All other comments make sense, I'll incorporate them into my patches. The AsyncCompositionManager include was needed to convert from a ViewTransform to a Matrix4x4.

	Botond Ballo [:botond]
	Comment 17 • 10 years ago

While continuing to review these patches, I found it helpful to re-read the current APZC tree building code. It was my first time looking at this code in a while and I had to page it back in, and during this process, I added some explanatory comments that I think would be helpful to have in the tree.

	Botond Ballo [:botond]
	Comment 18 • 10 years ago

After looking over these patches again, and the code they apply to, my primary feedback is this:

These patches replace one abstraction (the APZC tree) with another (the HitTestTreeNode tree) and touch a large surface area of code in the process. I'm not sure I'm convinced of the value of doing this. In most places where we use HitTestTreeNodes, we're just using them to get at their APZCs, i.e. we're really walking the APZC tree; it's really only for hit testing that we use the HitTestTreeNodes themselves. To me, this suggests that the APZC tree is the more natural abstraction.

I wonder if it would be possible to accomplish the same end with a more modest change. For example, what if we extracted the (event-regions, transform, optional-clip-rect) triple out into a HitTestData structure, and had APZC instances maintain an nsTArray<HitTestData>, with the elements held in z-order? Would that allow us to perform hit testing that's functionally equivalent to what's done in this patch, while retaining the APZC tree abstraction, and avoiding the large-surface-area changes in this patch?

Note that I'm not necessarily arguing for that approach over this one at this stage; I just want to make sure we've considered it.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 19 • 10 years ago

(In reply to Botond Ballo [:botond] from comment #18)
> I wonder if it would be possible to accomplish the same end with a more
> modest change. For example, what if we extracted the (event-regions,
> transform, optional-clip-rect) triple out into a HitTestData structure, and
> had APZC instances maintain an nsTArray<HitTestData>, with the elements held
> in z-order? Would that allow us to perform hit testing that's functionally
> equivalent to what's done in this patch, while retaining the APZC tree
> abstraction, and avoiding the large-surface-area changes in this patch?
> 

That wouldn't be functionally equivalent because it wouldn't be possible to represent easily scenarios where the HitTestData objects from different APZCs are interleaved in z-order. As i discovered while writing these patches that's not strictly required to fix this bug by itself, but it is required to fix the FIXME comment that i removed in these patches.

> Note that I'm not necessarily arguing for that approach over this one at
> this stage; I just want to make sure we've considered it.

Understood. FWIW i was also hesitant to make such a large change and I wussed out of doing the last bit, which is to make the HitTestingTreeNode tree map 1:1 to the layer tree. This would make the UPZCT and hit-testing code much cleaner, but mean that a HTTNode might have a null APZC instance. I still want to do this, but i want to wait until after we rip out the no-touch-events codepaths because it's too annoying to do right now.

Conceptually I do think APZCs don't need to know their siblings/children and as I made these changes I felt pretty satisfied with the way I was able to split out that information into the HTTNode without having to hack up anything in the APZC.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 20 • 10 years ago

Comment on attachment 8543083 [details] [diff] [review]
Part 0 - Add some explanatory comments to the APZC tree building code

Review of attachment 8543083 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!

	Botond Ballo [:botond]
	Comment 21 • 10 years ago

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #19)
> (In reply to Botond Ballo [:botond] from comment #18)
> > I wonder if it would be possible to accomplish the same end with a more
> > modest change. For example, what if we extracted the (event-regions,
> > transform, optional-clip-rect) triple out into a HitTestData structure, and
> > had APZC instances maintain an nsTArray<HitTestData>, with the elements held
> > in z-order? Would that allow us to perform hit testing that's functionally
> > equivalent to what's done in this patch, while retaining the APZC tree
> > abstraction, and avoiding the large-surface-area changes in this patch?
> > 
> 
> That wouldn't be functionally equivalent because it wouldn't be possible to
> represent easily scenarios where the HitTestData objects from different
> APZCs are interleaved in z-order. As i discovered while writing these
> patches that's not strictly required to fix this bug by itself, but it is
> required to fix the FIXME comment that i removed in these patches.

Hmm, good point.

> Understood. FWIW i was also hesitant to make such a large change and I
> wussed out of doing the last bit, which is to make the HitTestingTreeNode
> tree map 1:1 to the layer tree. This would make the UPZCT and hit-testing
> code much cleaner, but mean that a HTTNode might have a null APZC instance.
> I still want to do this, but i want to wait until after we rip out the
> no-touch-events codepaths because it's too annoying to do right now.

Is there a correctness reason to do this additional step, or is it just to make the UPZCT and hit-testing code cleaner?

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 22 • 10 years ago

(In reply to Botond Ballo [:botond] from comment #21)
> Is there a correctness reason to do this additional step, or is it just to
> make the UPZCT and hit-testing code cleaner?

There should be no correctness difference but it would make the code *much* cleaner and much less prone to bugs. Also it would likely have better performance characteristics since we wouldn't need to do so many region operations during UPZCT (which gets run a lot) and we would do a few more during the hit-test (which gets run less frequently).

	Botond Ballo [:botond]
	Comment 23 • 10 years ago

Comment on attachment 8540317 [details] [diff] [review]
Part 1 - Introduce HitTestingTreeNode

Review of attachment 8540317 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with comments addressed.

A general suggestion, I'll leave it up to you if you want to do this now or later:

I notice that, since the root node can have siblings, we currently do tree walks like so:

  // call site
  for (Node* node = root; node; node = node->prevSibling)
    RecursiveHelper(node)

  RecursiveHelper(Node* subtreeRoot):
    // do operation on |node|
    for (Node* node = subtreeRoot->lastChild; node; node = node->prevSibling)
      RecursiveHelper(node)

I wonder if it would instead be better to do tree walks like so:

  // call site
  RecursiveHelper(root)

  RecursiveHelper(Node* subtreeRoot):
    for (Node* node = subtreeRoot; node; node = node->prevSibling)
      // do operation on |node|
      ReecursiveHelper(node->lastChild)

This avoids some repetition.

I believe this could apply to FlushRepaints and GetAPZCAtPoint. In GetAPZCAtPoint in particular, it would avoid duplicating the long comment ("We might be able to do better if we keep looping ..."), which just gets longer in Part 2.

::: gfx/layers/apz/src/APZCTreeManager.cpp
@@ +509,4 @@
>      // Otherwise, use this APZC as the parent going downwards, and start off
>      // with its first child as the next sibling
> +    aParent = node;
> +    next = node->GetFirstChild();

In my Part 0 patch, I commented here:

+    // Note that |apzc| at this point will not have children corresponding to
+    // the subtree of aLayer (those children will be populated in the loop
+    // below). However, it might have children corresponding to the subtree of
+    // another layer that shares the same APZC and that has already been
+    // visited; the children added at this level will become prev-siblings
+    // of those.

Now that we are building a HitTestingTreeNode instead, would I be correct to say that |node->GetFirstChild()| should always be nullptr here? Can we assert to that effect?

@@ +619,2 @@
>    // moving towards the first child at this depth in the layer tree.
>    // If this layer doesn't have an APZC, we promote any APZCs in the subtree

"we promote any nodes"

@@ +1077,5 @@
>                                                    const ZoomConstraints& aConstraints)
>  {
>    mTreeLock.AssertCurrentThreadOwns();
>  
> +  aNode->Apzc()->UpdateZoomConstraints(aConstraints);

Note that we will now call UpdateZoomConstraints() on a given APZC once for each node associated with that APZC. Not necessarily a problem right now, but it might conceivably become one if UpdateZoomConstraints starts doing something expensive, or dispatching an event or something. Perhaps it's better, just on principle, to condition on IsPrimaryHolder()?

@@ +1092,4 @@
>  {
>    mTreeLock.AssertCurrentThreadOwns();
>  
> +  aNode->Apzc()->FlushRepaintForNewInputBlock();

See comment about UpdateZoomConstraints().

@@ +1288,5 @@
>    // The root may have siblings, so check those too
>    HitTestResult hitResult = NoApzcHit;
> +  for (HitTestingTreeNode* node = mRootNode; node; node = node->GetPrevSibling()) {
> +    target = GetAPZCAtPoint(node, aPoint.ToUnknownPoint(), &hitResult);
> +    if (target == node->Apzc() && node->GetPrevSibling() && target == node->GetPrevSibling()->Apzc()) {

(Here we're assuming something stronger - that if two nodes share an APZC, they are adjacent siblings - but I realize that this assumption gets narrowed to the {event regions}-disabled case in the Part 2 patch, so it should be fine.)

@@ +1363,5 @@
>        }
>      }
>      if (!scrollParent) {
> +      nsRefPtr<HitTestingTreeNode> parentNode = GetTargetNode(parent->GetGuid());
> +      scrollParent = FindTargetAPZC(parentNode, apzc->GetScrollHandoffParentId());

Devil's advocate: what if the root-for-layers-id APZC ('parent' here) has multiple nodes associated with it, and the APZC with the scroll-parent-id is not in the subtree of the one returned by GetTargetNode()?

I suggest we instead revise |FindTargetAPZC(APZC*, ViewID)| to be |FindTargetAPZC(LayersID, ViewID)| instead and just search from the root, and then just call that with (parent's layers id, scroll-parent-id) here. (If it weren't for that pesky pres-shell-id, we could just reuse |GetTargetAPZC(ScrollableLayerGuid)|.

@@ +1383,5 @@
>  }
>  
>  /* Find the apzc in the subtree rooted at aApzc that has the same layers id as
>     aApzc, and that has the given scroll id. Generally this function should be called
>     with aApzc being the root of its layers id subtree. */

Comment needs updating.

::: gfx/layers/apz/src/APZCTreeManager.h
@@ +406,5 @@
>      OverscrolledApzc,
>    };
>  
> +  nsRefPtr<HitTestingTreeNode> GetRootNode() const;
> +  already_AddRefed<HitTestingTreeNode> GetTargetNode(const ScrollableLayerGuid& aGuid);

Is it ever useful to get a node given a guid? It seems to me that if our input is a guid, we just want the APZC, so we might as well return that. That would avoid a lot of the 'node ? node->Apzc() : nullptr' repetition in APZCTreeManager.cpp. Of course the recursive helper can still take a HitTestingTreeNode*.

Also, both this and GetRootNode() can be private. (GetTargetAPZC(ScrollableLayerGuid) could have been private before the patch, too, I just realized.)

@@ +442,2 @@
>  
> +  HitTestingTreeNode* PrepareAPZCForLayer(const LayerMetricsWrapper& aLayer,

Should this be called PrepareNodeForLayer now?

@@ +457,5 @@
>     * This function walks the layer tree backwards through siblings and constructs the APZC
>     * tree also as a last-child-prev-sibling tree because that simplifies the hit detection
>     * code.
>     */
> +  HitTestingTreeNode* UpdatePanZoomControllerTree(TreeBuildingState& aState,

Technically, we no longer have a pan-zoom controller tree, so this method should be renamed, but I could buy an argument about the name having enough of a presence on bugzilla and such that it's better to leave it be.

::: gfx/layers/apz/src/AsyncPanZoomController.h
@@ +854,5 @@
>    /* ===================================================================
>     * The functions and members in this section are used to build a tree
>     * structure out of APZC instances. This tree can only be walked or
>     * manipulated while holding the lock in the associated APZCTreeManager
>     * instance.

This comment should no longer be talking about building an APZC tree.

::: gfx/layers/apz/src/HitTestingTreeNode.cpp
@@ +47,5 @@
> +{
> +  mLastChild = child;
> +  if (child) {
> +    child->mParent = this;
> +    child->Apzc()->SetParent(Apzc());

As discussed on IRC, we are assuming that 'this' and 'child' don't share an APZC (and more generally, that ancestor and descendant layers don't share an APZC), otherwise we'd be setting an APZC's parent pointer to itself (more generally, introducing a loop in the APZC parent pointer chain).

Please document this assertion, and perhaps add some relevant assertions (e.g. here, 'MOZ_ASSERT(child->Apzc() != Apzc())').

(Update: The assumption we make is actually a bit stronger: if two layers scroll together, then the net transform between each one and their common ancestor is the same. This means that their APZC-bearing ancestors must be the same (otherwise the differing APZCs would be a source of a async transforms that could be different), so it's valid to determine an APZC parent-child relationship based on a HitTestTreeNode parent-child relationship.)

::: gfx/layers/apz/src/HitTestingTreeNode.h
@@ +30,5 @@
> + * primary holder, only that that there will be exactly one for each APZC in
> + * the tree.
> + *
> + * Note that every HitTestingTreeNode instance will have a pointer to an APZC,
> + * and that pointer will be non-null.

Except if the node has been Destroy()ed - we should document this, particularly given that the accessor is named Apzc().

@@ +50,5 @@
> +  void Destroy();
> +
> +  /* Tree construction methods */
> +  void SetLastChild(HitTestingTreeNode* child);
> +  void SetPrevSibling(HitTestingTreeNode* sibling);

nit: aChild, aSibling

@@ +61,5 @@
> +  HitTestingTreeNode* GetParent() const;
> +
> +  /* APZC related methods */
> +  AsyncPanZoomController* Apzc() const;
> +  bool IsPrimaryHolderOfApzcWithGuid(const ScrollableLayerGuid& aGuid) const;

This method feels random from an interface perspective. I'd prefer just having an IsPrimaryHolder() method, and then having the caller compare the guid (with that being encapsulated into a function if desired). (But I don't feel strongly about it.)

	Botond Ballo [:botond]
	Comment 24 • 10 years ago

(In reply to Botond Ballo [:botond] from comment #23)
> I suggest we instead revise |FindTargetAPZC(APZC*, ViewID)| to be
> |FindTargetAPZC(LayersID, ViewID)| instead and just search from the root,
> and then just call that with (parent's layers id, scroll-parent-id) here.
> (If it weren't for that pesky pres-shell-id, we could just reuse
> |GetTargetAPZC(ScrollableLayerGuid)|.

If you feel like it, you can facilitate greater code reuse by having GetTargetAPZC(ScrollableLayerGuid) accept an optional comparator for the guids, and here passing one that ignores the pres shell id.

	Botond Ballo [:botond]
	Comment 25 • 10 years ago

Comment on attachment 8540317 [details] [diff] [review]
Part 1 - Introduce HitTestingTreeNode

Review of attachment 8540317 [details] [diff] [review]:
-----------------------------------------------------------------

Whoops, forgot to review TestAsyncPanZoomController.cpp.

::: gfx/tests/gtest/TestAsyncPanZoomController.cpp
@@ +2030,5 @@
>    CreatePotentiallyLeakingTree();
>    ScopedLayerTreeRegistration registration(0, root, mcc);
>  
>    manager->UpdatePanZoomControllerTree(nullptr, root, false, 0, 0);
> +  nsRefPtr<HitTestingTreeNode> root = manager->GetRootNode();

Given this, disregard my earlier comment about making GetRootNode() private.

@@ +2037,5 @@
>    EXPECT_EQ(ApzcOf(layers[0]), ApzcOf(layers[2])->GetParent());
>    EXPECT_EQ(ApzcOf(layers[2]), ApzcOf(layers[5]));
>  
> +  nsRefPtr<HitTestingTreeNode> node2 = root->GetFirstChild();
> +  nsRefPtr<HitTestingTreeNode> node5 = root->GetLastChild();

These variable definitions can be moved up a couple of lines and the variables used there.

	Botond Ballo [:botond]
	Comment 26 • 10 years ago

Comment on attachment 8540317 [details] [diff] [review]
Part 1 - Introduce HitTestingTreeNode

Review of attachment 8540317 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/apz/src/APZCTreeManager.cpp
@@ +437,5 @@
> +    // from mNodesToDestroy instead of creating a new one since those are going
> +    // to get discarded anyway.
> +    node = new HitTestingTreeNode(apzc, false);
> +    AttachNodeToTree(node, aParent, aNextSibling);
> +

Can we assert aAncestorTransform == apzc->GetAncestorTransform() here?

	Botond Ballo [:botond]
	Comment 27 • 10 years ago

Comment on attachment 8539470 [details] [diff] [review]
Part 2 - Move hit-testing data into HitTestingTreeNode

Review of attachment 8539470 [details] [diff] [review]:
-----------------------------------------------------------------

This looks pretty good!

I'd going to hold off my r+ until:

  1) The code where we combine event-regions in Layer pixels with
     a touch-sensitive region in ParentLayer pixels is corrected.

  2) I understand the changes to TestAsyncPanZoomController better.

::: gfx/layers/apz/src/APZCTreeManager.cpp
@@ -430,5 @@
> -    // and the async transform on it, a larger/smaller area of C may be unobscured.
> -    // However, when we combine the hit regions of A and C here we are ignoring the async
> -    // async transform and so we basically assume the same amount of C is always visible
> -    // on top of B. Fixing this doesn't appear to be very easy so I'm leaving it for
> -    // now in the hopes that we won't run into this problem a lot.

Glad we fixed this! :)

@@ +510,5 @@
>    if (gfxPrefs::LayoutEventRegionsEnabled() && (apzc || aState.mEventRegions.Length() > 0)) {
>      aState.mEventRegions.AppendElement(EventRegions());
>    }
>  
> +  nsIntRegion obscuredByUncles = obscured;

This would be a good place to define 'uncles' ("siblings of an ancestor"). Maybe also mention that we're saving the value of |obscured| at this point because it will change in the loop below, but we'll need the original value later.

@@ +572,5 @@
>              / ParentLayerToLayerScale(aLayer.Metrics().mPresShellResolution);
>          subtreeEventRegions.AndWith(ParentLayerIntRect::ToUntyped(
>              RoundedIn(touchSensitiveRegion
>                      * aLayer.Metrics().mDevPixelsPerCSSPixel
>                      * parentCumulativeResolution)));

I think this calculation needs to be adjusted now that |subtreeEventRegions| is in Layer pixels rather than ParentLayer pixels, since right now we're And-ing a LayerRegion with a ParentLayerRect.

@@ +599,5 @@
> +        clipRegion = nsIntRegion(ParentLayerIntRect::ToUntyped(RoundedToInt(aLayer.Metrics().mCompositionBounds)));
> +      }
> +      clipRegion.SubOut(obscuredByUncles);
> +
> +      node->SetHitTestData(subtreeEventRegions, aLayer.GetTransform(), &clipRegion);

Does that fact that clipRegion is never nullptr here mean that once the non-{event regions} code is out, we can store a clip rect (rather than a pointer to a clip rect) in the Node?

::: gfx/layers/apz/src/HitTestingTreeNode.cpp
@@ +128,5 @@
> +HitTestResult
> +HitTestingTreeNode::HitTest(const ParentLayerPoint& aPoint)
> +{
> +  // When event regions are disabled, we are actually storing the composition
> +  // bounds in mEventRegions.mHitRegion, and not using mTransform and

s/the composition bounds/the result of ComputeTouchSensitiveRegion()

@@ +145,5 @@
> +  // convert into Layer coordinate space
> +  gfx::Matrix4x4 localTransform = mTransform * gfx::Matrix4x4(mApzc->GetCurrentAsyncTransform());
> +  gfx::Point4D pointInLayerPixels = localTransform.Inverse().ProjectPoint(aPoint.ToUnknownPoint());
> +  if (!pointInLayerPixels.HasPositiveWCoord()) {
> +    return HitTestResult::NoApzcHit;

Not directly related to this patch, but does the need to call ProjectPoint() and then test for HasPositiveWCoord() here and in GetAPZCAtPoint() mean that we should be doing this in all the places we currently use TransformTo [1] (which uses 'Point Matrix4x4::operator*(Point)' instead) as well?

[1] http://mxr.mozilla.org/mozilla-central/source/layout/base/UnitTransforms.h?rev=8273b2521c2e#83

::: gfx/layers/apz/src/HitTestingTreeNode.h
@@ +70,5 @@
> +  /* Hit test related methods */
> +  void SetHitTestData(const EventRegions& aRegions,
> +                      const gfx::Matrix4x4& aTransform,
> +                      const nsIntRegion* aClipRegion);
> +  HitTestResult HitTest(const ParentLayerPoint& aPoint);

HitTest() can be const.

@@ +91,5 @@
> +   * regions stored here will be the union of the three event regions in the
> +   * ContainerLayer's layer pixel space. This means the event regions from the
> +   * PaintedLayer children will have been transformed and clipped according to
> +   * the individual properties on those layers but the ContainerLayer's event
> +   * regions will be used "raw". */

Can you clarify whether the event regions of APZC descendants are included or not? (My reading of the rest of the patch tells me they're not.)

@@ +104,5 @@
> +  /* This is the clip rect that the layer subtree corresponding to this node
> +   * is subject to. In the terms of the comment on mEventRegions, it is the clip
> +   * rect of the ContainerLayer. */
> +  bool mHasClipRegion;
> +  nsIntRegion mClipRegion;

Consider using Maybe<nsIntRegion>.

::: gfx/tests/gtest/TestAsyncPanZoomController.cpp
@@ +1696,5 @@
>        EventRegions er = aLayer->GetEventRegions();
>        nsIntRect scrollRect = LayerIntRect::ToUntyped(RoundedToInt(aScrollableRect * metrics.LayersPixelsPerCSSPixel()));
> +      er.mHitRegion = nsIntRegion(scrollRect);
> +      Matrix4x4 transform = aLayer->GetBaseTransform();
> +      aLayer->SetBaseTransform(transform.PostTranslate(layerBound.x, layerBound.y, 0));

Why are we moving the offset into the transform here?

@@ +1784,5 @@
>      };
>      Matrix4x4 transforms[] = {
>        Matrix4x4(),
>        Matrix4x4(),
> +      Matrix4x4::Translation(10, 60, 0).PostScale(2, 1, 1),

And here?

::: layout/base/Units.h
@@ +286,5 @@
>    }
> +
> +  static LayerPoint FromUnknown(const gfx::Point& aPoint) {
> +    return LayerPoint(aPoint.x, aPoint.y);
> +  }

I've been using one of my ViewAs overloads [1] for this purpose (example usage for this case: 'ViewAs<LayerPixel>(aPoint)'). This avoids having to define a different function for each pixel type.

[1] http://mxr.mozilla.org/mozilla-central/source/layout/base/UnitTransforms.h?rev=8273b2521c2e#60

	Botond Ballo [:botond]
	Comment 28 • 10 years ago

Comment on attachment 8539470 [details] [diff] [review]
Part 2 - Move hit-testing data into HitTestingTreeNode

Review of attachment 8539470 [details] [diff] [review]:
-----------------------------------------------------------------

I think we may have a couple of other coordinate system mismatches on our hands:

::: gfx/layers/apz/src/APZCTreeManager.cpp
@@ +376,3 @@
>      if (!gfxPrefs::LayoutEventRegionsEnabled()) {
> +      nsIntRegion unobscured = ComputeTouchSensitiveRegion(state->mController, aMetrics, aObscured);
> +      node->SetHitTestData(EventRegions(unobscured), Matrix4x4(), nullptr);

Here, Node::SetHitTestData expects a LayerRegion, but ComputeTouchSensitiveRegion computes a ParentLayerRegion 

(I really wish I had implemented strongly-typed regions before this work :( ).

@@ +597,5 @@
> +        // root scrollable layer in a process) fall back to using the comp
> +        // bounds which should be equivalent.
> +        clipRegion = nsIntRegion(ParentLayerIntRect::ToUntyped(RoundedToInt(aLayer.Metrics().mCompositionBounds)));
> +      }
> +      clipRegion.SubOut(obscuredByUncles);

Here, |obscuredByUncles| is initialized with |obscured|, which is in Layer pixels, but the clip rect is in ParentLayer pixels.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 29 • 10 years ago

This is in response to all your "Part 1" comments:

(In reply to Botond Ballo [:botond] from comment #12)
> What happens with multi-FrameMetrics? Would it be more accurate to say that
> there will be one node per (layer, scrollable metrics) pair?

Fixed

> @@ +19,5 @@
> > + * may share the same underlying APZC instance if the layers they respresent
> 
> represent

Fixed

> @@ +54,5 @@
> > +  HitTestingTreeNode* GetFirstChild() const;
> 
> Please document that this function is linear-time in the number of children
> (while other walking methods are constant-time).

Done

(In reply to Botond Ballo [:botond] from comment #23)
> I wonder if it would instead be better to do tree walks like so:
> 
>   // call site
>   RecursiveHelper(root)
> 
>   RecursiveHelper(Node* subtreeRoot):
>     for (Node* node = subtreeRoot; node; node = node->prevSibling)
>       // do operation on |node|
>       ReecursiveHelper(node->lastChild)
> 
> This avoids some repetition.

Good idea, I'll do this as an additional patch at the end of the queue to avoid rebasing issues.

> ::: gfx/layers/apz/src/APZCTreeManager.cpp
> In my Part 0 patch, I commented here:
> 
> +    // Note that |apzc| at this point will not have children corresponding
> to
> +    // the subtree of aLayer (those children will be populated in the loop
> +    // below). However, it might have children corresponding to the subtree
> of
> +    // another layer that shares the same APZC and that has already been
> +    // visited; the children added at this level will become prev-siblings
> +    // of those.
> 
> Now that we are building a HitTestingTreeNode instead, would I be correct to
> say that |node->GetFirstChild()| should always be nullptr here? Can we
> assert to that effect?

Yes, that is correct. I added an assert for it.

> @@ +619,2 @@
> >    // If this layer doesn't have an APZC, we promote any APZCs in the subtree
> 
> "we promote any nodes"

Fixed

> Note that we will now call UpdateZoomConstraints() on a given APZC once for
> each node associated with that APZC. Not necessarily a problem right now,
> but it might conceivably become one if UpdateZoomConstraints starts doing
> something expensive, or dispatching an event or something. Perhaps it's
> better, just on principle, to condition on IsPrimaryHolder()?

Good call, done.

> @@ +1092,4 @@
> > +  aNode->Apzc()->FlushRepaintForNewInputBlock();
> 
> See comment about UpdateZoomConstraints().

Updated here as well.

> @@ +1363,5 @@
> >        }
> >      }
> >      if (!scrollParent) {
> > +      nsRefPtr<HitTestingTreeNode> parentNode = GetTargetNode(parent->GetGuid());
> > +      scrollParent = FindTargetAPZC(parentNode, apzc->GetScrollHandoffParentId());
> 
> Devil's advocate: what if the root-for-layers-id APZC ('parent' here) has
> multiple nodes associated with it, and the APZC with the scroll-parent-id is
> not in the subtree of the one returned by GetTargetNode()?

Unlikely to happen in practice, but you're right that it's a theoretical possibility.

> I suggest we instead revise |FindTargetAPZC(APZC*, ViewID)| to be
> |FindTargetAPZC(LayersID, ViewID)| instead and just search from the root,
> and then just call that with (parent's layers id, scroll-parent-id) here.
> (If it weren't for that pesky pres-shell-id, we could just reuse
> |GetTargetAPZC(ScrollableLayerGuid)|.

I did end up using GetTargetAPZC (now called GetTargetNode) and adding an optional comparator for the guids like you suggested in comment 24.

> @@ +1383,5 @@
> >  /* Find the apzc in the subtree rooted at aApzc that has the same layers id as
> >     aApzc, and that has the given scroll id. Generally this function should be called
> >     with aApzc being the root of its layers id subtree. */
> 
> Comment needs updating.

This function got removed entirely since it wasn't needed after addressing the previous comment.

> ::: gfx/layers/apz/src/APZCTreeManager.h
> > +  nsRefPtr<HitTestingTreeNode> GetRootNode() const;
> > +  already_AddRefed<HitTestingTreeNode> GetTargetNode(const ScrollableLayerGuid& aGuid);
> 
> Is it ever useful to get a node given a guid? It seems to me that if our
> input is a guid, we just want the APZC, so we might as well return that.
> That would avoid a lot of the 'node ? node->Apzc() : nullptr' repetition in
> APZCTreeManager.cpp. Of course the recursive helper can still take a
> HitTestingTreeNode*.

There was only one case where I actually needed the node, in UpdateZoomConstraints. That function gets the target node and then passes the node into the UpdateZoomConstraintsRecursively helper so that the helper can walk down the tree from there. I didn't want to duplicate a bunch of code just for this one use site so I figured it was better to just have the function return a HitTestingTreeNode and extract the Apzc wherever needed. We'll have to touch this code again after we make the HitTestingTreeNode tree map 1:1 to the layer+metrics tree (because then node->Apzc() might be null) so I'd rather wait until we do that before fiddling with this further.

> Also, both this and GetRootNode() can be private.
> (GetTargetAPZC(ScrollableLayerGuid) could have been private before the
> patch, too, I just realized.)

Made GetTargetNode private, but GetRootNode is needed by tests as you noted in comment 25.

> @@ +442,2 @@
> > +  HitTestingTreeNode* PrepareAPZCForLayer(const LayerMetricsWrapper& aLayer,
> 
> Should this be called PrepareNodeForLayer now?

Yup, renamed.

> @@ +457,5 @@
> > +  HitTestingTreeNode* UpdatePanZoomControllerTree(TreeBuildingState& aState,
> 
> Technically, we no longer have a pan-zoom controller tree, so this method
> should be renamed, but I could buy an argument about the name having enough
> of a presence on bugzilla and such that it's better to leave it be.

I could also go either way on this. I think I'm leaning in favour of renaming it but I'll throw it in another patch at the end for rebasing sanity.

> ::: gfx/layers/apz/src/AsyncPanZoomController.h
> @@ +854,5 @@
> >    /* ===================================================================
> >     * The functions and members in this section are used to build a tree
> >     * structure out of APZC instances. This tree can only be walked or
> >     * manipulated while holding the lock in the associated APZCTreeManager
> >     * instance.
> 
> This comment should no longer be talking about building an APZC tree.

Updated.

> ::: gfx/layers/apz/src/HitTestingTreeNode.cpp
> @@ +47,5 @@
> > +    child->Apzc()->SetParent(Apzc());
> 
> As discussed on IRC, we are assuming that 'this' and 'child' don't share an
> APZC (and more generally, that ancestor and descendant layers don't share an
> APZC), otherwise we'd be setting an APZC's parent pointer to itself (more
> generally, introducing a loop in the APZC parent pointer chain).
> 
> Please document this assertion, and perhaps add some relevant assertions
> (e.g. here, 'MOZ_ASSERT(child->Apzc() != Apzc())').

Added this assertion and some comments.

> ::: gfx/layers/apz/src/HitTestingTreeNode.h
> > + * Note that every HitTestingTreeNode instance will have a pointer to an APZC,
> > + * and that pointer will be non-null.
> 
> Except if the node has been Destroy()ed - we should document this,
> particularly given that the accessor is named Apzc().

Done.

> @@ +50,5 @@
> > +  void SetLastChild(HitTestingTreeNode* child);
> > +  void SetPrevSibling(HitTestingTreeNode* sibling);
> 
> nit: aChild, aSibling

Whoops, done.

> @@ +61,5 @@
> > +  bool IsPrimaryHolderOfApzcWithGuid(const ScrollableLayerGuid& aGuid) const;
> 
> This method feels random from an interface perspective. I'd prefer just
> having an IsPrimaryHolder() method, and then having the caller compare the
> guid (with that being encapsulated into a function if desired). (But I don't
> feel strongly about it.)

Yeah I ended up needing an IsPrimaryHolder function to address a previous comment so I got rid of this and moved the guid check to the call sites.

(In reply to Botond Ballo [:botond] from comment #25)
> > +  nsRefPtr<HitTestingTreeNode> node2 = root->GetFirstChild();
> > +  nsRefPtr<HitTestingTreeNode> node5 = root->GetLastChild();
> 
> These variable definitions can be moved up a couple of lines and the
> variables used there.

Done

(In reply to Botond Ballo [:botond] from comment #26)
> > +    AttachNodeToTree(node, aParent, aNextSibling);
> > +
> 
> Can we assert aAncestorTransform == apzc->GetAncestorTransform() here?

Done, although I'm not really sure if it adds much value.

	Botond Ballo [:botond]
	Comment 30 • 10 years ago

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #29)
> > ::: gfx/layers/apz/src/APZCTreeManager.h
> > > +  nsRefPtr<HitTestingTreeNode> GetRootNode() const;
> > > +  already_AddRefed<HitTestingTreeNode> GetTargetNode(const ScrollableLayerGuid& aGuid);
> > 
> > Is it ever useful to get a node given a guid? It seems to me that if our
> > input is a guid, we just want the APZC, so we might as well return that.
> > That would avoid a lot of the 'node ? node->Apzc() : nullptr' repetition in
> > APZCTreeManager.cpp. Of course the recursive helper can still take a
> > HitTestingTreeNode*.
> 
> There was only one case where I actually needed the node, in
> UpdateZoomConstraints. That function gets the target node and then passes
> the node into the UpdateZoomConstraintsRecursively helper so that the helper
> can walk down the tree from there. I didn't want to duplicate a bunch of
> code just for this one use site so I figured it was better to just have the
> function return a HitTestingTreeNode and extract the Apzc wherever needed.
> We'll have to touch this code again after we make the HitTestingTreeNode
> tree map 1:1 to the layer+metrics tree (because then node->Apzc() might be
> null) so I'd rather wait until we do that before fiddling with this further.

Ah, I missed the use in UpdateZoomConstraints.

It sounds reasonable to wait until the further changes, as you describe.

> > > +    AttachNodeToTree(node, aParent, aNextSibling);
> > > +
> > 
> > Can we assert aAncestorTransform == apzc->GetAncestorTransform() here?
> 
> Done, although I'm not really sure if it adds much value.

I think it's useful because it documents an assumption we make about layer trees we receive, and allows us to catch potential violations of that assumption in the future.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 31 • 10 years ago

(In reply to Botond Ballo [:botond] from comment #13)se this because those
> > +      // descendants (and their their hit regions) move as |aLayer| scrolls.
> 
> double 'their'

Fixed

> ::: gfx/layers/apz/src/HitTestingTreeNode.cpp
> > +#include "mozilla/layers/AsyncCompositionManager.h"
> 
> Please add a '// for ___' comment here, because the need for this inclusion
> isn't obvious.

I added a comment for all of them, for consistency.

(In reply to Botond Ballo [:botond] from comment #27)
> >  
> > +  nsIntRegion obscuredByUncles = obscured;
> 
> This would be a good place to define 'uncles' ("siblings of an ancestor").
> Maybe also mention that we're saving the value of |obscured| at this point
> because it will change in the loop below, but we'll need the original value
> later.

I ended up reshuffling this code around, see my other comment below.

> @@ +572,5 @@
> >              / ParentLayerToLayerScale(aLayer.Metrics().mPresShellResolution);
> >          subtreeEventRegions.AndWith(ParentLayerIntRect::ToUntyped(
> >              RoundedIn(touchSensitiveRegion
> >                      * aLayer.Metrics().mDevPixelsPerCSSPixel
> >                      * parentCumulativeResolution)));
> 
> I think this calculation needs to be adjusted now that |subtreeEventRegions|
> is in Layer pixels rather than ParentLayer pixels, since right now we're
> And-ing a LayerRegion with a ParentLayerRect.

Good catch! I changed this code so that the touchSensitiveRegion (in ParentLayer space) is intersected with the clip region rather than being intersected with the subtreeEventRegions. That way I don't have to do any unnecessary transforms.

> > +      node->SetHitTestData(subtreeEventRegions, aLayer.GetTransform(), &clipRegion);
> 
> Does that fact that clipRegion is never nullptr here mean that once the
> non-{event regions} code is out, we can store a clip rect (rather than a
> pointer to a clip rect) in the Node?

Yes. In fact I ended up doing this because I changed the non-{event regions} callsite of this function to also pass in a clip rect (see my other comment below).

> ::: gfx/layers/apz/src/HitTestingTreeNode.cpp
> > +  // When event regions are disabled, we are actually storing the composition
> > +  // bounds in mEventRegions.mHitRegion, and not using mTransform and
> 
> s/the composition bounds/the result of ComputeTouchSensitiveRegion()

Reworded this comment entirely.

> @@ +145,5 @@
> > +  if (!pointInLayerPixels.HasPositiveWCoord()) {
> > +    return HitTestResult::NoApzcHit;
> 
> Not directly related to this patch, but does the need to call ProjectPoint()
> and then test for HasPositiveWCoord() here and in GetAPZCAtPoint() mean that
> we should be doing this in all the places we currently use TransformTo [1]
> (which uses 'Point Matrix4x4::operator*(Point)' instead) as well?

I'm not entirely sure of that myself, to be honest. I was under the impression that the HasPositiveWCoord check would only need to be done after ProjectPoint calls but I could be wrong.

> > +  HitTestResult HitTest(const ParentLayerPoint& aPoint);
> 
> HitTest() can be const.

Done

> @@ +91,5 @@
> > +   * PaintedLayer children will have been transformed and clipped according to
> > +   * the individual properties on those layers but the ContainerLayer's event
> > +   * regions will be used "raw". */
> 
> Can you clarify whether the event regions of APZC descendants are included
> or not? (My reading of the rest of the patch tells me they're not.)

Correct, I clarified this.

> > +  bool mHasClipRegion;
> > +  nsIntRegion mClipRegion;
> 
> Consider using Maybe<nsIntRegion>.

I got rid of the bool entirely.

> > +      aLayer->SetBaseTransform(transform.PostTranslate(layerBound.x, layerBound.y, 0));
> 
> Why are we moving the offset into the transform here?
>
> > +      Matrix4x4::Translation(10, 60, 0).PostScale(2, 1, 1),
> 
> And here?

Honestly I can't remember. The tests seem to pass without these changes so I took them out.
 
> I've been using one of my ViewAs overloads [1] for this purpose (example
> usage for this case: 'ViewAs<LayerPixel>(aPoint)'). This avoids having to
> define a different function for each pixel type.

Done

(In reply to Botond Ballo [:botond] from comment #28)
> ::: gfx/layers/apz/src/APZCTreeManager.cpp
> @@ +376,3 @@
> >      if (!gfxPrefs::LayoutEventRegionsEnabled()) {
> > +      nsIntRegion unobscured = ComputeTouchSensitiveRegion(state->mController, aMetrics, aObscured);
> > +      node->SetHitTestData(EventRegions(unobscured), Matrix4x4(), nullptr);
> 
> Here, Node::SetHitTestData expects a LayerRegion, but
> ComputeTouchSensitiveRegion computes a ParentLayerRegion 
> 
> (I really wish I had implemented strongly-typed regions before this work :(
> ).

Functionally the code I wrote here is actually correct, because in the event-regions-disabled case the HitTestingTreeNode::HitTest function uses the mEventRegions as a thing in ParentLayerPixels. But you're right that it's inconsistent and confusing, so I changed it. What I did here was (in the event-regions-disabled case) leave the event regions empty and pass "unobscured" as the clip region to SetHitTestData. This is cleaner conceptually, and also ensure we will always have a non-null clip region in a HitTestingTreeNode. This allows removing the bool mHasClipRegion. It also makes it so that the EventRegions are always in LayerPixel space and the clip region is always in ParentLayerPixel space.

> @@ +597,5 @@
> > +        // root scrollable layer in a process) fall back to using the comp
> > +        // bounds which should be equivalent.
> > +        clipRegion = nsIntRegion(ParentLayerIntRect::ToUntyped(RoundedToInt(aLayer.Metrics().mCompositionBounds)));
> > +      }
> > +      clipRegion.SubOut(obscuredByUncles);
> 
> Here, |obscuredByUncles| is initialized with |obscured|, which is in Layer
> pixels, but the clip rect is in ParentLayer pixels.

Good catch. I shuffled some of this around, so that instead of copying aObscured to obscured, transforming it, and then later copying it to obscuredByUncles, I copy aObscured to obscuredByUncles, and then later copy it to obscured which gets transformed into the child space. Seems cleaner and should be correct with respect to the coordinate systems.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 33 • 10 years ago

Enough stuff changed that I think a quick once-over is in order.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 34 • 10 years ago

Carrying r+

	Botond Ballo [:botond]
	Comment 38 • 10 years ago

Comment on attachment 8544137 [details] [diff] [review]
Part 1 - Introduce HitTestingTreeNode (v2)

Review of attachment 8544137 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, thanks! Just a couple minor comments.

::: gfx/layers/apz/src/APZCTreeManager.cpp
@@ +405,5 @@
>      PrintAPZCInfo(aLayer, apzc);
>  
>      // Bind the APZC instance into the tree of APZCs
> +    AttachNodeToTree(node, aParent, aNextSibling);
> +    MOZ_ASSERT(aAncestorTransform == apzc->GetAncestorTransform());

I think this assertion might deserve a comment, e.g. "Even though different layers associated with a given APZC may be at different levels in the layer tree (e.g. one being an uncle of another), we require from Layout that the CSS transforms up to their common ancestor be the same."

@@ +1068,5 @@
>  {
>    nsRefPtr<AsyncPanZoomController> target = nullptr;
>    if (aTargets.Length() > 0) {
> +    nsRefPtr<HitTestingTreeNode> node = GetTargetNode(aTargets[0]);
> +    target = (node ? node->Apzc() : nullptr);

I still see some value in having a thin wrapper GetTargetApzc(ScrollableLayerGuid) around GetTargetNode(ScrollableLayerGuid), but I don't feel strongly about it.

@@ -1356,5 @@
> -/* Find the apzc in the subtree rooted at aApzc that has the same layers id as
> -   aApzc, and that has the given scroll id. Generally this function should be called
> -   with aApzc being the root of its layers id subtree. */
> -AsyncPanZoomController*
> -APZCTreeManager::FindTargetAPZC(AsyncPanZoomController* aApzc, FrameMetrics::ViewID aScrollId)

Yay code removal!

::: gfx/layers/apz/src/APZCTreeManager.h
@@ +484,5 @@
> +  HitTestingTreeNode* UpdatePanZoomControllerTree(TreeBuildingState& aState,
> +                                                  const LayerMetricsWrapper& aLayer,
> +                                                  uint64_t aLayersId,
> +                                                  const gfx::Matrix4x4& aAncestorTransform,
> +                                                  HitTestingTreeNode* aParent,

Please adjust the parameter descriptions of |aParent| and |aNextSibling| as well, e.g. "the parent of any APZC built at this level" -> "the parent of any node built at this level".

::: gfx/layers/apz/src/AsyncPanZoomController.h
@@ +851,5 @@
>  
>    void StartSmoothScroll();
>  
>    /* ===================================================================
> +   * The functions and members in this section are used to ancestor chains

It appears you a word out.

	Botond Ballo [:botond]
	Comment 39 • 10 years ago

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #31)
> > @@ +145,5 @@
> > > +  if (!pointInLayerPixels.HasPositiveWCoord()) {
> > > +    return HitTestResult::NoApzcHit;
> > 
> > Not directly related to this patch, but does the need to call ProjectPoint()
> > and then test for HasPositiveWCoord() here and in GetAPZCAtPoint() mean that
> > we should be doing this in all the places we currently use TransformTo [1]
> > (which uses 'Point Matrix4x4::operator*(Point)' instead) as well?
> 
> I'm not entirely sure of that myself, to be honest. I was under the
> impression that the HasPositiveWCoord check would only need to be done after
> ProjectPoint calls but I could be wrong.

But when do we need ProjectPoint calls versus Matrix4x4::operator*(Point) calls? They're both operations that take a matrix and a point, and produce a point.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 40 • 10 years ago

(In reply to Botond Ballo [:botond] from comment #39)
> But when do we need ProjectPoint calls versus Matrix4x4::operator*(Point)
> calls? They're both operations that take a matrix and a point, and produce a
> point.

Matt Woodrow says at https://bugzilla.mozilla.org/show_bug.cgi?id=866232#c48:

"Use Invert().ProjectPoint() here, since it handles recovering the depth/perspective components if the transform is 3d."

So that's what I did! :)

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 41 • 10 years ago

(In reply to Botond Ballo [:botond] from comment #38)
> > +    AttachNodeToTree(node, aParent, aNextSibling);
> > +    MOZ_ASSERT(aAncestorTransform == apzc->GetAncestorTransform());
> 
> I think this assertion might deserve a comment, e.g. "Even though different
> layers associated with a given APZC may be at different levels in the layer
> tree (e.g. one being an uncle of another), we require from Layout that the
> CSS transforms up to their common ancestor be the same."

Oh! Now I understand why you wanted me to add that assertion - and I put in the wrong place. I put it in the branch of code where we are creating the APZC for the first time, rather than the branch where we re-use an APZC. I'll move it and add the comment.

> @@ +1068,5 @@
> > +    target = (node ? node->Apzc() : nullptr);
> 
> I still see some value in having a thin wrapper
> GetTargetApzc(ScrollableLayerGuid) around
> GetTargetNode(ScrollableLayerGuid), but I don't feel strongly about it.

Fair enough, I can add that.

> @@ -1356,5 @@
> > +  HitTestingTreeNode* UpdatePanZoomControllerTree(TreeBuildingState& aState,
> > +                                                  const LayerMetricsWrapper& aLayer,
> > +                                                  uint64_t aLayersId,
> > +                                                  const gfx::Matrix4x4& aAncestorTransform,
> > +                                                  HitTestingTreeNode* aParent,
> 
> Please adjust the parameter descriptions of |aParent| and |aNextSibling| as
> well, e.g. "the parent of any APZC built at this level" -> "the parent of
> any node built at this level".

Will do.

> ::: gfx/layers/apz/src/AsyncPanZoomController.h
> @@ +851,5 @@
> >  
> >    void StartSmoothScroll();
> >  
> >    /* ===================================================================
> > +   * The functions and members in this section are used to ancestor chains
> 
> It appears you a word out.

:) Fixed

	Botond Ballo [:botond]
	Comment 42 • 10 years ago

Comment on attachment 8544142 [details] [diff] [review]
Part 2 - Move hit-testing data into HitTestingTreeNode (v2)

Review of attachment 8544142 [details] [diff] [review]:
-----------------------------------------------------------------

This looks pretty good, thanks!

I have one concern, about when we transform obscured regions down from parents to children, and subtreeEventRegions up from children to parents. Based on some pictures I drew and code tracing, I think we might need to include the async transform when doing those transformations, but I'll write a gtest to be sure. Will hold off r+ pending the outcome of that.

::: gfx/layers/apz/src/APZCTreeManager.cpp
@@ +497,5 @@
>    if (aLayersId == childLayersId) {
> +    // If the child layer is in the same process, keep a copy of |aObscured|.
> +    // This value is in ParentLayerPixels and represents the area that is
> +    // obscured by |aLayer|'s younger uncles (i.e. any next-siblings of any
> +    // ancestor of |aLayer| in the same process. We will need this value later.

missing ')'

@@ +582,5 @@
> +    if (node) {
> +      // At this point in the code we have two different "obscured" regions.
> +      // There is |obscuredByUncles| which represents the hit regions of
> +      // |aLayer|'s younger uncles (i.e. the next-sibling chain of |aLayer|'s
> +      // parent). This obscured region does not move when aLayer is scrolled,

next-sibling chains of |aLayer|'s ancestors

::: gfx/layers/apz/src/HitTestingTreeNode.cpp
@@ +134,5 @@
> +  // When event regions are disabled, we are actually storing the
> +  // touch-sensitive section of the composition bounds in the clip rect, and we
> +  // don't need to use mTransform or mEventRegions.
> +  if (!gfxPrefs::LayoutEventRegionsEnabled()) {
> +    return mClipRegion.Contains(aPoint.x, aPoint.y)

Maybe assert that aRegions and aTransform are empty?

::: gfx/layers/apz/src/HitTestingTreeNode.h
@@ +95,5 @@
> +   * ContainerLayer's layer pixel space. This means the event regions from the
> +   * PaintedLayer children will have been transformed and clipped according to
> +   * the individual properties on those layers but the ContainerLayer's event
> +   * regions will be used "raw".
> +   * Note that this does area not necessarily include all the mEventRegions of

Some are words reordered.

@@ +97,5 @@
> +   * the individual properties on those layers but the ContainerLayer's event
> +   * regions will be used "raw".
> +   * Note that this does area not necessarily include all the mEventRegions of
> +   * descendants of this node (but may if some of the layers overlap with layers
> +   * of descendant nodes). */

Apologies for being picky but this comment is still not clear to me. What are the "layers that this node corresponds to"? 

If it's "layers that contain a FrameMetrics with this node's guid", then the statement is false, because mEventRegion also includes the regions of non-{APZC-bearing} descendants of such layers.

My understanding is it's "layers in the subtrees of layers that contain a FrameMetrics with this node's guid, but excluding the subtrees of layers that contain a FrameMetrics with a descendant node's guid". I think the comment should be explicit about that.

@@ +108,5 @@
> +  gfx::Matrix4x4 mTransform;
> +
> +  /* This is the clip rect that the layer subtree corresponding to this node
> +   * is subject to. In the terms of the comment on mEventRegions, it is the clip
> +   * rect of the ContainerLayer. */

Might be useful to say that this is in ParentLayer pixels (and, while we're at it, that mEventRegions is in Layer pixels). We can remove those comments once we introduce the actual strong types.

@@ +109,5 @@
> +
> +  /* This is the clip rect that the layer subtree corresponding to this node
> +   * is subject to. In the terms of the comment on mEventRegions, it is the clip
> +   * rect of the ContainerLayer. */
> +  nsIntRegion mClipRegion;

Every time you replace a variable that can have null as a possible value, with one that can't, God brings a kitten back to life :)

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 43 • 10 years ago

(In reply to Botond Ballo [:botond] from comment #42)
> I have one concern, about when we transform obscured regions down from
> parents to children, and subtreeEventRegions up from children to parents.
> Based on some pictures I drew and code tracing, I think we might need to
> include the async transform when doing those transformations, but I'll write
> a gtest to be sure. Will hold off r+ pending the outcome of that.
> 

Keep in mind that this code runs at tree-building time, and any async transform you grab at that point is a static snapshot of an ever-changing value. If you take an async transform at tree-building time and use it for hit-testing at some later point in time, that's almost certainly wrong. Definitely no more correct than not using the async transform at all.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 44 • 10 years ago

(In reply to Botond Ballo [:botond] from comment #42)
> 
> missing ')'

Fixed.

> 
> next-sibling chains of |aLayer|'s ancestors

Fixed

> > +  if (!gfxPrefs::LayoutEventRegionsEnabled()) {
> > +    return mClipRegion.Contains(aPoint.x, aPoint.y)
> 
> Maybe assert that aRegions and aTransform are empty?

Done

> > +   * Note that this does area not necessarily include all the mEventRegions of
> 
> Some are words reordered.

Whoops, fixed.

> @@ +97,5 @@
> Apologies for being picky but this comment is still not clear to me. What
> are the "layers that this node corresponds to"? 
> 
> If it's "layers that contain a FrameMetrics with this node's guid", then the
> statement is false, because mEventRegion also includes the regions of
> non-{APZC-bearing} descendants of such layers.
> 
> My understanding is it's "layers in the subtrees of layers that contain a
> FrameMetrics with this node's guid, but excluding the subtrees of layers
> that contain a FrameMetrics with a descendant node's guid". I think the
> comment should be explicit about that.

It's close, but not quite that. This is what I changed it to:

+  /* Let {L,M} be the {layer, scrollable metrics} pair that this node
+   * corresponds to in the layer tree. Then, mEventRegions contains the union
+   * of the event regions of all layers in L's subtree, excluding those layers
+   * which are contained in a descendant HitTestingTreeNode's mEventRegions.
+   * This value is stored in L's LayerPixel space.
+   * For example, if this HitTestingTreeNode maps to a ContainerLayer with
+   * scrollable metrics and which has two PaintedLayer children, the event
+   * regions stored here will be the union of the three event regions in the
+   * ContainerLayer's layer pixel space. This means the event regions from the
+   * PaintedLayer children will have been transformed and clipped according to
+   * the individual properties on those layers but the ContainerLayer's event
+   * regions will be used "raw". */

> Might be useful to say that this is in ParentLayer pixels (and, while we're
> at it, that mEventRegions is in Layer pixels). We can remove those comments
> once we introduce the actual strong types.

Done.

> Every time you replace a variable that can have null as a possible value,
> with one that can't, God brings a kitten back to life :)

Yay kittens!

	Botond Ballo [:botond]
	Comment 45 • 10 years ago

Comment on attachment 8544143 [details] [diff] [review]
Part 3 - Rearrange loops

Review of attachment 8544143 [details] [diff] [review]:
-----------------------------------------------------------------

I like it!

	Botond Ballo [:botond]
	Comment 46 • 10 years ago

Here's a gtest which demonstrates the problem with how we transform obscured regions.

The layer tree looks like this:
  
  R
 / \
 P U
 |
 C

R is the root, let's ignore that. P and C are parent and child scrollable layers, and U is a non-scrollable uncle.

U obscures part of P, which in the initial state of P, includes C. As you pan P, however, the portion of P that contains C can slide out from under U and become visible.

However, when we compute the clip region of C that we store in the HitTestingTreeNode, we intersect the clip region of C provided by Layout (basically, the composition bounds), with 'obscuredByUncles', without the async transform of P (from the pan) being applied to 'obscuredByUncles'. The resulting intersection under these conditions is empty, and as a result hit testing on C fails, even though P has been panned to make C visible.

(The patch includes some debug output statements that make this easier to see.)

Now, combining this with your observation (which is absolutely correct) ...

> Keep in mind that this code runs at tree-building time, and any async
> transform you grab at that point is a static snapshot of an ever-changing
> value. If you take an async transform at tree-building time and use it for
> hit-testing at some later point in time, that's almost certainly wrong.
> Definitely no more correct than not using the async transform at all.

... the pictures that emerges is fairly bleak - I think that to get this right, we'd essentially need to store the obscuring regions at each level of uncles separately in HitTestingTreeNode, and apply the at-the-moment async transforms of the ancestors to them in HitTest().

Let me know if you agree with this analysis. It's possible I'm overlooking something.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 47 • 10 years ago

(In reply to Botond Ballo [:botond] from comment #46)
> Created attachment 8544309 [details] [diff] [review]
> Gtest that demonstrates the problem with obscured regions and async
> transforms

Hmm. You're right that this case is not properly handled with these patches. It's also not properly handled with the code currently in m-c. However, I believe it will be properly handled with the "completed" HitTestingTree that I described in comment 19. In particular this means that the state after these patches is NOT functionally equivalent to the state with a complete HitTestingTree.

> ... the pictures that emerges is fairly bleak - I think that to get this
> right, we'd essentially need to store the obscuring regions at each level of
> uncles separately in HitTestingTreeNode, and apply the at-the-moment async
> transforms of the ancestors to them in HitTest().

I'd rather just implement the completed HitTestingTree. There's enough complexity here that I would feel much more confident with that in place. Thinking through it again I don't think it'll be that hard to implement even with the event-regions-disabled code paths. I'll work on that later today.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 48 • 10 years ago

Turns out it wasn't that hard to do after all. The gtests (including the one you wrote) pass with and without event-regions, but I still need to test on-device.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 50 • 10 years ago

Cleaned up your gtest so it can be landed.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 52 • 10 years ago

Now with miscellaneous fixes, improvements, and a cleanup of documentation and logging. Sorry for the ginormous patch but most of it is code deletion :)

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 53 • 10 years ago

Btw, technically these patches don't deal with the cross-process case described at [1] but now that event-regions are turned on in B2G we shouldn't run into that scenario anyway. I tested a little with event-regions turned off and I'm not seeing that case anymore. I'll test some more to make sure but if it doesn't happen I'd rather not put that code back in.

[1] http://mxr.mozilla.org/mozilla-central/source/gfx/layers/apz/src/APZCTreeManager.cpp?rev=3f980229dfc1#455

	Botond Ballo [:botond]
	Comment 54 • 10 years ago

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #47)
> However, I
> believe it will be properly handled with the "completed" HitTestingTree that
> I described in comment 19.

Oh, wow - I didn't appreciate until now that the completed HitTestingTree will remove the need to track obscuring regions altogether (because we'll just hit-test against the non-APZC nodes that would have been included in the obscuring region). Big win! :)

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 55 • 10 years ago

Indeed, that's why I figured it would simplify a lot of stuff.

Try push is at https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=26dba5d742fa, looking pretty green. I'm attaching a rollup patch of all the parts since it might be easier to review (or refer to) this way.

	Botond Ballo [:botond]
	Comment 56 • 10 years ago

Comment on attachment 8544142 [details] [diff] [review]
Part 2 - Move hit-testing data into HitTestingTreeNode (v2)

Review of attachment 8544142 [details] [diff] [review]:
-----------------------------------------------------------------

r+ on this as the concern I had about it is addressed in Part 5.

	Botond Ballo [:botond]
	Comment 57 • 10 years ago

Comment on attachment 8544724 [details] [diff] [review]
Part 5a - Allow the APZC in the HitTestingTreeNode to be null

Review of attachment 8544724 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/apz/src/HitTestingTreeNode.cpp
@@ +77,5 @@
>    if (aSibling) {
>      aSibling->mParent = mParent;
> +
> +    if (aSibling->GetApzc()) {
> +      AsyncPanZoomController* parent = mParent ? mParent-> GetNearestContainingApzc() : nullptr;

nit: stray space after 'mParent->'

@@ +98,5 @@
> +      GetApzc()->SetParent(nullptr);
> +    } else {
> +      MOZ_ASSERT(GetApzc()->GetParent() == nullptr);
> +    }
> +  }

Might be worth extracting a helper function:

void
HitTestingTreeNode::SetApzcParent(AsyncPanZoomController* aApzc) {
  // precondition: GetApzc() != nullptr
  if (IsPrimaryHolder()) {
    GetApzc()->SetParent(aApzc);
  } else {
    MOZ_ASSERT(GetApzc()->GetParent() == aApzc);
}

	Botond Ballo [:botond]
	Comment 58 • 10 years ago

Comment on attachment 8544725 [details] [diff] [review]
Part 5b - Fully implement the HitTestingTree

Review of attachment 8544725 [details] [diff] [review]:
-----------------------------------------------------------------

I *love* the amount of code deletion and conceptual simplification in this patch, particularly the simplification of GetApzcAtPoint(), and the removal of the obscured-regions stuff in UpdateHitTestTree(). Totally worth it!

r+ with (minor) comments addressed.

::: gfx/layers/apz/src/APZCTreeManager.cpp
@@ +253,5 @@
> +    nsIntRegion extraClip(ParentLayerIntRect::ToUntyped(RoundedIn(
> +        touchSensitiveRegion
> +        * aLayer.Metrics().GetDevPixelsPerCSSPixel()
> +        * parentCumulativeResolution)));
> +    clipRegion.AndWith(extraClip);

nsIntRegion has a method AndWith(nsIntRect), so there's no need to convert the touch-sensitive "region" (which is just a rect) to a region before intersecting,

@@ +293,5 @@
> +{
> +  if (gfxPrefs::LayoutEventRegionsEnabled()) {
> +    return aLayer.GetEventRegions();
> +  }
> +  return EventRegions(aLayer.GetVisibleRegion());

Could we just move this into LayerMetricsWrapper::GetEventRegions?

@@ +324,5 @@
>                                       HitTestingTreeNode* aParent,
>                                       HitTestingTreeNode* aNextSibling,
>                                       TreeBuildingState& aState)
>  {
> +  bool buildApzc = true;

I'd call this something like 'needsApzc' or 'shouldHaveApzc', because it being true doesn't mean we're going to "build" an APZC (we could reuse one).

@@ +342,5 @@
> +  if (!buildApzc) {
> +    node = RecycleOrCreateNode(aState, nullptr);
> +    AttachNodeToTree(node, aParent, aNextSibling);
> +    node->SetHitTestData(GetEventRegions(aLayer), aLayer.GetTransform(),
> +        aLayer.GetClipRect() ? Some(nsIntRegion(*aLayer.GetClipRect())) : Nothing());

If a non-member function 'nsIntRegion ToRegion(nsIntRect)' existed, this could be written:
  ToMaybe(aLayer.GetClipRect()).map(&ToRegion)

(This is more just an interesting observation than a suggestion that we introduce such a function.)

@@ -642,5 @@
> -      if (aLayer.GetClipRect()) {
> -        subtreeEventRegions.AndWith(*aLayer.GetClipRect());
> -      }
> -      aState.mEventRegions.LastElement().OrWith(subtreeEventRegions);
> -    }

*Awesome* amount of code deletion in this function! :D

::: gfx/layers/apz/src/APZCTreeManager.h
@@ +77,5 @@
>   * The bulk of the work of this class happens as part of the UpdateHitTestingTree
>   * function, which is when a layer tree update is received by the compositor.
> + * This function walks through the layer tree and creates a tree of
> + * HitTestingTreeNode instances to match the layer tree and for use in
> + * hit-testing on the input thread. APZC instances may be preserved across

for consistency, s/input thread/controller thread

@@ +460,4 @@
>     *
> +   * @param aState The current tree building state.
> +   * @param aLayer The (layer, metrics) pair which is the current position in
> +   *               the recursive walk of the layer tree. This calls builds a

s/calls/call

@@ +469,5 @@
>     *                           to the next APZC-bearing layer.
> +   * @param aParent The parent of any node built at this level.
> +   * @param aNextSibling The next sibling of any node built at this level.
> +   * @return The HitTestingTreeNode created at this level. This will always
> +   *         be non-null.

(I liked it when the initial columns of the descriptions lined up.)

::: gfx/layers/apz/src/HitTestingTreeNode.cpp
@@ +169,4 @@
>  {
>    mEventRegions = aRegions;
>    mTransform = aTransform;
>    mClipRegion = aClipRegion;

Maybe log the hit test data here, since log statements at the call sites are being removed?

@@ +231,5 @@
>    }
>    printf_stderr("%sHitTestingTreeNode (%p) APZC (%p) g=(%s) r=(%s) t=(%s) c=(%s)\n",
>      aPrefix, this, mApzc.get(), mApzc ? Stringify(mApzc->GetGuid()).c_str() : "",
>      Stringify(mEventRegions).c_str(), Stringify(mTransform).c_str(),
> +    Stringify(mClipRegion.valueOr(nsIntRegion())).c_str());

This doesn't distinguish between an empty clip region (for which IsOutsideClip() returns true for all points) and no clip region (for which IsOutsideClip() returns false for all points).

I'd prefer: mClipRegion ? Stringify(mClipRegion.ref()).c_str() : "none".

::: gfx/layers/apz/src/HitTestingTreeNode.h
@@ +32,5 @@
> + * are part of the same animated geometry root). If this happens, exactly one of
> + * the HitTestingTreeNode instances will be designated as the "primary holder"
> + * of the APZC. When this primary holder is destroyed, it will destroy the APZC
> + * along with it; in contrast, destroying non-primary-holder nodes will not
> + * destroy the APZC.

Please say explicitly that some but not all nodes correspond to a layer with scrollable metrics, and thus have an associated APZC.

@@ +82,5 @@
> +                      const Maybe<nsIntRegion>& aClipRegion);
> +  bool IsOutsideClip(const ParentLayerPoint& aPoint) const;
> +  /* Convert aPoint into the LayerPixel space for the layer corresponding to
> +   * this node. */
> +  gfx::Point4D Untransform(const ParentLayerPoint& aPoint) const;

Consider having this function return a Maybe<gfx::Point> (or Maybe<LayerPoint>). This 1) encapsulates the HasPositiveWCoord() call inside this function, and 2) forces call sites to deal with the fact that they may or may not be getting a point back (whereas right now a careless call site could potentially get a Point from the Point4D without checking the w coord).

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 59 • 10 years ago

(In reply to Botond Ballo [:botond] from comment #57)
> > +    if (aSibling->GetApzc()) {
> > +      AsyncPanZoomController* parent = mParent ? mParent-> GetNearestContainingApzc() : nullptr;
> 
> nit: stray space after 'mParent->'

Fixed

> Might be worth extracting a helper function:
> 
> void
> HitTestingTreeNode::SetApzcParent(AsyncPanZoomController* aApzc) {
>   // precondition: GetApzc() != nullptr
>   if (IsPrimaryHolder()) {
>     GetApzc()->SetParent(aApzc);
>   } else {
>     MOZ_ASSERT(GetApzc()->GetParent() == aApzc);
> }

Good call, done.

(In reply to Botond Ballo [:botond] from comment #58)
> Comment on attachment 8544725 [details] [diff] [review]
> Part 5b - Fully implement the HitTestingTree
>
> nsIntRegion has a method AndWith(nsIntRect), so there's no need to convert
> the touch-sensitive "region" (which is just a rect) to a region before
> intersecting,

Right, updated to not wrap in the nsIntRegion.
 
> @@ +293,5 @@
> > +{
> > +  if (gfxPrefs::LayoutEventRegionsEnabled()) {
> > +    return aLayer.GetEventRegions();
> > +  }
> > +  return EventRegions(aLayer.GetVisibleRegion());
> 
> Could we just move this into LayerMetricsWrapper::GetEventRegions?

I would rather not do this because it's putting logic into LayerMetricsWrapper that I don't think really belongs there. The only thing the LayerMetricsWrapper should be doing is wrapping the multi-framemetrics aspect of the layer tree, and throwing this event regions stuff in there seems wrong to me. I left this as-is but you can try to convince me if you feel strongly about this.

> > +  bool buildApzc = true;
> 
> I'd call this something like 'needsApzc' or 'shouldHaveApzc', because it
> being true doesn't mean we're going to "build" an APZC (we could reuse one).

Renamed to needsApzc

> If a non-member function 'nsIntRegion ToRegion(nsIntRect)' existed, this
> could be written:
>   ToMaybe(aLayer.GetClipRect()).map(&ToRegion)
> 
> (This is more just an interesting observation than a suggestion that we
> introduce such a function.)

There's a lot of places we wrap nsIntRect into nsIntRegion. I almost wish there was some magical implicit conversion that happened and that would also allow writing the ToMaybe(..).map(..) thing.

> > + * hit-testing on the input thread. APZC instances may be preserved across
> 
> for consistency, s/input thread/controller thread

Fixed

> > +   *               the recursive walk of the layer tree. This calls builds a
> 
> s/calls/call

Fixed

> > +   * @return The HitTestingTreeNode created at this level. This will always
> > +   *         be non-null.
> 
> (I liked it when the initial columns of the descriptions lined up.)

I kinda did too but it wasn't consistent with the rest of the file. Although really I just want clang-format to deal with this (I'll do it one of these days...).

> ::: gfx/layers/apz/src/HitTestingTreeNode.cpp
> @@ +169,4 @@
> >  {
> >    mEventRegions = aRegions;
> >    mTransform = aTransform;
> >    mClipRegion = aClipRegion;
> 
> Maybe log the hit test data here, since log statements at the call sites are
> being removed?

Wasn't really sure if this was needed, as the tree dump logs all of this in a much more useful format. Knowing it at the time it gets populated isn't that useful as long as you know what the full tree looks like. I left this as-is but I can add logging here if you have a specific need for it.

> >      Stringify(mEventRegions).c_str(), Stringify(mTransform).c_str(),
> > +    Stringify(mClipRegion.valueOr(nsIntRegion())).c_str());
> 
> This doesn't distinguish between an empty clip region (for which
> IsOutsideClip() returns true for all points) and no clip region (for which
> IsOutsideClip() returns false for all points).
> 
> I'd prefer: mClipRegion ? Stringify(mClipRegion.ref()).c_str() : "none".

Good catch, fixed.

> Please say explicitly that some but not all nodes correspond to a layer with
> scrollable metrics, and thus have an associated APZC.

Done

> > +  gfx::Point4D Untransform(const ParentLayerPoint& aPoint) const;
> 
> Consider having this function return a Maybe<gfx::Point> (or
> Maybe<LayerPoint>). This 1) encapsulates the HasPositiveWCoord() call inside
> this function, and 2) forces call sites to deal with the fact that they may
> or may not be getting a point back (whereas right now a careless call site
> could potentially get a Point from the Point4D without checking the w coord).

Ah, nice! I was trying to think of a way to do this but didn't think of using Maybe for it. I made it return a Maybe<LayerPoint> which works well and had to add a new PixelCastJustification to ViewAs the LayerPoint into a ParentLayerPoint as we recurse down a level.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 60 • 10 years ago

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/3352803d949b
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/e92ec65eb66b
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/83aa477d175b
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/aa2fa9528e52
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/7d4c6b7bf0d5
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/160acaa4fc44
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/6f5ff015b5e6
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/c8c65c2ef9bf
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/5d506eb6fd4c

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 61 • 10 years ago

Also https://hg.mozilla.org/integration/mozilla-inbound/rev/9cb763be208e

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 62 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/3352803d949b
https://hg.mozilla.org/mozilla-central/rev/e92ec65eb66b
https://hg.mozilla.org/mozilla-central/rev/83aa477d175b
https://hg.mozilla.org/mozilla-central/rev/aa2fa9528e52
https://hg.mozilla.org/mozilla-central/rev/7d4c6b7bf0d5
https://hg.mozilla.org/mozilla-central/rev/160acaa4fc44
https://hg.mozilla.org/mozilla-central/rev/6f5ff015b5e6
https://hg.mozilla.org/mozilla-central/rev/c8c65c2ef9bf
https://hg.mozilla.org/mozilla-central/rev/5d506eb6fd4c
https://hg.mozilla.org/mozilla-central/rev/9cb763be208e

	Botond Ballo [:botond]
	Comment 63 • 10 years ago

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #40)
> (In reply to Botond Ballo [:botond] from comment #39)
> > But when do we need ProjectPoint calls versus Matrix4x4::operator*(Point)
> > calls? They're both operations that take a matrix and a point, and produce a
> > point.
> 
> Matt Woodrow says at https://bugzilla.mozilla.org/show_bug.cgi?id=866232#c48:
> 
> "Use Invert().ProjectPoint() here, since it handles recovering the
> depth/perspective components if the transform is 3d."
> 
> So that's what I did! :)

I filed bug 1120683 to follow up on this.