This is a slight change of behaviour, but should be the correct thing to do I think.

Comment on attachment 8610873 [details] [diff] [review] Part 1. Add layers id to hit test nodes Review of attachment 8610873 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/layers/apz/src/HitTestingTreeNode.cpp @@ +237,5 @@ > mPrevSibling->Dump(aPrefix); > } > printf_stderr("%sHitTestingTreeNode (%p) APZC (%p) g=(%s) %s%sr=(%s) t=(%s) c=(%s)\n", > + aPrefix, this, mApzc.get(), > + mApzc ? Stringify(mApzc->GetGuid()).c_str() : nsPrintfCString("l=%llu", mLayersId).get(), Use |"l=%" PRIu64, ...| instead of |"l=%llu", ...| for better cross-platform compat

Comment on attachment 8610876 [details] [diff] [review] Search the tree for an apzc with the right layers id Review of attachment 8610876 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for splitting the patches so cleanly! Makes it easy to read :) ::: gfx/layers/apz/src/APZCTreeManager.cpp @@ +1451,5 @@ > return nullptr; > } > > +AsyncPanZoomController* > +APZCTreeManager::FindRootAzpcForLayersId(uint64_t aLayersId) const s/Azpc/Apzc/ @@ +1464,5 @@ > + queue.RemoveElementAt(0); > + > + AsyncPanZoomController* apzc = node->GetApzc(); > + if (apzc && apzc->GetLayersId() == aLayersId) { > + return GetRootAPZCForLayersId(apzc); I'm fairly sure that the first APZC you encounter for the layers id will be root one, so you don't need to call GetRootAPZCForLayersId here. (That would be true even if you did a recursive DFS search instead of BFS). I may have missed something though. @@ +1714,5 @@ > +APZCTreeManager::GetRootAPZCForLayersId(AsyncPanZoomController* aApzc) const > +{ > + AsyncPanZoomController* apzc = aApzc; > + while (apzc && !apzc->IsRootForLayersId()) { > + apzc = apzc->GetParent(); The function above this (RootAPZCForLayersId) is identical except for the locking/refptr stuff, you could trivially refactor it to use this function as a helper (assuming you end up keeping this function).

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #5) > I'm fairly sure that the first APZC you encounter for the layers id will be > root one, so you don't need to call GetRootAPZCForLayersId here. (That would > be true even if you did a recursive DFS search instead of BFS). I may have > missed something though. I think if we have a scrollable element inside a fixed element that is on top of the page we could encounter that first. Hmm, but would that apzc have the root apzc as it's parent? We use scrollparent id's to determine the parent apzc? Then that should work I think, the fixed content should have the root as it's scrollparent due to how display list building works and how we set scrollparent ids.

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #4) > Comment on attachment 8610873 [details] [diff] [review] > Add layers id to hit test nodes > > Review of attachment 8610873 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: gfx/layers/apz/src/HitTestingTreeNode.cpp > @@ +237,5 @@ > > mPrevSibling->Dump(aPrefix); > > } > > printf_stderr("%sHitTestingTreeNode (%p) APZC (%p) g=(%s) %s%sr=(%s) t=(%s) c=(%s)\n", > > + aPrefix, this, mApzc.get(), > > + mApzc ? Stringify(mApzc->GetGuid()).c_str() : nsPrintfCString("l=%llu", mLayersId).get(), > > Use |"l=%" PRIu64, ...| instead of |"l=%llu", ...| for better cross-platform > compat If we do this, we might as well make a corresponding change to AppendToString(stringstream, ScrollableLayerGuid) in LayersLogging.cpp for consistency. (The compiler doesn't enforce this because we were thus far unable to add MOZ_FORMAT_PRINTF to nsPrintfCString, see bug 1060419).

(In reply to Timothy Nikkel (:tn) from comment #6) > (In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #5) > > I'm fairly sure that the first APZC you encounter for the layers id will be > > root one, so you don't need to call GetRootAPZCForLayersId here. (That would > > be true even if you did a recursive DFS search instead of BFS). I may have > > missed something though. > > I think if we have a scrollable element inside a fixed element that is on > top of the page we could encounter that first. Hmm, but would that apzc have > the root apzc as it's parent? We use scrollparent id's to determine the > parent apzc? Then that should work I think, the fixed content should have > the root as it's scrollparent due to how display list building works and how > we set scrollparent ids. We do not use scroll parent ids for determining the APZC tree structure - it's determined solely by the layer structure. The scroll parent id is only used for building the overscroll handoff chain.

Comment on attachment 8610876 [details] [diff] [review] Search the tree for an apzc with the right layers id Review of attachment 8610876 [details] [diff] [review]: ----------------------------------------------------------------- Dropping review until we clear up the "finding the root APZC for a layers id" bit. ::: gfx/layers/apz/src/APZCTreeManager.cpp @@ +1456,5 @@ > +{ > + if (!mRootNode) { > + return nullptr; > + } > + nsTArray<HitTestingTreeNode*> queue; Would it be more appropriate to use a container with (amortized) constant-time removal at the front, like std::deque? @@ +1458,5 @@ > + return nullptr; > + } > + nsTArray<HitTestingTreeNode*> queue; > + queue.AppendElement(mRootNode); > + while (queue.Length()) { nit: I prefer !queue.IsEmpty() for things like this. @@ +1464,5 @@ > + queue.RemoveElementAt(0); > + > + AsyncPanZoomController* apzc = node->GetApzc(); > + if (apzc && apzc->GetLayersId() == aLayersId) { > + return GetRootAPZCForLayersId(apzc); So based on comment 6 and comment 8, just returning 'apzc' here isn't sufficient. However, walking up the the APZC tree from 'apzc' doesn't help either, because in the situation described in comment 6, the root APZC for the layers id would be a sibling of 'apzc', not an ancestor. Do we have enough information in the layer tree structure to be able to find the root APZC for a layers id in general? Do we need to start looking at scroll parent ids? @@ +1468,5 @@ > + return GetRootAPZCForLayersId(apzc); > + } > + > + HitTestingTreeNode* child = node->GetLastChild(); > + while (child) { nit: I think a for loop would read better.

(In reply to Botond Ballo [:botond] from comment #9) > So based on comment 6 and comment 8, just returning 'apzc' here isn't > sufficient. > > However, walking up the the APZC tree from 'apzc' doesn't help either, > because in the situation described in comment 6, the root APZC for the > layers id would be a sibling of 'apzc', not an ancestor. > > Do we have enough information in the layer tree structure to be able to find > the root APZC for a layers id in general? Do we need to start looking at > scroll parent ids? Looking at scroll parent ids relies on the scroll parent working the same way it currently does for out of flow items like fixed position (ie the scroll parent of scroll frames inside fixed pos content will be the root scroll frame). This is quirky, and not necessarily the right way to do things, so we might want to change it in the future. So it might be best to avoid using scroll parent ids. The only other way I can think of is to tag frame metrics with a flag that says "this frame metrics is for the rootscrollframe/rootelement of the root document in its process". Does that seem like a reasonable plan? So this also means that AsyncPanZoomController::IsRootForLayersId is not really correct, and we should look at it's users and adjust them if necessary.

(In reply to Timothy Nikkel (:tn) from comment #10) > The only other way I can think of is to tag frame metrics with a flag that > says "this frame metrics is for the rootscrollframe/rootelement of the root > document in its process". Does that seem like a reasonable plan? In the terminology of bug 1158424, this basically means storing mIsProcessRoot after all (except now we know it should be called something like mIsLayersIdRoot rather than mIsProcessRoot). Seems reasonable to me in the absence of a better idea. > So this also means that AsyncPanZoomController::IsRootForLayersId is not > really correct, and we should look at it's users and adjust them if > necessary. Yup. I basically say as much in bug 1158424 comment 9.

Sounds reasonable to me as well. With all the stuff being added to FrameMetrics we should probably up the priority of bug 1066763, but that's a separate issue.

I just want the shortest path to my goal in this bug, so I'm just going to rename AsyncPanZoomController::IsRootForLayersId so it is out of the way (and I can use the name in the next patch). I'll save the cleanup of this function for another bug.

I applied the review comments from comment 9 and comment 5 (some of them from comment 5 no longer apply since that code does not exist in this patch). Do I need to enter the monitor here? It's not going to cause any deadlock with other held locks? I'm not clear on the locking needs and ordering, so give that extra scrutiny.

Comment on attachment 8612777 [details] [diff] [review] Part 4. rename AsyncPanZoomController::IsRootForLayersId Review of attachment 8612777 [details] [diff] [review]: ----------------------------------------------------------------- (In reply to Timothy Nikkel (:tn) from comment #14) > I just want the shortest path to my goal in this bug, so I'm just going to > rename AsyncPanZoomController::IsRootForLayersId so it is out of the way > (and I can use the name in the next patch). I'll save the cleanup of this > function for another bug. Sounds good - I'll do the cleanup in bug 1158424.

Comment on attachment 8612778 [details] [diff] [review] Part 5. Search the tree for an apzc with the right layers id that is root Review of attachment 8612778 [details] [diff] [review]: ----------------------------------------------------------------- (In reply to Timothy Nikkel (:tn) from comment #15) > Do I need to enter the monitor here? It's not going to cause any deadlock > with other held locks? I'm not clear on the locking needs and ordering, so > give that extra scrutiny. mTreeLock should be held while running FindRootApzcForLayersId(). The only current caller, GetAPZCAtPoint(), is already holding it, but let's add 'mTreeLock.AssertCurrentThreadOwns()' at the top of FindRootApzcForLayersId() for the benefit of possible future callers.

(In reply to Botond Ballo [:botond] from comment #17) > (In reply to Timothy Nikkel (:tn) from comment #15) > > Do I need to enter the monitor here? It's not going to cause any deadlock > > with other held locks? I'm not clear on the locking needs and ordering, so > > give that extra scrutiny. > > mTreeLock should be held while running FindRootApzcForLayersId(). The only > current caller, GetAPZCAtPoint(), is already holding it, but let's add > 'mTreeLock.AssertCurrentThreadOwns()' at the top of > FindRootApzcForLayersId() for the benefit of possible future callers. Through an IRC discussion I realized that you were actually asking about acquiring AsyncPanZoomController::mMonitor in IsRootForLayersId(). Summarizing: it's safe to do so (it obeys the lock ordering described at the top of APZCTreeManager.h), and you should do it because mMonitor is described as protecting mFrameMetrics among other things.

This test triggers the assert in GetAPZCAtPoint about not returning a null azpc. Because we hit a layer with no apzc, and no apzc on any ancestor layer. So add a root apzc to the root layer (because that is more like what we have in practice), and then change the test to expect to get this root apzc. The test is intending to test that we don't get the apzc on the other child layer (that is covered by the layer with no apzc) so we are still testing what the test intends to test.

Comment on attachment 8613141 [details] [diff] [review] Part 6. Modify the test for bug 1119497 Review of attachment 8613141 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/tests/gtest/TestAsyncPanZoomController.cpp @@ +1893,5 @@ > static void SetScrollableFrameMetrics(Layer* aLayer, FrameMetrics::ViewID aScrollId, > CSSRect aScrollableRect = CSSRect(-1, -1, -1, -1)) { > FrameMetrics metrics; > metrics.SetScrollId(aScrollId); > + // By convention START_SCROLL_ID is the root, so mark it as such. Please mention that this convention applies to this test file only, not to production code, lest someone reading this gets the wrong the idea. @@ +2837,5 @@ > // LayerID 0 12 > + // 0 is the root and has an APZC > + // 1 is behind 2 and has an APZC > + // 2 entirely covers 1 and should take all the input events, but has no APZC > + // so hits to 2 should goto to the route APZC "go to the root"

https://hg.mozilla.org/integration/mozilla-inbound/rev/b63dd7e56dc6 https://hg.mozilla.org/integration/mozilla-inbound/rev/bab9b38fcc1a https://hg.mozilla.org/integration/mozilla-inbound/rev/b04610020a55 https://hg.mozilla.org/integration/mozilla-inbound/rev/44a885179fc3 https://hg.mozilla.org/integration/mozilla-inbound/rev/1f0fdd3b4007 https://hg.mozilla.org/integration/mozilla-inbound/rev/098390cdba51

https://hg.mozilla.org/mozilla-central/rev/b63dd7e56dc6 https://hg.mozilla.org/mozilla-central/rev/bab9b38fcc1a https://hg.mozilla.org/mozilla-central/rev/b04610020a55 https://hg.mozilla.org/mozilla-central/rev/44a885179fc3 https://hg.mozilla.org/mozilla-central/rev/1f0fdd3b4007 https://hg.mozilla.org/mozilla-central/rev/098390cdba51