Closed Bug 1156401 Opened 9 years ago Closed 9 years ago

Fix ContextMenu handling when building Fennec with C++ APZ enabled

Categories

(Core Graveyard :: Widget: Android, defect)

All
Android
defect
Not set
normal

Tracking

(firefox40 fixed)

RESOLVED FIXED
mozilla40
Tracking Status
firefox40 --- fixed

People

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

References

Details

Attachments

(2 files, 5 obsolete files)

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Firefox/31.0 Iceweasel/31.6.0
Build ID: 20150331233809

Steps to reproduce:

When building Fennec with APZC enable the contextMenu is not shown after long taps.
Blocks: apz-fennec
Status: UNCONFIRMED → NEW
Component: General → Widget: Android
Ever confirmed: true
OS: Linux → Android
Product: Firefox for Android → Core
Hardware: x86_64 → All
Summary: Fix ContextMenu handling when building Fennec with APCZ enabled → Fix ContextMenu handling when building Fennec with C++ APZ enabled
So... APZEventState::ProcessSingleTap never gets called when on Portrait mode. It does work on landscape. Not actually a landscape/portrait mode problem, is just that Landscape uses scroll and Portrait doesn't...

When it does get called on landscape, AGuid = { l=1, p=3, v=3 }

Following the unwinding the execution stack, we arrive at InputQueue::SetConfirmedTargetApzc
This function gets called in both cases, but with the correct aguid on landscape and null on portrait.
Checking it we see that we're geting an empty aTargetApzc.

This comes from APZCTreeManager::SetTargetAPZC
where aTargets[0] is { l=1, p=0, v=0 } for Landscape and { l=1, p=3, v=3 } for portrait.
It's beeing called by nsBaseWidget::SetConfirmedTargetAPZC....

Unwind a bit deeper, since APZCCallbackHelper::SendSetTargetAPZCNotification the guid looks to be
screwed (because we know it arrives in ProcessUntrasform as 1,3,3). 

So, the issue looks to be PrepareForSetTargetAPZCNotification.
When scrollAncestor is null (there's no need for scrolling),  dpElement becames null.
At the end guidIsValid=false, however we already edited the scrollableLayerGuid targetList with an invalid guid.
That same list will be used again by APZCTreeManager::SetTargetAPZC, and IMO, no apcz is found because of that.

So, or is that the problem, or APZCTreeManager::GetTargetAPZC should be smart to deal with this?
Assignee: nobody → danilo.eu
For the record after many false attempts we may have figured out the problem here. The log at http://logs.glob.uno/?c=mozilla%23apz#c6750 captures some of the discussion. Basically, the scrollAncestor is coming out null because we walk all the way up to the XUL document's root, but the XUL document doesn't have a root scrollable frame. This is actually ok, as long as we then fall back to getting the scrollId of the root frame of the XUL document (even though it doesn't have a scrollable frame associated with it), because it should still have an APZC.
I believe the other half of this fix is to ensure that the frame metrics for the root XUL document's root layer has a nonzero scroll id. Whether or not this happens depends on whether or not |content| is null in the call at http://mxr.mozilla.org/mozilla-central/source/layout/base/nsDisplayList.cpp?rev=8efcb1c3142e#1494 and so some fiddling with the ifdefs and pref-checks is needed there.
Attached patch wip.patch (obsolete) — Splinter Review
Attachment #8598780 - Flags: review?(bugmail.mozilla)
Attachment #8598780 - Flags: review?(botond)
Attached patch part1.patch (obsolete) — Splinter Review
Attachment #8598780 - Attachment is obsolete: true
Attachment #8598780 - Flags: review?(bugmail.mozilla)
Attachment #8598780 - Flags: review?(botond)
Attachment #8598822 - Flags: review?(bugmail.mozilla)
Attachment #8598822 - Flags: review?(botond)
Attached patch part2.patch (obsolete) — Splinter Review
Attachment #8598823 - Flags: review?(bugmail.mozilla)
Attachment #8598823 - Flags: review?(botond)
Comment on attachment 8598822 [details] [diff] [review]
part1.patch

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

r+ but botond should definitely review as well since most of this patch came from me.
Attachment #8598822 - Flags: review?(bugmail.mozilla) → review+
Comment on attachment 8598823 [details] [diff] [review]
part2.patch

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

> For some reason, painting wasn't working on fennec with apzc.
> FirstPaintViewport wasn't beeing called.

Reword this to be "Since SetFirstPaintViewport wasn't being called on Fennec with APZ, none of the painted content was being displayed."

::: gfx/layers/composite/AsyncCompositionManager.cpp
@@ +605,5 @@
>      // TODO: When we enable APZ on Fennec, we'll need to call SyncFrameMetrics here.
>      // When doing so, it might be useful to look at how it was called here before
>      // bug 1036967 removed the (dead) call.
>  
> +    if (mIsFirstPaint) {

Put all of this inside an ifdef MOZ_ANDROID_APZ
Attachment #8598823 - Flags: review?(bugmail.mozilla) → review+
Attached patch part1.patch (obsolete) — Splinter Review
r+ from latest review.
Attachment #8598822 - Attachment is obsolete: true
Attachment #8598822 - Flags: review?(botond)
Attachment #8598846 - Flags: review+
Attached patch part2.patch (obsolete) — Splinter Review
r+ from latest review
Attachment #8598823 - Attachment is obsolete: true
Attachment #8598823 - Flags: review?(botond)
Attachment #8598847 - Flags: review+
Blocks: 1159405
Botond still should review both of these patches.
Comment on attachment 8598846 [details] [diff] [review]
part1.patch

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

::: gfx/layers/apz/util/APZCCallbackHelper.cpp
@@ +501,5 @@
>  }
>  
> +
> +static dom::Element*
> +GetRootDocumentElementFor(nsIWidget* aWidget)

It might be useful to reuse this function in ChromeProcessController::InitializeRoot. We could move it to nsLayoutUtils to allow this.

I'll leave it up to you if you'd like to do this.

@@ +532,5 @@
>      nsLayoutUtils::GetFrameForPoint(aRootFrame, point, nsLayoutUtils::IGNORE_ROOT_SCROLL_FRAME);
>    nsIScrollableFrame* scrollAncestor = GetScrollableAncestorFrame(target);
> +  nsCOMPtr<dom::Element> dpElement = scrollAncestor
> +    ? GetDisplayportElementFor(scrollAncestor)
> +    : GetRootDocumentElementFor(aWidget);

We're making an assumption here that if there is no scrollAncestor, there is already a display port (otherwise we get into the CalculateAndSetDisplayPortMargins call at the end of the function which needs a scroll frame to calculate a displayport).

Please add a comment that documents this assumption, and perhaps a 'MOZ_ASSERT(scrollAncestor)', before that call.
Attachment #8598846 - Flags: review+
Comment on attachment 8598847 [details] [diff] [review]
part2.patch

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

::: gfx/layers/composite/AsyncCompositionManager.cpp
@@ +610,5 @@
> +    if (mIsFirstPaint) {
> +      CSSToLayerScale geckoZoom = metrics.LayersPixelsPerCSSPixel().ToScaleFactor();
> +      LayerIntPoint scrollOffsetLayerPixels = RoundedToInt(metrics.GetScrollOffset() * geckoZoom);
> +      mContentRect = metrics.GetScrollableRect();
> +      SetFirstPaintViewport(scrollOffsetLayerPixels,

Do we want to be doing this for all scrollable layers, or only the root?

I would think the latter, given that we're setting information about the "viewport", but I'm not familiar enough with the Java bits to say for sure.
(In reply to Botond Ballo [:botond] from comment #13)
> 
> Do we want to be doing this for all scrollable layers, or only the root?
> 
> I would think the latter, given that we're setting information about the
> "viewport", but I'm not familiar enough with the Java bits to say for sure.

Yeah it's more correct to only do it for the root. To fix the painting issue Danilo was seeing it doesn't really matter, since we just need to update the paint state in LayerView. If we end up needing proper metrics in Java later we'll need to fix this up more comprehensively anyway. Hopefully we can get rid of the Java dependency on the metrics though.
Comment on attachment 8598847 [details] [diff] [review]
part2.patch

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

OK, I suppose it's fine to do it for all scrollable frames for now.
Attachment #8598847 - Flags: review+
r+ from latest patch.
Attachment #8598846 - Attachment is obsolete: true
Attachment #8599854 - Flags: review+
r+ from latest patch.
Attachment #8598847 - Attachment is obsolete: true
Attachment #8599855 - Flags: review+
try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9a9f7ddca5b3 (also contains the changes from 1159405)
Keywords: checkin-needed
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.