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)
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)
1.68 KB,
patch
|
danilo.eu
:
review+
|
Details | Diff | Splinter Review |
5.60 KB,
patch
|
danilo.eu
:
review+
|
Details | Diff | Splinter Review |
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.
Updated•9 years ago
|
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
Assignee | ||
Comment 1•9 years ago
|
||
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 | ||
Updated•9 years ago
|
Assignee: nobody → danilo.eu
Comment 2•9 years ago
|
||
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.
Comment 3•9 years ago
|
||
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.
Assignee | ||
Comment 4•9 years ago
|
||
Attachment #8598780 -
Flags: review?(bugmail.mozilla)
Attachment #8598780 -
Flags: review?(botond)
Assignee | ||
Comment 5•9 years ago
|
||
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)
Assignee | ||
Comment 6•9 years ago
|
||
Attachment #8598823 -
Flags: review?(bugmail.mozilla)
Attachment #8598823 -
Flags: review?(botond)
Comment 7•9 years ago
|
||
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 8•9 years ago
|
||
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+
Assignee | ||
Comment 9•9 years ago
|
||
r+ from latest review.
Attachment #8598822 -
Attachment is obsolete: true
Attachment #8598822 -
Flags: review?(botond)
Attachment #8598846 -
Flags: review+
Assignee | ||
Comment 10•9 years ago
|
||
r+ from latest review
Attachment #8598823 -
Attachment is obsolete: true
Attachment #8598823 -
Flags: review?(botond)
Attachment #8598847 -
Flags: review+
Comment 11•9 years ago
|
||
Botond still should review both of these patches.
Comment 12•9 years ago
|
||
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.
Updated•9 years ago
|
Attachment #8598846 -
Flags: review+
Comment 13•9 years ago
|
||
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.
Comment 14•9 years ago
|
||
(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 15•9 years ago
|
||
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+
Assignee | ||
Comment 16•9 years ago
|
||
r+ from latest patch.
Attachment #8598846 -
Attachment is obsolete: true
Attachment #8599854 -
Flags: review+
Assignee | ||
Comment 17•9 years ago
|
||
r+ from latest patch.
Attachment #8598847 -
Attachment is obsolete: true
Attachment #8599855 -
Flags: review+
Assignee | ||
Comment 18•9 years ago
|
||
try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9a9f7ddca5b3 (also contains the changes from 1159405)
Assignee | ||
Updated•9 years ago
|
Keywords: checkin-needed
Comment 19•9 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/cbc880a746c6 https://hg.mozilla.org/integration/fx-team/rev/d4b430ab9d27
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/cbc880a746c6 https://hg.mozilla.org/mozilla-central/rev/d4b430ab9d27
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox40:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Updated•2 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•