1142866 - Implement wheel transactions in APZ

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

[David Anderson [:dvander] - inactive, e-mail if emergency]

Wheel transactions are an EventStateManager construct that prevent continuous scroll wheel events from falling into random scrollframes.

The logic that seems to be needed is:
 - Once a frame is scrolled, it should continue to be scrolled no matter what is underneath the cursor. This frame is said to be in a "wheel transaction".
 - A wheel transaction is cancelled in the following cases:
   - 1500ms elapses since the most recent wheel event.
   - If the mouse moves outside the scrollframe.
   - If a key, button, click, contextmenu, or drop event occurs.
   - If a mouse move within the scrollframe occurs within 100ms of the last wheel event,
     and then another wheel event occurs within 100ms of that mouse movement.

To do the last 3 requirements, APZ will need to know about those events.

Comment 1

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: wip.patch

Not ready for review yet, want to see if this is on the right track. The idea is:
 (1) don't allow overscrolling if in a wheel transaction and
 (2) construct overscroll handoff chains that only include apzcs
     that can be scrolled in the current direction.

The annoying part is that overscroll handoff chains are created in a lot of places, for example, pan gestures create a temporary one and just ignore the source InputBlock's chain. So this feels pretty hacky. I think it would be cleaner to, when we confirm the target APZC, to select a parent scrollable APZC if the target isn't scrollable. If it's in a wheel transaction then we're not going to be handing off scrolls anyway.

Comment 2

[Botond Ballo [:botond]]

(In reply to David Anderson [:dvander] from comment #1)
> The annoying part is that overscroll handoff chains are created in a lot of
> places, for example, pan gestures create a temporary one and just ignore the
> source InputBlock's chain.

That doesn't seem desirable.

Are you referring to the call to BuildOverscrollHandoffChain() in HandleSmoothScrollOverscroll() [1]?

[1] http://mxr.mozilla.org/mozilla-central/source/gfx/layers/apz/src/AsyncPanZoomController.cpp?rev=420b643efff1#2064

Comment 3

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

Comment on attachment 8577137 [details] [diff] [review]
wip.patch

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

::: gfx/layers/apz/src/APZCTreeManager.cpp
@@ +1238,5 @@
> +    bool choose = true;
> +    if (aOnlyIfScrollableWithin &&
> +        !apzc->CanScroll(aOnlyIfScrollableWithin->x, aOnlyIfScrollableWithin->y))
> +    {
> +      choose = false;

Instead of computing this when we build the handoff chain, can we just build the chain normally, and then when we walk the chain to actually do the handoff, skip over APZCs in the chain that can't scroll? It's not clear to me from comment 0 and comment 1 why you need to do this. Can you describe in a bit more detail the overscroll and handoff requirements for wheel events?

If I'm interpreting it correctly it sounds like for a given wheel block, only the innermost scrollable APZC should scroll. Once it reaches the end, no handoff occurs, but the next wheel block will scroll the containing APZC.

This sounds like logic that can be implemented in the AttemptScroll function as "if any part of the scroll is consumed, don't do a handoff". If needed you can even just set a flag on the current wheel block to indicate that scrolling in a particular direction should now be ignored because we've scrolled as far as we can in that direction for that wheel block.

::: gfx/layers/apz/src/InputQueue.h
@@ +93,5 @@
> +  // If true, overscroll handoff should not take place because a wheel
> +  // transaction is active and has not yet ended. This prevents scrolling into
> +  // other layers during a scroll motion that doesn't change the cursor
> +  // position.
> +  bool AllowScrollHandoffInWheelTransaction() const;

Instead of adding this function here, I'd rather you added a CurrentWheelBlock() function in AsyncPanZoomController similar to CurrentTouchBlock(), and then called CurrentWheelBlock()->AllowScrollHandoff().

Comment 4

[David Anderson [:dvander] - inactive, e-mail if emergency]

(In reply to Botond Ballo [:botond] from comment #2)
> (In reply to David Anderson [:dvander] from comment #1)
> > The annoying part is that overscroll handoff chains are created in a lot of
> > places, for example, pan gestures create a temporary one and just ignore the
> > source InputBlock's chain.
> 
> That doesn't seem desirable.
> 
> Are you referring to the call to BuildOverscrollHandoffChain() in
> HandleSmoothScrollOverscroll() [1]?

Yes - OnPanStart builds its own overscroll handoff chain as well though, since it creates a fake input block.

Comment 5

[David Anderson [:dvander] - inactive, e-mail if emergency]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #3)
> Instead of computing this when we build the handoff chain, can we just build
> the chain normally, and then when we walk the chain to actually do the
> handoff, skip over APZCs in the chain that can't scroll? It's not clear to
> me from comment 0 and comment 1 why you need to do this. Can you describe in
> a bit more detail the overscroll and handoff requirements for wheel events?
> 
> If I'm interpreting it correctly it sounds like for a given wheel block,
> only the innermost scrollable APZC should scroll. Once it reaches the end,
> no handoff occurs, but the next wheel block will scroll the containing APZC.
> 
> This sounds like logic that can be implemented in the AttemptScroll function
> as "if any part of the scroll is consumed, don't do a handoff". If needed
> you can even just set a flag on the current wheel block to indicate that
> scrolling in a particular direction should now be ignored because we've
> scrolled as far as we can in that direction for that wheel block.

What it was trying to solve is, say you have an inner apzc and outer apzc. If the inner apzc is scrolled all the way down, and the user scrolls down inside that apzc, the wheel block should only ever scroll the outer apzc. If the user scrolls back up before the 1.5s wheel timeout, the inner apzc should be ignored, and the outer apzc should scroll.

But just changing AttemptScroll should work. Basically in a wheel transaction there will only ever be one apzc that is considered.

Comment 6

[Botond Ballo [:botond]]

(In reply to David Anderson [:dvander] from comment #4)
> (In reply to Botond Ballo [:botond] from comment #2)
> > (In reply to David Anderson [:dvander] from comment #1)
> > > The annoying part is that overscroll handoff chains are created in a lot of
> > > places, for example, pan gestures create a temporary one and just ignore the
> > > source InputBlock's chain.
> > 
> > That doesn't seem desirable.
> > 
> > Are you referring to the call to BuildOverscrollHandoffChain() in
> > HandleSmoothScrollOverscroll() [1]?
> 
> Yes 

For fling animations, the animation keeps a reference to the overscroll handoff chain of the input block that triggered it, and then passes that to HandleFlingOverscroll rather than computing a new one.

Do you know if there's a reason we don't do the same for smooth scroll animations?

> OnPanStart builds its own overscroll handoff chain as well though,
> since it creates a fake input block.

In what sense is this input block fake? Is there a real input block that could be used instead?

Comment 7

[David Anderson [:dvander] - inactive, e-mail if emergency]

(In reply to Botond Ballo [:botond] from comment #6)
> > OnPanStart builds its own overscroll handoff chain as well though,
> > since it creates a fake input block.
> 
> In what sense is this input block fake? Is there a real input block that
> could be used instead?

Well, when simulated via wheel events, there is a real input block. I'm not sure about other cases. It is odd that it creates its own input block rather than relying on InputQueue to do it.

Comment 8

[Botond Ballo [:botond]]

(In reply to David Anderson [:dvander] from comment #7)
> (In reply to Botond Ballo [:botond] from comment #6)
> > > OnPanStart builds its own overscroll handoff chain as well though,
> > > since it creates a fake input block.
> > 
> > In what sense is this input block fake? Is there a real input block that
> > could be used instead?
> 
> Well, when simulated via wheel events, there is a real input block. I'm not
> sure about other cases. It is odd that it creates its own input block rather
> than relying on InputQueue to do it.

InputQueue doesn't create touch blocks for pan gesture inputs. I'm not familiar enough with pan gesture inputs to say whether this is deliberate because pan gesture inputs don't need the same queuing behaviour as touch inputs, or it was just never implemented. (Markus, do you know?)

In any case, though, InputQueue *does* create input blocks for wheel events, so if there is shared code between wheel events and pan gestures, it should probably be refactored so that for wheel events it uses those input blocks rather than creating its own.

Comment 9

[Markus Stange [:mstange]]

I don't know, but I wouldn't be surprised if the way PanGestures handled this stuff was completely wrong. There's probably no reason for them to be different from what touch events or wheel events do.

Kip might know more.

Comment 10

[Botond Ballo [:botond]]

CC'ing Kip to see if he has anything to add.

It seems to me that, absent arguments to the contrary, the cleanest way forward is to refactor things a bit to make the handling of input blocks and handoff chains more uniform across event types.

Comment 11

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: wip v2

This version skips mucking with OverscrollHandoffChain. Instead, it changes the target apzc of wheel blocks such that we skip past any that can't be scrolled. Then, overscrolling in a wheel transaction is just disabled. This approach feels a lot cleaner and seems to work ok.

Comment 12

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

Comment on attachment 8577920 [details] [diff] [review]
wip v2

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

Thanks, this generally looks a lot better.

::: gfx/layers/apz/src/AsyncPanZoomController.cpp
@@ +1416,5 @@
>  
> +void
> +AsyncPanZoomController::GetScrollWheelDelta(const ScrollWheelInput& aEvent,
> +                                            double& aDeltaX,
> +                                            double& aDeltaY) const

aOutDeltaX,Y

::: gfx/layers/apz/src/InputBlockState.cpp
@@ +167,5 @@
> +  for (size_t i = 0; i < mOverscrollHandoffChain->Length(); i++) {
> +    nsRefPtr<AsyncPanZoomController> apzc = mOverscrollHandoffChain->GetApzcAtIndex(i);
> +    if (apzc->CanScroll(aInitialEvent)) {
> +      if (apzc != mTargetApzc) {
> +        UpdateTargetApzc(apzc);

Note that you don't want to call UpdateTargetApzc until the target has been confirmed. If you walk the handoff chain of the tentative target that the APZCTM computes, you may end up with the wrong one here. So only do this if aTargetConfirmed is true, or after SetConfirmedTargetApzc has been called.

Also you could probably make this a function on OverscrollHandoffChain that uses a helper function FirstMatchingApzc(APZCPredicate) that's analogous AnyApzc.

Comment 13

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 1, wheel block support for transactions

Comment 14

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 2, end transactions

This replicates the logic from WheelTransaction::OnEvent() that happens on the main thread.

Comment 15

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 1, wheel block support for transactions

Whoops, these patch files got mixed together.

Comment 16

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 2, end transactions

Comment 17

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

Comment on attachment 8578426 [details] [diff] [review]
part 1, wheel block support for transactions

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

r+ with comments addressed (mostly nits). Thanks for all the detailed comments, they were very helpful! :)

::: gfx/layers/apz/src/AsyncPanZoomController.h
@@ +360,5 @@
> +  bool CanScroll(const ScrollWheelInput& aEvent) const;
> +
> +  // Return whether or not a scroll delta will be able to scroll in either
> +  // direction.
> +  bool CanScroll(double aOutDeltaX, double aOutDeltaY) const;

aDeltaX, aDeltaY

@@ +424,5 @@
>     * Helper methods for handling scroll wheel events.
>     */
>    nsEventStatus OnScrollWheel(const ScrollWheelInput& aEvent);
>  
> +  void GetScrollWheelDelta(const ScrollWheelInput& aEvent, double& aDeltaX, double& aDeltaY) const;

aOutDeltaX, aOutDeltaY

@@ +790,5 @@
>     */
>    void ResetInputState();
>  
> +  // Returns whether overscroll is allowed during a wheel event.
> +  bool AllowScrollHandoffInWheelTransaction() const;

Move this declaration to the section for scrolling, starts around line 878 in current m-c.

::: gfx/layers/apz/src/Axis.cpp
@@ +117,5 @@
>    mAxisLocked = false;
>  }
>  
> +bool Axis::CanScroll(double aDelta) const
> +{

Please move this function to be in between the CanScroll() and CanScrollNow() functions in this file, so related functions aren't scattered all over the place.

::: gfx/layers/apz/src/InputBlockState.cpp
@@ +154,5 @@
>  WheelBlockState::WheelBlockState(const nsRefPtr<AsyncPanZoomController>& aTargetApzc,
> +                                 bool aTargetConfirmed,
> +                                 const ScrollWheelInput& aInitialEvent)
> +  : CancelableBlockState(aTargetApzc, aTargetConfirmed),
> +    mTransactionEnded(false)

move comma to the next line under the :

@@ +178,2 @@
>  {
> +  mLastScrollDirection = gfxPoint(aEvent.mDeltaX, aEvent.mDeltaY);

mLastScrollDirection doesn't appear to be read anywhere, in this patch or the next one. Can we get rid of it? If I missed something and we can't get rid of it, we should use gfx::Point or one of the typed points instead.

@@ +254,5 @@
> +
> +  // If there's a recent mouse movement, we can time out the transaction early.
> +  if (!mLastMouseMove.IsNull()) {
> +    TimeDuration duration = TimeStamp::Now() - mLastMouseMove;
> +    if (duration.ToMilliseconds() > gfxPrefs::MouseWheelIgnoreMoveDelayMs()) {

This pref is only added in the second patch, it needs to move into this one

@@ +263,5 @@
> +  // If we're in a transaction, but we can't scroll the target apz anymore,
> +  // then we want to end the transaction if the event occurred > 1.5s after
> +  // the most recently seen wheel event.
> +  TimeDuration duration = aEvent.mTimeStamp - mLastEventTime;
> +  if (duration.ToMilliseconds() >= gfxPrefs::MouseWheelTransactionTimeoutMs()) {

The comment here says "but we can't scroll the target apz anymore" but the code doesn't check for that. Can you clarify?

::: gfx/layers/apz/src/InputBlockState.h
@@ +176,5 @@
>    }
>  
> +  // Returns whether or not the block should accept new events. If the APZC
> +  // has been destroyed, or the block is not part of a wheel transaction, then
> +  // this will return false.

Please use /** ... */ comments on methods for consistency. @return would be nice too.

@@ +177,5 @@
>  
> +  // Returns whether or not the block should accept new events. If the APZC
> +  // has been destroyed, or the block is not part of a wheel transaction, then
> +  // this will return false.
> +  bool AcceptNewEvent(const ScrollWheelInput& aEvent) const;

Would prefer this renamed to ShouldAcceptNewEvent

@@ +188,5 @@
> +
> +  // Mark the block as no longer participating in a wheel transaction. This
> +  // will force future wheel events to begin a new input block.
> +  void EndTransaction() {
> +    mTransactionEnded = true;

Move body to .cpp file

@@ +194,5 @@
> +
> +  // Returns whether or not overscrolling is prevented for this wheel block.
> +  bool AllowScrollHandoff() const;
> +
> +  void Update(const ScrollWheelInput& aEvent);

This function can be made private.

::: gfx/layers/apz/src/InputQueue.h
@@ +98,5 @@
> +  /**
> +   * If there is a wheel transaction, returns the WheelBlockState representing
> +   * the transaction. Otherwise, returns null.
> +   */
> +  WheelBlockState* CurrentWheelTransaction() const;

Since this can return null, rename it to GetCurrentWheelTransaction()

Comment 18

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

Comment on attachment 8578428 [details] [diff] [review]
part 2, end transactions

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

::: gfx/layers/apz/src/APZCTreeManager.cpp
@@ +819,5 @@
> +     if (!mouseEvent->IsReal()) {
> +       return;
> +     }
> +
> +     txn->OnMouseMove(aEvent.refPoint);

Instead of doing it this way, how about instead we do:

nsRefPtr<AsyncPanZoomController> target = GetTargetAPZC(aEvent.refPoint, nullptr);
if (target == txn->GetTargetAPZC()) {
  txn->OnMouseMove();
}

That would get rid of the Contains check. It seems not-so-nice to add that when we have perfectly good hit-testing available right here. If we need to walk up the tree a little bit that's fine too, it would still avoid having to add the Contains method.

Comment 19

[David Anderson [:dvander] - inactive, e-mail if emergency]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #17)
> @@ +178,2 @@
> >  {
> > +  mLastScrollDirection = gfxPoint(aEvent.mDeltaX, aEvent.mDeltaY);
> 
> mLastScrollDirection doesn't appear to be read anywhere, in this patch or
> the next one. Can we get rid of it? If I missed something and we can't get
> rid of it, we should use gfx::Point or one of the typed points instead.

Yup it's unused.

> @@ +254,5 @@
> > +
> > +  // If there's a recent mouse movement, we can time out the transaction early.
> > +  if (!mLastMouseMove.IsNull()) {
> > +    TimeDuration duration = TimeStamp::Now() - mLastMouseMove;
> > +    if (duration.ToMilliseconds() > gfxPrefs::MouseWheelIgnoreMoveDelayMs()) {
> 
> This pref is only added in the second patch, it needs to move into this one

Whoops, that chunk was supposed to be in the second patch.

> @@ +263,5 @@
> > +  // If we're in a transaction, but we can't scroll the target apz anymore,
> > +  // then we want to end the transaction if the event occurred > 1.5s after
> > +  // the most recently seen wheel event.
> > +  TimeDuration duration = aEvent.mTimeStamp - mLastEventTime;
> > +  if (duration.ToMilliseconds() >= gfxPrefs::MouseWheelTransactionTimeoutMs()) {
> 
> The comment here says "but we can't scroll the target apz anymore" but the
> code doesn't check for that. Can you clarify?

Removed that phrase, it was outdated.

Comment 20

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 1, w/ nits fixed

Comment 21

[David Anderson [:dvander] - inactive, e-mail if emergency]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #18)
> Comment on attachment 8578428 [details] [diff] [review]
> part 2, end transactions
> 
> Review of attachment 8578428 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/layers/apz/src/APZCTreeManager.cpp
> @@ +819,5 @@
> > +     if (!mouseEvent->IsReal()) {
> > +       return;
> > +     }
> > +
> > +     txn->OnMouseMove(aEvent.refPoint);
> 
> Instead of doing it this way, how about instead we do:
> 
> nsRefPtr<AsyncPanZoomController> target = GetTargetAPZC(aEvent.refPoint,
> nullptr);
> if (target == txn->GetTargetAPZC()) {
>   txn->OnMouseMove();
> }
> 
> That would get rid of the Contains check. It seems not-so-nice to add that
> when we have perfectly good hit-testing available right here. If we need to
> walk up the tree a little bit that's fine too, it would still avoid having
> to add the Contains method.

The reason I didn't do the hit test is to replicate this check:
 https://dxr.mozilla.org/mozilla-central/source/dom/events/WheelHandlingHelper.cpp#194

Which I think will consider the cursor inside the frame even if it's also technically inside an inner frame. I could walk up the apzc parent chain, but then I might catch apzcs that are technically outside the real apzc's bounds.

Comment 22

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

Comment on attachment 8578428 [details] [diff] [review]
part 2, end transactions

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

::: gfx/layers/apz/src/AsyncPanZoomController.cpp
@@ +1789,5 @@
> +    GetFrameMetrics().mCompositionBounds.ToIntRect(&cb);
> +  }
> +
> +  LayoutDeviceIntRect bounds = TransformTo<LayoutDevicePixel>(apzcToGecko, cb);
> +  return bounds.Contains(aPoint);

I'm not sure if this conversion/check is correct. I'm bouncing this to Botond for now since I have to head out but I can take a proper look at it tomorrow if he doesn't get to it first. Sorry for the lag! Rest of the patch looks fine.

Comment 23

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

Comment on attachment 8578428 [details] [diff] [review]
part 2, end transactions

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

::: gfx/layers/apz/src/AsyncPanZoomController.cpp
@@ +1789,5 @@
> +    GetFrameMetrics().mCompositionBounds.ToIntRect(&cb);
> +  }
> +
> +  LayoutDeviceIntRect bounds = TransformTo<LayoutDevicePixel>(apzcToGecko, cb);
> +  return bounds.Contains(aPoint);

Ok, I convinced myself that this check is wrong. At this point even though aPoint and bounds are in "LayoutDevice" space (after each having gone through a GetApzcToGeckoTransform transformation), I don't think they're actually in the same coordinate space. At the very least they're relative to different origins. For instance, bounds is still subject to transforms on the ancestor layers, and I don't think that's accounted for. In the normal hit-test code path that gets accounted for at [1]. I'm not yet sure what the correct code would be, but I strongly suspect you'd want to use the point in APZ space, rather than in Gecko space. I'll think about it a bit more but leave the flag for Botond as well.

[1] http://mxr.mozilla.org/mozilla-central/source/gfx/layers/apz/src/APZCTreeManager.cpp?rev=78dcb3a426f2#1338

Comment 24

[Botond Ballo [:botond]]

Comment on attachment 8578428 [details] [diff] [review]
part 2, end transactions

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

::: gfx/layers/apz/src/AsyncPanZoomController.cpp
@@ +1789,5 @@
> +    GetFrameMetrics().mCompositionBounds.ToIntRect(&cb);
> +  }
> +
> +  LayoutDeviceIntRect bounds = TransformTo<LayoutDevicePixel>(apzcToGecko, cb);
> +  return bounds.Contains(aPoint);

Kats, I'm not sure I follow this part:

  For instance, bounds is still subject to transforms on the ancestor layers, 
  and I don't think that's accounted for. In the normal hit-test code path 
  that gets accounted for at [1].

The linked code unapplies a single layer's worth of transforms (CSS and async) on the way from the root layer (screen coordinates) to the target APZC.

Here, those transformations have already happened to |aPoint| in APZCTreeManager::ProcessEvent(), which calls UpdateWheelTransaction() *after* transforming aEvent.refPoint (and then, both aEvent.refPoint and the composition bounds undergo a further transformation from APZC space to Gecko space, in ProcessEvent() and this function respectively).

Therefore, I believe this comparison is correct. Kats, please let me know if I missed something about your objection.

That said, it does feel a bit strange to choose Gecko space as the coordinate space in which to perform this comparison. It would feel more natural to me to perform the comparison in APZC space, which would mean 1) not transforming the composition bounds in any way, and 2) calling UpdateWheelTransaction() at a point when the screen -> apzc transform has been applied to aEvent.refPoint, but the apzc -> gecko transform hasn't yet.

Alternatively, we could do what Kats suggested above and perform hit testing (by calling GetTargetApzc(refPoint)) and compare the APZCs, instead of just testing against the composition bounds. Note that in this case, we want to call UpdateWheelTransaction() before even applying the screen -> apzc transform, as GetTargetApzc(point) expects a ScreenPoint.

The difference between these two alternatives, is whether we do full hit testing, or just testing against the composition bounds. Do we know which we want?

::: gfx/layers/apz/src/AsyncPanZoomController.h
@@ +354,5 @@
>     */
>    ParentLayerPoint ToParentLayerCoordinates(const ScreenPoint& aVector,
>                                              const ScreenPoint& aAnchor) const;
>  
> +  // Returns whether or not this apzc contains the given screen point.

Please be more specific: "Returns whether or not the given screen point is within this apzc's composition bounds."

Comment 25

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

(In reply to Botond Ballo [:botond] from comment #24)
> That said, it does feel a bit strange to choose Gecko space as the
> coordinate space in which to perform this comparison. It would feel more
> natural to me to perform the comparison in APZC space, which would mean 1)
> not transforming the composition bounds in any way, and 2) calling
> UpdateWheelTransaction() at a point when the screen -> apzc transform has
> been applied to aEvent.refPoint, but the apzc -> gecko transform hasn't yet.

This version makes sense to me... and after thinking about this and staring at the code some more I guess in the patch both composition bounds and the point just have an extra apzc -> gecko transform applied on top of this, so it must be equivalent. I think mentally I was getting hung up on the case where there's no transient async transform, in which case the point basically doesn't get transformed at all, and I couldn't figure out what applying the "gecko transform" to the composition bounds would do.

Comment 26

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

Comment on attachment 8578428 [details] [diff] [review]
part 2, end transactions

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

Ok, changing to a r+ but if it's not too hard to do what Botond suggested in comment 24 (comparison in APZ space) then I would prefer that as it's much easier to reason about.

Comment 27

[David Anderson [:dvander] - inactive, e-mail if emergency]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #26)
> Ok, changing to a r+ but if it's not too hard to do what Botond suggested in
> comment 24 (comparison in APZ space) then I would prefer that as it's much
> easier to reason about.

What does "APZ space" mean exactly? If the refpoint has been transformed to the hit tested apz's coordinate space, is it valid to compare that point within the input block's apz's coordinate space?

Attaching a new patch (w/out that change, yet) since I failed at replicating the main thread behavior in the first one.

 * A new wheel transaction times out after 1500ms. Timeouts are checked when any mouse or keyboard event occurs.
 * A mouse move outside an input block's apzc ends a transaction.
 * If a wheel event occurs 100ms after a mouse move inside an input block's apzc, the transaction is ended.
 * A key press, mouse button press/click, context menu or drop operation ends a transaction.
 * If an event will successfully scroll an apzc within a transaction, the transaction's timeout is reset. This means a transaction can timeout if the user is continuously scrolling a frame with no more scrollable area, but is not moving the mouse.

And:
 * A transaction is only started if the apzc can be scrolled by the event.

Comment 28

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 2, end transactions v2

Comment 29

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 3, test support

Add support for test.mousescroll, which is needed by the wheel transaction tests (and one other random test). I'm not a huge fan of this, but I don't see an easy way to write these tests without some kind of notification.

Comment 30

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 2, end transactions v3

Comment 31

[David Anderson [:dvander] - inactive, e-mail if emergency]

Comment on attachment 8579880 [details] [diff] [review]
part 2, end transactions v3

Botond could you check the new Contains() logic?

Comment 32

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

Comment on attachment 8579871 [details] [diff] [review]
part 3, test support

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

::: gfx/layers/apz/src/InputBlockState.cpp
@@ +305,5 @@
>    TBS_LOG("%p wheel transaction timed out\n", this);
>  
> +  if (gfxPrefs::MouseScrollTestingEnabled()) {
> +    nsRefPtr<AsyncPanZoomController> apzc = GetTargetApzc();
> +    if (nsRefPtr<GeckoContentController> controller = apzc->GetGeckoContentController()) {

Leave GetGeckoContentController private in AsyncPanZoomController, and add a separate public method to send this notification. Something like apzc->SendMozMouseScrollEvent(NS_LITERAL_STRING(...)). You can reuse that one at the other call site in AsyncPanZoomController.cpp.

Comment 33

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

Comment on attachment 8579880 [details] [diff] [review]
part 2, end transactions v3

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

Partial review. Patch looks good - no major issues but I want to spend some more time understanding how all the timing stuff works.

::: gfx/layers/apz/src/APZCTreeManager.cpp
@@ +825,5 @@
> +     if (!mouseEvent->IsReal()) {
> +       return;
> +     }
> +
> +     txn->OnMouseMove(aEvent.refPoint);

Here pass |ViewAs<ScreenIntPoint>(aEvent.refPoint, PixelCastJustification::LayoutDeviceToScreenForUntransformedEvent)| as the argument. You'll have to add LayoutDeviceToScreenForUntransformedEvent to the PixelCastJustification enum in layout/base/UnitTransforms.h, and also change the OnMouseMove and APZC::Contains functions to take ScreenIntPoint as the param instead of a LayoutDeviceIntPoint.

@@ +866,5 @@
>      Matrix4x4 transformToGecko = GetApzcToGeckoTransform(apzc);
>      Matrix4x4 outTransform = transformToApzc * transformToGecko;
>      aEvent.refPoint = TransformTo<LayoutDevicePixel>(outTransform, aEvent.refPoint);
>    }
> +

Remove blank line

::: gfx/layers/apz/src/AsyncPanZoomController.h
@@ +356,5 @@
>                                              const ScreenPoint& aAnchor) const;
>  
> +  // Returns whether or not this apzc contains the given screen point within
> +  // its composition bounds.
> +  bool Contains(const LayoutDeviceIntPoint& aPoint) const;

Move this function declaration down to around line 973, into the section for hit-testing (where Set/GetAncestorTransform live).

::: gfx/layers/apz/src/InputBlockState.cpp
@@ +261,5 @@
>    return "scroll wheel";
>  }
>  
>  bool
> +WheelBlockState::ShouldAcceptNewEvent(const ScrollWheelInput& aEvent)

I don't like that this function isn't const anymore; conceptually it doesn't seem like it should have side-effects. Can we move the MaybeTimeout call out of this function, and have the call site (which calls ShouldAcceptNewEvent) invoke that function directly?

Comment 34

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 2, end transactions v4

Cleaner version, with comment #33 nits addressed.

Comment 35

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

Comment on attachment 8580229 [details] [diff] [review]
part 2, end transactions v4

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

::: gfx/layers/apz/src/InputBlockState.cpp
@@ +203,2 @@
>    mLastEventTime = aEvent.mTimeStamp;
> +  mLastMouseMove = TimeStamp();

It took me forever to realize that this resets mLastMouseMove back to 0 so IsNull() on it returns true. Maybe add an inline comment "// reset move timer" to make it more obvious

@@ +216,1 @@
>    CancelableBlockState::DispatchImmediate(aEvent);

We can get rid of this override now that it does nothing but call the base class. And we can make it const again, I think.

Comment 36

[Botond Ballo [:botond]]

Comment on attachment 8580229 [details] [diff] [review]
part 2, end transactions v4

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

Looks good to me from a coordinate-systems perspective.

::: gfx/layers/apz/src/AsyncPanZoomController.cpp
@@ +1776,5 @@
>  }
>  
> +bool AsyncPanZoomController::Contains(const ScreenIntPoint& aPoint) const
> +{
> +  APZCTreeManager* treeManagerLocal = GetApzcTreeManager();

I don't think you need this variable any more.

Comment 37

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

Heads-up: I just landed bug 1142260 on inbound, so please make sure the new prefs you're adding are in alphabetical order.

Comment 38

[David Anderson [:dvander] - inactive, e-mail if emergency]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #37)
> Heads-up: I just landed bug 1142260 on inbound, so please make sure the new
> prefs you're adding are in alphabetical order.

Ok, but I'll keep all the mouse-related events together.

Comment 39

[David Anderson [:dvander] - inactive, e-mail if emergency]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/e24bb39a0c1b
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/841bf9082501
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/78a291fd62b3

w/ comments addressed

Comment 40

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e24bb39a0c1b
https://hg.mozilla.org/mozilla-central/rev/841bf9082501
https://hg.mozilla.org/mozilla-central/rev/78a291fd62b3

Comment 41

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

(In reply to David Anderson [:dvander] from comment #38)
> (In reply to Kartikaya Gupta (email:[e-mail]) from comment #37)
> > Heads-up: I just landed bug 1142260 on inbound, so please make sure the new
> > prefs you're adding are in alphabetical order.
> 
> Ok, but I'll keep all the mouse-related events together.

That.. is self-contradictory. The prefs are meant to be sorted globally in gfxPrefs.h so that we don't end up adding the same pref multiple times. Grouping of related prefs should happen automatically if they're named properly.

Comment 42

[David Anderson [:dvander] - inactive, e-mail if emergency]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #41)
> (In reply to David Anderson [:dvander] from comment #38)
> > (In reply to Kartikaya Gupta (email:[e-mail]) from comment #37)
> > > Heads-up: I just landed bug 1142260 on inbound, so please make sure the new
> > > prefs you're adding are in alphabetical order.
> > 
> > Ok, but I'll keep all the mouse-related events together.
> 
> That.. is self-contradictory. The prefs are meant to be sorted globally in
> gfxPrefs.h so that we don't end up adding the same pref multiple times.
> Grouping of related prefs should happen automatically if they're named
> properly.

They ain't!