712973 - Use InputReader from libui in gonk widget backend

Status: RESOLVED FIXED

(Core :: Widget, defect)

Description

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

The maguro device has an input-event space that's different than the screen space.  I'm going to hack in some quick code to handle the maguro, but in the long run we should use InputReader.  It's got all the code we need, minimal dependencies, works for more than just the touch input devices, and of course is tested ;).

Comment 1

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

(Forgot to add, the maguro also has virtual home/back/etc. keys.)

Comment 2

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

The general plan of attack here is
 - spawn off an input-processor thread from the gonk widget backend.  nsAppShell is a fine place to put this.
 - create an EventHub on that thread.
 - implement InputDispatcherInterface (?, i forget the exact name) and hook that up to EventHub.  InputReader sits in between the two.
 - on receiving events in our input dispatcher, post them to app shell for delivery to gecko.

Comment 3

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

mwu, do you want to take this?

Comment 4

[Michael Wu [:mwu]]

(In reply to Chris Jones [:cjones] [:warhammer] from comment #3)
> mwu, do you want to take this?

Yes, but I'm not going to work on it until I receive a maguro device.

Comment 5

[Michael Wu [:mwu]]

Attachment: Switch to EventHub and InputReader

We can probably strip away the fdhandler and epoll stuff as well but I want to do that in another bug.

Comment 6

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

Comment on attachment 590029 [details] [diff] [review]
Switch to EventHub and InputReader

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

> static void
>-sendMouseEvent(PRUint32 msg, struct timeval& time, int x, int y)
>+sendMouseEvent(PRUint32 msg, nsecs_t time, int x, int y)
> {
>     nsMouseEvent event(true, msg, NULL,
>                        nsMouseEvent::eReal, nsMouseEvent::eNormal);
> 
>     event.refPoint.x = x;
>     event.refPoint.y = y;
>-    event.time = timevalToMS(time);
>+    event.time = time / 1000;

1e6 ns / ms.  Too many zeros for me, helper function or constant to
convert between plz.

>@@ -152,18 +127,18 @@ sendMouseEvent(PRUint32 msg, struct timeval& time, int x, int y)
> static nsEventStatus
> sendKeyEventWithMsg(PRUint32 keyCode,
>                     PRUint32 msg,
>-                    const timeval &time,
>+                    nsecs_t time,
>                     PRUint32 flags)
> {
>     nsKeyEvent event(true, msg, NULL);
>     event.keyCode = keyCode;
>-    event.time = timevalToMS(time);
>+    event.time = time / 1000;

And here.

\>@@ -217,35 +192,57 @@ maybeSendKeyEvent(int keyCode, bool pressed, const timeval& time)
>     }
> }
> 
>-static void
>-maybeSendKeyEvent(const input_event& e)
>+namespace android {

Any reason these are in namespace android?  I don't care all that
much, it's just a little confusing on first read.

>+// GeckoInputReaderPolicy
>+bool
>+GeckoInputReaderPolicy::getDisplayInfo(int32_t displayId,
>+                                       int32_t* width,
>+                                       int32_t* height,
>+                                       int32_t* orientation)
> {
>-    if (e.type != EV_KEY) {
>-        VERBOSE_LOG("Got unknown key event type. type 0x%04x code 0x%04x value %d",
>-            e.type, e.code, e.value);
>-        return;
>-    }
>+    if (displayId)

Nit: 0 != displayId.  MOZ_ASSERT(0 == displayId) wfm too.

>+GeckoInputReaderPolicy::filterTouchEvents()
>+{
>+GeckoInputReaderPolicy::filterJumpyTouchEvents()

Moving the decl into nsAppShell.cpp will allow us to write inline
definitions for these trivial methods, and save some goop.

>@@ -551,6 +483,9 @@ nsAppShell::nsAppShell()
> 
> nsAppShell::~nsAppShell()
> {
>+    android::status_t result = mReaderThread->requestExitAndWait();

Nit: |using namespace android| for this and all the stuff below, plz.

>+    if (result)
>+        LOG("Could not stop reader thread - %d", result);
>     gAppShell = NULL;
> }
> 
>+    mReader = new android::InputReader(mEventHub, mReaderPolicy, mDispatcher);
>+    mReaderThread = new android::InputReaderThread(mReader);
> 
>+    android::status_t result = mReaderThread->run("InputReader", android::PRIORITY_URGENT_DISPLAY);
>+    NS_ENSURE_FALSE(result, NS_ERROR_UNEXPECTED);
>     return rv;
> }
> 
>@@ -655,6 +553,8 @@ nsAppShell::ProcessNextNativeEvent(bool mayWait)
>     for (int i = 0; i < event_count; i++)
>         mHandlers[events[i].data.u32].run();
> 
>+    mDispatcher->dispatchOnce();
>+
>     // NativeEventCallback always schedules more if it needs it
>     // so we can coalesce these.
>     // See the implementation in nsBaseAppShell.cpp for more info

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

>+#include "utils/threads.h"
>+#include "ui/EventHub.h"
>+#include "ui/InputReader.h"
>+#include "ui/InputDispatcher.h"

Let's localize this to nsAppShell.cpp, if possible.  See below.

>+namespace android {
>+

This could move out of namespace android pending question above.

>+struct InputData {

Nit: "InputEvent" plz.  UiEvent fine too.  InputData overly general.

>+class GeckoInputReaderPolicy : public InputReaderPolicyInterface {
>+class GeckoInputDispatcher : public InputDispatcherInterface {

Does sp<> allow holding a forward-declared class?  (I sure hope so!)
If so, let's forward declare these and move the decls into
nsAppShell.cpp.

>+    mozilla::Mutex mQueueLock;
>+    std::queue<InputData> mEventQueue;

Need comments here documenting which threads access these and how.

This patch looks really good.  I'd like to see the next version.

Comment 7

[Michael Wu [:mwu]]

Attachment: Switch to EventHub and InputReader, v2

Comment 8

[Michael Wu [:mwu]]

I've implemented most of the definition/include/namespacing shuffling suggestions.

(In reply to Chris Jones [:cjones] [:warhammer] from comment #6)
> Comment on attachment 590029 [details] [diff] [review]
> Switch to EventHub and InputReader
> 
> >diff --git a/widget/gonk/nsAppShell.cpp b/widget/gonk/nsAppShell.cpp
> 
> > static void
> >-sendMouseEvent(PRUint32 msg, struct timeval& time, int x, int y)
> >+sendMouseEvent(PRUint32 msg, nsecs_t time, int x, int y)
> > {
> >     nsMouseEvent event(true, msg, NULL,
> >                        nsMouseEvent::eReal, nsMouseEvent::eNormal);
> > 
> >     event.refPoint.x = x;
> >     event.refPoint.y = y;
> >-    event.time = timevalToMS(time);
> >+    event.time = time / 1000;
> 
> 1e6 ns / ms.  Too many zeros for me, helper function or constant to
> convert between plz.
> 

Urph. Not the first time I've screwed up this conversion.

> >+// GeckoInputReaderPolicy
> >+bool
> >+GeckoInputReaderPolicy::getDisplayInfo(int32_t displayId,
> >+                                       int32_t* width,
> >+                                       int32_t* height,
> >+                                       int32_t* orientation)
> > {
> >-    if (e.type != EV_KEY) {
> >-        VERBOSE_LOG("Got unknown key event type. type 0x%04x code 0x%04x value %d",
> >-            e.type, e.code, e.value);
> >-        return;
> >-    }
> >+    if (displayId)
> 
> Nit: 0 != displayId.  MOZ_ASSERT(0 == displayId) wfm too.
> 

I think comparing to zero looks terrible in C/C++.

> >+struct InputData {
> 
> Nit: "InputEvent" plz.  UiEvent fine too.  InputData overly general.
> 

Yeah but this isn't an Event really. At least not yet. *Event just wrong to me.

Comment 9

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

(In reply to Michael Wu [:mwu] from comment #8)
> > >+// GeckoInputReaderPolicy
> > >+bool
> > >+GeckoInputReaderPolicy::getDisplayInfo(int32_t displayId,
> > >+                                       int32_t* width,
> > >+                                       int32_t* height,
> > >+                                       int32_t* orientation)
> > > {
> > >-    if (e.type != EV_KEY) {
> > >-        VERBOSE_LOG("Got unknown key event type. type 0x%04x code 0x%04x value %d",
> > >-            e.type, e.code, e.value);
> > >-        return;
> > >-    }
> > >+    if (displayId)
> > 
> > Nit: 0 != displayId.  MOZ_ASSERT(0 == displayId) wfm too.
> > 
> 
> I think comparing to zero looks terrible in C/C++.
> 

I haven't looked at the patch, but if don't want to assert, make 0 a named constant.  Using the truth value of the displayId to test if it's the default display is very confusing.

> > >+struct InputData {
> > 
> > Nit: "InputEvent" plz.  UiEvent fine too.  InputData overly general.
> > 
> 
> Yeah but this isn't an Event really. At least not yet. *Event just wrong to
> me.

These originate from uevents, which are events fired when an input device registers a changed state.  These aren't gecko events, or DOM events yet, correct.  Sad about the overloading of "event".  I don't care too much about the name, but "InputData" is way too general, because of the ambiguity over "input".

Comment 10

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

Comment on attachment 590917 [details] [diff] [review]
Switch to EventHub and InputReader, v2

> >+    mozilla::Mutex mQueueLock;
> >+    std::queue<InputData> mEventQueue;
> 
> Need comments here documenting which threads access these and how.
> 

Didn't address this.

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

Global nit: converting to msecs in the input reader thread looks good.
But please change the instances of |time| in nsAppShell.{h,cpp} to
|timeMs| to make the unit explicit.  (Yes, nsEvent gets this wrong,
but let's not perpuate that mistake.)

>+struct InputData {

I don't care much about *Data vs. *Event, but please call this
UserInputEvent or something to resolve ambiguity on "input data".

>+GeckoInputReaderPolicy::getDisplayInfo(int32_t displayId,

>+    if (displayId)
>+        return false;
> 

That this trying to say, "if the display ID isn't the default (0),
bail" still isn't very clear to me.  Either
 - add a comment
 - compare to 0
 - make a symbolic constant kDefaultDisplay and compare to that

r=me with nits fixed.

Comment 11

[Michael Wu [:mwu]]

Attachment: Switch to EventHub and InputReader, v3

Comment 12

[Michael Wu [:mwu]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/df5b6590fcd3

Comment 13

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/df5b6590fcd3