We need to create proper drag input blocks when scrolling to properly wait for the confirmation of the events. This bug will land this part.

This was already partly reviewed here: https://bugzilla.mozilla.org/show_bug.cgi?id=1199885#c100.

Bug 1211612 - Add DragInputBlock for async scrollbars. r?kats

So I already had the block working only looking at events in the drag block. I just moved it a bit and restored the const and did appropriate renames. I believe this should address the review from 1199885#c100.

Comment on attachment 8669904 [details] MozReview Request: Bug 1211612 - Add DragInputBlock for async scrollbars. r=kats https://reviewboard.mozilla.org/r/21301/#review19185 Looks pretty good. There's a bunch of mostly minor issues. The big issue is the weirdness with mReceivedMouseUp, I think there's a few bugs there that might be cancelling each other out to some extent. See my comments below and let me know if the changes I'm suggesting break something. ::: gfx/layers/apz/src/InputBlockState.h:10 (Diff revision 1) > +#include "APZUtils.h" APZUtils.h and TouchCounter.h shouldn't need to be included here, please remove them if so ::: gfx/layers/apz/src/InputBlockState.h:16 (Diff revision 1) > +#include "mozilla/layers/AsyncDragMetrics.h" Move this up to just before nsAutoPtr for alphabetic goodness ::: gfx/layers/apz/src/InputBlockState.h:93 (Diff revision 1) > + virtual DragBlockState *AsMouseBlock() { s/AsMouseBlock/AsDragBlock/ ::: gfx/layers/apz/src/InputBlockState.h:272 (Diff revision 1) > + bool IsReadyForHandling() const override; SetContentResponse() and IsReadyForHandling() don't need to exist in this class, they just call the base class version anyway so there's no need to override them at all. ::: gfx/layers/apz/src/InputBlockState.h:293 (Diff revision 1) > + virtual void DispatchEvent(const InputData& aEvent) const override; drop the "virtual" keyword, it is redundant with "override" ::: gfx/layers/apz/src/InputBlockState.h:297 (Diff revision 1) > + mutable bool mReceivedMouseUp; This doesn't need to be mutable ::: gfx/layers/apz/src/InputBlockState.cpp:168 (Diff revision 1) > + , mReceivedMouseUp(true) Why is this initialized to true? I would expect it to start off as false ::: gfx/layers/apz/src/InputBlockState.cpp:176 (Diff revision 1) > + if (!mReceivedMouseUp) { This should not be checked here because DispatchEvent runs at a different time than AddEvent. That is, AddEvent runs when the input event arrives at the InputQueue, but DispatchEvent may run at some later time. So if the mouseup arrives but the block processing is held up (because of main thread delays for example) the events in the block won't get processed properly. See my comment in InputQueue::ReceiveMouseEvent for how to check this. ::: gfx/layers/apz/src/InputBlockState.cpp:180 (Diff revision 1) > + if (!mouseInput.TransformToLocal(mTransformToApzc)) { This is ok for now, but for future reference I think a better arrangement of this could be like so: - DragBlockState::DispatchEvent doesn't exist. Instead we just use the base version from CancelableBlockState::DispatchEvent - AsyncPanZoomController::HandleInputEvent adds a MOUSE_INPUT case to the switch statement, and passes that input to HandleDragEvent - HandleDragEvent does something like CurrentDragBlock()->GetDragMetrics() to get the drag metrics, rather than having them passed in. This way we would reuse more of the existing code paths and patterns rather than duplicating code. ::: gfx/layers/apz/src/InputBlockState.cpp:236 (Diff revision 1) > + return mReceivedMouseUp; I think this should be |return !mReceivedMouseUp;| - the block should not be discarded as long as the mouse-up hasn't been received. ::: gfx/layers/apz/src/InputBlockState.cpp:242 (Diff revision 1) > + return "mouse"; s/mouse/drag/ ::: gfx/layers/apz/src/InputQueue.h:9 (Diff revision 1) > +#include "APZUtils.h" I don't think these includes are needed, please remove them if they are not. ::: gfx/layers/apz/src/InputQueue.h:68 (Diff revision 1) > + * This function should be invoked when a drag has been completed. This This comment doesn't seem to reflect what the function is supposed to do. ::: gfx/layers/apz/src/InputQueue.h:71 (Diff revision 1) > + void SetConfirmedMouseBlock(uint64_t aInputBlockId, const nsRefPtr<AsyncPanZoomController>& aTargetApzc, Rename this to ConfirmDragBlock Also, move the second param onto its own line as well ::: gfx/layers/apz/src/InputQueue.cpp:183 (Diff revision 1) > + block = mInputBlockQueue.LastElement()->AsMouseBlock(); (Continued from my previous comment) Right after this, do: if (block && block->HasReceivedMouseUp()) { block = nullptr; } because we shouldn't add new input events to a block that has been "finished" (i.e. the mouse-up has been received). ::: gfx/layers/apz/src/InputQueue.cpp:191 (Diff revision 1) > + block = new DragBlockState(aTarget, aTargetConfirmed, aEvent); add a MOZ_ASSERT(newBlock) at the start of the |if| body ::: gfx/layers/apz/src/InputQueue.cpp:196 (Diff revision 1) > + INPQ_LOG("started new mouse block %p for target %p\n", block, aTarget.get()); s/mouse/drag/ ::: gfx/layers/apz/src/InputQueue.cpp:214 (Diff revision 1) > + if (!block->HasReceivedMouseUp()) { I don't understand what this check is doing here. It seems wrong and IMO should be removed. Perhaps it is a case of two bugs canceling each other out (the other bug being that the flag is initialized to true in the DragBlockState constructor). ::: gfx/layers/apz/src/InputQueue.cpp:577 (Diff revision 1) > + if (block->GetBlockId() == aInputBlockId) { Combine this into the enclosing if with &&

> APZUtils.h and TouchCounter.h shouldn't need to be included here, please remove them if so Ah right, the reason I keep including includes is that they are missing because of unification. YouCompleteMe gives warnings. I just fixed them as I went along. These changes don't really warrant their own bug/review cycle.

> This comment doesn't seem to reflect what the function is supposed to do. I replaced completed with started.

Comment on attachment 8669904 [details] MozReview Request: Bug 1211612 - Add DragInputBlock for async scrollbars. r=kats Bug 1211612 - Add DragInputBlock for async scrollbars. r?kats

The only new change not explicitly mentioned in your review: 223 bool mouseUp = aEvent.mType == MouseInput::MOUSE_UP && aEvent.IsLeftButton(); 224 if (mouseUp) { 225 block->ReceivedMouseUp(); 226 }

Comment on attachment 8669904 [details] MozReview Request: Bug 1211612 - Add DragInputBlock for async scrollbars. r=kats https://reviewboard.mozilla.org/r/21301/#review20683 ::: gfx/layers/apz/src/APZCTreeManager.cpp:698 (Diff revision 2) > + // but we wont find an APZC. Fallback to root APZC then. s/wont/won't/ ::: gfx/layers/apz/src/InputBlockState.h:260 (Diff revision 2) > + * A single block of mouse events. "A block of mouse events that are part of a drag" ::: gfx/layers/apz/src/InputBlockState.h:275 (Diff revision 2) > + bool HasReceivedMouseUp() { Move the body of this function to the .cpp ::: gfx/layers/apz/src/InputBlockState.h:279 (Diff revision 2) > + void ReceivedMouseUp() { We can get rid of this function, see my other comment ::: gfx/layers/apz/src/InputBlockState.h:289 (Diff revision 2) > + void SetDragMetrics(const AsyncDragMetrics& aDragMetrics) { Move the body of this function to the .cpp ::: gfx/layers/apz/src/InputQueue.h:68 (Diff revision 2) > + * This function should be invoked when a drag has been started. This I'm not sure what "this will cancel any previous mouse input blocks" means here. Replace this comment with "This function is invoked to confirm that the drag block should be handled by the APZ." ::: gfx/layers/apz/src/InputQueue.h:71 (Diff revision 2) > + void SetConfirmedDragBlock(uint64_t aInputBlockId, Rename this to ConfirmDragBlock ::: gfx/layers/apz/src/InputQueue.cpp:225 (Diff revision 2) > + block->ReceivedMouseUp(); I don't like duplicating this code here. If I understand correctly the reason you put this here is because sometimes the block is ready to be handled and so the call to MaybeHandleCurrentBlock above goes through DispatchImmediate with the event and returns true, so AddEvent doesn't get called. I think the correct solution here is to override DispatchImmediate in the DragBlockState to check for the mouseup and set mReceivedMouseUp directly. ::: gfx/layers/apz/src/InputQueue.cpp:591 (Diff revision 2) > + drop empty line

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #10) > ::: gfx/layers/apz/src/InputBlockState.h:275 > (Diff revision 2) > > + bool HasReceivedMouseUp() { > > Move the body of this function to the .cpp > > ::: gfx/layers/apz/src/InputBlockState.h:289 > (Diff revision 2) > > + void SetDragMetrics(const AsyncDragMetrics& aDragMetrics) { > > Move the body of this function to the .cpp > Is there really value in moving trivial getter/setter out of the header? We generally allow simple getter/setter in the header. In the past reviewers have request that this should be in the header. We should minimize churns of these details in the review unless we want to take a clear and consistent stance on this issue.

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #10) > ::: gfx/layers/apz/src/InputQueue.h:71 > (Diff revision 2) > > + void SetConfirmedDragBlock(uint64_t aInputBlockId, > > Rename this to ConfirmDragBlock Alright but this was just following after 'SetConfirmedTargetApzc' which is a similar function.

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #10) > ::: gfx/layers/apz/src/InputQueue.cpp:225 > (Diff revision 2) > > + block->ReceivedMouseUp(); > > I don't like duplicating this code here. If I understand correctly the > reason you put this here is because sometimes the block is ready to be > handled and so the call to MaybeHandleCurrentBlock above goes through > DispatchImmediate with the event and returns true, so AddEvent doesn't get > called. I think the correct solution here is to override DispatchImmediate > in the DragBlockState to check for the mouseup and set mReceivedMouseUp > directly. > Actually I forgot to remove the code from |DragBlockState::AddEvent|. Like you said I can't put it just into AddEvent. I also need to put it in DispatchImmediate. I'd rather just have it in one place |InputQueue::ReceiveMouseInput|. If I follow your suggestion then I will have to duplicate the code in two places. What do you think?

(In reply to Benoit Girard (:BenWa) from comment #11) > Is there really value in moving trivial getter/setter out of the header? We > generally allow simple getter/setter in the header. In the past reviewers > have request that this should be in the header. We should minimize churns of > these details in the review unless we want to take a clear and consistent > stance on this issue. It would be good if we had a project-wide code style guidance on this, but in the APZ code at least I prefer putting everything in .cpp files. Trivial functions tend to become nontrivial over time, and touching code in headers can result in expensive rebuilds unnecessarily. (In reply to Benoit Girard (:BenWa) from comment #12) > > Rename this to ConfirmDragBlock > > Alright but this was just following after 'SetConfirmedTargetApzc' which is > a similar function. Similar in some respects but also different. With a function named "SetConfirmedTargetApzc" you would expect that it takes an argument that is the "confirmed target apzc" and that's what it does. For a function named "SetConfirmedDragBlock" you would expect that it takes a "confirmed drag block" but it doesn't - it takes the things that are required to confirm an existing drag block. That's why I would like it renamed. (In reply to Benoit Girard (:BenWa) from comment #13) > Actually I forgot to remove the code from |DragBlockState::AddEvent|. Like > you said I can't put it just into AddEvent. I also need to put it in > DispatchImmediate. I'd rather just have it in one place > |InputQueue::ReceiveMouseInput|. If I follow your suggestion then I will > have to duplicate the code in two places. What do you think? Ok, that's fine. However then I would prefer to rename ReceivedMouseUp() to MarkMouseUpReceived() so that it's more clear that it's setting a flag.

Comment on attachment 8669904 [details] MozReview Request: Bug 1211612 - Add DragInputBlock for async scrollbars. r=kats Bug 1211612 - Add DragInputBlock for async scrollbars. r?kats

Comment on attachment 8669904 [details] MozReview Request: Bug 1211612 - Add DragInputBlock for async scrollbars. r=kats https://reviewboard.mozilla.org/r/21301/#review20791 r+ with the name correction. Thanks for putting up with all my nitpicky comments! ::: gfx/layers/apz/src/InputQueue.h:71 (Diff revision 3) > + void ConfirmedDragBlock(uint64_t aInputBlockId, "ConfirmDragBlock", not "ConfirmedDragBlock"

Comment on attachment 8669904 [details] MozReview Request: Bug 1211612 - Add DragInputBlock for async scrollbars. r=kats Bug 1211612 - Add DragInputBlock for async scrollbars. r=kats

https://hg.mozilla.org/integration/mozilla-inbound/rev/0095ee7c07a34c940a30836704b5dff9107be686 Bug 1211612 - Add DragInputBlock for async scrollbars. r=kats

removing the b2g 2.5 flag since this commit has been reverted due to an incorrect merge, sorry for the confusion

Copied from patches provided by Kats, https://github.com/staktrace/gecko-dev/commit/74c69d2f71658c6d5cee4bdc413d2cb1576969d3 We'd like meta-viewport could be enabled in v2.5. And since the patch was reviewed and rebased, I don't think an extra review is needed. Or should I ? [Approval Request Comment] Bug caused by (feature/regressing bug #): Bug 1211612 User impact if declined: Won't be able to get proper result about mouse hit-test if meta-viewport enabled. Testing completed: https://treeherder.mozilla.org/#/jobs?repo=try&revision=69b3a50fd790&group_state=expanded&filter-resultStatus=success&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=running&filter-resultStatus=pending&filter-resultStatus=runnable&filter-resultStatus=coalesced Risk to taking this patch (and alternatives if risky): Low String or UUID changes made by this patch: None

(In reply to Kilik Kuo [:kikuo][:kilikkuo] from comment #23) > Created attachment 8733706 [details] [diff] [review] > Re-based for V25. > > Copied from patches provided by Kats, > https://github.com/staktrace/gecko-dev/commit/ > 74c69d2f71658c6d5cee4bdc413d2cb1576969d3 > We'd like meta-viewport could be enabled in v2.5. And since the patch was > reviewed and rebased, I don't think an extra review is needed. Or should I ? > > [Approval Request Comment] > Bug caused by (feature/regressing bug #): Bug 1211612 > User impact if declined: Won't be able to get proper result about mouse > hit-test if meta-viewport enabled. > Testing completed: > https://treeherder.mozilla.org/#/ > jobs?repo=try&revision=69b3a50fd790&group_state=expanded&filter- > resultStatus=success&filter-resultStatus=testfailed&filter- > resultStatus=busted&filter-resultStatus=exception&filter- > resultStatus=retry&filter-resultStatus=usercancel&filter- > resultStatus=running&filter-resultStatus=pending&filter- > resultStatus=runnable&filter-resultStatus=coalesced > Risk to taking this patch (and alternatives if risky): Low > String or UUID changes made by this patch: None I think I'm mis-understanding but I don't see how this patch will help with meta-viewport? It's the base of a feature that isn't ready to ship.

(In reply to Kilik Kuo [:kikuo][:kilikkuo] from comment #23) > [Approval Request Comment] > Bug caused by (feature/regressing bug #): Bug 1211612 This should say "Bug 1241491". (In reply to Benoit Girard (:BenWa) from comment #24) > I think I'm mis-understanding but I don't see how this patch will help with > meta-viewport? It's the base of a feature that isn't ready to ship. This patch is a code dependency for bug 1242690, which fixes the hit-testing of mouse events when there is a resolution applied (in some scenarios).

Comment on attachment 8733706 [details] [diff] [review] Re-based for V25. Review of attachment 8733706 [details] [diff] [review]: ----------------------------------------------------------------- Sorry for not explaining this well. As Comment 25 said. I think a r+ is required for uplift. Hi Kats, could you help review this ?

The version of the patch you had seems to have yourself as the author. I'm updating it to reflect BenWa as the correct author. I already reviewed it for m-c, so it doesn't need re-reviewing for b2g44, but I'm marking it r+ anyway. Carrying approval flag for b2g44 from comment 23.

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #27) > Created attachment 8734347 [details] [diff] [review] > Re-based for b2g44 > > The version of the patch you had seems to have yourself as the author. I'm > updating it to reflect BenWa as the correct author. I already reviewed it > for m-c, so it doesn't need re-reviewing for b2g44, but I'm marking it r+ > anyway. Carrying approval flag for b2g44 from comment 23. Oops sorry, I forgot to modify hg user information to reflect that. Thank you for the reminder, and thanks for the help :)

Comment on attachment 8734347 [details] [diff] [review] Re-based for b2g44 Approve for TV 2.5

Hi Gary, Please also help to mark "status-b2g-v2.5:fixed" in tracking b2g flag. Thanks!

(In reply to Josh Cheng [:josh] from comment #31) > Hi Gary, > Please also help to mark "status-b2g-v2.5:fixed" in tracking b2g flag. > Thanks! got it, thank you!