719647 - Add Touch Event support to Gonk widget backend

Status: RESOLVED FIXED

(Core :: Widget, defect)

Description

[Michael Wu [:mwu]]

Attachment: Add support for touch events to Gonk

Comment 1

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 590033 [details] [diff] [review]
Add support for touch events to Gonk

>diff --git a/widget/gonk/nsAppShell.cpp b/widget/gonk/nsAppShell.cpp

> static nsEventStatus
>+sendTouchEvent(PRUint32 msg, nsecs_t time,
>+               nsDOMTouch **touches, int32_t touchCount)

const nsTArray& touches

> // GeckoInputDispatcher
>+GeckoInputDispatcher::~GeckoInputDispatcher()

Not needed, see below.

>@@ -339,19 +371,50 @@ GeckoInputDispatcher::dispatchOnce()
>         PRUint32 msg;
>         switch (data.action) {
>         case AMOTION_EVENT_ACTION_DOWN:
>+        case AMOTION_EVENT_ACTION_POINTER_DOWN:
>+            msg = NS_TOUCH_START;
>+            break;
>+        case AMOTION_EVENT_ACTION_MOVE:
>+            msg = NS_TOUCH_MOVE;
>+            break;
>+        case AMOTION_EVENT_ACTION_UP:
>+        case AMOTION_EVENT_ACTION_POINTER_UP:
>+            msg = NS_TOUCH_END;
>+            break;
>+        case AMOTION_EVENT_ACTION_OUTSIDE:
>+        case AMOTION_EVENT_ACTION_CANCEL:
>+            msg = NS_TOUCH_CANCEL;
>+            break;
>+        }
>+        nsEventStatus status =
>+            sendTouchEvent(msg,
>+                           data.eventTime,
>+                           data.motion.touches,
>+                           data.motion.touchCount);
>+
>+        switch (data.action) {
>+        case AMOTION_EVENT_ACTION_DOWN:

Why do we unconditionally dispatch mouse events too?  Are there some
docs I can read, or another example you were looking at?

(I'm not trying to imply that this isn't right, I just don't know.)

>diff --git a/widget/gonk/nsAppShell.h b/widget/gonk/nsAppShell.h

>@@ -93,7 +94,8 @@ struct InputData {
>             int32_t scanCode;
>         } key;
>         struct {
>-            PointerCoords coords;
>+            nsDOMTouch **touches;
>+            int32_t touchCount;

|nsTArray<nsDOMTouch>* touches;|.  Bonus points for something like
|typedef nsTArray<nsDOMTouch> TouchArray;|.

|delete touches| in the destructor here, but only if the type is
motion.

>@@ -147,7 +149,7 @@ public:
>     virtual status_t unregisterInputChannel(const sp<InputChannel>& inputChannel);
> 
> protected:
>-    virtual ~GeckoInputDispatcher() {}
>+    virtual ~GeckoInputDispatcher();

This isn't needed with the change above.

r- for memory management scheme here, too fragile.  The rest looks OK
except for the mouse-event dispatch that I don't understand.

Comment 2

[Michael Wu [:mwu]]

Attachment: Add support for touch events to Gonk, v2

Comment 3

[Michael Wu [:mwu]]

(In reply to Chris Jones [:cjones] [:warhammer] from comment #1)
> >@@ -339,19 +371,50 @@ GeckoInputDispatcher::dispatchOnce()
> >         PRUint32 msg;
> >         switch (data.action) {
> >         case AMOTION_EVENT_ACTION_DOWN:
> >+        case AMOTION_EVENT_ACTION_POINTER_DOWN:
> >+            msg = NS_TOUCH_START;
> >+            break;
> >+        case AMOTION_EVENT_ACTION_MOVE:
> >+            msg = NS_TOUCH_MOVE;
> >+            break;
> >+        case AMOTION_EVENT_ACTION_UP:
> >+        case AMOTION_EVENT_ACTION_POINTER_UP:
> >+            msg = NS_TOUCH_END;
> >+            break;
> >+        case AMOTION_EVENT_ACTION_OUTSIDE:
> >+        case AMOTION_EVENT_ACTION_CANCEL:
> >+            msg = NS_TOUCH_CANCEL;
> >+            break;
> >+        }
> >+        nsEventStatus status =
> >+            sendTouchEvent(msg,
> >+                           data.eventTime,
> >+                           data.motion.touches,
> >+                           data.motion.touchCount);
> >+
> >+        switch (data.action) {
> >+        case AMOTION_EVENT_ACTION_DOWN:
> 
> Why do we unconditionally dispatch mouse events too?  Are there some
> docs I can read, or another example you were looking at?
> 
> (I'm not trying to imply that this isn't right, I just don't know.)
> 

This is similar to the pattern used for killing key press events that are normally fired after a key down. That being said, I think it's an insane pattern so I've removed it. The Android implementation isn't using this either.

> >diff --git a/widget/gonk/nsAppShell.h b/widget/gonk/nsAppShell.h
> 
> >@@ -93,7 +94,8 @@ struct InputData {
> >             int32_t scanCode;
> >         } key;
> >         struct {
> >-            PointerCoords coords;
> >+            nsDOMTouch **touches;
> >+            int32_t touchCount;
> 
> |nsTArray<nsDOMTouch>* touches;|.  Bonus points for something like
> |typedef nsTArray<nsDOMTouch> TouchArray;|.
> 

So, I was going to write about how objects with non-trivial constructors can't be used in unions, but as it turns out, they're giving people that footgun in C++11. However, our gcc doesn't support it.

Comment 4

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to Michael Wu [:mwu] from comment #3)
> (In reply to Chris Jones [:cjones] [:warhammer] from comment #1)
> > |nsTArray<nsDOMTouch>* touches;|.  Bonus points for something like
> > |typedef nsTArray<nsDOMTouch> TouchArray;|.
> > 
> 
> So, I was going to write about how objects with non-trivial constructors
> can't be used in unions, but as it turns out, they're giving people that
> footgun in C++11. However, our gcc doesn't support it.

I don't understand how that's relevant.  Pointers are POD.

Comment 5

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 590920 [details] [diff] [review]
Add support for touch events to Gonk, v2

Better, but use a pointer to an nsTArray to cut down on the potential footgunning here.

Comment 6

[Michael Wu [:mwu]]

(In reply to Chris Jones [:cjones] [:warhammer] from comment #4)
> (In reply to Michael Wu [:mwu] from comment #3)
> > (In reply to Chris Jones [:cjones] [:warhammer] from comment #1)
> > > |nsTArray<nsDOMTouch>* touches;|.  Bonus points for something like
> > > |typedef nsTArray<nsDOMTouch> TouchArray;|.
> > > 
> > 
> > So, I was going to write about how objects with non-trivial constructors
> > can't be used in unions, but as it turns out, they're giving people that
> > footgun in C++11. However, our gcc doesn't support it.
> 
> I don't understand how that's relevant.  Pointers are POD.

Opps. I thought you meant a nsTArray.

Comment 7

[Michael Wu [:mwu]]

Attachment: Add support for touch events to Gonk, v3

Comment 8

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

A pointer to an nsTArray:

  nsTArray<nsDOMTouch>* touches;

If you typedef the nstarray goop, it's a little more readable

  typedef nsTArray<nsDOMTouch> TouchArray;
  TouchArray* touches;

What I want to avoid at all costs is us manually managing array memory.

Comment 9

[Michael Wu [:mwu]]

Attachment: Add support for touch events to Gonk, v4

This patch changes a bunch of things.

1. nsDOMTouch is now generated by the main thread instead of in notifyMotion. This is because we can't allocate nsDOMTouch on another thread without the code complaining about things being used on different threads on a debug build.
2. No extra allocation is necessary for filling out UserInputData. We store all the touch points directly in UserInputData. The InputReader code is restricted to MAX_POINTERS so we just reserve space for ten points all the time instead of dynamically allocating.
3. The handling of points being lifted has been changed to match what the gecko platform code expects. (only the point being lifted is sent)
4. The event handling code on the main thread no longer copies UserInputData. It just takes a reference.

Comment 10

[Michael Wu [:mwu]]

Also, that #include "nsDOMTouch.h" can be moved to nsAppShell.cpp and I've done so in my local copy.

Comment 11

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 592279 [details] [diff] [review]
Add support for touch events to Gonk, v4

>diff --git a/widget/gonk/nsAppShell.cpp b/widget/gonk/nsAppShell.cpp
 
>+struct UserInputData {

>+        struct {
>+            int32_t touchCount;
>+            int32_t touchIds[MAX_POINTERS];
>+            PointerCoords touches[MAX_POINTERS];

Having a |struct Touch { int32_t id; PointerCoords coords; };| helper
would mean having only one array here, which means only one bounds
check in loops, one array deref, etc. in the rest of the patch.  I'd
prefer you did this.

>+static void
>+addDOMTouch(UserInputData &data, nsTouchEvent &event, int i)

Nit: I prefer |Foo& x| style.  ISTR trying to clean this up, but I may
misremember.  Either way, please go with the majority style in here.

>+{
>+    const PointerCoords *coord = &data.motion.touches[i];

const Touch& coord = data.motion.touches[i];

> void
> GeckoInputDispatcher::dispatchOnce()

>+    mEventQueue.pop();
>+    if (!mEventQueue.empty())
>+        gAppShell->NotifyNativeEvent();
> }

Please move all this code together under the front() above it.  That
will make it impossible for someone to accidentally insert a return
and cause duplicate or orphaned events.

> 
> 
>@@ -508,7 +575,13 @@ GeckoInputDispatcher::notifyMotion(nsecs_t eventTime,
>     data.action = action;
>     data.flags = flags;
>     data.metaState = metaState;
>-    data.motion.coords = *pointerCoords;
>+    MOZ_ASSERT(pointerCount <= MAX_POINTERS);
>+    data.motion.touchCount = NS_MIN(pointerCount,
>+                                    NS_ARRAY_LENGTH(data.motion.touches));

ArrayLength()

r=me with those fixed.

Comment 12

[Michael Wu [:mwu]]

(In reply to Chris Jones [:cjones] [:warhammer] from comment #11)
> > void
> > GeckoInputDispatcher::dispatchOnce()
> 
> >+    mEventQueue.pop();
> >+    if (!mEventQueue.empty())
> >+        gAppShell->NotifyNativeEvent();
> > }
> 
> Please move all this code together under the front() above it.  That
> will make it impossible for someone to accidentally insert a return
> and cause duplicate or orphaned events.
> 

Is that safe? We only take a reference to the data and release the lock, so if we pop it, the queue might modify it since it's been removed from the queue.

Comment 13

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Oh, you're taking a reference to the queue front.  I missed that in the "crazy" & syntax ;).  No, you're right, that's not safe.

I would just copy out the event and pop it.

Comment 14

[Michael Wu [:mwu]]

(In reply to Chris Jones [:cjones] [:warhammer] from comment #13)
> Oh, you're taking a reference to the queue front.  I missed that in the
> "crazy" & syntax ;).  No, you're right, that's not safe.
> 
> I would just copy out the event and pop it.

The struct was 28 bytes big before, but it's 424 bytes now that PointerCoords and pointerIds are built into the struct. That's why I avoided a copy there. Up to you whether it's worth avoiding the copy here or not.

Comment 15

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Should be ~25 instructions to copy.  Let's go with safe first.  Can optimize later.

I forgot to note in the review that the fat struct might be a perf issue when enqueuing a lot of events, but I'm also happy to punt that until we can profile.

Comment 16

[Michael Wu [:mwu]]

Attachment: Add support for touch events to Gonk, v5

Comment 17

[Michael Wu [:mwu]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5fc3efb1d791

Comment 18

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/5fc3efb1d791