Fix "Found a non-root APZ with no handoff parent" warning when root process has a root XUL document with scrollable subframes

RESOLVED FIXED in Firefox 40

Status

()

RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: danilo.eu, Assigned: danilo.eu)

Tracking

Trunk
mozilla40
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox40 fixed)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

4 years ago
When running Fennec with C++ APZ, we get a warning for each tap.
(Assignee)

Updated

4 years ago
Assignee: nobody → danilo.eu
Depends on: 1156401
(Assignee)

Comment 1

4 years ago
Created attachment 8598853 [details] [diff] [review]
bug-1159405.patch
Attachment #8598853 - Flags: review?(bugmail.mozilla)
Attachment #8598853 - Flags: review?(botond)
(Assignee)

Updated

4 years ago
Blocks: 776030
Sorry for the delay, I wanted to do a build locally and check some assumptions of mine. I'll review this today for sure, but in the meantime you should be able to continue working since this just fixes a warning and shouldn't block you.
Ok, so it turns out this warning can be triggered in desktop builds as well with APZ enabled, if you load a page like about:preferences which lives in the root process and is scrollable. It's basically the same presshell arrangement as we see in Fennec, where there's a root presShell for the XUL window (which is the presShell in PaintFrame) and then there's a child presShell in the same process which generates a scrollable layer, and the handoff parent for that scrollable layer is not set even though there is an APZ higher in the tree (from the root document's root element).

... Anyway so that means we actually don't need the ifdef in this patch (sorry about that!) and we'll probably want to adjust the comment accordingly.
(Assignee)

Comment 4

4 years ago
I'll rewrite the patch. Thanks.
Comment on attachment 8598853 [details] [diff] [review]
bug-1159405.patch

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

::: layout/base/nsLayoutUtils.cpp
@@ +3113,5 @@
>            id = nsLayoutUtils::FindOrCreateIDFor(content);
>          }
>        }
>      }
> +#if defined(MOZ_ANDROID_APZ)

Remove ifdef (sorry again about telling you to add it in the first place)

@@ +3114,5 @@
>          }
>        }
>      }
> +#if defined(MOZ_ANDROID_APZ)
> +    else if (!ignoreViewportScrolling && !presContext->IsRootContentDocument()) {

I think this condition can be changed to "else if (presShell->GetDocument() && presShell->GetDocument()->IsRootDisplayDocument()) {" to match the code at http://mxr.mozilla.org/mozilla-central/source/layout/base/nsLayoutUtils.cpp?rev=18d118c05f8a#1889.

@@ +3118,5 @@
> +    else if (!ignoreViewportScrolling && !presContext->IsRootContentDocument()) {
> +      // For fennec, when ignoreVirwPortScrolling and presContext->IsRootContentDocument() are
> +      // false, it means that the screen doesn't have a scrollable root
> +      // element. In that case, we want the ViewID to be the ID of the top
> +      // most element.

This comment should be made more generic:

"In cases where the root document is a XUL document, we want to take the ViewID from the root element, as that will be the ViewID of the root APZ in the tree."
Attachment #8598853 - Flags: review?(bugmail.mozilla) → review-
Blocks: 1013364
Component: Widget: Android → Layout
OS: Android → All
Summary: Fix "Found a non-root APZ with no handoff parent" warning when running Fennec with C++ APZ → Fix "Found a non-root APZ with no handoff parent" warning when root process has a root XUL document with scrollable subframes
(Assignee)

Comment 6

4 years ago
Created attachment 8599367 [details] [diff] [review]
bug-1159405.patch
Attachment #8598853 - Attachment is obsolete: true
Attachment #8598853 - Flags: review?(botond)
Attachment #8599367 - Flags: review?(bugmail.mozilla)
Comment on attachment 8599367 [details] [diff] [review]
bug-1159405.patch

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

Update commit message to be more descriptive, e.g. "Ensure we set the handoff parent for scrollable frames in a process with a root XUL document"

::: layout/base/nsLayoutUtils.cpp
@@ +3113,5 @@
>            id = nsLayoutUtils::FindOrCreateIDFor(content);
>          }
>        }
>      }
> +    else if (presShell->GetDocument() && presShell->GetDocument()->IsRootDisplayDocument()) {

Move else up to follow } on previous line

@@ +3116,5 @@
>      }
> +    else if (presShell->GetDocument() && presShell->GetDocument()->IsRootDisplayDocument()) {
> +      // In cases where the root document is a XUL document, we want to take
> +      // the ViewID from the root element, as that will be the ViewID of the
> +      // root APZ in the tree.".

Remove trailing ".
Attachment #8599367 - Flags: review?(bugmail.mozilla) → review?(botond)
(Assignee)

Comment 8

4 years ago
Created attachment 8599378 [details] [diff] [review]
bug-1159405.patch

done.
Attachment #8599367 - Attachment is obsolete: true
Attachment #8599367 - Flags: review?(botond)
Attachment #8599378 - Flags: review?(bugmail.mozilla)
Attachment #8599378 - Flags: review?(bugmail.mozilla) → review?(botond)
Comment on attachment 8599378 [details] [diff] [review]
bug-1159405.patch

Hmm, isn't this going to create a non-null scrollid for the root layer in non-APZ fennec? And wouldn't that break things?
Comment on attachment 8599378 [details] [diff] [review]
bug-1159405.patch

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

I think Timothy's right in comment 9, we don't want to do this on non-APZ Fennec.

So, it seems we should have an #ifdef after all, but it should be:

  #if !defined(MOZ_WIDGET_ANDROID) || defined(MOZ_ANDROID_APZ)

(just like the one in the change to PaintRoot in bug 1156401, which can be thought of as going hand-in-hand with this change).

r+ with that change.

http://mxr.mozilla.org/mozilla-central/source/layout/base/nsDisplayList.cpp?rev=8efcb1c3142e#1487

::: layout/base/nsLayoutUtils.cpp
@@ +3115,5 @@
>        }
> +    } else if (presShell->GetDocument() && presShell->GetDocument()->IsRootDisplayDocument()) {
> +      // In cases where the root document is a XUL document, we want to take
> +      // the ViewID from the root element, as that will be the ViewID of the
> +      // root APZ in the tree.

s/APZ/APZC 

("APZ" (Async Panning and Zooming) refers to the feature (or the module of the code that implements it), while "APZC" (AsyncPanZoomController) is the object that does async panning and zooming for a single scrollable frame.)
Attachment #8599378 - Flags: review?(botond) → review+
(Assignee)

Comment 11

4 years ago
Created attachment 8599853 [details] [diff] [review]
bug-1159405.patch

r+ from botond
Attachment #8599378 - Attachment is obsolete: true
Attachment #8599853 - Flags: review+
(Assignee)

Comment 12

4 years ago
try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9a9f7ddca5b3 (also contains the changes from 1156401)
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/35408d0b3835
Status: NEW → RESOLVED
Last Resolved: 4 years ago
status-firefox40: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
You need to log in before you can comment on or make changes to this bug.