If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Remove MOZ_SINGLE_PROCESS_APZ ifdef conditions added in bug 1224015

RESOLVED FIXED in Firefox 45

Status

()

Core
Panning and Zooming
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: kats, Assigned: rbarker)

Tracking

Trunk
mozilla45
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox45 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(4 attachments)

Created attachment 8693005 [details] [diff] [review]
WIP

In bug 1224015 part 3 (https://hg.mozilla.org/mozilla-central/rev/d1e9567751fd) we added a bunch of ifdefs to get stuff landed without breaking non-Fennec platforms. However we should look into the issues there (both user-visible correctness problems and test failures) and address those, and eventually back out the ifdef patch.

I have a patch locally that (after backing out the ifdefs) seems to result in the correct user-visible behaviour on both B2G and Fennec, although there are still some test failures. The patch is in line with my mental model of how things should work, so I think that the patch is at least a subset of the right solution. I'm attaching the patch here.
I push this patch, rebased on top of the bug 1224015 landing and the backout of the ifdefs part, to Try, to see how it fares:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e21ef44dfd59
Comment on attachment 8693005 [details] [diff] [review]
WIP

>@@ -2106,19 +2106,17 @@ nsLayoutUtils::GetEventCoordinatesRelativeTo(nsIWidget* aWidget,
>     if (frameWidget && frameWidget == aWidget) {
>       // Special case this cause it happens a lot.
>       // This also fixes bug 664707, events in the extra-special case of select
>       // dropdown popups that are transformed.
>       nsPresContext* presContext = aFrame->PresContext();
>       nsPoint pt(presContext->DevPixelsToAppUnits(aPoint.x),
>                  presContext->DevPixelsToAppUnits(aPoint.y));
>-      pt = pt - view->ViewToWidgetOffset();
>-      pt = pt.RemoveResolution(presContext->PresShell()->GetCumulativeScaleResolution());
>-      return pt;
>+      return pt - view->ViewToWidgetOffset();
>     }
>   }

If this removal was required, then likely more removal of resolution code added bug 1224015 is needed.
So Botond explain to me that we only want to remove the resolution when transforming the event points to a non-root document, as root documents already have it removed in ApplyCallbackTransform. So comment 2 was incorrect.
Timothy pointed out on IRC that while removing the RemoveResolution() call from the short-circuit path of GetEventCoordinatesRelativeTo() accomplishes the goal of getting GetEventCoordinatesRelativeTo() to only unapply a non-root-document resolution, some of the other pieces of Layout code ifdef'ed in bug 1224015 (such as nsLayoutUtils::TranslateViewToWidget()) can still potentially unapply a root document resolution, leading to a double unapplication on B2G.

To fix this, I tried a slightly different approach where instead of omitting a RemoveResolution() call, I modified GetCumulativeScaleResolution() (which is used to determine the resolution to unapply in all these places) to exclude the resolution on the root document.

A Try push with that change [1] is looking much better!

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=06506990e448
Nice! That makes sense to me, and seems like there's just one B2G failure to tackle now (the android C2 is permafailing on m-c as well).
(Assignee)

Comment 6

2 years ago
Since C++APZ won't ride the train for Fennec until 46 at the earliest (correct?) we need to make sure Fennec without C++APZ and this patch still work. Has this patch been pushed to try with C++APZ disabled for Fennec?
Yeah, good point. I did a try push using the same patches as the above try push, but with APZ disabled for fennec:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ad740209e8b6
^ try push looks good for Fennec.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #5)
> Nice! That makes sense to me, and seems like there's just one B2G failure to
> tackle now (the android C2 is permafailing on m-c as well).

The remaining B2G emulator failure is layout/base/tests/chrome/test_bug370436.html. Looking into it.
(In reply to Botond Ballo [:botond] from comment #9)
> The remaining B2G emulator failure is
> layout/base/tests/chrome/test_bug370436.html. Looking into it.

Well one interesting thing about this test is that there's a (non-root) resolution in the chrome document. I'm guessing this has to do with the test being a chrome mochitest.

However, this scenario should still be handled correctly by the various codepaths for dealing with a non-root resolution, as on Fennec.

Comment 11

2 years ago
diagnosis
It looks like PresShell::AdjustContextMenuEvent() is computing and setting a value of refPoint [1] that doesn't meet our conventions for what's stored in refPoint (coordinates with the resolution applied).

[1] https://dxr.mozilla.org/mozilla-central/rev/e02b17a2b5b8df7bb84f325fc08eedd2f3cab755/layout/base/nsPresShell.cpp#8220
After some discussion with Timothy, it seems that fixing PresShell::AdjustContextMenuEvent() is not straightforward, but it's not really necessary either, as we don't have context menus on platforms that have resolutions. Since the test in question itself concerns context menus, it seems reasonable to disable it on b2g.

New try push with test disabled:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=cfefc6b2815a
(In reply to Botond Ballo [:botond] from comment #12)
> New try push with test disabled:
> 
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=cfefc6b2815a

Looking good!
Created attachment 8697022 [details]
MozReview Request: Bug 1228597 - Remove the MOZ_SINGLE_PROCESS_APZ define. r=tn

Bug 1228597 - Remove the MOZ_SINGLE_PROCESS_APZ define. r=tn

Code previously guarded by this (added in bug 1224015) will now run
unconditionally.
Attachment #8697022 - Flags: review?(tnikkel)
Created attachment 8697023 [details]
MozReview Request: Bug 1228597 - Clean up code paths that (un)apply a pres shell resolution. r=tn

Bug 1228597 - Clean up code paths that (un)apply a pres shell resolution. r=tn

A clear separation is introduced between paths that deal with a root
document resolution (at the process boundary in e10s setups) and paths
that deal with a non-root document resolution (elsewhere in Layout code).

This allows both code paths to run on all platforms.
Attachment #8697023 - Flags: review?(tnikkel)
Created attachment 8697024 [details]
MozReview Request: Bug 1228597 - Disable a test that uses context menus on b2g. r=tn

Bug 1228597 - Disable a test that uses context menus on b2g. r=tn

Getting the test to pass would involve changing code that deals with
context menus to be aware of pres shell resolutions.
Attachment #8697024 - Flags: review?(tnikkel)
Comment on attachment 8697022 [details]
MozReview Request: Bug 1228597 - Remove the MOZ_SINGLE_PROCESS_APZ define. r=tn

https://reviewboard.mozilla.org/r/27483/#review24775

I'm assuming this is a straight backout of the patch which added these. r+ based on that.
Attachment #8697022 - Flags: review?(tnikkel) → review+
Comment on attachment 8697024 [details]
MozReview Request: Bug 1228597 - Disable a test that uses context menus on b2g. r=tn

https://reviewboard.mozilla.org/r/27487/#review24777
Attachment #8697024 - Flags: review?(tnikkel) → review+
(In reply to Timothy Nikkel (:tn) from comment #17)
> I'm assuming this is a straight backout of the patch which added these.

Yep.
Comment on attachment 8697023 [details]
MozReview Request: Bug 1228597 - Clean up code paths that (un)apply a pres shell resolution. r=tn

https://reviewboard.mozilla.org/r/27485/#review24779

::: gfx/layers/apz/util/APZCCallbackHelper.cpp:450
(Diff revision 1)
> +        return input;

kats or someone else should probably review this.

::: layout/base/nsPresShell.cpp:5333
(Diff revision 1)
>  float PresShell::GetCumulativeScaleResolution()

We need to rename GetCumulativeScaleResolution (because it differs from the other get cumulative resolution function), otherwise we are just asking for problems.
Attachment #8697023 - Flags: review?(tnikkel) → review+
(In reply to Timothy Nikkel (:tn) from comment #20)
> https://reviewboard.mozilla.org/r/27485/#review24779
> 
> ::: gfx/layers/apz/util/APZCCallbackHelper.cpp:450
> (Diff revision 1)
> > +        return input;
> 
> kats or someone else should probably review this.

Kats wrote this part of the patch. It has my r+ from a coordinate-systems point of view. If you're happy with the implementation of GetRootDocumentPresShell(), I think we can declare this part of the patch to be reviewed :)

> ::: layout/base/nsPresShell.cpp:5333
> (Diff revision 1)
> >  float PresShell::GetCumulativeScaleResolution()
> 
> We need to rename GetCumulativeScaleResolution (because it differs from the
> other get cumulative resolution function), otherwise we are just asking for
> problems.

Good point, will do.

Comment 22

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/73246a388339
https://hg.mozilla.org/integration/mozilla-inbound/rev/b391d6a7f3a6
https://hg.mozilla.org/integration/mozilla-inbound/rev/f5d0b9f5e3f6
Renamed GetCumulativeScaleResolution() to GetCumulativeNonRootScaleResolution() and landed.

Comment 24

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/73246a388339
https://hg.mozilla.org/mozilla-central/rev/b391d6a7f3a6
https://hg.mozilla.org/mozilla-central/rev/f5d0b9f5e3f6
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox45: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
Depends on: 1233056
You need to log in before you can comment on or make changes to this bug.