Closed
Bug 719647
Opened 12 years ago
Closed 12 years ago
Add Touch Event support to Gonk widget backend
Categories
(Core :: Widget, defect)
Tracking
()
RESOLVED
FIXED
mozilla12
People
(Reporter: mwu, Assigned: mwu)
References
Details
Attachments
(1 file, 4 obsolete files)
6.51 KB,
patch
|
Details | Diff | Splinter Review |
No description provided.
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•12 years ago
|
||
Attachment #590033 -
Attachment is obsolete: true
Attachment #590920 -
Flags: review?(jones.chris.g)
Assignee | ||
Comment 3•12 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•12 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•12 years ago
|
||
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•12 years ago
|
||
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•12 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•12 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•12 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•12 years ago
|
||
Attachment #592279 -
Attachment is obsolete: true
Assignee | ||
Comment 17•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/5fc3efb1d791
Comment 18•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/5fc3efb1d791
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla12
You need to log in
before you can comment on or make changes to this bug.
Description
•