Closed Bug 914829 Opened 11 years ago Closed 11 years ago

MetroInput should forward touch input to apz first, then to content

Categories

(Core Graveyard :: Widget: WinRT, defect)

Product:
Core Graveyard
Component:
Widget: WinRT
Version:
26 Branch
Platform:
x86_64
Windows 8.1
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
mozilla26

People

(Reporter: jimm, Assigned: jimm)

References

Details

Attachments

(1 file, 1 obsolete file)

patch v.2
(deleted), patch
TimAbraldes
: review+
kats
: review+
Details | Diff | Splinter Review 
<kats> jimm: so in that code the event is dispatched to content first, and then sent to the APZC if it's not prevent-default'ed, right?
<jimm> yes
<kats> jimm: APZC has a different mechanism to handle that scenario, which we should switch to using unless you have a good argument against
<kats> the problem with the current method is that DispatchEvent can take a long time as it will run scripts
<kats> and then if the scripts end up not doing the prevent default you have high latency in the visible scroll from apzc
<kats> jimm: instead the event should be given to apzc first, and then dispatched to the content
<kats> jimm: and if content does a prevent default on it, you can notify apzc to cancel
<kats> jimm: apzc has a built-in timeout, so if the prevent default is taking too long then it will just do the scroll anyway. this is similar to what we do on fennec
<kats> does that make sense?
<jimm> seems to. I'll file a bug on it, cc you in. 
<kats> jimm: excellent, thanks
<kats> this will also help nicklebedev with implementing his stuff
<jimm> kats: does apz avoid doing any scrolling until after the first touchmove?
<kats> jimm: yeah, it has to. it doesn't know which direction to scroll until a touchmove
<jimm> ok so if the front end cancels the first touchmove and we call apz->cancel(), we won't see any jitter
<yuan> sorry I have to miss the metro mtg today. will check the notes
<kats> jimm: to be honest i haven't examined the APZC code that does that in very much detail, but i know it exists and is in use on b2g. if there are bugs then we should fix them
<jimm> we do that a lot :)
<kats> jimm: yes i believe that should work
<jimm> ok cool
The API that needs to be called to notify the APZC of whether or not preventDefault was called is at [1].

The equivalent call on B2G happens at [2]. This happens *after* the input event is handed to the APZC (at [3]).

[1] http://hg.mozilla.org/mozilla-central/file/efc53394043e/gfx/layers/composite/APZCTreeManager.h#l195
[2] http://hg.mozilla.org/mozilla-central/file/efc53394043e/dom/ipc/TabChild.cpp#l1926
[3] http://hg.mozilla.org/mozilla-central/file/efc53394043e/dom/ipc/TabParent.cpp#l716
Blocks: 795567
Attached patch patch v.1 (obsolete) (deleted) — — Splinter Review 
Put this together and generally appears to be working, except the APZCTreeManager currently ignores the return results from the AsyncPanZoomController it delivers events too. Seems wrong. The events are delivered here - 

http://mxr.mozilla.org/mozilla-central/source/gfx/layers/composite/APZCTreeManager.cpp#241
Assignee: nobody → jmathies
The tree manager does appear to respect return results in the other ReceiveInputEvent where it generates a new nsInputEvent. However we don't need this method, we already have an nsInputEvent for sending. Kats, should I update the first ReceiveInputEvent?
Flags: needinfo?(bugmail.mozilla)
(In reply to Jim Mathies [:jimm] from comment #3)
> Kats, should
> I update the first ReceiveInputEvent?

Yes please.
Flags: needinfo?(bugmail.mozilla)
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #4)
> (In reply to Jim Mathies [:jimm] from comment #3)
> > Kats, should
> > I update the first ReceiveInputEvent?
> 
> Yes please.

In the other method I noticed we are applying transforms to the input data the apz hands back. Is that due to the data stored in the apz being transformed to match apz's world, and as such it needs to be transformed back before it's delivered to gecko? Just want to make sure the event coords we send from widget that do not go through the apz are correct in cases where we have say, active zoom state.
(In reply to Jim Mathies [:jimm] from comment #5)
> (In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #4)
> > (In reply to Jim Mathies [:jimm] from comment #3)
> > > Kats, should
> > > I update the first ReceiveInputEvent?
> > 
> > Yes please.
> 
> In the other method I noticed we are applying transforms to the input data
> the apz hands back. Is that due to the data stored in the apz being
> transformed to match apz's world, and as such it needs to be transformed
> back before it's delivered to gecko? Just want to make sure the event coords
> we send from widget that do not go through the apz are correct in cases
> where we have say, active zoom state.

Looks like they do need to be transformed. Which means all of our gecko input (touch, simple gestures, mouse, etc..) will need to get transformed by the apz.
Attached patch patch v.2 (deleted) — — Splinter Review 
tabraldes -> widget changes
Attachment #803016 - Attachment is obsolete: true
Attachment #803062 - Flags: review?(tabraldes)
Comment on attachment 803062 [details] [diff] [review]
patch v.2

kats -> minimal apz changes
Attachment #803062 - Flags: review?(bugmail.mozilla)
Yeah, ideally we would have only one ReceiveInputEvent method, which takes two InputData objects, one as an in-parameter and one as an out-parameter. That can then be called from threads other than the gecko thread (we need this ability for Fennec) and will return the untransformed input event (in a coordinate system that Gecko knows about). The untransformed input event should then be handed to Gecko (on the Gecko thread) via the usual DispatchWidgetEvent or whatever.

Currently it's kind of a mess because we have the two functions, one of which (the nsInputEvent version) does the untransform but can only be called from the Gecko thread and the other one (the InputData version) doesn't do the untransform but can be called from non-Gecko threads.

We will need some follow-up bugs to sort all this out. I was hoping to get to that once the current B2G 1.2 code crunch is past but if you have some spare cycles please feel free to take this on.
Comment on attachment 803062 [details] [diff] [review]
patch v.2

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

::: widget/windows/winrt/MetroWidget.cpp
@@ +982,5 @@
> +  LogFunction();
> +  if (!MetroWidget::sAPZC) {
> +    return;
> +  }
> +  MetroWidget::sAPZC->ContentReceivedTouch(mRootLayerTreeId, true);

Note that you can also call ContentReceivedTouch(.., false) if you know that content is *not* consuming the events. This should improve latency a little because then APZC won't have to wait for the full 300ms timeout before processing the input events. (See code relating to TimeoutTouchListeners in AsyncPanZoomController.cpp)
Attachment #803062 - Flags: review?(bugmail.mozilla) → review+
Comment on attachment 803062 [details] [diff] [review]
patch v.2

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

Looks good, but please change the AutoDelete class: Either make it specific to events or remove it entirely. From what I've read, we can't rely on deleting a void* to work properly.

::: widget/windows/winrt/MetroInput.cpp
@@ +188,5 @@
> +    AutoDelete(void* aPtr) :
> +      mPtr(aPtr) {}
> +    ~AutoDelete() {
> +      if (mPtr) {
> +        delete mPtr;

Deleting a void* doesn't do what we want, at least according to stackoverflow (and the C++ spec, as quoted on stackoverflow)

@@ +1124,5 @@
> +   *  check our result and set mTouchStartDefaultPrevented or
> +   *  mTouchMoveDefaultPrevented appropriately. Deliver touch events to the apz
> +   *  (ignoring return result) and to content and return the content event
> +   *  status result to our caller.
> +   * 2) mTouchStartDefaultPrevented or mTouchStartDefaultPrevented are true

nit: mTouchStartDefaultPrevented is repeated here

@@ +1127,5 @@
> +   *  status result to our caller.
> +   * 2) mTouchStartDefaultPrevented or mTouchStartDefaultPrevented are true
> +   *  Deliver touch directly to content and bypass the apz. Our callers
> +   *  handle calling cancel for the touch sequence on the apz.
> +   * 3) mTouchStartDefaultPrevented and mTouchStartDefaultPrevented are false

nit: same nit as above
Attachment #803062 - Flags: review?(tabraldes) → review+
(In reply to Tim Abraldes [:TimAbraldes] [:tabraldes] from comment #11)
> 
> ::: widget/windows/winrt/MetroInput.cpp
> @@ +188,5 @@
> > +    AutoDelete(void* aPtr) :
> > +      mPtr(aPtr) {}
> > +    ~AutoDelete() {
> > +      if (mPtr) {
> > +        delete mPtr;
> 
> Deleting a void* doesn't do what we want, at least according to
> stackoverflow (and the C++ spec, as quoted on stackoverflow)

Updated to take an event.

> nit: mTouchStartDefaultPrevented is repeated here
> 
> nit: same nit as above

Updated. Thanks!
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #14)
> Backed out along with bug 914829 for intermittent mochitest-mc crashes.

** Bug 914331 **
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #15)
> (In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #14)
> > Backed out along with bug 914829 for intermittent mochitest-mc crashes.
> 
> ** Bug 914331 **

That's the work that's causing the crashing. I can probably reland this after I rejigger my queue to test.
https://hg.mozilla.org/mozilla-central/rev/e97b40579201
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
Comment on attachment 803062 [details] [diff] [review]
patch v.2

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

::: widget/windows/winrt/MetroInput.cpp
@@ +181,5 @@
>      return PL_DHASH_NEXT;
>    }
> +
> +  // Helper for making sure event ptrs get freed.
> +  class AutoDelete

What's wrong with nsAutoPtr?
OS: Windows 8 Metro → Windows 8.1
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: