Minimal support for APZC on Mac

RESOLVED FIXED in mozilla32

Status

()

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

People

(Reporter: mstange, Assigned: mstange)

Tracking

(Blocks: 2 bugs)

Trunk
mozilla32
All
Mac OS X
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(11 attachments, 9 obsolete attachments)

4.28 KB, patch
kats
: review+
Details | Diff | Splinter Review
1.81 KB, patch
kats
: review+
Details | Diff | Splinter Review
5.76 KB, patch
smichaud
: review+
Details | Diff | Splinter Review
3.95 KB, patch
kats
: review+
Details | Diff | Splinter Review
7.70 KB, patch
smichaud
: review+
Details | Diff | Splinter Review
3.26 KB, patch
smichaud
: review+
Details | Diff | Splinter Review
9.96 KB, patch
smichaud
: review+
Details | Diff | Splinter Review
8.86 KB, patch
smichaud
: review+
Details | Diff | Splinter Review
1.20 KB, patch
smichaud
: review+
Details | Diff | Splinter Review
17.47 KB, patch
Details | Diff | Splinter Review
7.41 KB, patch
Details | Diff | Splinter Review
(Assignee)

Description

4 years ago
Created attachment 8340698 [details] [diff] [review]
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.
(Assignee)

Comment 1

4 years ago
Created attachment 8340813 [details] [diff] [review]
wip

I forced on creation of nsDisplayScrollInfoLayers for all scrollable frames and now it works on normal Firefox Desktop Mac, too. Yay!
Attachment #8340698 - Attachment is obsolete: true
(Assignee)

Comment 2

4 years ago
Created attachment 8342449 [details] [diff] [review]
wip
Attachment #8340813 - Attachment is obsolete: true
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.
(Assignee)

Comment 4

4 years ago
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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.
(Assignee)

Comment 5

4 years ago
Created attachment 8343780 [details] [diff] [review]
wip

disabled zooming and added some overscroll bounce physics
Attachment #8342449 - Attachment is obsolete: true
(Assignee)

Comment 6

4 years ago
Here's a test build with working overscroll bouncing:
https://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/mstange@themasta.com-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 :)
I just tested it. It shows a lot of promise. sub-APZC appears to be working well which is nice.

Comment 8

4 years ago
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

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

With OS X 10.9 on 2010 13" MBA.
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.
(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.
Alias: apzc-mac
Alias: apzc-mac → apz-mac
Depends on: 990972
No longer depends on: 990972
Blocks: 1005111
Blocks: 1008819
(Assignee)

Comment 12

3 years ago
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.
(Assignee)

Updated

3 years ago
Depends on: 1009786
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.

Updated

3 years ago
Blocks: 1011833
(Assignee)

Comment 14

3 years ago
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.
Alright sounds good! Let's wait until this is further along before closing bug 1011833.
(Assignee)

Comment 16

3 years ago
Created 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.

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.
Attachment #8343780 - Attachment is obsolete: true
Attachment #8425486 - Flags: review?(bugmail.mozilla)
(Assignee)

Comment 17

3 years ago
Created attachment 8425487 [details] [diff] [review]
part 2: Add default values for some AsyncPanZoom prefs to all.js.
Assignee: nobody → mstange
Status: NEW → ASSIGNED
Attachment #8425487 - Flags: review?(bugmail.mozilla)
(Assignee)

Comment 18

3 years ago
Created attachment 8425491 [details] [diff] [review]
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).
Attachment #8425491 - Flags: review?(bugmail.mozilla)
(Assignee)

Comment 19

3 years ago
Created attachment 8425494 [details] [diff] [review]
part 4: Build scrollable layers for actively scrolled subframes with scroll info layers.

This attempts to solve one part of bug 1009786.
Attachment #8425494 - Flags: review?(roc)
(Assignee)

Comment 20

3 years ago
Created attachment 8425499 [details] [diff] [review]
part 5: Add EventThreadRunner class that can listen to scroll events without main thread involvement.
Attachment #8425499 - Flags: review?(smichaud)
(Assignee)

Comment 21

3 years ago
Created attachment 8425501 [details] [diff] [review]
part 6: Add a basic GeckoContentController implementation for async panning on Mac.
Attachment #8425501 - Flags: review?(bugmail.mozilla)
(Assignee)

Comment 22

3 years ago
Created attachment 8425506 [details] [diff] [review]
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
Attachment #8425506 - Flags: review?(smichaud)
(Assignee)

Comment 23

3 years ago
Created attachment 8425508 [details] [diff] [review]
part 8: Add a helper method for the conversion of window coordinates to device pixels.
Attachment #8425508 - Flags: review?(smichaud)
(Assignee)

Comment 24

3 years ago
Created attachment 8425510 [details] [diff] [review]
part 9: Pass scroll events to the APZC tree manager on the async event thread.
Attachment #8425510 - Flags: review?(smichaud)
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.
Attachment #8425486 - Flags: review?(bugmail.mozilla) → review+
(Assignee)

Comment 26

3 years ago
That's all I have for now. Please feel free to steal reviews when it makes sense.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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.
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.
(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.
Attachment #8425487 - Flags: review?(bugmail.mozilla) → review+
Attachment #8425501 - Flags: review?(bugmail.mozilla) → review+
(Assignee)

Comment 30

3 years ago
(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.
(Assignee)

Updated

3 years ago
Blocks: 1013364
(Assignee)

Comment 31

3 years ago
I've filed bug 1013364 to track the remaining issues that we need to fix before turning this on by default.
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
Attachment #8425494 - Flags: review?(roc) → review+
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.
Attachment #8425491 - Flags: review?(bugmail.mozilla)
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 :-)
Attachment #8425499 - Flags: review?(smichaud) → review+
Attachment #8425506 - Flags: review?(smichaud) → review+
Attachment #8425508 - Flags: review?(smichaud) → review+
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.
Attachment #8425510 - Flags: review?(smichaud) → review+
(Assignee)

Comment 36

3 years ago
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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).
(Assignee)

Comment 37

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

Comment 38

3 years ago
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.
Attachment #8425494 - Attachment is obsolete: true
Attachment #8425494 - Flags: review-
(Assignee)

Comment 39

3 years ago
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
Keywords: leave-open
(Assignee)

Comment 40

3 years ago
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.
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
(Assignee)

Updated

3 years ago
Depends on: 1020389
(Assignee)

Comment 42

3 years ago
Relanded part 2 with a test fix from bug 1020389:
https://hg.mozilla.org/integration/mozilla-inbound/rev/75ab95a6f890
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
Depends on: 1021044
(Assignee)

Comment 44

3 years ago
Created attachment 8435805 [details] [diff] [review]
part 3: Add event structs for Mac touchpad scroll events.

(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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.
Attachment #8425491 - Attachment is obsolete: true
Attachment #8435805 - Flags: review?(bugmail.mozilla)
(Assignee)

Comment 45

3 years ago
(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.
(Assignee)

Comment 46

3 years ago
(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.
(Assignee)

Comment 47

3 years ago
Created attachment 8435819 [details] [diff] [review]
part 9: Add a 10.6 compatibility wrapper for event phases.
(Assignee)

Updated

3 years ago
Attachment #8435819 - Attachment is patch: true
Attachment #8435819 - Flags: review?(smichaud)
(Assignee)

Comment 48

3 years ago
Created attachment 8435821 [details] [diff] [review]
part 10: Add a 10.6 compatibility wrapper for hasPreciseScrollingDeltas and scrollingDeltaX/Y.
Attachment #8435821 - Flags: review?(smichaud)
(Assignee)

Comment 49

3 years ago
Created attachment 8435822 [details] [diff] [review]
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.
Attachment #8425510 - Attachment is obsolete: true
Attachment #8435822 - Flags: review?(smichaud)
(Assignee)

Updated

3 years ago
Attachment #8435819 - Attachment description: part 9 Add a 10.6 compatibility wrapper for event phases. → part 9: Add a 10.6 compatibility wrapper for event phases.
(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?
(Assignee)

Comment 51

3 years ago
It definitely should, good catch! I don't know how that got in there.
Attachment #8435819 - Flags: review?(smichaud) → review+
Attachment #8435821 - Flags: review?(smichaud) → review+
Attachment #8435822 - Flags: review?(smichaud) → review+
(Assignee)

Comment 52

3 years ago
Created attachment 8435894 [details] [diff] [review]
part 12: Install the event tap only for wheel events
Attachment #8435894 - Flags: review?(smichaud)
Attachment #8435894 - Flags: review?(smichaud) → review+
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
Attachment #8435805 - Flags: review?(bugmail.mozilla) → review+
(Assignee)

Comment 54

3 years ago
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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.
(Assignee)

Comment 55

3 years ago
Created attachment 8435981 [details] [diff] [review]
part 3: Add event structs for Mac touchpad scroll events.

updated to tip with added TimeStamp field
Attachment #8435805 - Attachment is obsolete: true
(Assignee)

Comment 56

3 years ago
Created attachment 8435984 [details] [diff] [review]
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.
Attachment #8435822 - Attachment is obsolete: true
(Assignee)

Comment 57

3 years ago
https://tbpl.mozilla.org/?tree=Try&rev=bd3839872013
(Assignee)

Comment 58

3 years ago
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.
Keywords: leave-open
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
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32

Updated

3 years ago
Depends on: 1022206
(Assignee)

Updated

3 years ago
No longer depends on: 1009786
See Also: → bug 958868
Depends on: 1024303
Depends on: 1024305
Depends on: 1024307

Updated

3 years ago
Blocks: 1025481
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 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.
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).
https://hg.mozilla.org/mozilla-central/rev/76a138fbc1c3
Depends on: 1101030

Comment 64

3 years ago
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 ?
(Assignee)

Comment 65

3 years ago
We haven't added support for pinch gestures yet. We're focusing on getting scrolling right first.
Alias: apz-mac
You need to log in before you can comment on or make changes to this bug.