Add Touch Event support to Gonk widget backend

RESOLVED FIXED in mozilla12

Status

()

Core
Widget
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: mwu, Assigned: mwu)

Tracking

(Blocks: 1 bug)

unspecified
mozilla12
All
Gonk (Firefox OS)
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Assignee)

Description

5 years ago
Created attachment 590033 [details] [diff] [review]
Add support for touch events to Gonk
Attachment #590033 - Flags: review?(jones.chris.g)
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.
Attachment #590033 - Flags: review?(jones.chris.g) → review-
(Assignee)

Comment 2

5 years ago
Created attachment 590920 [details] [diff] [review]
Add support for touch events to Gonk, v2
Attachment #590033 - Attachment is obsolete: true
Attachment #590920 - Flags: review?(jones.chris.g)
(Assignee)

Comment 3

5 years ago
(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.
(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 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.
Attachment #590920 - Flags: review?(jones.chris.g)
(Assignee)

Comment 6

5 years ago
(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.
(Assignee)

Comment 7

5 years ago
Created attachment 591581 [details] [diff] [review]
Add support for touch events to Gonk, v3
Attachment #590920 - Attachment is obsolete: true
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.
(Assignee)

Comment 9

5 years ago
Created attachment 592279 [details] [diff] [review]
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.
Attachment #591581 - Attachment is obsolete: true
Attachment #592279 - Flags: review?(jones.chris.g)
(Assignee)

Comment 10

5 years ago
Also, that #include "nsDOMTouch.h" can be moved to nsAppShell.cpp and I've done so in my local copy.
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.
Attachment #592279 - Flags: review?(jones.chris.g) → review+
(Assignee)

Comment 12

5 years ago
(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.
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.
(Assignee)

Comment 14

5 years ago
(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.
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.
(Assignee)

Comment 16

5 years ago
Created attachment 592926 [details] [diff] [review]
Add support for touch events to Gonk, v5
Attachment #592279 - Attachment is obsolete: true
(Assignee)

Comment 17

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/5fc3efb1d791
https://hg.mozilla.org/mozilla-central/rev/5fc3efb1d791
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla12
(Assignee)

Updated

5 years ago
Blocks: 722812
(Assignee)

Updated

5 years ago
Blocks: 723183
Blocks: 726557
You need to log in before you can comment on or make changes to this bug.