Last Comment Bug 774139 - Dispatch touch events to out-of-process content
: Dispatch touch events to out-of-process content
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM: Events (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla17
Assigned To: Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
:
Mentors:
: 761934 (view as bug list)
Depends on: 774813 775403
Blocks: 819119 b2g-e10s-work 750977 761934 774458
  Show dependency treegraph
 
Reported: 2012-07-15 18:02 PDT by Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
Modified: 2012-12-06 14:55 PST (History)
11 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
+


Attachments
Forward touch events across processes (18.53 KB, patch)
2012-07-15 19:59 PDT, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
felipc: review+
bugs: review+
Details | Diff | Review
part 0: Rename 'NO' typename because that's a constant in Objective-C++ (1.91 KB, patch)
2012-07-17 01:39 PDT, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
bzbarsky: review+
Details | Diff | Review
part 1.1: Dispatch touch events with touch points (to be folded into previous patch) (3.95 KB, patch)
2012-07-17 01:40 PDT, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
no flags Details | Diff | Review
part 1.1: Dispatch touch events with touch points (to be folded into previous patch) , v2 (4.74 KB, patch)
2012-07-17 15:42 PDT, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
bugs: review+
Details | Diff | Review

Description Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-07-15 18:02:51 PDT
We don't do this.  We need to.

Setting to block bug 761934 because it will mostly likely fix it.

We need this for 750977 because our async pan/zoom controller needs to use touch events to compute the gestures it requires.
Comment 1 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-07-15 18:03:24 PDT
This is a basecamp blocker, for many different reasons! :)
Comment 2 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-07-15 19:59:23 PDT
Created attachment 642468 [details] [diff] [review]
Forward touch events across processes
Comment 3 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-07-15 19:59:49 PDT
*** Bug 761934 has been marked as a duplicate of this bug. ***
Comment 4 :Felipe Gomes (needinfo me!) 2012-07-15 22:35:41 PDT
Comment on attachment 642468 [details] [diff] [review]
Forward touch events across processes

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

This looks good to me. There are just two caveats that you need to be aware of:

1. The strategy you have set up probably won't be enough to implement the behavior of preventDefault() per the spec*, unless Wes set up some magic for that. The spec says that a call to preventDefault on touchstart or the first touchmove should prevent any mouse events associated with that touch point. This means that a webpage can call preventDefault only on touchstart, which in this code will block mousedown, but won't block the following mousemoves/up to be dispatched (because preventDefault might not be called on them).  (many webpages might not know or been relying on this though, so this is something to consider for your web-compat priorities)

Probably the best way to handle this will be to always forward touch and mouse, and let the child process keep track of that and block dispatch of the mouse events in that case. (This would have the advantage that you wouldn't need to create and dispatch the phony nsMouseEvent manually).
(It'd be worth to talk to Wes to see in which level he handled this on the platform.)

*http://www.w3.org/TR/touch-events/ sec. 5.4 and 5.6


2. Note that with this patch you'll block all mouse events from being dispatched in the parent when the target is a remote frame. If there is UI code using mouse events instead of touch that will be a problem. Solving (1) with what I proposed would also solve this I think.


The following are do-what-you-want-about-them nits:

::: content/events/src/nsEventStateManager.h
@@ +447,5 @@
>    mozilla::dom::TabParent *GetCrossProcessTarget();
>    bool IsTargetCrossProcess(nsGUIEvent *aEvent);
>  
> +  bool DispatchCrossProcessEvent(nsEvent* aEvent, nsIFrameLoader* remote,
> +                                 nsEventStatus *aStatus);

the change from void to bool is returned in HandleCrossProcessEvent but ultimately unused. Did you change it for future use/correctness?

My initial idea was that HandleCrossProcessEvent would return true/false if it was given an event that is meant to be dispatched cross-process (i.e., similar to the purpose of CrossProcessSafeEvent now), not if it successfully did it, mainly because I didn't think that the Send* functions could fail in any way that would useful to handle.

You can keep the change, I'm just explaining why it is the way it is at the moment.

::: dom/ipc/TabChild.cpp
@@ +704,5 @@
> +        refPoint = aEvent.touches[0]->mRefPoint;
> +    }
> +
> +    nsMouseEvent event(true, msg, NULL,
> +                       nsMouseEvent::eReal, nsMouseEvent::eNormal);

instead of true you could use NS_IS_TRUSTED_EVENT, assuming the flags are correctly created at the origin

you could also set, for completeness:
event.inputSource = nsIDOMMouseEvent::MOZ_SOURCE_TOUCH;
Comment 5 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-07-16 01:01:10 PDT
(In reply to Felipe Gomes (:felipe) from comment #4)
>
> 1. The strategy you have set up probably won't be enough to implement the
> behavior of preventDefault() per the spec*, unless Wes set up some magic for
> that.

This implementation follows what the gonk widget backend does[1], so if this patch violates spec so does the current widget code.

If we implement these semantics in Gecko proper, I would expect ConsumeNoDefault to be returned from the event dispatch, and both the gonk widget backend and the cross-process code will adhere to spec.

> Probably the best way to handle this will be to always forward touch and
> mouse, and let the child process keep track of that and block dispatch of
> the mouse events in that case.

This is excessively expensive, because it will require a round-trip through content to dispatch the synthesized mouse event.  I would actually prefer to delegate this logic to the content process.
 
> 2. Note that with this patch you'll block all mouse events from being
> dispatched in the parent when the target is a remote frame. If there is UI
> code using mouse events instead of touch that will be a problem. Solving (1)
> with what I proposed would also solve this I think.
> 

That should be fine, at least for b2g --- any web content loaded into the main process (for v1, the "system" app and browser app) will be touch-event-aware, and should use capturing listeners when they might want to take events away from remote content.

> ::: content/events/src/nsEventStateManager.h
> @@ +447,5 @@
> >    mozilla::dom::TabParent *GetCrossProcessTarget();
> >    bool IsTargetCrossProcess(nsGUIEvent *aEvent);
> >  
> > +  bool DispatchCrossProcessEvent(nsEvent* aEvent, nsIFrameLoader* remote,
> > +                                 nsEventStatus *aStatus);
> 
> the change from void to bool is returned in HandleCrossProcessEvent but
> ultimately unused. Did you change it for future use/correctness?
> 

Just to make the code simpler, so that I can simply return from the case statements :).

> My initial idea was that HandleCrossProcessEvent would return true/false if
> it was given an event that is meant to be dispatched cross-process (i.e.,
> similar to the purpose of CrossProcessSafeEvent now), not if it successfully
> did it, mainly because I didn't think that the Send* functions could fail in
> any way that would useful to handle.
> 

I think that's still basically true.  Since we currently ignore the return value from HandleCrossProcessEvent(), I don't think it matters all that much currently.

> You can keep the change, I'm just explaining why it is the way it is at the
> moment.
> 

Sure, will keep.  Thanks for explaining.

> ::: dom/ipc/TabChild.cpp
> @@ +704,5 @@
> > +        refPoint = aEvent.touches[0]->mRefPoint;
> > +    }
> > +
> > +    nsMouseEvent event(true, msg, NULL,
> > +                       nsMouseEvent::eReal, nsMouseEvent::eNormal);
> 
> instead of true you could use NS_IS_TRUSTED_EVENT, assuming the flags are
> correctly created at the origin
> 
> you could also set, for completeness:
> event.inputSource = nsIDOMMouseEvent::MOZ_SOURCE_TOUCH;

Will make these changes.

Thanks again for the quick review.

[1] http://mxr.mozilla.org/mozilla-central/source/widget/gonk/nsAppShell.cpp#332
Comment 6 Olli Pettay [:smaug] 2012-07-16 07:45:42 PDT
(In reply to Felipe Gomes (:felipe) from comment #4)
> 1. The strategy you have set up probably won't be enough to implement the
> behavior of preventDefault() per the spec*, unless Wes set up some magic for
> that. The spec says that a call to preventDefault on touchstart or the first
> touchmove should prevent any mouse events associated with that touch point.
I believe this behavior is needed by web compatibility.
I don't recall how it is done in Fennec.
Comment 7 Wesley Johnston (:wesj) 2012-07-16 09:31:26 PDT
I'll have to read this to know what you're doing, but in Fennec (and in every other mobile browser in the world) mouse events are only sent at the end of a touch series. i.e. after the user taps down and up, if preventDefault wasn't called we send mousemove, mousedown, mouseup, click (the mousemove could be sent on touchstart if you wanted, but might lead to strange behaviors on some sites, so might be better delayed by 50ms or so). Last I heard (from PPK), B2G was the only mobile browser not following this convention. I assumed at the time because touch events support was fairly new. Is that still going on? We prevent sending mouse events (but still send them for XUL Fennec) here: http://mxr.mozilla.org/mozilla-central/source/widget/android/nsWindow.cpp#1290

You should be able to trust the status returned from dispatching the event. PresShell keeps track of whether or not preventDefault can be called on this particular touch series, and keeps returning prevent default for all events in the set. (i.e. if the page calls preventDefault on touchstart, we will return status = nsEventStatus_eConsumeNoDefault for all subsequent events in the "set"). Likewise, a site calling preventDefault on the third touchmove event won't affect the state returned.

The platform isn't smart enough (yet) to discern between calling preventDefault on the first finger in a two finger touch, and calling preventDefault on the second. Once you've called on either touch, we always return status = nsEventStatus_eConsumeNoDefault for all touch events until there are no fingers on the screen.

Some optimizations there originally led to problems. Fixing them was non-trivial.
"content" sends the preventDefault message back to java from nsWindow.cpp here: http://mxr.mozilla.org/mozilla-central/source/widget/android/nsWindow.cpp#1393
java handles the touch events here touchEventHandler.java here: http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/gfx/TouchEventHandler.java

I can go into more details here or on IRC. Kats also wrote a lot of this nsWindow and TouchEventHandler code, so feel free to ask him.
Comment 8 Wesley Johnston (:wesj) 2012-07-16 09:35:51 PDT
(In reply to Wesley Johnston (:wesj) from comment #7)
> The platform isn't smart enough (yet) to discern between calling
> preventDefault on the first finger in a two finger touch, and calling
> preventDefault on the second. Once you've called on either touch, we always
> return status = nsEventStatus_eConsumeNoDefault for all touch events until
> there are no fingers on the screen.

I also spent some time looking at this behavior on various mobile browsers. They are not consistent. I feel like our current behavior is a little bit restrictive to web authors (I'm sure there's some weird behavior you can only get by allowing a second finger to pan but not the first), but also far more predictable. Call preventDefault and everything platformy stops.
Comment 9 Olli Pettay [:smaug] 2012-07-16 09:38:48 PDT
Comment on attachment 642468 [details] [diff] [review]
Forward touch events across processes

Based on that, r-
Comment 10 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-07-16 10:47:37 PDT
Wes, I don't understand what the widget backend is responsible for, i.e. what core gecko *isn't* doing.  Is that documented anywhere?
Comment 11 Wesley Johnston (:wesj) 2012-07-16 10:54:54 PDT
Not well. Sorry :(

Widget backend is responsible for dispatching the touch events and monitoring the response to see if preventDefault was called. If preventDefault wasn't called, its responsible for doing any other work processing the events for gesture detection, scrolling, etc. Part of gesture detection could be dispatching mouse events if a tap is detected as well (i.e. making desktop websites compatible). However, on Android tap/longtap/doubletap/pinchzoom all use the system's detectors (pinchzoom is using a custom detector written in Java now) so that ends up being a little bit out of band of our widget stuff. Those system events are sent to Javascript in browser.js where they are handled. I'd love to move some of that into widget or platform if possible. Happy to help write some of that.

http://mxr.mozilla.org/mozilla-aurora/source/mobile/android/base/ui/PanZoomController.java#980
Comment 12 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-07-16 11:06:08 PDT
So I still don't understand ... what layer queues(?) the touch events and uses them to dispatch mouse events?  Is that gecko or the widget backend?
Comment 13 Wesley Johnston (:wesj) 2012-07-16 11:14:28 PDT
Java sends a message to browser.js which dispatches the mouse events using DOMWindowUtils.

In more detail, touch events and are sent from the Java UI thread to the Gecko thread (nsWindow.cpp widget code) here:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/gfx/TouchEventHandler.java#199

Gecko monitors the events to see if preventDefault was called:
http://mxr.mozilla.org/mozilla-central/source/widget/android/nsWindow.cpp#1396

In the mean time, Java queues the touch events and waits to see if preventDefault is called. It also runs a timer so if we don't hear back from Gecko within some minimum time limit, we send the touches on anyway (i.e. we have to keep scrolling async even under bad conditions).

If preventDefault is not called (or the timer fires), We process the events in Java which includes sending them to a Java gesture detector:
http://mxr.mozilla.org/mozilla-aurora/source/mobile/android/base/gfx/TouchEventHandler.java#243

Which (if it detects a tap) calls:
http://mxr.mozilla.org/mozilla-aurora/source/mobile/android/base/ui/PanZoomController.java#994

which sends a message to browser.js:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#3365

which fires mouse events for us. Better? To much?
Comment 14 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-07-16 11:24:31 PDT
I fear that as long as this is done in platform- and frontend-specific code, we're going to suffer from the problem of fragmentation of behavior you refer to.

smaug, wes, since the gonk widget backend is already not adhering to spec, and that's an orthogonal problem, how about we take this patch as-is and fix both in a followup?  The current patch won't regress behavior.
Comment 15 Olli Pettay [:smaug] 2012-07-16 11:27:03 PDT
As far as I know, that patch isn't web compatible, so I don't understand why we should take it.
Comment 16 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-07-16 11:28:10 PDT
Apparently, neither is gonk's nsWindow.cpp.
Comment 17 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-07-16 11:37:03 PDT
The calculus to me is

In current m-c
 - gonk/b2g isn't spec compliant with touch events
 - content processes don't receive touch events at all
 - we can't use async pan/zoom in gecko

If we take this patch
 - gonk/b2g isn't spec compliant with touch events
 - content process will receive touch events, which un-breaks a lot of content
 - we can use async pan/zoom

That's why I propose landing this patch now, and then fixing gonk/b2g in a separate bug.
Comment 18 Wesley Johnston (:wesj) 2012-07-16 11:38:22 PDT
To move our click handlers into java (and make touch handling slightly better), Fennec would require something like bug 547997 to land (i.e. redirect clicks to something close by).
Comment 19 Wesley Johnston (:wesj) 2012-07-16 11:39:36 PDT
oops. s/into java/out of javascript/.

my 2 cents on landing, I have a feeling that if gonk isn't fixed here it won't be, but that's a decision for someone else to make I guess.
Comment 20 Olli Pettay [:smaug] 2012-07-16 11:48:28 PDT
(In reply to Chris Jones [:cjones] [:warhammer] from comment #17)
> The calculus to me is
> 
> In current m-c
>  - gonk/b2g isn't spec compliant with touch events
>  - content processes don't receive touch events at all
>  - we can't use async pan/zoom in gecko
> 
> If we take this patch
>  - gonk/b2g isn't spec compliant with touch events
>  - content process will receive touch events, which un-breaks a lot of
> content

And break a lot of content
Comment 21 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-07-16 11:49:41 PDT
(In reply to Olli Pettay [:smaug] from comment #20)
> (In reply to Chris Jones [:cjones] [:warhammer] from comment #17)
> > The calculus to me is
> > 
> > In current m-c
> >  - gonk/b2g isn't spec compliant with touch events
> >  - content processes don't receive touch events at all
> >  - we can't use async pan/zoom in gecko
> > 
> > If we take this patch
> >  - gonk/b2g isn't spec compliant with touch events
> >  - content process will receive touch events, which un-breaks a lot of
> > content
> 
> And break a lot of content

No, it's *already* broken wrt spec compliance.  That's my point.  *No* touch events breaks more content than non-spec-compliant touch events.
Comment 22 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-07-16 11:50:27 PDT
(In reply to Wesley Johnston (:wesj) from comment #19)
> oops. s/into java/out of javascript/.
> 
> my 2 cents on landing, I have a feeling that if gonk isn't fixed here it
> won't be, but that's a decision for someone else to make I guess.

You impugn my honor, sir!  It will absolutely be fixed!
Comment 23 Olli Pettay [:smaug] 2012-07-16 12:55:57 PDT
Comment on attachment 642468 [details] [diff] [review]
Forward touch events across processes

Ok, let's take this for now to let
b2g apps be oop, but someone needs to fix touch event for b2g properly.
Comment 24 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-07-16 13:04:22 PDT
The key piece of missing information here, per IRC chat, was this affects *all* cross-process content in b2g: "browser tabs" *and* apps.  Sorry that wasn't clear.
Comment 25 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-07-16 14:23:56 PDT
I added FIXME/bug 774458 comments to widget/gonk/nsAppShell.cpp and TabChild.
Comment 26 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-07-16 14:47:05 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/433ca26168bb
Comment 27 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-07-16 15:24:09 PDT
Lost inbound roulette, backed out in

https://hg.mozilla.org/integration/mozilla-inbound/rev/0ed13a015623

... for breaking generated dom-bindings code?!?
Comment 28 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-07-16 16:52:01 PDT
These patches require pulling in DOM headers to serialize nsTouchEvent (sigh).  The even-serializing code is used for plugins, and on mac we have some Object-C++ code that ends up including this header.

dombindings.h (pretty sure not autogenerated?) typedef's NameOps to NO, but ... NO is a constant/keyword in Objective-C++ :(.  I'm changing the typedef to NP.  It's not pretty, but a problem we were probably going to run into eventually.
Comment 29 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-07-16 17:21:16 PDT
On the test page at http://people.mozilla.com/~cjones/events.html, I see touch events delivered properly in fennec, but not even touchstart delivered on b2g.  Hmmmm ....
Comment 30 Wesley Johnston (:wesj) 2012-07-16 18:42:37 PDT
The page at http://www.quirksmode.org/m/tests/touch.html# is also useful for testing (although its pretty confusing as well). You can hit record, tap the colored boxes at the right, and it will output events. Settings lets you turn things like preventDefault on and off.
Comment 31 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-07-16 23:35:32 PDT
smaug, wesj, I'm getting the following behavior
 - touchstart is delivered to DispatchCrossProcessEvent(), from nsEventStateManager::PostHandleEvent()
 - thereafter, only synthetic mouse events are delivered (except for the synthesized touchend)

I think that makes sense, given the DOM spec.

I looked at what preventDefault() seems to do in nsPresShell::DispatchTouchEvent(), for touchstart.  It seems to set nsIPresShell::gPreventMouseEvents.

If, in DispatchCrossProcessEvent(), I manually set gPreventMouseEvents=true when receiving touchstart, then I see touchstart (followed by a synthetic mouse event generated in the content process).  But then I don't see *either* touchmove *or* synthetic mousemove, at all, in DispatchCrossProcessEvent().  It seems that nsEventStateManager::PostHandleEvent() isn't being called at all.

If you guys have pointers on what to do here, would love to hear! :)

I'm going to try hacking in a preventDefault=true in DispatchTouchEvent() when the target is <iframe remote>, and see if that "works".  I sort of doubt it though ...
Comment 32 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-07-16 23:49:19 PDT
OK, found some more stuff ... for the first touchmove event, we have the right event target (the <iframe remote>) in DispatchTouchEvent(), but somehow we've lost that by the time we hit HandleCrossProcessEvent() ...
Comment 33 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-07-16 23:55:44 PDT
Aha!  The event target appears to be set per nsDOMTouch, and HandleCrossProcessEvent() isn't checking that.
Comment 34 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-07-17 01:39:59 PDT
Created attachment 642889 [details] [diff] [review]
part 0: Rename 'NO' typename because that's a constant in Objective-C++

Sigh.
Comment 35 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-07-17 01:40:45 PDT
Created attachment 642890 [details] [diff] [review]
part 1.1: Dispatch touch events with touch points (to be folded into previous patch)
Comment 36 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-07-17 02:29:00 PDT
Oleg, heads up: it looks like the Qt builder doesn't like this patch

https://tbpl.mozilla.org/php/getParsedLog.php?id=13596284&tree=Try

I think we're going to need some |#undef signals| sprinkled around.
Comment 37 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-07-17 11:42:13 PDT
Comment on attachment 642889 [details] [diff] [review]
part 0: Rename 'NO' typename because that's a constant in Objective-C++

bz suggests NO->NOp
Comment 38 Boris Zbarsky [:bz] 2012-07-17 11:56:07 PDT
Comment on attachment 642889 [details] [diff] [review]
part 0: Rename 'NO' typename because that's a constant in Objective-C++

r=me with that
Comment 39 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-07-17 15:16:08 PDT
(In reply to Boris Zbarsky (:bz) from comment #38)
> Comment on attachment 642889 [details] [diff] [review]
> part 0: Rename 'NO' typename because that's a constant in Objective-C++
> 
> r=me with that

s/NP/NOp/ locally.
Comment 40 Olli Pettay [:smaug] 2012-07-17 15:25:43 PDT
Comment on attachment 642890 [details] [diff] [review]
part 1.1: Dispatch touch events with touch points (to be folded into previous patch)

># HG changeset patch
># User Chris Jones <jones.chris.g@gmail.com>
># Date 1342514297 25200
># Node ID bd9284d7f18e3f0b87afe525509216d160efe17d
># Parent  560298345ceadef460e2711f5c10987f00b96f83
>Bug 774139, part 1.1: Dispatch touch events with touch points. r=smaug
>
>diff --git a/content/events/src/nsEventStateManager.cpp b/content/events/src/nsEventStateManager.cpp
>--- a/content/events/src/nsEventStateManager.cpp
>+++ b/content/events/src/nsEventStateManager.cpp
>@@ -1731,50 +1731,79 @@ CrossProcessSafeEvent(const nsEvent& aEv
>     return false;
>   }
> }
> 
> bool
> nsEventStateManager::HandleCrossProcessEvent(nsEvent *aEvent,
>                                              nsIFrame* aTargetFrame,
>                                              nsEventStatus *aStatus) {
>-  if (!CrossProcessSafeEvent(*aEvent)) {
>+  if (*aStatus == nsEventStatus_eConsumeNoDefault ||
>+      !CrossProcessSafeEvent(*aEvent)) {
>     return false;
>   }
>-  nsIContent* target = mCurrentTargetContent;
>-  if (!target && aTargetFrame) {
>-    target = aTargetFrame->GetContent();
>+
>+  nsAutoTArray<nsCOMPtr<nsIContent>, 1> targets;
>+  if (aEvent->eventStructType != NS_TOUCH_EVENT ||
>+      aEvent->message == NS_TOUCH_START) {
>+    nsIContent* target = mCurrentTargetContent;
>+    if (!target && aTargetFrame) {
>+      target = aTargetFrame->GetContent();
>+    }
>+    if (IsRemoteTarget(target)) {
>+      targets.AppendElement(target);
>+    }
>+  } else {
>+    nsTouchEvent* touchEvent = static_cast<nsTouchEvent*>(aEvent);
>+    const nsTArray<nsCOMPtr<nsIDOMTouch> >& touches = touchEvent->touches;
>+    for (PRUint32 i = 0; i < touches.Length(); ++i) {
>+      nsIDOMTouch* touch = touches[i];
>+      // NB: the |mChanged| check is an optimization, subprocesses can
>+      // compute this for themselves.
>+      if (!touch || !touch->mChanged) {
>+        continue;
>+      }
>+      nsCOMPtr<nsPIDOMEventTarget> targetPtr;
nsCOMPtr<nsIDOMEventTarget> targetPtr;


>+      touch->GetTarget(getter_AddRefs(targetPtr));
>+      if (!targetPtr) {
>+        continue;
>+      }
>+      nsCOMPtr<nsIContent> target = do_QueryInterface(targetPtr);
>+      if (target && IsRemoteTarget(target) &&
>+          targets.NoIndex == targets.BinaryIndexOf(target)) {
>+        targets.InsertElementSorted(target);
Why sorted? Why binaryindexof?
Comment 41 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-07-17 15:30:30 PDT
Thanks smaug!

(In reply to Olli Pettay [:smaug] from comment #40)

> >+      nsCOMPtr<nsPIDOMEventTarget> targetPtr;
> nsCOMPtr<nsIDOMEventTarget> targetPtr;
> 

Done.

> 
> >+      touch->GetTarget(getter_AddRefs(targetPtr));
> >+      if (!targetPtr) {
> >+        continue;
> >+      }
> >+      nsCOMPtr<nsIContent> target = do_QueryInterface(targetPtr);
> >+      if (target && IsRemoteTarget(target) &&
> >+          targets.NoIndex == targets.BinaryIndexOf(target)) {
> >+        targets.InsertElementSorted(target);
> Why sorted? Why binaryindexof?

The elements of the array (remote targets) need to be unique for correctness.  Any number of touches can point at the same target.
Comment 42 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-07-17 15:42:47 PDT
Created attachment 643167 [details] [diff] [review]
part 1.1: Dispatch touch events with touch points (to be folded into previous patch) , v2

Switched away from sorted array and added comments.
Comment 43 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-07-17 15:47:09 PDT
Comment on attachment 643167 [details] [diff] [review]
part 1.1: Dispatch touch events with touch points (to be folded into previous patch) , v2

>+      if (IsRemoteTarget(target) &&
>+          targets.NoIndex == targets.Contains(target)) {

Obvious typo, --> !Contains(target) locally.

Note You need to log in before you can comment on or make changes to this bug.