Danilo Cesar Lemes de Paula Assignee
	Description • 9 years ago

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

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

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.

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

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.

	Danilo Cesar Lemes de Paula Assignee
	Comment 4 • 9 years ago

I'll rewrite the patch. Thanks.

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

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

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

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

	Danilo Cesar Lemes de Paula Assignee
	Comment 8 • 9 years ago

done.

	Timothy Nikkel (:tnikkel)
	Comment 9 • 9 years ago

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?

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

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

	Danilo Cesar Lemes de Paula Assignee
	Comment 11 • 9 years ago

r+ from botond

	Danilo Cesar Lemes de Paula Assignee
	Comment 12 • 9 years ago

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

	Pulsebot
	Comment 13 • 9 years ago

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

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

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