Status

()

defect
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: BenWa, Assigned: BenWa)

Tracking

42 Branch
mozilla45
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking-b2g:2.5+, firefox45 fixed, b2g-v2.5 fixed)

Details

(Whiteboard: [ft:conndevices])

Attachments

(2 attachments, 1 obsolete attachment)

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.
Assignee: nobody → bgirard
Bug 1211612 - Add DragInputBlock for async scrollbars. r?kats
Attachment #8669904 - Flags: review?(bugmail.mozilla)
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 &&
Attachment #8669904 - Flags: review?(bugmail.mozilla)
> 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
Attachment #8669904 - Flags: review?(bugmail.mozilla)
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
Attachment #8669904 - Flags: review?(bugmail.mozilla)
(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
Attachment #8669904 - Flags: review?(bugmail.mozilla)
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"
Attachment #8669904 - Flags: review?(bugmail.mozilla) → review+
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
Attachment #8669904 - Attachment description: MozReview Request: Bug 1211612 - Add DragInputBlock for async scrollbars. r?kats → MozReview Request: Bug 1211612 - Add DragInputBlock for async scrollbars. r=kats
https://hg.mozilla.org/mozilla-central/rev/0095ee7c07a3
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
removing the b2g 2.5 flag since this commit has been reverted due to an incorrect merge, sorry for the confusion
Posted patch Re-based for V25. (obsolete) — Splinter Review
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
Attachment #8733706 - Flags: approval‑mozilla‑b2g44?
(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 ?
Attachment #8733706 - Flags: review?(bugmail.mozilla)
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.
Attachment #8733706 - Attachment is obsolete: true
Attachment #8733706 - Flags: review?(bugmail.mozilla)
Attachment #8733706 - Flags: approval‑mozilla‑b2g44?
Attachment #8734347 - Flags: review+
Attachment #8734347 - Flags: approval‑mozilla‑b2g44?
(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 :)
blocking-b2g: --- → 2.5+
Whiteboard: [ft:conndevices]
Comment on attachment 8734347 [details] [diff] [review]
Re-based for b2g44

Approve for TV 2.5
Attachment #8734347 - Flags: approval‑mozilla‑b2g44? → approval‑mozilla‑b2g44+
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!
You need to log in before you can comment on or make changes to this bug.