ContentReceivedTouch doesn't actually cancel event processing in the apzc

RESOLVED FIXED in mozilla28

Status

()

Core
Panning and Zooming
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: jimm, Assigned: jimm)

Tracking

26 Branch
mozilla28
x86_64
All
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [block28])

Attachments

(6 attachments, 21 obsolete attachments)

6.71 KB, patch
TimAbraldes
: review+
kats
: checkin+
Details | Diff | Splinter Review
16.47 KB, patch
kats
: review+
Details | Diff | Splinter Review
17.12 KB, patch
mbrubeck
: review+
kats
: feedback+
Details | Diff | Splinter Review
1.47 KB, patch
mbrubeck
: review+
Details | Diff | Splinter Review
10.27 KB, patch
kats
: review+
Details | Diff | Splinter Review
3.81 KB, text/html
Details
(Assignee)

Description

5 years ago
This looks like a bug, so filing to investigate. In ReceiveInputEvent we identify the apz that will be the target of touch input and store it in mApzcForInputBlock  -

http://mxr.mozilla.org/mozilla-central/source/gfx/layers/composite/APZCTreeManager.cpp#426

If a caller decides to cancel the touch sequence, the tree manager delivers the ContentReceivedTouch call to an apz based a guid. In the metro case, this would be the root apz.

http://mxr.mozilla.org/mozilla-central/source/gfx/layers/composite/APZCTreeManager.cpp#426

I think ContentReceivedTouch should be calling ContentReceivedTouch on the apz stored in mApzcForInputBlock if it exists.

Kats any thoughts?
(Assignee)

Comment 1

5 years ago
Also it's not clear to me what AsyncPanZoomController::ContentReceivedTouch is trying to do.

http://mxr.mozilla.org/mozilla-central/source/gfx/layers/ipc/AsyncPanZoomController.cpp#1317

Assume we call ContentReceivedTouch w/aPreventDefault = true when touchstart or the first touchmove have preventDefault called on them. If we continue to send input to the tree manager, it looks like that input will still be delivered and processed by an apz. I would expect that the only processing needed past that point would be event coordinate transformation in the tree manager.
(Assignee)

Comment 2

5 years ago
Just confirmed that if continue to send events after calling ContentReceivedTouch w/aPreventDefault = true on a touch start or first touch move the apz continues to react to input. So this needs to be updated. Alternatively I can add a new tree manager method for transforming event data that doesn't forward input input to the apz. Then i can handle this in widget.
(Assignee)

Updated

5 years ago
Blocks: 886321
(Assignee)

Updated

5 years ago
Blocks: 915213
No longer blocks: 886321
(In reply to Jim Mathies [:jimm] from comment #0)
> If a caller decides to cancel the touch sequence, the tree manager delivers
> the ContentReceivedTouch call to an apz based a guid. In the metro case,
> this would be the root apz.
> 
> http://mxr.mozilla.org/mozilla-central/source/gfx/layers/composite/
> APZCTreeManager.cpp#426
> 
> I think ContentReceivedTouch should be calling ContentReceivedTouch on the
> apz stored in mApzcForInputBlock if it exists.
> 
> Kats any thoughts?

Yes, this appears to be a bug. Sending it to mApzcForInputBlock will work some of the time, but it's possible for mApzcForInputBlock to be nulled out before the ContentReceivedTouch call comes in, so we'll need something a bit more robust to handle this scenario.

(In reply to Jim Mathies [:jimm] from comment #2)
> Just confirmed that if continue to send events after calling
> ContentReceivedTouch w/aPreventDefault = true on a touch start or first
> touch move the apz continues to react to input. So this needs to be updated.

Agreed, this needs to be updated. I was just looking at the code and it doesn't appear to handle this case at all, but I was assuming it did. To fix this I would like to add some code to AsyncPanZoomController to basically drop the rest of the events in the "touch event block" (as defined at http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/gfx/TouchEventHandler.java#26) if aPreventDefault is true.

> Alternatively I can add a new tree manager method for transforming event
> data that doesn't forward input input to the apz. Then i can handle this in
> widget.

I would prefer to avoid doing this.
(Assignee)

Updated

5 years ago
Assignee: nobody → jmathies
(Assignee)

Comment 4

5 years ago
Went digging for uses of ContentReceivedTouch. The only real use I could find was in TabChild here - 

http://mxr.mozilla.org/mozilla-central/source/dom/ipc/TabParent.cpp#1583
http://mxr.mozilla.org/mozilla-central/source/dom/ipc/TabChild.cpp#1847

Which looks like it gets called on every touch event except touch moves, which are handled in another method. RecvRealTouchEvent hands back gPreventMouseEvents which looks right to me in terms of how it gets set -

http://mxr.mozilla.org/mozilla-central/source/layout/base/nsPresShell.cpp#7001

(Kinda gross that we use a global bool for this.)

Also it looks like the guid passed in from RenderFrameParent is associated with the foreground tab - 

http://mxr.mozilla.org/mozilla-central/source/layout/ipc/RenderFrameParent.cpp#1030
(Assignee)

Comment 5

5 years ago
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #3)
> Yes, this appears to be a bug. Sending it to mApzcForInputBlock will work
> some of the time, but it's possible for mApzcForInputBlock to be nulled out
> before the ContentReceivedTouch call comes in, so we'll need something a bit
> more robust to handle this scenario.

Hmm, I don't see the issue here. The only place we null out mApzcForInputBlock is on a MULTITOUCH_CANCEL or MULTITOUCH_END, in which case the touch start and touch move have already been processed.

http://mxr.mozilla.org/mozilla-central/source/gfx/layers/composite/APZCTreeManager.cpp#265

If we are more explicit in the definition of ContentReceivedTouch (must be called on touch start or the first touch move) then we should be ok using mApzcForInputBlock.
(Assignee)

Comment 6

5 years ago
So in working on this I've found that on metro mFrameMetrics.mMayHaveTouchListeners is always false for the layer we scroll (and for the discovered mApzcForInputBlock). So we never get to state WAITING_LISTENERS and can't cancel at all.
(In reply to Jim Mathies [:jimm] from comment #5)
> Hmm, I don't see the issue here. The only place we null out
> mApzcForInputBlock is on a MULTITOUCH_CANCEL or MULTITOUCH_END, in which
> case the touch start and touch move have already been processed.
> 
> http://mxr.mozilla.org/mozilla-central/source/gfx/layers/composite/
> APZCTreeManager.cpp#265
> 
> If we are more explicit in the definition of ContentReceivedTouch (must be
> called on touch start or the first touch move) then we should be ok using
> mApzcForInputBlock.

I guess on Metro this is ok because the input events are sent to APZC on the gecko thread, but this is not true across all platforms. On Android, the flow could look like this:

UI thread sends MULTITOUCH_START to APZC
UI thread hands untransformed MULTITOUCH_START to Gecko thread
UI thread sends MULTITOUCH_MOVE to APZC
UI thread hands untransformed MULTITOUCH_MOVE to Gecko thread
UI thread sends MULTITOUCH_END to APZC (mApzcForInputBlock is nulled out)
UI thread hands untransformed MULTITOUCH_END to Gecko thread
Gecko thread processes MULTITOUCH_START, sends ContentReceivedTouch(true) to APZC

In this flow the call to ContentReceivedTouch would have a null mApzcForInputBlock and would not get delivered properly. On Metro all of this happens on the Gecko thread so it's not a problem.
(In reply to Jim Mathies [:jimm] from comment #6)
> So in working on this I've found that on metro
> mFrameMetrics.mMayHaveTouchListeners is always false for the layer we scroll
> (and for the discovered mApzcForInputBlock). So we never get to state
> WAITING_LISTENERS and can't cancel at all.

That seems wrong.
(Assignee)

Comment 9

5 years ago
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #7)
> (In reply to Jim Mathies [:jimm] from comment #5)
> > Hmm, I don't see the issue here. The only place we null out
> > mApzcForInputBlock is on a MULTITOUCH_CANCEL or MULTITOUCH_END, in which
> > case the touch start and touch move have already been processed.
> > 
> > http://mxr.mozilla.org/mozilla-central/source/gfx/layers/composite/
> > APZCTreeManager.cpp#265
> > 
> > If we are more explicit in the definition of ContentReceivedTouch (must be
> > called on touch start or the first touch move) then we should be ok using
> > mApzcForInputBlock.
> 
> I guess on Metro this is ok because the input events are sent to APZC on the
> gecko thread, but this is not true across all platforms. On Android, the
> flow could look like this:
> 
> UI thread sends MULTITOUCH_START to APZC
> UI thread hands untransformed MULTITOUCH_START to Gecko thread
> UI thread sends MULTITOUCH_MOVE to APZC
> UI thread hands untransformed MULTITOUCH_MOVE to Gecko thread
> UI thread sends MULTITOUCH_END to APZC (mApzcForInputBlock is nulled out)
> UI thread hands untransformed MULTITOUCH_END to Gecko thread
> Gecko thread processes MULTITOUCH_START, sends ContentReceivedTouch(true) to
> APZC
> 
> In this flow the call to ContentReceivedTouch would have a null
> mApzcForInputBlock and would not get delivered properly. On Metro all of
> this happens on the Gecko thread so it's not a problem.

Hmm, I didn't realize there was that much delay. Seems like the simplest solution then would be to keep mApzcForInputBlock around longer and find a more appropriate time to clear it, if that's even necessary.
(Assignee)

Comment 10

5 years ago
(In reply to Jim Mathies [:jimm] from comment #9)
> (In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #7)
> > UI thread sends MULTITOUCH_START to APZC
> > UI thread hands untransformed MULTITOUCH_START to Gecko thread
> > UI thread sends MULTITOUCH_MOVE to APZC
> > UI thread hands untransformed MULTITOUCH_MOVE to Gecko thread
> > UI thread sends MULTITOUCH_END to APZC (mApzcForInputBlock is nulled out)
> > UI thread hands untransformed MULTITOUCH_END to Gecko thread
> > Gecko thread processes MULTITOUCH_START, sends ContentReceivedTouch(true) to
> > APZC
> > 
> > In this flow the call to ContentReceivedTouch would have a null
> > mApzcForInputBlock and would not get delivered properly. On Metro all of
> > this happens on the Gecko thread so it's not a problem.
> 
> Hmm, I didn't realize there was that much delay. Seems like the simplest
> solution then would be to keep mApzcForInputBlock around longer and find a
> more appropriate time to clear it, if that's even necessary.

We might have to tie mApzcForInputBlock to the input batch somehow I think, if there's a chance this could wrap. e.g. we might get 

UI thread sends MULTITOUCH_START(1) to APZC
UI thread hands untransformed MULTITOUCH_START to Gecko thread
UI thread sends MULTITOUCH_MOVE to APZC
UI thread hands untransformed MULTITOUCH_MOVE to Gecko thread
UI thread sends MULTITOUCH_END to APZC (mApzcForInputBlock is nulled out)
UI thread hands untransformed MULTITOUCH_END to Gecko thread
UI thread sends MULTITOUCH_START(2) to APZC
Gecko thread processes MULTITOUCH_START(1), sends ContentReceivedTouch(true) to APZC
(Assignee)

Comment 11

5 years ago
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #8)
> (In reply to Jim Mathies [:jimm] from comment #6)
> > So in working on this I've found that on metro
> > mFrameMetrics.mMayHaveTouchListeners is always false for the layer we scroll
> > (and for the discovered mApzcForInputBlock). So we never get to state
> > WAITING_LISTENERS and can't cancel at all.
> 
> That seems wrong.

Yeah I'm going to try debugging nsDisplayList tomorrow, see what's going on.
(In reply to Jim Mathies [:jimm] from comment #10)
> We might have to tie mApzcForInputBlock to the input batch somehow I think,
> if there's a chance this could wrap. e.g. we might get 

Yes, it could wrap. One option is to return the ScrollableLayerGuid of the APZC back out along with the untransformed event, and then the caller can ensure that they pass it back in when calling ContentReceivedTouch. That way we can guarantee it gets the right APZC.
(Assignee)

Comment 13

5 years ago
Still trying to understand what's going on here. Doing soem logging in nsDisplayList and down in APZTreeManager, I see two distinct layer trees being build, and we seem to be attaching to the wrong one. 

For the main browser:

> PaintRoot
dl  container: A3D3DA8 touch: 0 scrollid: 1 uri: 
dl  container: A3D5F58 touch: 1 scrollid: 0 uri: chrome://browser/content/browser.xul
> UpdatePanZoomControllerTree
apz container: A6CD4B8 touch: 1 scrollid: 0 scrollable: 0 uri: chrome://browser/content/browser.xul
apz container: A9005B8 touch: 0 scrollid: 0 scrollable: 0 uri: 
apz container: ED208F0 touch: 0 scrollid: 0 scrollable: 0 uri: 
apz container: 9970058 touch: 0 scrollid: 1 scrollable: 1 uri: 
controller found for container: 9970058
< UpdatePanZoomControllerTree
< PaintRoot

This is odd since I can pan content, but afaict our controller isn't attached to the browser frame. Note I'm pausing PaintRoot here to give the compositor thread time to update within the same time segment.

Immediately after this in display list I see another PaintRoot call - 

> PaintRoot
dl  container: A3D4C18 touch: 1 scrollid: 1 uri: https://bug915213.bugzilla.mozilla.org/attachment.cgi?id=803131
< PaintRoot

which is my touch demo loaded in a browser tab. The compositor doesn't update here at all. Not sure whats going on, but will continue to dig.
(Assignee)

Comment 14

5 years ago
Dumping the frame type helps a bit here - 

> PaintRoot
dl  container: A0FA3C0 touch: 0 scrollid: 1 type: CanvasFrame
dl  container: 4BE85C0 touch: 1 scrollid: 0 type: ViewportFrame
> UpdatePanZoomControllerTree
apz container: 9D0DA30 touch: 1 scrollid: 0 scrollable: 0 type: ViewportFrame
apz container: A0FED68 touch: 0 scrollid: 0 scrollable: 0 type: 
apz container: 95CC5C0 touch: 0 scrollid: 0 scrollable: 0 type: 
apz container: A0EF1D0 touch: 0 scrollid: 1 scrollable: 1 type: CanvasFrame
controller found for container: A0EF1D0
< UpdatePanZoomControllerTree
< PaintRootntRoot

I do not understand why the controller is attached to a CanvasFrame instead of ViewportFrame. I'm not even sure where the canvas frame comes from, although we do have a canvas in our nav bar.
(Assignee)

Comment 15

5 years ago
Created attachment 808586 [details]
dump-58.html

I did a dump of the paint list and things look OK. Although we still have this issue where a page that listens for touch does not set mMayHaveTouchListeners in frame metrics. I'm going to split that off from this work.
(Assignee)

Updated

5 years ago
No longer blocks: 915213
Depends on: 915213
(Assignee)

Updated

5 years ago
Blocks: 886321
(Assignee)

Updated

5 years ago
Assignee: jmathies → nobody
Component: Graphics: Layers → Panning and Zooming
(Assignee)

Updated

5 years ago
Assignee: nobody → jmathies
(Assignee)

Updated

5 years ago
Depends on: 931146
(Assignee)

Updated

5 years ago
Attachment #808586 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
No longer depends on: 931146
(Assignee)

Comment 16

5 years ago
Created attachment 822527 [details] [diff] [review]
fix ContentReceivedTouch v.1
Attachment #822527 - Flags: review?(bugmail.mozilla)
(Assignee)

Comment 17

5 years ago
> Agreed, this needs to be updated. I was just looking at the code and it
> doesn't appear to handle this case at all, but I was assuming it did. To fix
> this I would like to add some code to AsyncPanZoomController to basically
> drop the rest of the events in the "touch event block" (as defined at
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/gfx/
> TouchEventHandler.java#26) if aPreventDefault is true.

Note this patch addresses this issue, it doesn't address the nulled out mApzcForInputBlock when events wrap. I'll file a follow up for that.
(Assignee)

Updated

5 years ago
Summary: ContentReceivedTouch doesn't cancel touch sequence on the appropriate apz → ContentReceivedTouch doesn't actually cancel event processing in the apzc
(Assignee)

Updated

5 years ago
Whiteboard: [blocker]

Updated

5 years ago
Whiteboard: [blocker] → [block28]
Comment on attachment 822527 [details] [diff] [review]
fix ContentReceivedTouch v.1

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

Sorry for the delayed review; I had to re-familiarize myself with some of this code. This looks fine, although some of the code you're touching is dead and should be removed (bug 906877).
Attachment #822527 - Flags: review?(bugmail.mozilla) → review+
(Assignee)

Comment 19

5 years ago
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #18)
> Comment on attachment 822527 [details] [diff] [review]
> fix ContentReceivedTouch v.1
> 
> Review of attachment 822527 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Sorry for the delayed review; I had to re-familiarize myself with some of
> this code. This looks fine, although some of the code you're touching is
> dead and should be removed (bug 906877).

I had a feeling this was the case since BrowserElementPanning is going away here pretty soon?
Some bits of BrowserElementPanning can be removed now (the parts that deal with stealing subframe scrolling from APZC) but a lot of it is still used on B2G. That will eventually go away as well but probably not in Gecko 28.
I just landed bug 906877 on b2g-inbound which I belated realized also removes the CancelDefaultPanZoom function from AsyncPanZoomController.cpp. Feel free to resurrect it as a private function in AsyncPanZoomController since you still need it for this patch.
(Assignee)

Comment 22

5 years ago
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #21)
> I just landed bug 906877 on b2g-inbound which I belated realized also
> removes the CancelDefaultPanZoom function from AsyncPanZoomController.cpp.
> Feel free to resurrect it as a private function in AsyncPanZoomController
> since you still need it for this patch.

My patch is already on fx-team. :)
(Assignee)

Comment 23

5 years ago
https://hg.mozilla.org/integration/fx-team/rev/ef51f8a92b09

backed out, I'll clean this up whenever the trees open.
Whoops, sorry! I didn't see the fx-team landing. Thanks for backing yours out although I'd have been fine with you backing mine out :)
(Assignee)

Comment 25

5 years ago
Created attachment 825328 [details] [diff] [review]
fix ContentReceivedTouch v.2

This adds back the touch block cancelation, and also adds good touch block tracking. One of the things I've noticed with this tracking is that multi-touch can leave the apz in an unexpected state which might be the cause of bugs like bug 915289 and bug 927027.
Attachment #822527 - Attachment is obsolete: true
Attachment #825328 - Flags: review?(bugmail.mozilla)
(Assignee)

Comment 26

5 years ago
Created attachment 825329 [details] [diff] [review]
metroinput touch point debugging v.1

Some useful debugging routines for our touch event sending. Right now everything looks great.
Attachment #825329 - Flags: review?(tabraldes)
(Assignee)

Comment 27

5 years ago
Comment on attachment 825328 [details] [diff] [review]
fix ContentReceivedTouch v.2

Hmm, so looks like GestureEventListener can filter out multitouch before it reaches the touch handlers, so I'm going to have to account for that as well.
Attachment #825328 - Flags: review?(bugmail.mozilla)
(Assignee)

Comment 28

5 years ago
Created attachment 825400 [details] [diff] [review]
fix ContentReceivedTouch v.3

Ok this is working now with one exception, bug 927033 currently breaks block tracking. As soon as we get one touchend in a multitouch set, the touch block apz clears, and things start to fall apart from there.
(Assignee)

Updated

5 years ago
Depends on: 927033
(Assignee)

Updated

5 years ago
Attachment #825328 - Attachment is obsolete: true
Comment on attachment 825329 [details] [diff] [review]
metroinput touch point debugging v.1

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

Seems fine to me!

::: widget/windows/winrt/MetroAppShell.cpp
@@ +221,5 @@
>  // queue and need to make sure they get dispatched before the stack unwinds.
>  void // static
>  MetroAppShell::MarkEventQueueForPurge()
>  {
> +  //LogFunction();

Let's just remove these; don't comment them out. Commented-out code just becomes clutter.

@@ +242,5 @@
>    if (!sWillEmptyThreadQueue) {
>      return;
>    }
>  
> +  //LogFunction();

Let's just remove these; don't comment them out. Commented-out code just becomes clutter.

::: widget/windows/winrt/MetroInput.cpp
@@ +1159,5 @@
>      NS_NewRunnableMethod(this, &MetroInput::DeliverNextQueuedTouchEvent);
>    NS_DispatchToCurrentThread(runnable);
>  }
>  
> +static void DumpTouchIds(const char* aMsg, WidgetTouchEvent* aEvent)

I'm a fan of using the unnamed namespace instead of declaring functions |static|. I know that the style guide says we prefer |static| [1], I just disagree. (Don't change this; I just wanted to complain :)

[1] https://developer.mozilla.org/en-US/docs/Developer_Guide/Coding_Style#Anonymous_namespaces

@@ +1183,5 @@
> +  nsTArray< nsRefPtr<dom::Touch> >& touches = aEvent->touches;
> +  for (uint32_t i = 0; i < touches.Length(); ++i) {
> +    dom::Touch* touch = touches[i];
> +    if (!touch) {
> +      continue;

Can this actually happen? Should we warn?

@@ +1186,5 @@
> +    if (!touch) {
> +      continue;
> +    }
> +    int32_t id = touch->Identifier();
> +    WinUtils::Log("   id=%d target=%s", id, aMsg);

Should we rename |aMsg| to something like |aTarget|?
Attachment #825329 - Flags: review?(tabraldes) → review+
(Assignee)

Comment 30

5 years ago
Created attachment 825415 [details] [diff] [review]
fix ContentReceivedTouch v.3

With matt's patch in bug 927033 this is working well.
Attachment #825400 - Attachment is obsolete: true
Attachment #825415 - Flags: review?(bugmail.mozilla)
Comment on attachment 825415 [details] [diff] [review]
fix ContentReceivedTouch v.3

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

A few nits below. But my main concern is that the APZCTreeManager currently has code to redirect input events from from one APZC instance to another in the middle of a touch block, which can leave your touch-track code in an inconsistent state. Specifically, say you have a page with an iframe, there is an APZC for the root document and one for the iframe. If you start panning with one finger on the iframe, and the add a second finger to turn it into a pinch, the panning-related touch events will be directed to the inner APZC but as soon as the second touch point appears the events will be directed to the parent APZC. The inner APZC is never notified of the switch and so will still think there is one finger down; if both fingers are subsequently lifted then the inner APZC will be in a bad state.

I think overall the approach of this patch is fine but it needs to handle this case. The simplest way would be to add another function to clear the state when the APZCTreeManager starts routing events to the parent APZC.

::: gfx/layers/ipc/AsyncPanZoomController.cpp
@@ +56,5 @@
>  // #define APZC_LOG(...) printf_stderr("APZC: " __VA_ARGS__)
> +// #include "WinUtils.h"
> +// #define APZC_LOG(...) mozilla::widget::WinUtils::Log("APZC: " __VA_ARGS__)
> +#define APZC_LOG_FM(fm, prefix, ...)
> +#ifndef APZC_LOG_FM

I'd rather leave this the way it was originally unless you have a strong argument against. As we discussed on IRC printf_stderr also goes to the debugger if there is one attached so for most use cases the existing logging code should be fine.

@@ +483,5 @@
> +          APZC_LOG("%p touchblock: *warning* mismatch id=%d\n", this, multiTouchInput.mTouches[idx].mIdentifier);
> +          MOZ_ASSERT("mismatched touch pointer ids in this touch block!");
> +        }
> +        APZC_LOG("%p touchblock: remove id=%d\n", this, multiTouchInput.mTouches[idx].mIdentifier);
> +        mTouchBlockPointIds.RemoveElement(multiTouchInput.mTouches[idx].mIdentifier);

Can you just use the return value from RemoveElement rather than doing a separate call to Contains above?

::: gfx/layers/ipc/AsyncPanZoomController.h
@@ +520,5 @@
>    already_AddRefed<GeckoContentController> GetGeckoContentController();
>    already_AddRefed<GestureEventListener> GetGestureEventListener();
>  
> +  bool mDisableNextTouchBlock;
> +  nsTArray<uint32_t> mTouchBlockPointIds;

I think this should be int32_t, since that's what mIdentifier is.
Attachment #825415 - Flags: review?(bugmail.mozilla) → review-
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #31)
> I think overall the approach of this patch is fine but it needs to handle
> this case. The simplest way would be to add another function to clear the
> state when the APZCTreeManager starts routing events to the parent APZC.

Thinking about this a bit more, it might be simpler to move this touch block code into APZCTreeManager and build on top of what mbrubeck did in bug 927033.
(Assignee)

Comment 33

5 years ago
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #32)
> (In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #31)
> > I think overall the approach of this patch is fine but it needs to handle
> > this case. The simplest way would be to add another function to clear the
> > state when the APZCTreeManager starts routing events to the parent APZC.
> 
> Thinking about this a bit more, it might be simpler to move this touch block
> code into APZCTreeManager and build on top of what mbrubeck did in bug
> 927033.

Ok, I can do that.

I'm concerned though about wrapping here. (My original patch didn't deal with this either really.) What are the chances we'll get a delayed ContentReceivedTouch for a previous touch block on b2g or android?

Seems like the tree manager needs a way to correlate an apz to a touch block, and to track a set of touch ids for touch blocks. Also as you pointed out it also needs to be able to shift all this information from one apz to another in the event that the target apz changes. ContentReceivedTouch also needs to identify the touch block it's associated with. Or maybe this is all overkill?

For metro wrapping isn't an issue, so maybe we could fix ContentReceivedTouch here ignoring wrapping, and file a follow up on revamping how we track state.
(Assignee)

Comment 34

5 years ago
Note I can improve my existing patch ever so slightly here, by checking mTouchBlockPointIds.Length() before setting mDisableThisTouchBlock to true. This way a simple touchdown -> touchup -> ContentReceivedTouch(true) can't kill the subsequent touch block accidentally. But this doesn't protect against a really late ContentReceivedTouch(true) that comes in after we start processing the next block on the input thread.
(Assignee)

Updated

5 years ago
Whiteboard: [block28] → [block28][leave-open]
(Assignee)

Comment 35

5 years ago
Created attachment 826053 [details] [diff] [review]
fix ContentReceivedTouch v.4

This relies on the touch counting we do in bug 927033. If you like I can bring in the pointer id tracking I had in the previous patch.
Attachment #825415 - Attachment is obsolete: true
Attachment #826053 - Flags: feedback?(bugmail.mozilla)
(Assignee)

Comment 36

5 years ago
I wonder if we should make ContentReceivedTouch an internal tree manager call that it calls after receiving touchcancel for all touch points while the target apzc is in state WAITING_LISTENERS? Since our touch tracking expects touchcancels for all points anyway, it seems silly to expect callers to call both ContentReceivedTouch and send touchcancel events.

The flip side of this would be to keep ContentReceivedTouch as public and require callers to call it, but not require touchcancel events.
(Assignee)

Updated

5 years ago
Attachment #825329 - Attachment description: metroinput touch point debugging v.1 → metroinput touch point debugging v.1 [checkedin]
Comment on attachment 826053 [details] [diff] [review]
fix ContentReceivedTouch v.4

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

Sorry for the late feedback; I've been try to focus as much as I can to figure out the zooming issues and so have been putting everything else off :( I do prefer the approach of this patch to the previous one.

::: gfx/layers/composite/APZCTreeManager.cpp
@@ +242,5 @@
>        const MultiTouchInput& multiTouchInput = aEvent.AsMultiTouchInput();
>        if (multiTouchInput.mType == MultiTouchInput::MULTITOUCH_START) {
>          mTouchCount++;
>          mApzcForInputBlock = GetTargetAPZC(ScreenPoint(multiTouchInput.mTouches[0].mScreenPoint));
> +        nsRefPtr<AsyncPanZoomController> oldBlockApzc = mApzcForInputBlock;

This part isn't right, I think. Consider the case where you have a page with an iframe. You put your finger down on the iframe and start panning. At this point mApzcForInputBlock is the APZC for the iframe, and all events are getting directed straight there. Then, while your finger is still down, you drag it outside the bounds of the iframe. mApzcForInputBlock is still the iframe, and events are still going there. Now you put down a second finger (also outside the iframe) to pinch. At this point this MULTITOUCH_START code will run, GetTargetAPZC(mTouches[0]) will return the top-level page, and that's what you're going to set as your oldBlockApzc, when really you want the oldBlockApzc to be the APZC for the iframe.

@@ +256,5 @@
> +          // it's not going to be receiving any additional input for this touch
> +          // block.
> +          if (oldBlockApzc && oldBlockApzc != mApzcForInputBlock) {
> +            oldBlockApzc->CancelTouchBlock();
> +          }

I think you can move this if condition outside the for loop. No events are actually sent to the APZC inside the loop so there's no need to cancel touch blocks on them.

::: gfx/layers/ipc/AsyncPanZoomController.cpp
@@ +54,5 @@
>  
>  #define APZC_LOG(...)
>  // #define APZC_LOG(...) printf_stderr("APZC: " __VA_ARGS__)
> +#define APZC_LOG_FM(fm, prefix, ...)
> +#ifndef APZC_LOG_FM

I'd prefer if you just left this as-is. Having the framemetrics logging on by default is quite useful.
Attachment #826053 - Flags: feedback?(bugmail.mozilla) → feedback+
(In reply to Jim Mathies [:jimm] from comment #33)
> I'm concerned though about wrapping here. (My original patch didn't deal
> with this either really.) What are the chances we'll get a delayed
> ContentReceivedTouch for a previous touch block on b2g or android?

It is quite possible for this to happen on android at least, because of the additional UI thread that operates separately from the gecko thread. Lags in gecko can easily result in the wrapping scenario. Less likely on b2g and (for now) metro but still possible. It would be good to come up with a solution that handles the wrapping scenario as well.

> Seems like the tree manager needs a way to correlate an apz to a touch
> block, and to track a set of touch ids for touch blocks. Also as you pointed
> out it also needs to be able to shift all this information from one apz to
> another in the event that the target apz changes. ContentReceivedTouch also
> needs to identify the touch block it's associated with. Or maybe this is all
> overkill?

I think you can get by without some of this. It would be worth reading through the documentation in mobile/android/base/gfx/TouchEventHandler.java to see how I solved this problem there. Some elements of that can be reused here, appropriately modified for the multi-apzc cases.

> For metro wrapping isn't an issue, so maybe we could fix
> ContentReceivedTouch here ignoring wrapping, and file a follow up on
> revamping how we track state.

I'm ok with doing this as a fallback but I'd like to spend a bit of time trying to see if we can at least come up with a plan on how to solve the wrapping problem.
(Assignee)

Updated

5 years ago
Attachment #826053 - Attachment is obsolete: true
(Assignee)

Comment 39

5 years ago
Created attachment 828699 [details] [diff] [review]
part 1 - return the guid of the target apzc v.1
(Assignee)

Comment 40

5 years ago
Created attachment 828700 [details] [diff] [review]
part 2 - metro widget guid changes v.1
(Assignee)

Comment 41

5 years ago
Created attachment 828702 [details] [diff] [review]
part 3 - port balancing logic v.1
(Assignee)

Comment 42

5 years ago
Created attachment 828737 [details] [diff] [review]
part 1 - return the guid of the target apzc v.1
Attachment #828699 - Attachment is obsolete: true
(Assignee)

Comment 43

5 years ago
Created attachment 828738 [details] [diff] [review]
part 2 - metro widget guid changes v.1
Attachment #828700 - Attachment is obsolete: true
(Assignee)

Comment 44

5 years ago
Created attachment 828739 [details] [diff] [review]
part 3 - port balancing logic v.1
Attachment #828702 - Attachment is obsolete: true
(Assignee)

Comment 45

5 years ago
Created attachment 828740 [details] [diff] [review]
part 4 - deal with apz target changes in tree manager v.1
(Assignee)

Comment 46

5 years ago
Created attachment 828744 [details] [diff] [review]
part 5 - debugging output (optional)
(Assignee)

Updated

5 years ago
Blocks: 936085
(Assignee)

Comment 47

5 years ago
Comment on attachment 828737 [details] [diff] [review]
part 1 - return the guid of the target apzc v.1

matt's going to land bug 927033 shortly so these should be good to go.
Attachment #828737 - Flags: review?(bugmail.mozilla)
(Assignee)

Comment 48

5 years ago
Comment on attachment 828738 [details] [diff] [review]
part 2 - metro widget guid changes v.1

Metro widget part of tracking the guid plus some refactoring. The core change is in MetroInput where we call ApzReceiveInputEvent retrieving the guid, and then hand that back in calls to ApzContentConsumingTouch/ApzContentIgnoringTouch.

The refactoring is a move toward having a non-static tree manager object - we plan to support multiple widgets next year.
Attachment #828738 - Flags: review?(bugmail.mozilla)
(Assignee)

Comment 49

5 years ago
Created attachment 828930 [details] [diff] [review]
part 3 - port balancing logic v.1
Attachment #828739 - Attachment is obsolete: true
(Assignee)

Comment 50

5 years ago
Created attachment 828931 [details] [diff] [review]
part 4 - deal with apz target changes in tree manager v.1
Attachment #828740 - Attachment is obsolete: true
(Assignee)

Comment 51

5 years ago
Created attachment 828932 [details] [diff] [review]
part 5 - touchstart fix for metro input v.1
Attachment #828744 - Attachment is obsolete: true
(Assignee)

Comment 52

5 years ago
Created attachment 828934 [details] [diff] [review]
part 6 - logging (optional)
(Assignee)

Updated

5 years ago
Attachment #828932 - Flags: review?(mbrubeck)
(Assignee)

Comment 53

5 years ago
As soon as I finish up a build, I'll post patches merged to tip.
(Assignee)

Comment 54

5 years ago
Created attachment 829294 [details] [diff] [review]
part 1 - return the guid of the target apzc v.1
Attachment #828737 - Attachment is obsolete: true
Attachment #828737 - Flags: review?(bugmail.mozilla)
Attachment #829294 - Flags: review?(bugmail.mozilla)
(Assignee)

Comment 55

5 years ago
Created attachment 829297 [details] [diff] [review]
part 2 - metro widget guid changes v.1

matt mind reviewing this one? it's widget related. See comment 48 for a description.
Attachment #828738 - Attachment is obsolete: true
Attachment #828738 - Flags: review?(bugmail.mozilla)
Attachment #829297 - Flags: review?(mbrubeck)
(Assignee)

Comment 56

5 years ago
Created attachment 829299 [details] [diff] [review]
part 3 - port balancing logic v.1
Attachment #828930 - Attachment is obsolete: true
(Assignee)

Comment 57

5 years ago
Created attachment 829300 [details] [diff] [review]
part 4 - deal with apz target changes in tree manager v.1
Attachment #828931 - Attachment is obsolete: true
(Assignee)

Comment 58

5 years ago
Created attachment 829302 [details] [diff] [review]
part 3 - touchstart fix for metro input v.1
Attachment #828932 - Attachment is obsolete: true
Attachment #828932 - Flags: review?(mbrubeck)
Attachment #829302 - Flags: review?(mbrubeck)
(Assignee)

Comment 59

5 years ago
Created attachment 829303 [details] [diff] [review]
part 6 - logging (optional)
Attachment #828934 - Attachment is obsolete: true
Comment on attachment 829294 [details] [diff] [review]
part 1 - return the guid of the target apzc v.1

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

::: gfx/layers/ipc/AsyncPanZoomController.h
@@ +224,5 @@
>     */
>    nsEventStatus HandleInputEvent(const InputData& aEvent);
>  
>    /**
> +   * Returns the scrollable guid of this apzc.

Change this to "Populates the provided object with the scrollable guid of this apzc". Maybe also rename the variable to aOutGuid for consistency.
Attachment #829294 - Flags: review?(bugmail.mozilla) → review+
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #60)
> Comment on attachment 829294 [details] [diff] [review]
> part 1 - return the guid of the target apzc v.1
> 
> Review of attachment 829294 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/layers/ipc/AsyncPanZoomController.h
> @@ +224,5 @@
> >     */
> >    nsEventStatus HandleInputEvent(const InputData& aEvent);
> >  
> >    /**
> > +   * Returns the scrollable guid of this apzc.
> 
> Change this to "Populates the provided object with the scrollable guid of
> this apzc". Maybe also rename the variable to aOutGuid for consistency.

Or, to be even more consistent, aGuidOut (like e.g. aTransformToApzcOut [1]).

[1] http://mxr.mozilla.org/mozilla-central/source/gfx/layers/composite/APZCTreeManager.cpp#744
(Assignee)

Comment 62

5 years ago
Comment on attachment 829299 [details] [diff] [review]
part 3 - port balancing logic v.1

up next..
Attachment #829299 - Flags: review?(bugmail.mozilla)
Comment on attachment 829299 [details] [diff] [review]
part 3 - port balancing logic v.1

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

So for the most part this looks good. One problem is that by itself this change will probably have unexpected effects on B2G, because B2G is also going to end up using this code (and incorrectly). So we'll need to fix that up before we can land this. The other thing is that code assumes the metro definition of a touch block, where the block extends from the first touch down to the last touch up. While that would be fine if this were just on Metro, it means we'll have to use the same definition on B2G. As discussed during our meeting today, I'm not sure if it's better to use this definition on both B2G and Metro now and then change them both later, or just change it now in Metro and then use the right one going forward.
Comment on attachment 829299 [details] [diff] [review]
part 3 - port balancing logic v.1

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

Minusing for now, some comments below.

::: gfx/layers/ipc/AsyncPanZoomController.cpp
@@ +325,5 @@
> +  // Clear any pending touch block timeouts
> +  while (!mTouchListenerTimeoutTasks.IsEmpty()) {
> +    CancelableTask* task = mTouchListenerTimeoutTasks[0];
> +    task->Cancel();
> +    mTouchListenerTimeoutTasks.RemoveElementAt(0);

This code will run on the compositor thread, so access to mTouchListenerTimeoutTasks needs to be thread-safe.

@@ +375,5 @@
>      const MultiTouchInput& multiTouchInput = aEvent.AsMultiTouchInput();
>      if (multiTouchInput.mType == MultiTouchInput::MULTITOUCH_START) {
> +      // Only set this state if we are on the start of a new touch block.
> +      if (queueHasClosedBlock) {
> +        SetState(WAITING_LISTENERS);

I think there's a scenario here that falls through the cracks:

Say you put a finger down and move it around some. APZC goes into the WAITING_LISTENERS state and builds up a queue of touches. Then you get a ContentReceivedTouch(false) and so ProcessTouchBlock runs and clears out the queue. As part of the processing of the touch block mState ends up in state TOUCHING or PANNING or something.

Now you put a second finger down. In this case TouchQueueHasClosedBlock() will return true because the queue is empty, all of the conditions here will pass, and we'll set state WAITING_LISTENERS and queue up a TimeoutTouchListeners call.

However, because this is a second-finger-goes-down touch block, it is inconsistent with Metro's definition of a touch block, and so Metro will never inform the APZC of the ContentReceivedTouch status of this touch block. This will throw the balance out of whack. Or am I missing something?

@@ +387,5 @@
> +      if (multiTouchInput.mType == MultiTouchInput::MULTITOUCH_START) {
> +        // Only fire off a new TimeoutTouchListener for the first touchstart in
> +        // a touch block.
> +        if (queueHasClosedBlock) {
> +          CancelableTask* task = 

nit: trailing whitespace

@@ +1428,5 @@
>  }
>  
> +bool AsyncPanZoomController::TouchQueueHasClosedBlock() {
> +  MonitorAutoLock lock(mTouchQueueMonitor);
> +  int32_t balanceCount = 0;

s/balanceCount/pointerCount/ ?

@@ +1498,1 @@
>    if (!mFrameMetrics.mMayHaveTouchListeners) {

Eventually we'll have to deal with mMayHaveTouchListeners changing but right now I think that never happens (another bug)

::: gfx/layers/ipc/AsyncPanZoomController.h
@@ +738,5 @@
> +   /* An array of touch timeouts that have been sent */
> +  nsTArray<CancelableTask*> mTouchListenerTimeoutTasks;
> +   /* The "processing balance" value */
> +  int32_t mProcessingBalance;
> +   /* mTouchQueue monitor */

This comment isn't much use. Might as well remove it, but update the comments of the other variables to indicate that they are protected by this monitor.
Attachment #829299 - Flags: review?(bugmail.mozilla) → review-
Comment on attachment 829300 [details] [diff] [review]
part 4 - deal with apz target changes in tree manager v.1

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

::: gfx/layers/ipc/AsyncPanZoomController.cpp
@@ +1462,5 @@
> +  MonitorAutoLock lock(mTouchQueueMonitor);
> +  mTouchQueue.Clear();
> +  // We can't cancel ContentReceivedTouch calls, so save the
> +  // number of calls we are expecting so we can ignore them.
> +  mIgnoreContentReceivedTouchCount = mContentReceivedTouchCount;

I don't know if we need to do this or the call to ClearTouchTimers() above. The processing balance already tracks what's outstanding. As long as the mTouchQueue is cleared we should be fine with this APZC receiving whatever ContentReceivedTouch and TimeoutTouchListeners are outstanding, because they will effectively be no-ops on an empty touch queue.
Attachment #829300 - Flags: feedback+
Comment on attachment 825329 [details] [diff] [review]
metroinput touch point debugging v.1

In other components we set the checkin+ flag on landed patches in multi-patch bugs. Hope you don't mind if we do the same here.
Attachment #825329 - Attachment description: metroinput touch point debugging v.1 [checkedin] → metroinput touch point debugging v.1
Attachment #825329 - Flags: checkin+
Attachment #829297 - Flags: review?(mbrubeck) → review+
Attachment #829302 - Flags: review?(mbrubeck) → review+
(Assignee)

Comment 67

5 years ago
Comment on attachment 829299 [details] [diff] [review]
part 3 - port balancing logic v.1

Not going to do this now since B2G is totally incompatible with it.
Attachment #829299 - Attachment is obsolete: true
(Assignee)

Comment 68

5 years ago
Comment on attachment 829300 [details] [diff] [review]
part 4 - deal with apz target changes in tree manager v.1

If we still need some part of this I'll resurrect it.
Attachment #829300 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #829303 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #829302 - Attachment description: part 5 - touchstart fix for metro input v.1 → part 3 - touchstart fix for metro input v.1
(Assignee)

Comment 69

5 years ago
Created attachment 829546 [details] [diff] [review]
part 4 - add a transform helper to tree manager

Last part of this that we need here. Per your suggestion I skipped the transform caching and did basically the same thing we were doing with mouse events before. Works pretty well on a canvas drawing test case I have which I'll post.
Attachment #829546 - Flags: review?(bugmail.mozilla)
(Assignee)

Comment 70

5 years ago
Created attachment 829548 [details]
cavas drawing test case
Comment on attachment 829546 [details] [diff] [review]
part 4 - add a transform helper to tree manager

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

r=me with the change described below.

::: gfx/layers/composite/APZCTreeManager.h
@@ +194,5 @@
> +   * A helper for transforming coordinates to gecko coordinate space.
> +   *
> +   * @param aPoint point to transform
> +   */
> +  void TransformCoordinateToGecko(LayoutDeviceIntPoint& aPoint);

I'd prefer if this function took a const ScreenIntPoint as input and returned a LayoutDeviceIntPoint as output. That will match the actual operation that is happening. Eventually I want input events to have both as well rather than doing the in-place modification with confusing coordinate systems like they do now.
Attachment #829546 - Flags: review?(bugmail.mozilla) → review+
(Assignee)

Comment 72

5 years ago
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #71)
> Comment on attachment 829546 [details] [diff] [review]
> part 4 - add a transform helper to tree manager
> 
> Review of attachment 829546 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me with the change described below.
> 
> ::: gfx/layers/composite/APZCTreeManager.h
> @@ +194,5 @@
> > +   * A helper for transforming coordinates to gecko coordinate space.
> > +   *
> > +   * @param aPoint point to transform
> > +   */
> > +  void TransformCoordinateToGecko(LayoutDeviceIntPoint& aPoint);
> 
> I'd prefer if this function took a const ScreenIntPoint as input and
> returned a LayoutDeviceIntPoint as output. That will match the actual
> operation that is happening. Eventually I want input events to have both as
> well rather than doing the in-place modification with confusing coordinate
> systems like they do now.

Ok, no problem will update.

I pushed everything to try, will wait for the results there and if green, I'll push this over the weekend.
(Assignee)

Updated

5 years ago
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Whiteboard: [block28][leave-open] → [block28]
Target Milestone: --- → mozilla28
You need to log in before you can comment on or make changes to this bug.