if the layer we hit doesn't have an apzc (and no ancestor does either) search the tree for the root apzc with the same layers id

RESOLVED FIXED in Firefox 41

Status

()

Core
Panning and Zooming
RESOLVED FIXED
3 years ago
6 months ago

People

(Reporter: tnikkel, Assigned: tnikkel)

Tracking

(Depends on: 2 bugs)

unspecified
mozilla41
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox41 fixed)

Details

Attachments

(6 attachments, 1 obsolete attachment)

Comment hidden (empty)
(Assignee)

Comment 1

3 years ago
Created attachment 8610873 [details] [diff] [review]
Part 1. Add layers id to hit test nodes
Attachment #8610873 - Flags: review?(botond)
(Assignee)

Comment 2

3 years ago
Created attachment 8610875 [details] [diff] [review]
Part 2. only look for an azpc with the same layers id

This is a slight change of behaviour, but should be the correct thing to do I think.
Attachment #8610875 - Flags: review?(botond)
(Assignee)

Comment 3

3 years ago
Created attachment 8610876 [details] [diff] [review]
Search the tree for an apzc with the right layers id
Attachment #8610876 - Flags: review?(botond)
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).
(Assignee)

Comment 6

3 years ago
(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.

Updated

3 years ago
Attachment #8610873 - Flags: review?(botond) → review+
(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).

Updated

3 years ago
Attachment #8610875 - Flags: review?(botond) → review+
(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.
Attachment #8610876 - Flags: review?(botond)
(Assignee)

Comment 10

3 years ago
(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.
Flags: needinfo?(bugmail.mozilla)
Flags: needinfo?(botond)
(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.
Flags: needinfo?(botond)
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.
Flags: needinfo?(bugmail.mozilla)
(Assignee)

Comment 13

3 years ago
Created attachment 8612773 [details] [diff] [review]
Part 3. Add IsLayersIdRoot to frame metrics
Attachment #8612773 - Flags: review?(botond)
(Assignee)

Updated

3 years ago
Attachment #8610873 - Attachment description: Add layers id to hit test nodes → Part 1. Add layers id to hit test nodes
(Assignee)

Updated

3 years ago
Attachment #8610876 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Attachment #8610875 - Attachment description: only look for an azpc with the same layers id → Part 2. only look for an azpc with the same layers id
(Assignee)

Updated

3 years ago
Attachment #8612773 - Attachment description: Add IsLayersIdRoot to frame metrics → Part 3. Add IsLayersIdRoot to frame metrics
(Assignee)

Comment 14

3 years ago
Created attachment 8612777 [details] [diff] [review]
Part 4. rename AsyncPanZoomController::IsRootForLayersId

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.
Attachment #8612777 - Flags: review?(botond)
(Assignee)

Comment 15

3 years ago
Created attachment 8612778 [details] [diff] [review]
Part 5. Search the tree for an apzc with the right layers id that is root

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.
Attachment #8612778 - Flags: review?(botond)

Updated

3 years ago
Attachment #8612773 - Flags: review?(botond) → review+

Updated

3 years ago
Blocks: 1158424
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.
Attachment #8612777 - Flags: review?(botond) → review+
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.
Attachment #8612778 - Flags: review?(botond) → review+
(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.
(Assignee)

Comment 19

3 years ago
Created attachment 8613141 [details] [diff] [review]
Part 6. Modify the test for bug 1119497

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.
Attachment #8613141 - Flags: review?(botond)
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"
Attachment #8613141 - Flags: review?(botond) → review+
(Assignee)

Comment 21

3 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b102cdc91601

Comment 22

3 years ago
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
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox41: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
(Assignee)

Updated

3 years ago
Depends on: 1172310
Depends on: 1173108
Depends on: 1180899

Updated

6 months ago
Depends on: 1364440

Updated

6 months ago
Depends on: 1364443
You need to log in before you can comment on or make changes to this bug.