1228597 - Remove MOZ_SINGLE_PROCESS_APZ ifdef conditions added in bug 1224015

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

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

Attachment: 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.

Comment 1

[Botond Ballo [:botond]]

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 2

[Timothy Nikkel (:tnikkel)]

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.

Comment 3

[Timothy Nikkel (:tnikkel)]

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.

Comment 4

[Botond Ballo [:botond]]

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

Comment 5

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

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

Comment 6

[Randall Barker [:rbarker]]

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?

Comment 7

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

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

Comment 8

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

^ try push looks good for Fennec.

Comment 9

[Botond Ballo [:botond]]

(In reply to Kartikaya Gupta (email:[e-mail]) 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.

Comment 10

[Botond Ballo [:botond]]

(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

[Botond Ballo [:botond]]

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

Comment 12

[Botond Ballo [:botond]]

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

Comment 13

[Botond Ballo [:botond]]

(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!

Comment 14

[Botond Ballo [:botond]]

Attachment: 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.

Comment 15

[Botond Ballo [:botond]]

Attachment: 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.

Comment 16

[Botond Ballo [:botond]]

Attachment: 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.

Comment 17

[Timothy Nikkel (: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.

Comment 18

[Timothy Nikkel (:tnikkel)]

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

Comment 19

[Botond Ballo [:botond]]

(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 20

[Timothy Nikkel (:tnikkel)]

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.

Comment 21

[Botond Ballo [:botond]]

(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

[Pulsebot]

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

Comment 23

[Botond Ballo [:botond]]

Renamed GetCumulativeScaleResolution() to GetCumulativeNonRootScaleResolution() and landed.

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/73246a388339
https://hg.mozilla.org/mozilla-central/rev/b391d6a7f3a6
https://hg.mozilla.org/mozilla-central/rev/f5d0b9f5e3f6