Merge lp:~mir-team/mir/fix-missing-hover-exit-event into lp:mir

[Robert Carr]

Enable pointer exit events. (LP: #1418569)

This branch fixes https://bugs.launchpad.net/bugs/1418569 . See the changes to test_client_input for the semantic changes to client.

Doing so required two semantic changes to the input dispatcher stack:

There is a sort of extra layer of paranoia check in the InputDispatcher which prevents events from being dispatched to non foreground windows (e.g. the focused window or the target of the current touch gesture). We need to allow dispatching pointer exit events to the non foreground window though (as this is how they become non foreground!) InputDispatcher.cpp:l1347.

Then around 1226 we assume just because we do not have a touched window there is nothing to dispatch in the touch state so we need to set injection state failed. In fact we need to continue (if the new branch at l1232 is not taken we will fall to 1317) and check if there are hover exit events to dispatch. If there are still no targets (e.g. no touched window (foreground) and no hover exit event) then we can finally abort the dispatch cycle now at l1361

[Robert Carr (robertcarr)]

I accidentally left the debug defines on but I am going to make a case they should be left on.

1. I've turned them on and off dozens of times.
2. They only show up with MIR_SERVER_LEGACY_INPUT_REPORT=log ANDROID_LOG_TAGS=*:v

[Daniel van Vugt (vanvugt)]

The debug flags need some careful assessment, possibly profiling. All too often formatting hidden debug strings at high frequency can have a significant CPU hit.

review: Needs Information

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:2315
http://jenkins.qa.ubuntu.com/job/mir-ci/2952/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/1273
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/1272
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/1229
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/949
        deb: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/949/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/1229
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/1229/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/4268
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/17991

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-ci/2952/rebuild

review: Approve (continuous-integration)

[Daniel van Vugt (vanvugt)]

Rather than forcing someone to go to the trouble of profiling and proving that the debug logging doesn't hurt performance during input, I suggest it's quicker and safer to just not enable the debug flags on trunk.

review: Needs Fixing

[Chris Halse Rogers (raof)]

Looks sensible apart from the debug; either verify that it doesn't add significant CPU useage in the non-reporting case, or turn it back off.

review: Needs Fixing

[Alan Griffiths (alan-griffiths)]

We should replace the logging with something we can be sure doesn't waste time formatting or on memory allocation. Until then we should have it switched off.

review: Needs Fixing

[Robert Carr (robertcarr)]

Hi guys. I'll change it back of course.

I hate to be snotty but im a little frustrated that this is even brought up. I think everyones intuition that this is something worth being concerned about is incorrect by 6 (!) orders of magnitude. My computer from early 2011 can format 10 million strings in 2.5 seconds as opposed to the 120 input events we see a second.

>> #include <stdio.h>
>> #include <stdlib.h>

>> int main()
>> {
>> for (int i = 0; i < 10000000; i++)
>> {
>> char output[1024];
>> snprintf(output, 1024, "%d %s %d", rand(), "Kittens", i);
>> }
>> }

[Robert Carr (robertcarr)]

Want to apologize a little for my choice of words. Not actually personally frustrated with anyone :) Obviously we are all concerned about defending the value of Mir.

Just not sure how else to communicate that I think we may be prone to a bit of Fear-Uncertainty-Doubt when it comes to performance.

:)

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:2316
http://jenkins.qa.ubuntu.com/job/mir-ci/2965/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/1289/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/1288/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/1245/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/962/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/1245/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-ci/2965/rebuild

review: Needs Fixing (continuous-integration)

[Daniel van Vugt (vanvugt)]

Unfortunately this is something learned from experience. Both in previous projects and in Mir last year (see bug 1343074).

[Daniel van Vugt (vanvugt)]

review: Abstain

[Robert Carr (robertcarr)]

Fixed stray character at beginning of file.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:2318
http://jenkins.qa.ubuntu.com/job/mir-ci/3001/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/1331
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/1330
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/1287
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/998
        deb: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/998/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/1287
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/1287/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/4320
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/18130

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-ci/3001/rebuild

review: Approve (continuous-integration)

[Chris Halse Rogers (raof)]

Looks good.

review: Approve

[Alan Griffiths (alan-griffiths)]

OK

review: Approve

[Kevin DuBois (kdub)]

okay by me

review: Approve

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:2319
http://jenkins.qa.ubuntu.com/job/mir-ci/3012/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/1348
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/1347
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/1304
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/1009
        deb: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/1009/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/1304
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/1304/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/4336
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/18172

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-ci/3012/rebuild

review: Approve (continuous-integration)

=== modified file '3rd_party/android-input/android/frameworks/base/services/input/InputDispatcher.cpp'
--- 3rd_party/android-input/android/frameworks/base/services/input/InputDispatcher.cpp	2015-02-13 06:12:34 +0000
+++ 3rd_party/android-input/android/frameworks/base/services/input/InputDispatcher.cpp	2015-02-18 18:13:06 +0000
@@ -1224,36 +1224,36 @@
                             "eventually add a new window when it finishes starting up.");
                     goto Unresponsive;
                 }
-
-                ALOGI("Dropping event because there is no touched window.");
-                injectionResult = INPUT_EVENT_INJECTION_FAILED;
-                goto Failed;
-            }
-        }
-
-        // Set target flags.
-        int32_t targetFlags = InputTarget::FLAG_FOREGROUND | InputTarget::FLAG_DISPATCH_AS_IS;
-        if (isSplit) {
-            targetFlags |= InputTarget::FLAG_SPLIT;
-        }
-        if (isWindowObscuredAtPointLocked(newTouchedWindowHandle, x, y)) {
-            targetFlags |= InputTarget::FLAG_WINDOW_IS_OBSCURED;
-        }
-
-        // Update hover state.
-        if (isHoverAction) {
-            newHoverWindowHandle = newTouchedWindowHandle;
-        } else if (maskedAction == AMOTION_EVENT_ACTION_SCROLL) {
-            newHoverWindowHandle = mLastHoverWindowHandle;
-        }
-
-        // Update the temporary touch state.
-        IntSet pointerIds;
-        if (isSplit) {
-            int32_t pointerId = entry->pointerProperties[pointerIndex].id;
-            pointerIds.insert(pointerId);
-        }
-        mTempTouchState.addOrUpdateWindow(newTouchedWindowHandle, targetFlags, pointerIds);
+            }
+        }
+
+        // We may still not have a window handle but we can't just abort the dispatch
+        // cycle because there could be hover exits to dispatch.
+        if (newTouchedWindowHandle != NULL) {
+            // Set target flags.
+            int32_t targetFlags = InputTarget::FLAG_FOREGROUND | InputTarget::FLAG_DISPATCH_AS_IS;
+            if (isSplit) {
+                targetFlags |= InputTarget::FLAG_SPLIT;
+            }
+            if (isWindowObscuredAtPointLocked(newTouchedWindowHandle, x, y)) {
+                targetFlags |= InputTarget::FLAG_WINDOW_IS_OBSCURED;
+            }
+
+            // Update hover state.
+            if (isHoverAction) {
+                newHoverWindowHandle = newTouchedWindowHandle;
+            } else if (maskedAction == AMOTION_EVENT_ACTION_SCROLL) {
+                newHoverWindowHandle = mLastHoverWindowHandle;
+            }
+            
+            // Update the temporary touch state.
+            IntSet pointerIds;
+            if (isSplit) {
+                int32_t pointerId = entry->pointerProperties[pointerIndex].id;
+                pointerIds.insert(pointerId);
+            }
+            mTempTouchState.addOrUpdateWindow(newTouchedWindowHandle, targetFlags, pointerIds);
+        }
     } else {
         /* Case 2: Pointer move, up, cancel or non-splittable pointer down. */
 
@@ -1277,6 +1277,9 @@
             sp<InputWindowHandle> oldTouchedWindowHandle =
                     mTempTouchState.getFirstForegroundWindowHandle();
             sp<InputWindowHandle> newTouchedWindowHandle = findTouchedWindowAtLocked(x, y);
+
+            newHoverWindowHandle = newTouchedWindowHandle;
+            
             if (oldTouchedWindowHandle != newTouchedWindowHandle
                     && newTouchedWindowHandle != NULL) {
 #if DEBUG_FOCUS
@@ -1339,7 +1342,9 @@
         bool haveForegroundWindow = false;
         for (size_t i = 0; i < mTempTouchState.windows.size(); i++) {
             const TouchedWindow& touchedWindow = mTempTouchState.windows[i];
-            if (touchedWindow.targetFlags & InputTarget::FLAG_FOREGROUND) {
+            if (touchedWindow.targetFlags & InputTarget::FLAG_FOREGROUND ||
+                // We allow dispatching hover exit events to non foreground windows.
+                touchedWindow.targetFlags & InputTarget::FLAG_DISPATCH_AS_HOVER_EXIT) {
                 haveForegroundWindow = true;
                 if (! checkInjectionPermission(touchedWindow.windowHandle,
                         entry->injectionState)) {

=== modified file 'tests/acceptance-tests/throwback/test_client_input.cpp'
--- tests/acceptance-tests/throwback/test_client_input.cpp	2015-02-13 06:12:34 +0000
+++ tests/acceptance-tests/throwback/test_client_input.cpp	2015-02-18 18:13:06 +0000
@@ -271,7 +271,7 @@
         mis::a_key_down_event().of_scancode(KEY_4));
 }
 
-TEST_F(TestClientInput, clients_receive_pointer_inside_window)
+TEST_F(TestClientInput, clients_receive_pointer_inside_window_and_crossing_events)
 {
     using namespace testing;
 
@@ -285,7 +285,8 @@
                 handle_input(
                     mt::PointerEventWithPosition(
                         InputClient::surface_width - 1,
-                        InputClient::surface_height - 1)))
+                        InputClient::surface_height - 1)));
+    EXPECT_CALL(client.handler, handle_input(mt::PointerLeaveEvent()))
         .WillOnce(mt::WakeUp(&client.all_events_received));
     // But we should not receive an event for the second movement outside of our surface!