Closed Bug 1013432 Opened 11 years ago Closed 10 years ago

APZ needs mouse wheel transactions

Categories

(Core :: Panning and Zooming, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla37

People

(Reporter: mstange, Assigned: dvander)

References

Details

Attachments

(7 files, 6 obsolete files)

9.32 KB, patch
kats
: review+
Details | Diff | Splinter Review
32.77 KB, patch
kats
: review+
Details | Diff | Splinter Review
9.61 KB, patch
kats
: review+
Details | Diff | Splinter Review
7.35 KB, patch
kats
: review+
Details | Diff | Splinter Review
20.71 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
12.35 KB, patch
kats
: review+
Details | Diff | Splinter Review
28.77 KB, patch
kats
: review+
Details | Diff | Splinter Review
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.
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.
Assignee: nobody → dvander
Blocks: 1086162
Attachment #8528155 - Attachment description: bug1013432-part1.patch → part 1, factor out cancel/content state
Attached patch part 2, make input queue generic (obsolete) — Splinter Review
Attachment #8528156 - Attachment is obsolete: true
Attachment #8528155 - Flags: review?(bugmail.mozilla)
Attached patch part 2, make input queue generic (obsolete) — Splinter Review
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.
Attachment #8528203 - Attachment is obsolete: true
Attachment #8529423 - Flags: review?(bugmail.mozilla)
Attachment #8528213 - Flags: review?(bugmail.mozilla)
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.
Attachment #8528220 - Flags: review?(bugmail.mozilla)
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.
Attachment #8529429 - Flags: review?(tnikkel)
Attachment #8529429 - Attachment description: bug1013432-part5.patch → part 5, add scroll listeners to dispatch-to-content regions
Attachment #8529261 - Attachment is obsolete: true
This splits apart SendSetTargetAPZCNotification into two helpers, so it can then be re-used by RecvMouseWheelEvent.
Attachment #8529371 - Attachment is obsolete: true
Attachment #8529432 - Flags: review?(bugmail.mozilla)
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.
Attachment #8529383 - Flags: review?(bugmail.mozilla)
Status: NEW → ASSIGNED
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.
Attachment #8529429 - Flags: review?(tnikkel) → review+
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).
Attachment #8528155 - Flags: review?(bugmail.mozilla) → review+
(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.
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
Attachment #8529423 - Flags: review?(bugmail.mozilla) → feedback+
Attachment #8528213 - Flags: review?(bugmail.mozilla) → review+
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).
Attachment #8528220 - Flags: review?(bugmail.mozilla) → review+
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 :)
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.
Attachment #8529432 - Flags: review?(bugmail.mozilla) → review+
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.
Attachment #8529383 - Flags: review?(bugmail.mozilla) → review+
(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.
(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.
w/ most comments addressed (thanks for the fast reviews, btw!)
Attachment #8529423 - Attachment is obsolete: true
Attachment #8530748 - Flags: review?(bugmail.mozilla)
(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.
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!
Attachment #8530748 - Flags: review?(bugmail.mozilla) → review+
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.
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?
(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.
(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
... 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
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: