Implement back/forward swiping with APZ

RESOLVED FIXED in Firefox 43

Status

()

defect
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: mstange, Assigned: mstange)

Tracking

Trunk
mozilla43
All
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox43 fixed)

Details

Attachments

(21 attachments, 2 obsolete attachments)

3.87 KB, text/html
Details
40 bytes, text/x-review-board-request
kats
: review+
Details
40 bytes, text/x-review-board-request
kats
: review+
Details
40 bytes, text/x-review-board-request
kats
: review+
Details
40 bytes, text/x-review-board-request
kats
: review+
Details
40 bytes, text/x-review-board-request
kip
: review+
Details
40 bytes, text/x-review-board-request
kats
: review+
Details
40 bytes, text/x-review-board-request
kats
: review+
Details
40 bytes, text/x-review-board-request
kats
: review+
Details
40 bytes, text/x-review-board-request
kats
: review+
Details
40 bytes, text/x-review-board-request
kats
: review+
Details
40 bytes, text/x-review-board-request
masayuki
: review+
Details
40 bytes, text/x-review-board-request
kats
: review+
Details
40 bytes, text/x-review-board-request
masayuki
: review+
Details
40 bytes, text/x-review-board-request
kats
: review+
Details
40 bytes, text/x-review-board-request
kats
: review+
Details
40 bytes, text/x-review-board-request
kats
: review+
Details
40 bytes, text/x-review-board-request
kats
: review+
Details
40 bytes, text/x-review-board-request
kats
: review+
Details
40 bytes, text/x-review-board-request
kats
: review+
Details
40 bytes, text/x-review-board-request
kats
: review+
Details
At the moment, back/forward swiping is kicked of in nsChildView.mm from wheel events that are processed on the main thread. However, when we use async panning for scrolling, the main thread usually doesn't see any wheel events, and the swipe gesture cannot be started.

We may need the APZC to send events to the main thread that are analogous to those we currently get from the Cocoa swipe callback, plus an initial event that lets us prevent normal APZ overscroll in the right cases.

Comment 1

5 years ago
Is there a way back/forward swiping can be reimplemented with APZ (preferably with as little MT processing as possible) that avoids Bug 927702 from happening?
Yes, implementing it with APZ would avoid it, since we don't have access to the buggy API on the APZ thread.
Here's an implementation idea. This way we wouldn't have to call back to the main thread before starting the swipe because all the necessary layers and swipe positioning information would already be known to the APZC.

Updated

5 years ago
See Also: → 927702
Bug 1016035 - Remove the ability to swipe vertically. r?kats

This was intended for a snapshot-based overscroll animation which we're not going to use because APZ solves the same problem in a much better way.
Attachment #8653273 - Flags: review?(bugmail.mozilla)
Bug 1016035 - Move some code around. r?kats
Attachment #8653274 - Flags: review?(bugmail.mozilla)
Bug 1016035 - Refactor the code that decides whether we want to start a swipe. r?kats
Attachment #8653275 - Flags: review?(bugmail.mozilla)
Bug 1016035 - More swipe refactoring. r?kats
Attachment #8653276 - Flags: review?(bugmail.mozilla)
Bug 1016035 - Make the threshold in AxisPhysicsMSDModel::IsFinished controllable by the caller. r?kip
Attachment #8653277 - Flags: review?(kgilbert)
Bug 1016035 - Implement the swipe animation ourselves instead of calling the NSEvent trackSwipe API. r?kats
Attachment #8653278 - Flags: review?(bugmail.mozilla)
Bug 1016035 - Add a MozSwipeGestureMayStart event. r?kats

Having this event means that we don't have to wait for content to find out whether it's scrollable in the case that no swipe should be happening anyway.
Attachment #8653279 - Flags: review?(bugmail.mozilla)
Bug 1016035 - Move swipe tracking code into DispatchAPZWheelInputEvent. r?kats
Attachment #8653280 - Flags: review?(bugmail.mozilla)
Bug 1016035 - Split up MaybeTrackScrollEventAsSwipe into SendMayStartSwipe and TrackScrollEventAsSwipe. r?kats
Attachment #8653281 - Flags: review?(bugmail.mozilla)
Bug 1016035 - Send MayStartSwipe event before sending the wheel event. r?kats
Attachment #8653282 - Flags: review?(bugmail.mozilla)
Bug 1016035 - Add mCanTriggerSwipe and TriggersSwipe(). r?masayuki
Attachment #8653283 - Flags: review?(masayuki)
Bug 1016035 - Make APZEventState report defaultPrevented=true when the event started a swipe. r?kats
Attachment #8653284 - Flags: review?(bugmail.mozilla)
Bug 1016035 - Don't set scroll overflow information on events that APZ already used for scrolling. r?masayuki
Attachment #8653285 - Flags: review?(masayuki)
Bug 1016035 - Also mark widgetWheelEvents that are handled by APZ with mCanStartSwipe. r?kats
Attachment #8653286 - Flags: review?(bugmail.mozilla)
Bug 1016035 - Set overscroll information on potential swipe start events that have been processed by APZ. r?masayuki
Attachment #8653287 - Flags: review?(masayuki)
Bug 1016035 - Add nsIWidget::ReportSwipeStart and call it after processing wheel events that can trigger swipes. r?kats
Attachment #8653288 - Flags: review?(bugmail.mozilla)
Bug 1016035 - Delay the processing of a PanGestureInput block until we know whether it's a swipe. r?kats
Attachment #8653289 - Flags: review?(bugmail.mozilla)
Bug 1016035 - Put PanGestureInput events into a queue if they can end up as a swipe. r?kats
Attachment #8653290 - Flags: review?(bugmail.mozilla)
Bug 1016035 - Replay the queue to the swipe tracker once the swipe start confirmation arrives. r?kats
Attachment #8653291 - Flags: review?(bugmail.mozilla)
Bug 1016035 - Remove const in a few places. r?kats

The next patch will need to modify the event in the PanGestureState constructor in order to pass information back to nsChildView.
Attachment #8653292 - Flags: review?(bugmail.mozilla)
Bug 1016035 - Don't wait for content to say that we need to swipe if APZ has enough information. r?kats
Attachment #8653293 - Flags: review?(bugmail.mozilla)
Bug 1016035 - Swallow the rest of the scroll gesture after swiping without APZ. r?kats

With APZ this is handled because starting a swipe interrupts the PanGesture input block, and momentum events don't start a new input block so APZ ignores them.
Attachment #8653294 - Flags: review?(bugmail.mozilla)
Assignee: nobody → mstange
Status: NEW → ASSIGNED
Comment on attachment 8653273 [details]
MozReview Request: Bug 1016035 - Remove the ability to swipe vertically. r?kats

https://reviewboard.mozilla.org/r/17405/#review15537

I don't know this code but deleting code is usually pretty safe :)
Attachment #8653273 - Flags: review?(bugmail.mozilla) → review+
Comment on attachment 8653274 [details]
MozReview Request: Bug 1016035 - Move some code around. r?kats

https://reviewboard.mozilla.org/r/17407/#review15541

::: widget/cocoa/nsChildView.mm:4954
(Diff revision 1)
> +    if (!mGeckoChild) {

This seems kinda wrong, because we just used mGeckoChild on the previous line assuming it was non-null...

::: widget/cocoa/nsChildView.mm:4959
(Diff revision 1)
> +    // overflowDeltaX and overflowDeltaY tell us when the user has tried to

Move this comment down to inside the if condition
Attachment #8653274 - Flags: review?(bugmail.mozilla)
Comment on attachment 8653275 [details]
MozReview Request: Bug 1016035 - Refactor the code that decides whether we want to start a swipe. r?kats

https://reviewboard.mozilla.org/r/17409/#review15547

::: widget/cocoa/nsChildView.mm:4943
(Diff revision 1)
> +    bool canTriggerSwipe = [self shouldConsiderStartingSwipeFromEvent:theEvent];

Can we move this down to just before where it is used (inside the ifdef)? AFAICT there's no reason it needs to be before DispatchAPZWheelEvent.
Attachment #8653275 - Flags: review?(bugmail.mozilla) → review+
Attachment #8653276 - Flags: review?(bugmail.mozilla) → review+
Comment on attachment 8653278 [details]
MozReview Request: Bug 1016035 - Implement the swipe animation ourselves instead of calling the NSEvent trackSwipe API. r?kats

https://reviewboard.mozilla.org/r/17415/#review15551

Looks pretty good but I want to see the rest of the patch queue and let it sink in a bit before I do a second pass.

::: widget/cocoa/SwipeTracker.h:49
(Diff revision 1)
> +  virtual void WillRefresh(mozilla::TimeStamp aTime) override;

Drop the "virtual" since "override" implies it

::: widget/cocoa/SwipeTracker.h:55
(Diff revision 1)
> +  double SwipeSuccessTargetValue() const;

The notion of "SwipeSuccess" could use an explanatory comment. I see this phrase in two method names but it's not obvious what it means.

::: widget/cocoa/SwipeTracker.mm:7
(Diff revision 1)
> +#include "SwipeTracker.h"

Leave a blank line after including SwipeTracker.h

::: widget/cocoa/SwipeTracker.mm:22
(Diff revision 1)
> +using namespace mozilla;

I'd prefer to avoid "using namespace" since it can screw with unified builds. You should just be able to wrap the body of this file in a "namespace mozilla {...}" block and get the same effect.

::: widget/cocoa/SwipeTracker.mm:30
(Diff revision 1)
> +  , mEventPosition(RoundedToInt(aSwipeStartEvent.mPanStartPoint * ScreenToLayoutDeviceScale(1)))

Use ViewAs<>

::: widget/cocoa/SwipeTracker.mm:49
(Diff revision 1)
> +    }

else MOZ_ASSERT(false) because we're leaking stuff and we should know about it.

::: widget/cocoa/SwipeTracker.mm:162
(Diff revision 1)
> +    }

else MOZ_ASSERT(false) because we're leaking stuff and we should know about it.

Might even be good to extract a helper to do this removal and setting the flag to false, and call it from both Destroy() and here

::: widget/cocoa/SwipeTracker.mm:177
(Diff revision 1)
> +  mWidget.SwipeFinished(); 

trailing ws

::: widget/cocoa/SwipeTracker.mm:183
(Diff revision 1)
> +  nsIWidgetListener* widgetListener = mWidget.GetWidgetListener();

This seems a little scary to me, since it's not obvious to me that you'll be getting the same refresh driver always. You might end up registering the observer on one refresh driver and unregistering it on another, causing a leak. Usually in cases like this I try to keep a refptr to the target (the refresh driver in this case) in the class, so that I know I'm unregistering on the same object that I registered it on. I'm not sure if you can have a refptr to the refresh driver but having one to the presshell should be ok. It would at least guard against the presShell in the widget changing out from under you between the register and unregister.

::: widget/cocoa/nsChildView.mm:2583
(Diff revision 1)
> +    RoundedToInt(aSwipeStartEvent.mPanStartPoint * ScreenToLayoutDeviceScale(1));

ViewAs<>
Attachment #8653278 - Flags: review?(bugmail.mozilla)
Comment on attachment 8653279 [details]
MozReview Request: Bug 1016035 - Add a MozSwipeGestureMayStart event. r?kats

https://reviewboard.mozilla.org/r/17417/#review15579

Doesn't look like the IDL changes need a UUID bump here but the hg push hook will probably yell at you when you try to land this unless you add IGNORE IDL to the commit message.

::: browser/base/content/browser-gestureSupport.js:67
(Diff revision 1)
> +        this._setupSwipeGesture();

For consistency with the next couple of case statements, move the preventDefault() up to before the setupSwipeGesture() call.
Attachment #8653279 - Flags: review?(bugmail.mozilla) → review+
https://reviewboard.mozilla.org/r/17407/#review15541

> This seems kinda wrong, because we just used mGeckoChild on the previous line assuming it was non-null...

The idea here is: Dispatching the event can run arbitrary JS, which might close the window and thus destroy the widget, which nulls out mGeckoChild. It's not pretty but that's how other parts of this code have handled this problem, and I'm just moving code here.
https://reviewboard.mozilla.org/r/17415/#review15551

> else MOZ_ASSERT(false) because we're leaking stuff and we should know about it.

We're not necessarily leaking stuff. If the refresh driver has gone away, it has dropped its reference to us. If it's still alive and we just can't get at it anymore, then we're in trouble.
Matt Woodrow tells me it should be safe to keep an nsRefPtr<nsRefreshDriver>, so I'm going to do that and avoid this case.

> else MOZ_ASSERT(false) because we're leaking stuff and we should know about it.
> 
> Might even be good to extract a helper to do this removal and setting the flag to false, and call it from both Destroy() and here

I'll extract it.

> This seems a little scary to me, since it's not obvious to me that you'll be getting the same refresh driver always. You might end up registering the observer on one refresh driver and unregistering it on another, causing a leak. Usually in cases like this I try to keep a refptr to the target (the refresh driver in this case) in the class, so that I know I'm unregistering on the same object that I registered it on. I'm not sure if you can have a refptr to the refresh driver but having one to the presshell should be ok. It would at least guard against the presShell in the widget changing out from under you between the register and unregister.

Good Point. nsICanvasRenderingContextInternal also has an nsRefPtr<nsRefreshDriver>, so it should be safe to use.
Comment on attachment 8653277 [details]
MozReview Request: Bug 1016035 - Make the threshold in AxisPhysicsMSDModel::IsFinished controllable by the caller. r?kip

https://reviewboard.mozilla.org/r/17413/#review15611

This looks good, Ship it!
Attachment #8653277 - Flags: review?(kgilbert) → review+
(In reply to Markus Stange [:mstange] from comment #33)
> The idea here is: Dispatching the event can run arbitrary JS, which might
> close the window and thus destroy the widget, which nulls out mGeckoChild.
> It's not pretty but that's how other parts of this code have handled this
> problem, and I'm just moving code here.

Ah I see. Worth a comment then. I assume we're not doing a kungFuDeathGrip for a good reason here.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #36)
> (In reply to Markus Stange [:mstange] from comment #33)
> > The idea here is: Dispatching the event can run arbitrary JS, which might
> > close the window and thus destroy the widget, which nulls out mGeckoChild.
> > It's not pretty but that's how other parts of this code have handled this
> > problem, and I'm just moving code here.
> 
> Ah I see. Worth a comment then. I assume we're not doing a kungFuDeathGrip
> for a good reason here.

Well, the old code didn't have the comment, and a later patch is removing the line anyway, so I'm not going to add the comment.

We might still need a few kunFuDeathGrips in the nsChildView methods that send events. I haven't really thought about all the implications there.
Comment on attachment 8653283 [details]
MozReview Request: Bug 1016035 - Add mCanTriggerSwipe and TriggersSwipe(). r?masayuki

https://reviewboard.mozilla.org/r/17425/#review15643

::: widget/BasicEvents.h:120
(Diff revision 1)
> +  bool mCanTriggerSwipe : 1;

Why don't you add this flag to WidgetWheelEvent directly? BaseEventFlags should be used only when the flag is used by two or more event classes.
Attachment #8653283 - Flags: review?(masayuki)
Comment on attachment 8653285 [details]
MozReview Request: Bug 1016035 - Set scroll overflow information on potential swipe start events that have been processed by APZ. r?masayuki

https://reviewboard.mozilla.org/r/17429/#review15647

::: dom/events/EventStateManager.cpp:3134
(Diff revision 1)
> -          // If we don't handle the wheel event, all of the delta values must
> +          if (!wheelEvent->mFlags.mHandledByAPZ) {

nit:

if (wheelEvent->mFlags.mHandledByAPZ) {
  break;
}

is better.
Comment on attachment 8653287 [details]
MozReview Request: Bug 1016035 - Set overscroll information on potential swipe start events that have been processed by APZ. r?masayuki

https://reviewboard.mozilla.org/r/17433/#review15649

::: dom/events/EventStateManager.cpp:3074
(Diff revision 1)
> +      if (wheelEvent->mFlags.mHandledByAPZ && wheelEvent->mFlags.mCanTriggerSwipe &&
> +          !ComputeScrollTarget(aTargetFrame, wheelEvent, COMPUTE_DEFAULT_ACTION_TARGET)) {
> +        // No scrollframe is available for scrolling in the requested direction.
> +        wheelEvent->mViewPortIsOverscrolled = true;
> +        wheelEvent->overflowDeltaX = wheelEvent->deltaX;
> +        wheelEvent->overflowDeltaY = wheelEvent->deltaY;
> +        WheelPrefs::GetInstance()->
> +          CancelApplyingUserPrefsFromOverflowDelta(wheelEvent);
> +      }

Why don't you include this into the above |if (wheelEvent->mFlags.mHandledByAPZ| block or the case ACTION_NONE below? I like the latter better because this handles default action as ACTION_NONE. When other developers changes the code in case ACTION_NONE, this block should be checked by their eyes.
Comment on attachment 8653273 [details]
MozReview Request: Bug 1016035 - Remove the ability to swipe vertically. r?kats

Bug 1016035 - Remove the ability to swipe vertically. r?kats

This was intended for a snapshot-based overscroll animation which we're not going to use because APZ solves the same problem in a much better way.
Comment on attachment 8653274 [details]
MozReview Request: Bug 1016035 - Move some code around. r?kats

Bug 1016035 - Move some code around. r?kats
Attachment #8653274 - Flags: review?(bugmail.mozilla)
Comment on attachment 8653275 [details]
MozReview Request: Bug 1016035 - Refactor the code that decides whether we want to start a swipe. r?kats

Bug 1016035 - Refactor the code that decides whether we want to start a swipe. r?kats
Comment on attachment 8653276 [details]
MozReview Request: Bug 1016035 - More swipe refactoring. r?kats

Bug 1016035 - More swipe refactoring. r?kats
Comment on attachment 8653277 [details]
MozReview Request: Bug 1016035 - Make the threshold in AxisPhysicsMSDModel::IsFinished controllable by the caller. r?kip

Bug 1016035 - Make the threshold in AxisPhysicsMSDModel::IsFinished controllable by the caller. r?kip
Comment on attachment 8653278 [details]
MozReview Request: Bug 1016035 - Implement the swipe animation ourselves instead of calling the NSEvent trackSwipe API. r?kats

Bug 1016035 - Implement the swipe animation ourselves instead of calling the NSEvent trackSwipe API. r?kats
Attachment #8653278 - Flags: review?(bugmail.mozilla)
Comment on attachment 8653279 [details]
MozReview Request: Bug 1016035 - Add a MozSwipeGestureMayStart event. r?kats

Bug 1016035 - Add a MozSwipeGestureMayStart event. r?kats

Having this event means that we don't have to wait for content to find out whether it's scrollable in the case that no swipe should be happening anyway.

IGNORE IDL because I'm only changing comments in the IDL files.
Comment on attachment 8653280 [details]
MozReview Request: Bug 1016035 - Move swipe tracking code into DispatchAPZWheelInputEvent. r?kats

Bug 1016035 - Move swipe tracking code into DispatchAPZWheelInputEvent. r?kats
Comment on attachment 8653281 [details]
MozReview Request: Bug 1016035 - Split up MaybeTrackScrollEventAsSwipe into SendMayStartSwipe and TrackScrollEventAsSwipe. r?kats

Bug 1016035 - Split up MaybeTrackScrollEventAsSwipe into SendMayStartSwipe and TrackScrollEventAsSwipe. r?kats
Comment on attachment 8653282 [details]
MozReview Request: Bug 1016035 - Send MayStartSwipe event before sending the wheel event. r?kats

Bug 1016035 - Send MayStartSwipe event before sending the wheel event. r?kats
Comment on attachment 8653283 [details]
MozReview Request: Bug 1016035 - Add mCanTriggerSwipe and TriggersSwipe(). r?masayuki

Bug 1016035 - Add mCanTriggerSwipe and TriggersSwipe(). r?masayuki
Attachment #8653283 - Flags: review?(masayuki)
Comment on attachment 8653284 [details]
MozReview Request: Bug 1016035 - Make APZEventState report defaultPrevented=true when the event started a swipe. r?kats

Bug 1016035 - Make APZEventState report defaultPrevented=true when the event started a swipe. r?kats
Comment on attachment 8653286 [details]
MozReview Request: Bug 1016035 - Also mark widgetWheelEvents that are handled by APZ with mCanStartSwipe. r?kats

Bug 1016035 - Also mark widgetWheelEvents that are handled by APZ with mCanStartSwipe. r?kats
Comment on attachment 8653285 [details]
MozReview Request: Bug 1016035 - Set scroll overflow information on potential swipe start events that have been processed by APZ. r?masayuki

Bug 1016035 - Set scroll overflow information on potential swipe start events that have been processed by APZ. r?masayuki
Attachment #8653285 - Attachment description: MozReview Request: Bug 1016035 - Don't set scroll overflow information on events that APZ already used for scrolling. r?masayuki → MozReview Request: Bug 1016035 - Set scroll overflow information on potential swipe start events that have been processed by APZ. r?masayuki
Attachment #8653285 - Flags: review?(masayuki)
Comment on attachment 8653289 [details]
MozReview Request: Bug 1016035 - Delay the processing of a PanGestureInput block until we know whether it's a swipe. r?kats

Bug 1016035 - Delay the processing of a PanGestureInput block until we know whether it's a swipe. r?kats
Comment on attachment 8653290 [details]
MozReview Request: Bug 1016035 - Put PanGestureInput events into a queue if they can end up as a swipe. r?kats

Bug 1016035 - Put PanGestureInput events into a queue if they can end up as a swipe. r?kats
Comment on attachment 8653288 [details]
MozReview Request: Bug 1016035 - Add nsIWidget::ReportSwipeStart and call it after processing wheel events that can trigger swipes. r?kats

Bug 1016035 - Add nsIWidget::ReportSwipeStart and call it after processing wheel events that can trigger swipes. r?kats
Comment on attachment 8653291 [details]
MozReview Request: Bug 1016035 - Replay the queue to the swipe tracker once the swipe start confirmation arrives. r?kats

Bug 1016035 - Replay the queue to the swipe tracker once the swipe start confirmation arrives. r?kats
Comment on attachment 8653292 [details]
MozReview Request: Bug 1016035 - Remove const in a few places. r?kats

Bug 1016035 - Remove const in a few places. r?kats

The next patch will need to modify the event in the PanGestureState constructor in order to pass information back to nsChildView.
Comment on attachment 8653293 [details]
MozReview Request: Bug 1016035 - Don't wait for content to say that we need to swipe if APZ has enough information. r?kats

Bug 1016035 - Don't wait for content to say that we need to swipe if APZ has enough information. r?kats
Comment on attachment 8653294 [details]
MozReview Request: Bug 1016035 - Swallow the rest of the scroll gesture after swiping without APZ. r?kats

Bug 1016035 - Swallow the rest of the scroll gesture after swiping without APZ. r?kats

With APZ this is handled because starting a swipe interrupts the PanGesture input block, and momentum events don't start a new input block so APZ ignores them.
Attachment #8653287 - Attachment is obsolete: true
Comment on attachment 8653283 [details]
MozReview Request: Bug 1016035 - Add mCanTriggerSwipe and TriggersSwipe(). r?masayuki

https://reviewboard.mozilla.org/r/17425/#review15663
Attachment #8653283 - Flags: review?(masayuki) → review+
Comment on attachment 8653285 [details]
MozReview Request: Bug 1016035 - Set scroll overflow information on potential swipe start events that have been processed by APZ. r?masayuki

https://reviewboard.mozilla.org/r/17429/#review15665

::: dom/events/EventStateManager.cpp:3140
(Diff revision 2)
> +            if (ComputeScrollTarget(aTargetFrame, wheelEvent, COMPUTE_DEFAULT_ACTION_TARGET)) {

This line is over 80 characters.

::: dom/events/EventStateManager.cpp:3140
(Diff revision 2)
> +            if (ComputeScrollTarget(aTargetFrame, wheelEvent, COMPUTE_DEFAULT_ACTION_TARGET)) {
> +              // Something is scrollable in the requested direction, so setting
> +              // scroll overflow would be wrong.
> +              break;

I'm confused now. Why do we need to check scroll target when the default action is none? And then, what was performed (handled) by APZ?
Attachment #8653285 - Flags: review?(masayuki)
https://reviewboard.mozilla.org/r/17429/#review15665

> I'm confused now. Why do we need to check scroll target when the default action is none? And then, what was performed (handled) by APZ?

Yeah, sorry, it is very confusing. I'd appreciate any tips on how to make this code easier to understand.

There are three cases here:
 1. No APZ: The behavior should be the same as before.
 2. APZ for events without mCanTriggerSwipe: APZ handles scrolling and nobody looks at the scroll overflow fields.
 3. APZ for events with mCanTriggerSwipe: For these events, APZ will handle the scrolling, but it delays the actual scrolling until after it gets a response from content. And the content response depends on event.TriggersSwipe(), which looks at the scroll overflow values. That's why the event needs to know whether something is scrollable even if it has already been processed by APZ. (So in that case "mHandledByAPZ" means "APZ looked at this event and might do scrolling based on it", and not "APZ has already scrolled in response to this event".)

The patch two before this one and the patch three after this one add the event.TriggersSwipe() checks, if you want to look at them.
https://reviewboard.mozilla.org/r/17429/#review15665

> Yeah, sorry, it is very confusing. I'd appreciate any tips on how to make this code easier to understand.
> 
> There are three cases here:
>  1. No APZ: The behavior should be the same as before.
>  2. APZ for events without mCanTriggerSwipe: APZ handles scrolling and nobody looks at the scroll overflow fields.
>  3. APZ for events with mCanTriggerSwipe: For these events, APZ will handle the scrolling, but it delays the actual scrolling until after it gets a response from content. And the content response depends on event.TriggersSwipe(), which looks at the scroll overflow values. That's why the event needs to know whether something is scrollable even if it has already been processed by APZ. (So in that case "mHandledByAPZ" means "APZ looked at this event and might do scrolling based on it", and not "APZ has already scrolled in response to this event".)
> 
> The patch two before this one and the patch three after this one add the event.TriggersSwipe() checks, if you want to look at them.

Ah, I see. Could you add comment before the if statement?
Comment on attachment 8653274 [details]
MozReview Request: Bug 1016035 - Move some code around. r?kats

https://reviewboard.mozilla.org/r/17407/#review15695
Attachment #8653274 - Flags: review?(bugmail.mozilla) → review+
Comment on attachment 8653278 [details]
MozReview Request: Bug 1016035 - Implement the swipe animation ourselves instead of calling the NSEvent trackSwipe API. r?kats

https://reviewboard.mozilla.org/r/17415/#review15701
Attachment #8653278 - Flags: review?(bugmail.mozilla) → review+
Comment on attachment 8653280 [details]
MozReview Request: Bug 1016035 - Move swipe tracking code into DispatchAPZWheelInputEvent. r?kats

https://reviewboard.mozilla.org/r/17419/#review15703
Attachment #8653280 - Flags: review?(bugmail.mozilla) → review+
Comment on attachment 8653281 [details]
MozReview Request: Bug 1016035 - Split up MaybeTrackScrollEventAsSwipe into SendMayStartSwipe and TrackScrollEventAsSwipe. r?kats

https://reviewboard.mozilla.org/r/17421/#review15707

::: widget/cocoa/nsChildView.h:555
(Diff revision 2)
> -  void MaybeTrackScrollEventAsSwipe(const mozilla::PanGestureInput& aSwipeStartEvent);
> +  struct SwipeInfo {

Does this stuff need to be public? I'd prefer protected or private.
Attachment #8653281 - Flags: review?(bugmail.mozilla) → review+
Attachment #8653282 - Flags: review?(bugmail.mozilla) → review+
Comment on attachment 8653282 [details]
MozReview Request: Bug 1016035 - Send MayStartSwipe event before sending the wheel event. r?kats

https://reviewboard.mozilla.org/r/17423/#review15711

::: widget/cocoa/nsChildView.mm:2770
(Diff revision 2)
> +        SwipeInfo swipeInfo = SendMayStartSwipe(panInput);

Should the event still be sent if the event's deltas are zero? I feel like the SendMayStartSwipe call should be inside the if condition still (although before the DispatchEvent)
Comment on attachment 8653284 [details]
MozReview Request: Bug 1016035 - Make APZEventState report defaultPrevented=true when the event started a swipe. r?kats

https://reviewboard.mozilla.org/r/17427/#review15713
Attachment #8653284 - Flags: review?(bugmail.mozilla) → review+
Comment on attachment 8653286 [details]
MozReview Request: Bug 1016035 - Also mark widgetWheelEvents that are handled by APZ with mCanStartSwipe. r?kats

https://reviewboard.mozilla.org/r/17431/#review15715
Attachment #8653286 - Flags: review?(bugmail.mozilla) → review+
https://reviewboard.mozilla.org/r/17423/#review15711

> Should the event still be sent if the event's deltas are zero? I feel like the SendMayStartSwipe call should be inside the if condition still (although before the DispatchEvent)

Looking at this again, I think it would be better to just remove the if condition completely. If aCanTriggerSwipe is true, we know that this is an event that we want to send to Gecko, regardless of what it looks like (though in reality it will always be an NS_WHEEL_WHEEL event with a deltaX != 0).
https://reviewboard.mozilla.org/r/17429/#review15665

> Ah, I see. Could you add comment before the if statement?

Sure.
Comment on attachment 8653289 [details]
MozReview Request: Bug 1016035 - Delay the processing of a PanGestureInput block until we know whether it's a swipe. r?kats

https://reviewboard.mozilla.org/r/17437/#review15727
Attachment #8653289 - Flags: review?(bugmail.mozilla) → review+
Comment on attachment 8653290 [details]
MozReview Request: Bug 1016035 - Put PanGestureInput events into a queue if they can end up as a swipe. r?kats

https://reviewboard.mozilla.org/r/17439/#review15731
Attachment #8653290 - Flags: review?(bugmail.mozilla) → review+
Comment on attachment 8653288 [details]
MozReview Request: Bug 1016035 - Add nsIWidget::ReportSwipeStart and call it after processing wheel events that can trigger swipes. r?kats

https://reviewboard.mozilla.org/r/17435/#review15735
Attachment #8653288 - Flags: review?(bugmail.mozilla) → review+
Attachment #8653291 - Flags: review?(bugmail.mozilla) → review+
Comment on attachment 8653291 [details]
MozReview Request: Bug 1016035 - Replay the queue to the swipe tracker once the swipe start confirmation arrives. r?kats

https://reviewboard.mozilla.org/r/17441/#review15737
Comment on attachment 8653293 [details]
MozReview Request: Bug 1016035 - Don't wait for content to say that we need to swipe if APZ has enough information. r?kats

https://reviewboard.mozilla.org/r/17445/#review15755

::: gfx/layers/apz/src/InputBlockState.cpp:457
(Diff revision 2)
> +    aInitialEvent.mHorizontalScrollStartSuccess =

As discussed on IRC I think a better approach would be to use the return value of APZCTreeManager::ReceiveInputEvent to send this information back to the widget. This should map closely to the existing semantics of that function and will avoid having to mutate the PanGestureInput. This will also make it unnecessary to un-const all of the things.
Attachment #8653293 - Flags: review?(bugmail.mozilla)
Comment on attachment 8653292 [details]
MozReview Request: Bug 1016035 - Remove const in a few places. r?kats

https://reviewboard.mozilla.org/r/17443/#review15757

Clearing review flag on this patch for now (hopefully that's what MozReview does)
Attachment #8653292 - Flags: review?(bugmail.mozilla)
Attachment #8653294 - Flags: review?(bugmail.mozilla) → review+
Comment on attachment 8653294 [details]
MozReview Request: Bug 1016035 - Swallow the rest of the scroll gesture after swiping without APZ. r?kats

https://reviewboard.mozilla.org/r/17447/#review15759
https://reviewboard.mozilla.org/r/17429/#review15665

> Sure.

I'm changing the code around a little to make this easier to grasp.
https://reviewboard.mozilla.org/r/17421/#review15707

> Does this stuff need to be public? I'd prefer protected or private.

I'm making it protected.
Comment on attachment 8653280 [details]
MozReview Request: Bug 1016035 - Move swipe tracking code into DispatchAPZWheelInputEvent. r?kats

Bug 1016035 - Move swipe tracking code into DispatchAPZWheelInputEvent. r?kats
Comment on attachment 8653281 [details]
MozReview Request: Bug 1016035 - Split up MaybeTrackScrollEventAsSwipe into SendMayStartSwipe and TrackScrollEventAsSwipe. r?kats

Bug 1016035 - Split up MaybeTrackScrollEventAsSwipe into SendMayStartSwipe and TrackScrollEventAsSwipe. r?kats
Comment on attachment 8653282 [details]
MozReview Request: Bug 1016035 - Send MayStartSwipe event before sending the wheel event. r?kats

Bug 1016035 - Send MayStartSwipe event before sending the wheel event. r?kats
Comment on attachment 8653283 [details]
MozReview Request: Bug 1016035 - Add mCanTriggerSwipe and TriggersSwipe(). r?masayuki

Bug 1016035 - Add mCanTriggerSwipe and TriggersSwipe(). r?masayuki
Comment on attachment 8653284 [details]
MozReview Request: Bug 1016035 - Make APZEventState report defaultPrevented=true when the event started a swipe. r?kats

Bug 1016035 - Make APZEventState report defaultPrevented=true when the event started a swipe. r?kats
Comment on attachment 8653286 [details]
MozReview Request: Bug 1016035 - Also mark widgetWheelEvents that are handled by APZ with mCanStartSwipe. r?kats

Bug 1016035 - Also mark widgetWheelEvents that are handled by APZ with mCanStartSwipe. r?kats
Comment on attachment 8653285 [details]
MozReview Request: Bug 1016035 - Set scroll overflow information on potential swipe start events that have been processed by APZ. r?masayuki

Bug 1016035 - Set scroll overflow information on potential swipe start events that have been processed by APZ. r?masayuki
Attachment #8653285 - Flags: review?(masayuki)
Comment on attachment 8653289 [details]
MozReview Request: Bug 1016035 - Delay the processing of a PanGestureInput block until we know whether it's a swipe. r?kats

Bug 1016035 - Delay the processing of a PanGestureInput block until we know whether it's a swipe. r?kats
Comment on attachment 8653290 [details]
MozReview Request: Bug 1016035 - Put PanGestureInput events into a queue if they can end up as a swipe. r?kats

Bug 1016035 - Put PanGestureInput events into a queue if they can end up as a swipe. r?kats
Comment on attachment 8653288 [details]
MozReview Request: Bug 1016035 - Add nsIWidget::ReportSwipeStart and call it after processing wheel events that can trigger swipes. r?kats

Bug 1016035 - Add nsIWidget::ReportSwipeStart and call it after processing wheel events that can trigger swipes. r?kats
Comment on attachment 8653291 [details]
MozReview Request: Bug 1016035 - Replay the queue to the swipe tracker once the swipe start confirmation arrives. r?kats

Bug 1016035 - Replay the queue to the swipe tracker once the swipe start confirmation arrives. r?kats
Comment on attachment 8653293 [details]
MozReview Request: Bug 1016035 - Don't wait for content to say that we need to swipe if APZ has enough information. r?kats

Bug 1016035 - Don't wait for content to say that we need to swipe if APZ has enough information. r?kats
Attachment #8653293 - Flags: review?(bugmail.mozilla)
Comment on attachment 8653294 [details]
MozReview Request: Bug 1016035 - Swallow the rest of the scroll gesture after swiping without APZ. r?kats

Bug 1016035 - Swallow the rest of the scroll gesture after swiping without APZ. r?kats

With APZ this is handled because starting a swipe interrupts the PanGesture input block, and momentum events don't start a new input block so APZ ignores them.
Attachment #8653292 - Attachment is obsolete: true
Comment on attachment 8653285 [details]
MozReview Request: Bug 1016035 - Set scroll overflow information on potential swipe start events that have been processed by APZ. r?masayuki

https://reviewboard.mozilla.org/r/17429/#review15817

Thanks!
Attachment #8653285 - Flags: review?(masayuki) → review+
Comment on attachment 8653293 [details]
MozReview Request: Bug 1016035 - Don't wait for content to say that we need to swipe if APZ has enough information. r?kats

https://reviewboard.mozilla.org/r/17445/#review15827

Much better, thanks!

::: widget/InputData.h:347
(Diff revision 3)
> +  bool mRequiresContentResponseIfCannotScrollHorizontallyInStartDirection;

This is quite a mouthful but I can't think of anything better... plus it's kinda amusing :)
Attachment #8653293 - Flags: review?(bugmail.mozilla) → review+
You need to log in before you can comment on or make changes to this bug.