944938 - Minimal support for APZC on Mac

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

[Markus Stange [:mstange]]

Attachment: wip

With this patch plus the one in bug 944937, I can successfully and smoothly scroll and zoom in Firefox Metro on Mac when building with --enable-metro.

The code can definitely not land in this state, but this is enough to be able to debug many APZC problems on OS X. It does not work in Firefox Desktop (maybe related to subframes?). I'd expect it to work in B2G Desktop but I can't get that to launch.

Comment 1

[Markus Stange [:mstange]]

Attachment: wip

I forced on creation of nsDisplayScrollInfoLayers for all scrollable frames and now it works on normal Firefox Desktop Mac, too. Yay!

Comment 2

[Markus Stange [:mstange]]

Attachment: wip

Comment 3

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

Do you by any chance have a plan for dealing with the scrollbars with APZC enabled? If my mental model is correct then with your patch the scrollbars will reflect the synchronous scroll position (i.e. what gecko last painted) rather than the async scroll position, and so they will appear to jank. I'm looking at the same problem over in bug 814435 but I was wondering if maybe you had something in the works for that.

Comment 4

[Markus Stange [:mstange]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #3)
> Do you by any chance have a plan for dealing with the scrollbars with APZC
> enabled?

Not really, no. I'm watching bug 814435 with much curiosity :)
I'll post my thoughts there.

> If my mental model is correct then with your patch the scrollbars
> will reflect the synchronous scroll position (i.e. what gecko last painted)
> rather than the async scroll position, and so they will appear to jank.

Correct, that's what happens at the moment.

Comment 5

[Markus Stange [:mstange]]

Attachment: wip

disabled zooming and added some overscroll bounce physics

Comment 6

[Markus Stange [:mstange]]

Here's a test build with working overscroll bouncing:
https://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-7ed2dd462008/try-macosx64/firefox-28.0a1.en-US.mac.dmg

It breaks more stuff than I can count, so please ignore all bugs for now :)

Comment 7

[Benoit Girard (:BenWa)]

I just tested it. It shows a lot of promise. sub-APZC appears to be working well which is nice.

Comment 8

[Avi Halachmi (:avih)]

Just tested it a bit, fantastic!

- Checkerboard (solid gray with this patch) rarely visible, and only on very quick scrolls.
- Bounce top/bottom works well.
- Bounce left/right also works and prevents back/forward gestures.
- Scrollable subsection (e.g. http://www.quackit.com/html/codes/html_scroll_box.cfm) also works but: 1. scrollbar updates less frequently (not a biggie). 2. It sometimes shake (left/right few px) even when not scrolling - couldn't reproduce this with normal nightly.
- Works fantastic with the notoriously terrible http://w3c.github.io/webcomponents/spec/shadow/
- Didn't get any kernel panic which I was asked to expect (used for about 15 mins so far).

Very nice :)

Comment 9

[Avi Halachmi (:avih)]

(In reply to Avi Halachmi (:avih) from comment #8)
> Just tested it a bit, fantastic!

With OS X 10.9 on 2010 13" MBA.

Comment 10

[Avi Halachmi (:avih)]

Also:
- The shaking also appears on bugzilla's CC list after scrolling it.
- Some layout errors (e.g. on zimbra's reading pane - when at the bottom or on the right), where the scrolled content isn't clipped properly while scrolling.
- Sometimes there's an initial lag (200ms-ish?) on first scroll before the content starts moving. On further scrolls of the same page it seems to respond instantly.

Comment 11

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

(In reply to Avi Halachmi (:avih) from comment #8)
> http://www.quackit.com/html/codes/html_scroll_box.cfm) also works but: 1.
> scrollbar updates less frequently (not a biggie).

I haven't looked at this patch in detail yet but bug 814435 might have fixed this. If not it should just be a matter of setting the right prefs and styling.

Comment 12

[Markus Stange [:mstange]]

I've started updating the patches for this bug to trunk. I need to fix a few bugs and change how momentum scrolling is handled, then I'll attach the patches for review.

This bug will only implement the minimal set that's necessary to test APZC, that means:
 - preffed off by default
 - only panning, no zooming
 - no main thread wheel event support
 - and probably plenty of bugs.

Comment 13

[Benoit Girard (:BenWa)]

Markus have you considered splitting off the APZC changes to deal with new types of input and the eventtap? We could decouple the two and get APZC working via the main thread with e10s enabled on mac and follow up with windows.

Comment 14

[Markus Stange [:mstange]]

Yes, I've already split the patches that way (and more ways), but the event tap is actually the smallest problem. I'm about to change how I used the InputEvent struct for wheel events - my current patches handle momentum scrolling by ignoring the momentum events and using the FlingAnimation, but that won't work with some devices (namely the Magic Mouse) which don't send an event on touchstart, so we wouldn't be able to stop our fling animation at that point. As soon as I've changed that part I'll attach the patches.
It's probably not worth doing bug 1011833 for Mac. All the problems we need to solve there are the hard problems that also need solving here; the part of getting the events is not a problem. But it probably is worth doing something like that for Windows.

Comment 15

[Benoit Girard (:BenWa)]

Alright sounds good! Let's wait until this is further along before closing bug 1011833.

Comment 16

[Markus Stange [:mstange]]

Attachment: part 1: Move the layers.async-pan-zoom.enabled pref to gfxPrefs and allow it to override the apz subframe pref.

The idea is that we give Mac the default values layers.async-pan-zoom.enabled=false (for now) and apz.subframe.enabled=true. Then flipping only the layers.async-pan-zoom.enabled can toggle whether we create scrollable layers.

Comment 17

[Markus Stange [:mstange]]

Attachment: part 2: Add default values for some AsyncPanZoom prefs to all.js.

Comment 18

[Markus Stange [:mstange]]

Attachment: part 3: Add event structs for Mac touchpad scroll events.

I'm not really happy with the names I've chosen in this patch; any suggestions are appreciated. I'm also unsure whether the struct should have "Gesture" in the name or not (and whether it should be handled in AsyncPanZoomController::HandleInputEvent or in AsyncPanZoomController::HandleGestureEvent).

Comment 19

[Markus Stange [:mstange]]

Attachment: part 4: Build scrollable layers for actively scrolled subframes with scroll info layers.

This attempts to solve one part of bug 1009786.

Comment 20

[Markus Stange [:mstange]]

Attachment: part 5: Add EventThreadRunner class that can listen to scroll events without main thread involvement.

Comment 21

[Markus Stange [:mstange]]

Attachment: part 6: Add a basic GeckoContentController implementation for async panning on Mac.

Comment 22

[Markus Stange [:mstange]]

Attachment: part 7: Create an APZCTM controller for the compositor parent on Mac with APZ enabled, and launch the async event thread when necessary.

not sure who the best person to review this one is

Comment 23

[Markus Stange [:mstange]]

Attachment: part 8: Add a helper method for the conversion of window coordinates to device pixels.

Comment 24

[Markus Stange [:mstange]]

Attachment: part 9: Pass scroll events to the APZC tree manager on the async event thread.

Comment 25

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

Comment on attachment 8425486 [details] [diff] [review]
part 1: Move the layers.async-pan-zoom.enabled pref to gfxPrefs and allow it to override the apz subframe pref.

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

I'm a little concerned about http://mxr.mozilla.org/mozilla-central/source/b2g/components/OopCommandLine.js#32 because that might be used on B2G desktop. I'm not really sure. If it is then your patch will make B2G desktop automatically get sub-APZ which it wouldn't have had before. Probably worth testing locally or in a try push.

The other thing I'd like to do is put layers.async-pan-zoom.enabled in all.js and set it to false. I realized recently that prefs that are just in gfxPrefs.h but not in the prefs files don't have the default value accessible from JS and since this pref potentially might be accessed from JS I'd like to avoid that footgun.

Comment 26

[Markus Stange [:mstange]]

That's all I have for now. Please feel free to steal reviews when it makes sense.

Comment 27

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

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #25)
> The other thing I'd like to do is put layers.async-pan-zoom.enabled in
> all.js and set it to false. I realized recently that prefs that are just in
> gfxPrefs.h but not in the prefs files don't have the default value
> accessible from JS and since this pref potentially might be accessed from JS
> I'd like to avoid that footgun.

Never mind, I see you did that in part 2.

Comment 28

[Steven Michaud [:smichaud] (Retired)]

Is the following the main reference for APZC?

https://wiki.mozilla.org/Platform/GFX/APZ#AsyncPanZoomControllers

And why are you doing this, Markus? :-)  Is the intent eventually to get it into Mac releases?  Or is it mostly a testbed for other platforms?

How should I go about testing this?  Do I need to build on the Mac with --enable-metro?  Does that even work any more?

I will get to these reviews as I can, but for now I won't consider them urgent.  My current plan is to finish them all over the next two weeks.  Please let me know if that will be a problem.

Comment 29

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

(In reply to Steven Michaud from comment #28)
> Is the following the main reference for APZC?
> 
> https://wiki.mozilla.org/Platform/GFX/APZ#AsyncPanZoomControllers

That page has most of the documentation, yes. I'm also working on more documentation over in bug 992122 - the URL on that bug has my WIP so far.

Comment 30

[Markus Stange [:mstange]]

(In reply to Steven Michaud from comment #28)
> And why are you doing this, Markus? :-)  Is the intent eventually to get it
> into Mac releases?  Or is it mostly a testbed for other platforms?

Both. Eventually we want scrolling on all platforms to happen through async panning. It's going to be a while until APZ is stable enough to turn it on for Desktop Firefox by default, so until then it will primarily be used to fix B2G bugs, but we do want to turn it on at some point.

> How should I go about testing this?

Just build normally, and then flip the pref layers.async-pan-zoom.enabled to true.

>  Do I need to build on the Mac with
> --enable-metro?  Does that even work any more?

You don't :-)
It probably doesn't work any more, I don't know. Using --enable-metro was just my first attempt when I tried this, but these patches are intended for normal desktop Firefox.

> I will get to these reviews as I can, but for now I won't consider them
> urgent.  My current plan is to finish them all over the next two weeks. 
> Please let me know if that will be a problem.

That's ok, they're not particularly urgent, but the sooner they're in, the less they will rot, and some of the touched code is changed around regularly.

Comment 31

[Markus Stange [:mstange]]

I've filed bug 1013364 to track the remaining issues that we need to fix before turning this on by default.

Comment 32

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

Comment on attachment 8425491 [details] [diff] [review]
part 3: Add event structs for Mac touchpad scroll events.

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

::: gfx/layers/apz/src/APZCTreeManager.cpp
@@ +437,5 @@
> +    } case PANGESTURE_INPUT: {
> +      const PanGestureInput& panInput = aEvent.AsPanGestureInput();
> +      nsRefPtr<AsyncPanZoomController> apzc = GetTargetAPZC(panInput.mPanStartPoint);
> +      if (apzc) {
> +        BuildOverscrollHandoffChain(apzc);

Probably only need to build the handoff chain on certain types of input events, like PANGESTURE_MAYSTART or PANGESTURE_START.

::: gfx/layers/apz/src/AsyncPanZoomController.h
@@ +867,5 @@
>     * and passes it on to APZCTreeManager::DispatchScroll() in the event of
>     * overscroll.
>     */
>    void AttemptScroll(const ScreenPoint& aStartPoint, const ScreenPoint& aEndPoint,
> +                     const ScreenPoint& aAdditionalDisplacement = ScreenPoint(0, 0),

Since there's only one caller for this function, and you already modified it, I don't see the need for the default value on this parameter. I'd rather make it non-optional.

::: gfx/layers/apz/src/Axis.cpp
@@ +32,5 @@
>  
> +void Axis::UpdateWithTouchAtDevicePoint(int32_t aPos, const TimeDuration& aTimeDelta,
> +                                        int32_t aAdditionalDisplacement) {
> +  float newVelocity = mAxisLocked ? 0 :
> +    (mPos - aPos - aAdditionalDisplacement) / aTimeDelta.ToMilliseconds();

Not sure why this change was needed; you could have subtracted aEvent.mPanDisplacement.x from aEvent.mStartPoint.x in APZC::OnPan, right?

::: widget/InputData.h
@@ +185,5 @@
> + */
> +class PanGestureInput : public InputData
> +{
> +public:
> +  enum PanGestureType

It would be nice to document these event types - for some of them the meaning and when they are dispatched is nonobvious. Without this it's hard for me to verify the handling in AsyncPanZoomController.cpp is correct.

@@ +199,5 @@
> +  };
> +
> +  PanGestureInput(PanGestureType aType,
> +                    uint32_t aTime,
> +                    const ScreenPoint& aPanStartPoint,

nit: line these up

Comment 33

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

Comment on attachment 8425491 [details] [diff] [review]
part 3: Add event structs for Mac touchpad scroll events.

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

Dropping review for now; I don't really want to review this without knowing when all the PANGESTURE_xxx events are fired and what they mean.

Comment 34

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 8425499 [details] [diff] [review]
part 5: Add EventThreadRunner class that can listen to scroll events without main thread involvement.

I'm not going to have time to test these patches anytime soon.  So I decided just to look over them and see if I find any problems.

The main problem here is that you're relying on event taps, which in my experience are *very* flaky.  Also, the kCGEventTapOptionListenOnly option means that there's a significant delay between when the event happens and when the event tap receives it -- on the order (if I remember right) of a significant fraction of a second.

Can you think of any alternative?  I'll try to myself, over the next few days.

And here's a nit:

+@interface EventThreadRunner(Private)
+- (void)handleEvent:(CGEventRef)cgEvent type:(CGEventType)type;
+- (void)runEventThread;
+- (void)shutdownAndReleaseCalledOnEventThread;
+- (void)shutdownAndReleaseCalledOnAnyThread;
+- (void)handleEvent:(CGEventRef)cgEvent type:(CGEventType)type;
+@end

handleEvent is in there twice :-)

Comment 35

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 8425510 [details] [diff] [review]
part 9: Pass scroll events to the APZC tree manager on the async event thread.

Note that [NSEvent phase] and [NSEvent momentumPhase] are only available on OS X 10.7 and up.  I assume you're dealing with that by other means.

Comment 36

[Markus Stange [:mstange]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #25)
> Comment on attachment 8425486 [details] [diff] [review]
> part 1: Move the layers.async-pan-zoom.enabled pref to gfxPrefs and allow it
> to override the apz subframe pref.
> 
> Review of attachment 8425486 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'm a little concerned about
> http://mxr.mozilla.org/mozilla-central/source/b2g/components/OopCommandLine.
> js#32 because that might be used on B2G desktop. I'm not really sure. If it
> is then your patch will make B2G desktop automatically get sub-APZ which it
> wouldn't have had before. Probably worth testing locally or in a try push.

I've started a try run with B2G Desktop tests on Linux + Mac with apz.subframe.enabled set to true everywhere: https://tbpl.mozilla.org/?tree=Try&rev=7dcbc703bfa0

However, it looks like the code you referenced only runs when B2G is started with the -oop command line option, which apparently only happens during the R-oop and G-oop tests, and at the moment those only run on Linux (at least that's what https://tbpl.mozilla.org/?tree=Mozilla-Inbound&showall=1 is showing me).

Comment 37

[Markus Stange [:mstange]]

Oh, and M-oop too. The Linux *-oop tests were green in the try push, so I'm going to go ahead with this.

Comment 38

[Markus Stange [:mstange]]

Comment on attachment 8425494 [details] [diff] [review]
part 4: Build scrollable layers for actively scrolled subframes with scroll info layers.

This patch is actually a bad idea because it will cause us to build scroll layers for frames that don't have display port yet, resulting in a scroll layer that's only sized to the scroll port with no extra prepared content. And then when we actually set a display port, the scroll layer may be built somewhere else, at least in the root subdocument case.
What should happen instead is: If scrolling is active, a display port should be set, and that should trigger scroll layer building as before. I'll take care of that in bug 1009786.

Comment 39

[Markus Stange [:mstange]]

Landed parts 1, 2, 5, 6, 7, 8:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9c65721ef16b
https://hg.mozilla.org/integration/mozilla-inbound/rev/f62d16bc1c3e
https://hg.mozilla.org/integration/mozilla-inbound/rev/b1440ad70335
https://hg.mozilla.org/integration/mozilla-inbound/rev/2dd00ac41fbe
https://hg.mozilla.org/integration/mozilla-inbound/rev/19e5e8dff4f9
https://hg.mozilla.org/integration/mozilla-inbound/rev/4f336ae97569

Comment 40

[Markus Stange [:mstange]]

Comment on attachment 8425487 [details] [diff] [review]
part 2: Add default values for some AsyncPanZoom prefs to all.js.

>diff --git a/modules/libpref/src/init/all.js b/modules/libpref/src/init/all.js

>+pref("apz.max_velocity_queue_size", 2);
>+pref("apz.fling_friction", "0.004");

I didn't land these two pref changes because we won't be using FlingAnimation on Mac, instead the fling will be controlled by the momentum scroll events that we get from OS X.
And the max_velocity_queue_size change made the gtests GfxPrefs.OnceValues and AsyncPanZoomControllerTester.OverScrollPanning fail.

Comment 41

[Carsten Book [:Tomcat]]

sorry had to backout https://hg.mozilla.org/integration/mozilla-inbound/rev/f62d16bc1c3e for test failures like https://tbpl.mozilla.org/php/getParsedLog.php?id=41027906&tree=Mozilla-Inbound

Comment 42

[Markus Stange [:mstange]]

Relanded part 2 with a test fix from bug 1020389:
https://hg.mozilla.org/integration/mozilla-inbound/rev/75ab95a6f890

Comment 43

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9c65721ef16b
https://hg.mozilla.org/mozilla-central/rev/b1440ad70335
https://hg.mozilla.org/mozilla-central/rev/2dd00ac41fbe
https://hg.mozilla.org/mozilla-central/rev/19e5e8dff4f9
https://hg.mozilla.org/mozilla-central/rev/4f336ae97569
https://hg.mozilla.org/mozilla-central/rev/75ab95a6f890

Comment 44

[Markus Stange [:mstange]]

Attachment: part 3: Add event structs for Mac touchpad scroll events.

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #32)
> Comment on attachment 8425491 [details] [diff] [review]
> part 3: Add event structs for Mac touchpad scroll events.
> 
> Review of attachment 8425491 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/layers/apz/src/APZCTreeManager.cpp
> @@ +437,5 @@
> > +    } case PANGESTURE_INPUT: {
> > +      const PanGestureInput& panInput = aEvent.AsPanGestureInput();
> > +      nsRefPtr<AsyncPanZoomController> apzc = GetTargetAPZC(panInput.mPanStartPoint);
> > +      if (apzc) {
> > +        BuildOverscrollHandoffChain(apzc);
> 
> Probably only need to build the handoff chain on certain types of input
> events, like PANGESTURE_MAYSTART or PANGESTURE_START.

I've restricted it to PANGESTURE_START and PANGESTURE_MOMENTUMSTART. The latter is because I clear the chain on PANGESTURE_END, because at that point we don't know whether the scroll gesture will continue through momentum.

I've reverted the additionalDisplacement changes; at some point I thought they were necessary but at the moment it doesn't look like they are.

> ::: widget/InputData.h
> @@ +185,5 @@
> > + */
> > +class PanGestureInput : public InputData
> > +{
> > +public:
> > +  enum PanGestureType
> 
> It would be nice to document these event types - for some of them the
> meaning and when they are dispatched is nonobvious. Without this it's hard
> for me to verify the handling in AsyncPanZoomController.cpp is correct.

Sorry about this. Yes, they definitely needed some serious documentation. I've added some now.

Comment 45

[Markus Stange [:mstange]]

(In reply to Steven Michaud from comment #34)
> Comment on attachment 8425499 [details] [diff] [review]
> part 5: Add EventThreadRunner class that can listen to scroll events without
> main thread involvement.

> The main problem here is that you're relying on event taps, which in my
> experience are *very* flaky.  Also, the kCGEventTapOptionListenOnly option
> means that there's a significant delay between when the event happens and
> when the event tap receives it -- on the order (if I remember right) of a
> significant fraction of a second.

I remember two symptoms of flakiness with the event taps we've been using in the past: High WindowServer CPU usage if Firefox was paused in gdb for a long time, and spurious mouse events when the process was killed. I haven't seen those symptoms with the mouse wheel event taps yet. Moreover, there's a major difference between the event tap we've used before and the one in this patch: The one in this patch is only installed for events that are targeted at our own process - it's not a system global event tap. I hope that any funkiness that arises from a long queue of unprocessed events will be limited by that aspect.

I haven't had any problems with delayed events. Scrolling is much more responsive with eventtap-APZ than with traditional main thread scrolling.

> Can you think of any alternative?  I'll try to myself, over the next few
> days.

The only alternative I've found is +[NSEvent _addConcurrentEventMonitorMatchingMask:identifier:handler:], but it's undocumented and only available on 10.9 and up.

Comment 46

[Markus Stange [:mstange]]

(In reply to Steven Michaud from comment #35)
> Note that [NSEvent phase] and [NSEvent momentumPhase] are only available on
> OS X 10.7 and up.  I assume you're dealing with that by other means.

Thanks for the reminder, I hadn't paid attention to that. I'll attach two more patches to deal with it.

Comment 47

[Markus Stange [:mstange]]

Attachment: part 9: Add a 10.6 compatibility wrapper for event phases.

Comment 48

[Markus Stange [:mstange]]

Attachment: part 10: Add a 10.6 compatibility wrapper for hasPreciseScrollingDeltas and scrollingDeltaX/Y.

Comment 49

[Markus Stange [:mstange]]

Attachment: part 11: Pass scroll events to the APZC tree manager on the async event thread.

This is now using nsCocoaUtils::Event(Momentum)Phase, supports legacy scroll events and has more comments.

Comment 50

[Steven Michaud [:smichaud] (Retired)]

(In reply to comment #45)

> The one in this patch is only installed for events that are targeted
> at our own process - it's not a system global event tap. I hope that
> any funkiness that arises from a long queue of unprocessed events
> will be limited by that aspect.

Fair enough.  I'd forgotten that our pre-existing use of event taps is
to get (mouse-down) events for all processes.

It may also help that your event tap listener is on a secondary
thread.

Let's just keep an eye out for problems.  If they happen, finding an
alternative will probably require serious reverse engineering -- which
is probably best to postpone until we really need it.

Just now I noticed something I missed previously:

+  CFMachPortRef eventPort =
+    CGEventTapCreateForPSN(&currentProcess,
+                           kCGHeadInsertEventTap,
+                           kCGEventTapOptionListenOnly,
+                           kCGEventMaskForAllEvents, <--
+                           HandleEvent,
+                           self);

Your event tap is for all events.  Shouldn't it just be for wheel
events?

Comment 51

[Markus Stange [:mstange]]

It definitely should, good catch! I don't know how that got in there.

Comment 52

[Markus Stange [:mstange]]

Attachment: part 12: Install the event tap only for wheel events

Comment 53

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

Comment on attachment 8435805 [details] [diff] [review]
part 3: Add event structs for Mac touchpad scroll events.

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

LGTM

::: gfx/layers/apz/src/APZCTreeManager.cpp
@@ +448,5 @@
> +    } case PANGESTURE_INPUT: {
> +      const PanGestureInput& panInput = aEvent.AsPanGestureInput();
> +      bool inOverscrolledApzc = false;
> +      nsRefPtr<AsyncPanZoomController> apzc = GetTargetAPZC(panInput.mPanStartPoint,
> +                                                            &inOverscrolledApzc);

This is going to run on every pan event, which means that you might do a pan that starts by scrolling one layer and then switches to scrolling another layer if stuff changes. That's how mac desktop works now (i.e. a fling will carry into another tab if you tab-switch) and I assume retaining this behaviour is intentional. If it's not we can do some stuff with mApzcForInputBlock here.

::: widget/InputData.h
@@ +247,5 @@
> +
> +  PanGestureType mType;
> +  ScreenPoint mPanStartPoint;
> +
> +  // Only non-zero if mType is PANGESTURE_PAN or PANGESTURE_MONTEUMPAN.

typo: _MOMENTUMPAN

Comment 54

[Markus Stange [:mstange]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #53)
> Comment on attachment 8435805 [details] [diff] [review]
> part 3: Add event structs for Mac touchpad scroll events.
> 
> Review of attachment 8435805 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> LGTM
> 
> ::: gfx/layers/apz/src/APZCTreeManager.cpp
> @@ +448,5 @@
> > +    } case PANGESTURE_INPUT: {
> > +      const PanGestureInput& panInput = aEvent.AsPanGestureInput();
> > +      bool inOverscrolledApzc = false;
> > +      nsRefPtr<AsyncPanZoomController> apzc = GetTargetAPZC(panInput.mPanStartPoint,
> > +                                                            &inOverscrolledApzc);
> 
> This is going to run on every pan event, which means that you might do a pan
> that starts by scrolling one layer and then switches to scrolling another
> layer if stuff changes. That's how mac desktop works now (i.e. a fling will
> carry into another tab if you tab-switch) and I assume retaining this
> behaviour is intentional. If it's not we can do some stuff with
> mApzcForInputBlock here.

It's not all that intentional. But let's keep it as-is for now, I'll look at it in more detail in or after bug 1013432.

Comment 55

[Markus Stange [:mstange]]

Attachment: part 3: Add event structs for Mac touchpad scroll events.

updated to tip with added TimeStamp field

Comment 56

[Markus Stange [:mstange]]

Attachment: part 11: Pass scroll events to the APZC tree manager on the async event thread.

updated to tip with additional TimeStamp field initialization

And I've made one other small change that keeps us from dispatching momentum pan events twice.

Comment 57

[Markus Stange [:mstange]]

https://tbpl.mozilla.org/?tree=Try&rev=bd3839872013

Comment 58

[Markus Stange [:mstange]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c6cb512f1119
https://hg.mozilla.org/integration/mozilla-inbound/rev/7eda6c9105c2
https://hg.mozilla.org/integration/mozilla-inbound/rev/cde19559da0f
https://hg.mozilla.org/integration/mozilla-inbound/rev/2dd8b0d9fc44
https://hg.mozilla.org/integration/mozilla-inbound/rev/ec4f84a6cc5a

This can be tested by flipping layers.async-pan-zoom.enabled to true.

Comment 59

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c6cb512f1119
https://hg.mozilla.org/mozilla-central/rev/7eda6c9105c2
https://hg.mozilla.org/mozilla-central/rev/cde19559da0f
https://hg.mozilla.org/mozilla-central/rev/2dd8b0d9fc44
https://hg.mozilla.org/mozilla-central/rev/ec4f84a6cc5a

Comment 60

[:kip (Kearwood Gilbert)]

This has made debugging APZ issues much easier, thanks!  In my case; however, it was necessary to also enable the layers.offmainthreadcomposition.force-basic pref to enable APZ.  As these patches enable mouse wheel events to occur only with APZ, one symptom of inactive APZ is that the mouse wheel is effectively disabled until force-basic is enabled.

Comment 61

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

Comment on attachment 8425487 [details] [diff] [review]
part 2: Add default values for some AsyncPanZoom prefs to all.js.

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

::: modules/libpref/src/init/all.js
@@ +358,5 @@
> +pref("apz.pan_repaint_interval", 16);
> +pref("apz.max_velocity_queue_size", 2);
> +pref("apz.fling_friction", "0.004");
> +pref("apz.apz.x_skate_size_multiplier", "2.5");
> +pref("apz.apz.y_skate_size_multiplier", "3.5");

I just noticed these pref names are wrong. I can fix it after bug 927946 lands (don't want to cause a merge conflict on a new contributor's first patch!), but commenting here in case I forget.

Comment 62

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

Fixed them: https://hg.mozilla.org/integration/mozilla-inbound/rev/76a138fbc1c3 (this patch landed in 34, so 32 and 33 still have the buggy names, but they're not used so it doesn't matter).

Comment 63

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/76a138fbc1c3

Comment 64

[dynamicdeveloper]

Hey guys, 

I have been following the coming addition to Async Pan and Zoom for over a year. I'm so happy that progress is being made into this, I would love to come back to Firefox on my Mac. 

Question : I downloaded firefox latest install, enabled "layers.async-pan-zoom.enabled" , and set pinch to zoom to "true".

I'm still not getting any response from pinch gestures.Am I missing something ?

Comment 65

[Markus Stange [:mstange]]

We haven't added support for pinch gestures yet. We're focusing on getting scrolling right first.