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

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Danilo Cesar Lemes de Paula]

When running Fennec with C++ APZ, we get a warning for each tap.

Comment 1

[Danilo Cesar Lemes de Paula]

Attachment: bug-1159405.patch

Comment 2

[Kartikaya Gupta (email:[e-mail])]

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.

Comment 3

[Kartikaya Gupta (email:[e-mail])]

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.

Comment 4

[Danilo Cesar Lemes de Paula]

I'll rewrite the patch. Thanks.

Comment 5

[Kartikaya Gupta (email:[e-mail])]

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."

Comment 6

[Danilo Cesar Lemes de Paula]

Attachment: bug-1159405.patch

Comment 7

[Kartikaya Gupta (email:[e-mail])]

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 ".

Comment 8

[Danilo Cesar Lemes de Paula]

Attachment: bug-1159405.patch

done.

Comment 9

[Timothy Nikkel (:tnikkel)]

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 10

[Botond Ballo [:botond]]

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.)

Comment 11

[Danilo Cesar Lemes de Paula]

Attachment: bug-1159405.patch

r+ from botond

Comment 12

[Danilo Cesar Lemes de Paula]

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

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/35408d0b3835

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/35408d0b3835