Markus Stange [:mstange] Reporter
	Description • 10 years ago

Main thread wheel event handling has something called WheelTransaction, which directs wheel event to the appropriate scrollable frame.
APZ wheel handling has something called scroll handoff, which automatically causes scrolling to resume at an ancestor scrollable frame when an inner scrollable frame has been scrolled to the edge.

The biggest difference between them is that APZ does eager handoff, whereas WheelTransaction tries to keep scrolling locked to one frame until the mouse moves or the transaction times out.

We need APZ scrolling to do something similar to these WheelTransactions.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 1 • 10 years ago

Stolen from an IRC conversation I had with dvander about support for wheel events on windows:

--
The way I envision dealing with scrollwheel events is like this:

1) extract another class from TouchBlockState, maybe "CancelableBlockState" that extends from InputBlockState and that TouchBlockState extends. move stuff dealing with content response into this class from TouchBlockState
2) In InputQueue::ReceiveInputEvent, when getting wheel events create a new CancelableBlockState and put the wheel event into it
3) update widget code to also dispatch the wheel event to content and send the various notifications to the APZ code

In step 2 it may be possible to create a single input block for a sequence of wheel events that arrive close together that may improve performance without sacrificing too much of the control that web content currently has.
--

The "single input block for a sequence of wheel events" sounds like the same idea as a WheelTransaction, so we should use this bug to do that work.

	David Anderson [:dvander] - inactive, e-mail if emergency Assignee
	Comment 11 • 10 years ago

This is mostly just shuffling code around and splitting functions apart for re-use. Some functions on InputBlockState are now virtual since they'll behave differently for wheel events.

The "MustStayActive" call is because wheel events don't seem to need the same restriction.

	David Anderson [:dvander] - inactive, e-mail if emergency Assignee
	Comment 12 • 10 years ago

Comment on attachment 8528220 [details] [diff] [review]
part 4, add wheel events to the input queue

This creates a new WheelBlockState class for wheel events, and hooks it up to the InputQueue.

	David Anderson [:dvander] - inactive, e-mail if emergency Assignee
	Comment 13 • 10 years ago

This adds both event-region and non-event-region support for scroll hit testing. For non-event-region hit testing, it re-uses the touch listener flag on FrameMetrics since that'll go away soon anyway.

tn, not sure if you're the right reviewer for this, feel free to redirect/unflag if not.

	David Anderson [:dvander] - inactive, e-mail if emergency Assignee
	Comment 14 • 10 years ago

This splits apart SendSetTargetAPZCNotification into two helpers, so it can then be re-used by RecvMouseWheelEvent.

	David Anderson [:dvander] - inactive, e-mail if emergency Assignee
	Comment 15 • 10 years ago

Comment on attachment 8529383 [details] [diff] [review]
part 7, move wheel handling from nsWindow to RenderFrameParent

For now it's simpler to handle the event in TabParent rather than the widget itself, so we can use the existing GeckoContentController.

	Timothy Nikkel (:tnikkel)
	Comment 16 • 10 years ago

Comment on attachment 8529429 [details] [diff] [review]
part 5, add scroll listeners to dispatch-to-content regions

A DOM peer should review the dom stuff. Also, I could be wrong but I think you need an iid bump in nsPIDOMWindow.h.

>@@ -765,17 +766,17 @@ nsDisplayScrollLayer::ComputeFrameMetrics(nsIFrame* aForFrame,
>-        metrics.SetMayHaveTouchListeners(innerWin->HasTouchEventListeners());
>+        metrics.SetMayHaveTouchListeners(innerWin->HasApzAwareEventListeners());

SetMayHaveTouchListeners on FrameMetrics meaning HasApzAwareEventListeners when HasApzAwareEventListeners also means "HasTouchEventListeners() || HasScrollWheelEventListeners" is going to be confusing.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 17 • 10 years ago

Comment on attachment 8528155 [details] [diff] [review]
part 1, factor out cancel/content state

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

::: gfx/layers/apz/src/InputBlockState.cpp
@@ +108,5 @@
>  
>  bool
> +CancelableBlockState::IsDefaultPrevented() const
> +{
> +  MOZ_ASSERT(mContentResponded || mContentResponseTimerExpired);

MOZ_ASSERT(ContentRespondedOrTimedOut()); (or whatever you rename the function to...)

::: gfx/layers/apz/src/InputBlockState.h
@@ +82,5 @@
> +
> +  /**
> +   * @returns true if web content responded or timed out.
> +   */
> +  bool ContentRespondedOrTimedOut() const;

nit: I would really like to call this function IsReadyForHandling() and have it virtual so that the one in TouchBlockState overrides it. Conceptually that feels correct to me, but it might be messy to make this virtual. In that case we can call it IsReady(). Either of those is fine by me - I'll leave it to you. I don't like the current name because it doesn't feel like the right level of abstraction (too explicit).

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 18 • 10 years ago

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #17)
> nit: I would really like to call this function IsReadyForHandling() and have
> it virtual so that the one in TouchBlockState overrides it. Conceptually
> that feels correct to me, but it might be messy to make this virtual.

Since you make it virtual in part 2 anyway, please do this.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 19 • 10 years ago

Comment on attachment 8529423 [details] [diff] [review]
part 2, make input queue generic

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

This looks good! I have a bunch of mostly minor comments, but enough of them that I'd like to see the updated patch.

::: gfx/layers/apz/src/InputBlockState.h
@@ +40,2 @@
>    bool IsTargetConfirmed() const;
> +

I think this can stay protected, you don't seem to be calling it anywhere (in this patch at least).

@@ +95,5 @@
> +  /**
> +   * @return true iff this block has received all the information needed
> +   *         to properly dispatch the events in the block.
> +   */
> +  virtual bool IsReadyForHandling() const = 0;

As mentioned in a previous comment, roll ContentRespondedOrTimedOut into this function

@@ +100,5 @@
> +
> +  /**
> +   * Returns whether or not this block has pending events.
> +   */
> +  virtual bool HasEvents() const = 0;

I feel like all these new virtual functions you're adding properly belong on InputBlockState rather than CancelableBlockState. We can leave them here for now though and move them up when we implement the pan gesture stuff. It'll be annoying to move them up now since we do instantiate InputBlockState directly in AsyncPanZoomController.cpp

@@ +108,5 @@
> +   */
> +  virtual void DropEvents() = 0;
> +
> +  /**
> +   * Process and deplete all events given an apzc.

"Process all events given an apzc, leaving this block depleted."

@@ +119,5 @@
> +   */
> +  virtual bool MustStayActive() = 0;
> +
> +  /**
> +   * Return a descriptibe name for the block kind.

"descriptive"

::: gfx/layers/apz/src/InputQueue.cpp
@@ +77,5 @@
> +    CancelAnimationsForNewBlock(block);
> +    MaybeRequestContentResponse(aTarget, block);
> +  } else {
> +    if (!mPendingBlockQueue.IsEmpty())
> +      block = mPendingBlockQueue.LastElement().get()->AsTouchBlock();

braces around single-line blocks please

@@ +128,5 @@
> +    if (aBlock->GetOverscrollHandoffChain()->HasFastMovingApzc()) {
> +      // If we're already in a fast fling, then we want the touch event to stop the fling
> +      // and to disallow the touch event from being used as part of a fling.
> +      if (TouchBlockState* touch = aBlock->AsTouchBlock()) {
> +        touch->DisallowSingleTap();

This will need to be rebased on top of bug 1085404 but it should be straightforward

@@ +267,5 @@
>    bool success = false;
> +  for (size_t i = 0; i < mPendingBlockQueue.Length(); i++) {
> +    if (mPendingBlockQueue[i]->GetBlockId() == aInputBlockId) {
> +      TouchBlockState *block = mPendingBlockQueue[i]->AsTouchBlock();
> +      success = block->SetContentResponse(aPreventDefault);

Don't need to do AsTouchBlock() here since SetContentResponse is on the CancelableBlock class.

@@ +305,5 @@
>    bool success = false;
> +  for (size_t i = 0; i < mPendingBlockQueue.Length(); i++) {
> +    if (mPendingBlockQueue[i]->GetBlockId() == aInputBlockId) {
> +      TouchBlockState *block = mPendingBlockQueue[i]->AsTouchBlock();
> +      success = block->SetAllowedTouchBehaviors(aBehaviors);

Add a |if (!block) { NS_WARNING(...); return; }| thing here. It should never happen but we should guard against bad input where the block id passed in is for a non-touch block.

@@ -286,5 @@
>        target->ResetInputState();
>      } else {
> -      while (curBlock->HasEvents()) {
> -        target->HandleInputEvent(curBlock->RemoveFirstEvent());
> -      }

This means you can make RemoveFirstEvent() private in TouchBlockState

@@ +346,5 @@
> +    if (mPendingBlockQueue.Length() == 1 && curBlock->MustStayActive()) {
> +      // If |curBlock| is the only block in the queue, then it is still active
> +      // and we cannot remove it yet. We only know that the block is over
> +      // when we start a new one of the same type. This block will be removed
> +      // by the code in SweepDepletedBlocks, whenever a new block is added.

Update this comment:

"Some types of blocks (e.g. touch blocks) accumulate events until the next input block is started. Therefore we cannot remove the block from the queue until we have started another block. This block will be removed by the code in SweepDepletedBlocks whenever a new block is added."

::: gfx/layers/apz/src/InputQueue.h
@@ +78,5 @@
>    uint64_t InjectNewTouchBlock(AsyncPanZoomController* aTarget);
>    /**
> +   * Returns the pending input block at the head of the queue.
> +   */
> +  CancelableBlockState* CurrentPendingBlock() const;

I don't like the use of the word "pending" because it makes it sound like it hasn't been processed at all (which admittedly is what my documentation says). In fact the block returned here may have been partially processed already.

So I'd like to call this function "CurrentBlock()" and remove all instances of the word "pending" in this file.

@@ +122,5 @@
> +
> +  /**
> +   * Processes the current block if it's ready for handling.
> +   */
> +  bool MaybeHandleCurrentPendingBlock(const nsRefPtr<AsyncPanZoomController>& aTarget,

Rename to MaybeHandleCurrentBlock

@@ +131,5 @@
>    void MainThreadTimeout(const uint64_t& aInputBlockId);
>    void ProcessPendingInputBlocks();
>  
>  private:
>    // The queue of touch blocks that have not yet been processed.

Update this comment to be ... "not yet been fully processed"

@@ +133,5 @@
>  
>  private:
>    // The queue of touch blocks that have not yet been processed.
>    // This member must only be accessed on the controller/UI thread.
> +  nsTArray<UniquePtr<CancelableBlockState>> mPendingBlockQueue;

Rename to mInputBlockQueue

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 20 • 10 years ago

Comment on attachment 8528220 [details] [diff] [review]
part 4, add wheel events to the input queue

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

::: gfx/layers/apz/src/InputBlockState.cpp
@@ +136,5 @@
> +{
> +  if (!IsTargetConfirmed()) {
> +    return false;
> +  }
> +  if (!ContentRespondedOrTimedOut()) {

This should turn into CancelableBlockState::IsReadyForHandling() after the changes from part 1.

Also it occurs to me that we should probably rename InputBlockState::IsTargetConfirmed() to InputBlockState::IsReadyForHandling() and chain these things better but we can do that later (again, probably when we do pan gesture stuff). Just making a note here in case I forget.

::: gfx/layers/apz/src/InputBlockState.h
@@ +17,5 @@
>  class AsyncPanZoomController;
>  class OverscrollHandoffChain;
>  class CancelableBlockState;
> +class TouchBlockState;
> +class WheelBlockState;

The forward-declare of TouchBlockState should probably be moved into part 2.

@@ +139,5 @@
> + * A single block of wheel events.
> + */
> +class WheelBlockState : public CancelableBlockState
> +{
> + public:

nit: public/private keywords should be aligned with the class-level braces. I just noticed you did this in CancelableBlockState as well.

::: gfx/layers/apz/src/InputQueue.cpp
@@ +152,5 @@
> +  if (!MaybeHandleCurrentPendingBlock(target, block, aEvent)) {
> +    block->AddEvent(aEvent.AsScrollWheelInput());
> +  }
> +
> +  return nsEventStatus_eIgnore;

return nsEventStatus_eConsumeDoDefault here since we are not sure whether or not the event will be used (see documentation on APZCTreeManager::ReceiveInputEvent).

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 21 • 10 years ago

Comment on attachment 8529429 [details] [diff] [review]
part 5, add scroll listeners to dispatch-to-content regions

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

(In reply to Timothy Nikkel (:tn) from comment #16)
> SetMayHaveTouchListeners on FrameMetrics meaning HasApzAwareEventListeners
> when HasApzAwareEventListeners also means "HasTouchEventListeners() ||
> HasScrollWheelEventListeners" is going to be confusing.

Yeah but since we're going to be getting rid of the mMayHaveTouchListeners flag from FrameMetrics entirely (once the event-regions hit-testing is baked) I'm ok with leaving it this way. Although it might be worth throwing in a comment in FrameMetrics.h on the mMayHaveTouchListeners field saying that it will also be true when wheel listeners are present.

::: gfx/layers/apz/src/APZCTreeManager.cpp
@@ +610,5 @@
>            TransformTo<ParentLayerPixel>(transformToApzc, wheelInput.mOrigin);
>  
>          result = mInputQueue->ReceiveInputEvent(
>            apzc,
> +          /* aTargetConfirmed = */ hitResult == ApzcHitRegion,

heh, whoops :)

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 22 • 10 years ago

Comment on attachment 8529432 [details] [diff] [review]
part 6, add mouse wheel transaction support

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

Looks good! I'm planning on moving a bunch of this code into APZCCallbackHelper in the future but this is fine for now.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 23 • 10 years ago

Comment on attachment 8529383 [details] [diff] [review]
part 7, move wheel handling from nsWindow to RenderFrameParent

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

Hmm.. this is actually moving the opposite direction of what I plan to be doing in bug 920036. That is, instead of routing events to the gecko root process and *then* to the APZCTreeManager, I want to route events through the APZCTreeManager *first* and then to gecko root/child processes. That being said I understand that you probably need this in place for now for things to work more correctly. Just keep in mind that this will probably be changed back later and I would avoid relying too much on this event flow.

::: gfx/layers/apz/src/APZCTreeManager.cpp
@@ +904,5 @@
> +                                   uint64_t* aOutInputBlockId)
> +{
> +  ScrollWheelInput::ScrollMode scrollMode = ScrollWheelInput::SCROLLMODE_INSTANT;
> +  if (Preferences::GetBool("general.smoothScroll"))
> +    scrollMode = ScrollWheelInput::SCROLLMODE_SMOOTH;

braces

@@ +916,5 @@
> +                         aEvent.lineOrPageDeltaY);
> +
> +  nsEventStatus status = ReceiveInputEvent(input, aOutTargetGuid, aOutInputBlockId);
> +  aEvent.refPoint.x = input.mLocalOrigin.x;
> +  aEvent.refPoint.y = input.mLocalOrigin.y;

These should be using input.mOrigin instead of input.mLocalOrigin.

::: widget/windows/nsWindow.cpp
@@ -3739,5 @@
>    DispatchEvent(event, status);
>    return ConvertStatus(status);
>  }
>  
> -nsEventStatus nsWindow::MaybeDispatchAsyncWheelEvent(WidgetGUIEvent* aEvent)

You can also remove the #include "InputData.h" that you added to this file in bug 1086162.

	David Anderson [:dvander] - inactive, e-mail if emergency Assignee
	Comment 24 • 10 years ago

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #23)
> Comment on attachment 8529383 [details] [diff] [review]
> part 7, move wheel handling from nsWindow to RenderFrameParent
> 
> Review of attachment 8529383 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Hmm.. this is actually moving the opposite direction of what I plan to be
> doing in bug 920036.

Yeah, I gathered that... I was going for "fastest thing to get working". I'd like to do the right thing as a follow-up since ideally we'd take the events off the widget as soon as possible.

	David Anderson [:dvander] - inactive, e-mail if emergency Assignee
	Comment 25 • 10 years ago

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #19)
> Comment on attachment 8529423 [details] [diff] [review]
> part 2, make input queue generic
> 
> Review of attachment 8529423 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks good! I have a bunch of mostly minor comments, but enough of them
> that I'd like to see the updated patch.
> 
> ::: gfx/layers/apz/src/InputBlockState.h
> @@ +40,2 @@
> >    bool IsTargetConfirmed() const;
> > +
> 
> I think this can stay protected, you don't seem to be calling it anywhere
> (in this patch at least).

In part 4 it gets used to determine whether or not it should wait for a content response - but maybe that could be IsReadyForHandling?

> @@ +305,5 @@
> >    bool success = false;
> > +  for (size_t i = 0; i < mPendingBlockQueue.Length(); i++) {
> > +    if (mPendingBlockQueue[i]->GetBlockId() == aInputBlockId) {
> > +      TouchBlockState *block = mPendingBlockQueue[i]->AsTouchBlock();
> > +      success = block->SetAllowedTouchBehaviors(aBehaviors);
> 
> Add a |if (!block) { NS_WARNING(...); return; }| thing here. It should never
> happen but we should guard against bad input where the block id passed in is
> for a non-touch block.

We'd have already died earlier calling GetBlockId() if it were null - I could test earlier (or warn if no block id was found, but that could pseudo-race with apz timing out a block).

> I don't like the use of the word "pending" because it makes it sound like it
> hasn't been processed at all (which admittedly is what my documentation
> says). In fact the block returned here may have been partially processed
> already.
> 
> So I'd like to call this function "CurrentBlock()" and remove all instances
> of the word "pending" in this file.

I like that much better too.

	David Anderson [:dvander] - inactive, e-mail if emergency Assignee
	Comment 26 • 10 years ago

w/ most comments addressed

(thanks for the fast reviews, btw!)

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 27 • 10 years ago

(In reply to David Anderson [:dvander] from comment #25)
> > >    bool IsTargetConfirmed() const;
> > > +
> > 
> > I think this can stay protected, you don't seem to be calling it anywhere
> > (in this patch at least).
> 
> In part 4 it gets used to determine whether or not it should wait for a
> content response - but maybe that could be IsReadyForHandling?

Yeah, we can call it IsReadyForHandling then.

> > @@ +305,5 @@
> > >    bool success = false;
> > > +  for (size_t i = 0; i < mPendingBlockQueue.Length(); i++) {
> > > +    if (mPendingBlockQueue[i]->GetBlockId() == aInputBlockId) {
> > > +      TouchBlockState *block = mPendingBlockQueue[i]->AsTouchBlock();
> > > +      success = block->SetAllowedTouchBehaviors(aBehaviors);
> > 
> > Add a |if (!block) { NS_WARNING(...); return; }| thing here. It should never
> > happen but we should guard against bad input where the block id passed in is
> > for a non-touch block.
> 
> We'd have already died earlier calling GetBlockId() if it were null - I
> could test earlier (or warn if no block id was found, but that could
> pseudo-race with apz timing out a block).

Not necessarily. The case I'm concerned about is that mPendingBlockQueue[i] is non-null (and so GetBlockId() works fine) but it's not a touch block and so AsTouchBlock() returns null. This is a plausible scenario if somebody calls SetAllowedTouchBehaviours with a block id that corresponds to a wheel block, for example.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 28 • 10 years ago

Comment on attachment 8530748 [details] [diff] [review]
bug1013432-part2.patch

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

r+ with stuff in comment 27 addressed. Thanks!

	David Anderson [:dvander] - inactive, e-mail if emergency Assignee
	Comment 29 • 10 years ago

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/0f96a3aa598c
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/6a9fef0398d3
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/897cea69a39f
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/d0bef02fe977
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/89858cf28204
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/5493ccef57ca
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/a7f3fbbc6dbc

	Ryan VanderMeulen [:RyanVM]
	Comment 30 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/0f96a3aa598c
https://hg.mozilla.org/mozilla-central/rev/6a9fef0398d3
https://hg.mozilla.org/mozilla-central/rev/897cea69a39f
https://hg.mozilla.org/mozilla-central/rev/d0bef02fe977
https://hg.mozilla.org/mozilla-central/rev/89858cf28204
https://hg.mozilla.org/mozilla-central/rev/5493ccef57ca
https://hg.mozilla.org/mozilla-central/rev/a7f3fbbc6dbc

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 31 • 10 years ago

This landing regressed bug 1085404/bug 1105836 due to a rebase error. I have a patch locally to fix it, will land it once the tree is open.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 32 • 10 years ago

Fix the rebase error:

https://hg.mozilla.org/integration/mozilla-inbound/rev/a8e3b6311afa

	Mark Banner (:standard8)
	Comment 33 • 10 years ago

This changeset created a file "InputBlockState.h.orig":

https://hg.mozilla.org/mozilla-central/rev/89858cf28204

I'm assuming that wasn't intentional, but should the file have been something else, or can we just remove it?

	Botond Ballo [:botond]
	Comment 34 • 10 years ago

(In reply to Mark Banner (:standard8) from comment #33)
> This changeset created a file "InputBlockState.h.orig":
> 
> https://hg.mozilla.org/mozilla-central/rev/89858cf28204
> 
> I'm assuming that wasn't intentional, but should the file have been
> something else, or can we just remove it?

That's a file created by mercurial when it runs into a merge conflict. It must have been added to source control by a accident. It can be removed.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 35 • 10 years ago

(In reply to Mark Banner (:standard8) (away until 15th Dec) from comment #33)
> This changeset created a file "InputBlockState.h.orig":
> 
> https://hg.mozilla.org/mozilla-central/rev/89858cf28204
> 
> I'm assuming that wasn't intentional, but should the file have been
> something else, or can we just remove it?

Thanks for the heads-up! I landed a patch to remove it:

https://hg.mozilla.org/integration/mozilla-inbound/rev/3eef539e4553

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 36 • 10 years ago

... and a third follow-up to this bug to fix a build error when INPQ_LOG is enabled.

https://hg.mozilla.org/integration/mozilla-inbound/rev/e114f15ec1bd

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 37 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/a8e3b6311afa
https://hg.mozilla.org/mozilla-central/rev/3eef539e4553
https://hg.mozilla.org/mozilla-central/rev/e114f15ec1bd