Bug 81462 – [chromium] Implement fling-by-wheel on compositor thread

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[James Robinson]

[chromium] Implement fling-by-wheel on compositor thread

[James Robinson]

Created attachment 132475 [details]
Patch

[James Robinson]

This is a sketch (completely untested) of fling-by-sequence-of-wheel events on the compositor thread.  There's one FIXME for behavior in WebCompositorInputHandlerImpl::scrollBy() that'll require copying the wheel fling parameters back to the main thread if we start a fling on the compositor thread but hit a non-fast scrollable area.  I'll start working on hooking that up once the main thread fling animation stuff is in place.  The only things we really need to get back to the main thread are the start time of the fling and the mouse position, so it's just 1 double and 2 ints.

[James Robinson]

Created attachment 132478 [details]
Patch

[Nat Duca]

Comment on attachment 132478 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=132478&action=review

Pretty cool. Bikeshed comments as usual.

> Source/WebCore/platform/graphics/chromium/cc/CCInputHandler.h:81
> +    // Request a new frame with updated animation state. Typically called by CCInputHandler::animate().

Whatabout
 // Request another callback to InputHandler::animate()

> Source/WebKit/chromium/src/WebCompositorInputHandlerImpl.cpp:171
> +WebCompositorInputHandlerImpl::EventDisposition WebCompositorInputHandlerImpl::handleInputEventInternal(const WebInputEvent& event)

the 'internal' suffix in here is a bit generic. Can we either comment the function explaining its role versus the outer one, or tweak its naming so its more obvious? Nothing jumps to mind, so if I was here, I'd do a //

> Source/WebKit/chromium/tests/WebCompositorInputHandlerImplTest.cpp:178
> +    EXPECT_CALL(m_mockClient, didNotHandleInputEvent(true));

iirw, the mock is going to verify all these during ~Test. The way this test is written, it looks like its designed for immediate verification --- we should either force mock verification [I know theres a way, I just forget how] or put all the expectations up front so that its obvious that they're all verified together.

> Source/WebKit/chromium/tests/WebCompositorInputHandlerImplTest.cpp:207
> +    // Since we didn't handle the event, the caller shouldn't send us any Update/End messages.

was there supposed to be something here?

[Adrienne Walker]

Comment on attachment 132478 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=132478&action=review

> Source/WebKit/chromium/src/WebCompositorInputHandlerImpl.cpp:-252
> -    // GestureScrollEnd with non-zero velocity (deltaX/Y) signals start of Fling.
> -    if (!gestureEvent.deltaX && !gestureEvent.deltaY) {
> -        m_client->didHandleInputEvent();
> -        return true;
> -    }

Was this just vestigial and GestureFlingStart is the only way fling gestures start these days?

[James Robinson]

Comment on attachment 132478 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=132478&action=review

>> Source/WebCore/platform/graphics/chromium/cc/CCInputHandler.h:81
>> +    // Request a new frame with updated animation state. Typically called by CCInputHandler::animate().
> 
> Whatabout
>  // Request another callback to InputHandler::animate()

Good idea, done

>> Source/WebKit/chromium/src/WebCompositorInputHandlerImpl.cpp:171
>> +WebCompositorInputHandlerImpl::EventDisposition WebCompositorInputHandlerImpl::handleInputEventInternal(const WebInputEvent& event)
> 
> the 'internal' suffix in here is a bit generic. Can we either comment the function explaining its role versus the outer one, or tweak its naming so its more obvious? Nothing jumps to mind, so if I was here, I'd do a //

Added a comment to the header, lemme know if it's any clearer

>> Source/WebKit/chromium/src/WebCompositorInputHandlerImpl.cpp:-252
>> -    }
> 
> Was this just vestigial and GestureFlingStart is the only way fling gestures start these days?

This was part of the old plan to have GestureScrollEnd trigger flings. The chromium-side code to generate this event never landed, and it conflicts with how Android uses these events.

>> Source/WebKit/chromium/tests/WebCompositorInputHandlerImplTest.cpp:178
>> +    EXPECT_CALL(m_mockClient, didNotHandleInputEvent(true));
> 
> iirw, the mock is going to verify all these during ~Test. The way this test is written, it looks like its designed for immediate verification --- we should either force mock verification [I know theres a way, I just forget how] or put all the expectations up front so that its obvious that they're all verified together.

Hmmm - this stuff is tricky. Let me check with Shawn, we ran into some gremlins when trying to force expectations earlier.

>> Source/WebKit/chromium/tests/WebCompositorInputHandlerImplTest.cpp:207
>> +    // Since we didn't handle the event, the caller shouldn't send us any Update/End messages.
> 
> was there supposed to be something here?

nope, this was more a comment to me explaining why there aren't cases for ScrollUpdate/ScrollEnd.  This isn't very helpful for other people reading the code though - I'll remove

[James Robinson]

Created attachment 132636 [details]
Patch

[Adrienne Walker]

(In reply to comment #4)

> > Source/WebKit/chromium/tests/WebCompositorInputHandlerImplTest.cpp:178
> > +    EXPECT_CALL(m_mockClient, didNotHandleInputEvent(true));
> 
> iirw, the mock is going to verify all these during ~Test. The way this test is written, it looks like its designed for immediate verification --- we should either force mock verification [I know theres a way, I just forget how] or put all the expectations up front so that its obvious that they're all verified together.

I totally agree that it'd be better if it were immediately verified, but I think keeping the expectations with the code that generates them isn't bad.  If you change part of the test, it's much easier to remove the relevant expectations if they're adjacent, rather than in some upfront bundle.

[Adrienne Walker]

Comment on attachment 132636 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=132636&action=review

R=me.

> Source/WebKit/chromium/src/WebCompositorInputHandlerImpl.cpp:179
> +            TRACE_EVENT_INSTANT2("cc", "WebCompositorInputHandlerImpl::handleInput wheel scroll", "deltaX", -wheelEvent.deltaX, "deltaY", -wheelEvent.deltaY);

Ooh, good thinking in adding trace events in this last patch.  That'll make debugging any problems from input a lot easier.

[Shawn Singh]

Comment on attachment 132478 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=132478&action=review

>>> Source/WebKit/chromium/tests/WebCompositorInputHandlerImplTest.cpp:178
>>> +    EXPECT_CALL(m_mockClient, didNotHandleInputEvent(true));
>> 
>> iirw, the mock is going to verify all these during ~Test. The way this test is written, it looks like its designed for immediate verification --- we should either force mock verification [I know theres a way, I just forget how] or put all the expectations up front so that its obvious that they're all verified together.
> 
> Hmmm - this stuff is tricky. Let me check with Shawn, we ran into some gremlins when trying to force expectations earlier.

Nat and James are exactly right... In the GMock documentation (the gmock dummy guide) its explained that we should _not_ interleave EXPECT_CALL and mock calls.  I think EXPECT_CALL overrides its previous expectations and so all these previous expectations are actually not being tested.

If you guys want to keep this test as immediate verification, then we should explicitly tell mock to verify expectations in the middle of the code (and clear the expectation) using Mock::verifyAndClearExpectations.  You can find more documentation about that in the docs (specifically, the cookbook guide for gmock).  Examples of verifyAndClearExpectations can be found in LayerChromiumTest, too.

[Adrienne Walker]

Comment on attachment 132636 [details]
Patch

Thanks for chiming in Shawn! I clearly had some bad assumptions about gmock.  I'll s/R+/R-/ until the test issues get sorted.

[James Robinson]

For the record, here's an of broken code passing one of the tests as written.

This test:

TEST_F(WebCompositorInputHandlerImplTest, gestureFlingIgnored)
{
    gesture.type = WebKit::WebInputEvent::GestureFlingStart;
    EXPECT_CALL(m_mockClient, didNotHandleInputEvent(true));
    m_inputHandler->handleInputEvent(gesture);

    gesture.type = WebKit::WebInputEvent::GestureFlingCancel;
    EXPECT_CALL(m_mockClient, didNotHandleInputEvent(false));
    m_inputHandler->handleInputEvent(gesture);
}

would pass even if handleInputEvent() was implemented as:

    if (gesture.type == WebKit::WebInputEvent::GestureFlingStart)
        return;
    if (gesture.type == WebKit::WebInputEvent::GestureFlingCancel)
        client->didNotHandleInputEvent(false);
        client->didNotHandleInputEvent(true)


even though this code is clearly buggy.  This modified test fails as expected:

TEST_F(WebCompositorInputHandlerImplTest, gestureFlingIgnored)
{
    gesture.type = WebKit::WebInputEvent::GestureFlingStart;
    EXPECT_CALL(m_mockClient, didNotHandleInputEvent(true));
    m_inputHandler->handleInputEvent(gesture);

    testing::Mock::VerifyAndClearExpectations(&m_mockClient);

    gesture.type = WebKit::WebInputEvent::GestureFlingCancel;
    EXPECT_CALL(m_mockClient, didNotHandleInputEvent(false));
    m_inputHandler->handleInputEvent(gesture);
}

I'll add Verify...() calls as appropriate through the tests and post an updated patch.

[James Robinson]

Created attachment 132683 [details]
Patch

[James Robinson]

Updated to verify more aggressively.  Most tests want to verify mocks on both the WebCompositorInputHandlerClient and the CCInputHandlerClient, so I added a utility function to the fixture to set up expectations for both of these.  gestureFlingAnimate() has a section with animate() calls but no handleInputEvent() calls, so it only expects calls on the CCInputHandlerClient.  For this test I'm manually calling VerifyAnd...() on that interface only.

I also renamed the mock clients to make them a bit clearer (I kept getting the mock WebCompositorInputHandlerClient and mock CCInputHandlerClient mixed up).  I think having m_mockClient mean WebCompositorInputHandlerClient makes sense, since the test is called WebCompositorInputHandlerImplTest.  The other client gets a longer name.

[Shawn Singh]

Comment on attachment 132683 [details]
Patch

Yeah, as far as the mock EXPECT_CALL and verifyAndClearExpectations issue is concerned, I think this is good now. =)

Only one other thought:

View in context: https://bugs.webkit.org/attachment.cgi?id=132683&action=review

> Source/WebKit/chromium/tests/WebCompositorInputHandlerImplTest.cpp:203
> +    verifyAndResetMocks();

If I understand the intention here, maybe we should make the enum a direct arg of verifyAndResetMocks and it probably doesn't need to be stored in the text fixture?  That might make it clearer that we're resetting mocks to the next expectation behavior.

[Adrienne Walker]

Comment on attachment 132683 [details]
Patch

R=me.  Thanks for fixing up the tests.

[James Robinson]

Created attachment 132708 [details]
Patch

[James Robinson]

Diff:


diff --git a/Source/WebKit/chromium/src/WebCompositorInputHandlerImpl.cpp b/Source/WebKit/chromium/src/WebCompositorInputHandlerImpl.cpp
index 0f7afd4..f8a3cfd 100644
--- a/Source/WebKit/chromium/src/WebCompositorInputHandlerImpl.cpp
+++ b/Source/WebKit/chromium/src/WebCompositorInputHandlerImpl.cpp
@@ -181,9 +181,9 @@ WebCompositorInputHandlerImpl::EventDisposition WebCompositorInputHandlerImpl::h
             m_inputHandlerClient->scrollEnd();
             return DidHandle;
         case CCInputHandlerClient::ScrollIgnored:
-            return DidNotHandle;
-        case CCInputHandlerClient::ScrollFailed:
             return DropEvent;
+        case CCInputHandlerClient::ScrollFailed:
+            return DidNotHandle;
         }
     } else if (event.type == WebInputEvent::GestureScrollBegin) {
         ASSERT(!m_gestureScrollStarted);
diff --git a/Source/WebKit/chromium/tests/WebCompositorInputHandlerImplTest.cpp b/Source/WebKit/chromium/tests/WebCompositorInputHandlerImplTest.cpp
index e2e551d..fa8247b 100644
--- a/Source/WebKit/chromium/tests/WebCompositorInputHandlerImplTest.cpp
+++ b/Source/WebKit/chromium/tests/WebCompositorInputHandlerImplTest.cpp
@@ -346,7 +346,6 @@ TEST_F(WebCompositorInputHandlerImplTest, gestureFlingAnimates)
     m_inputHandler->handleInputEvent(gesture);
 
     testing::Mock::VerifyAndClearExpectations(&m_mockCCInputHandlerClient);
-
     // The first animate call should let us pick up an animation start time, but we
     // shouldn't actually move anywhere just yet. The first frame after the fling start
     // will typically include the last scroll from the gesture that lead to the scroll
@@ -372,7 +371,7 @@ TEST_F(WebCompositorInputHandlerImplTest, gestureFlingAnimates)
     // Add tests for this once it's implemented.
     EXPECT_CALL(m_mockCCInputHandlerClient, scheduleAnimation());
     EXPECT_CALL(m_mockCCInputHandlerClient, scrollBegin(testing::_, testing::_))
-        .WillOnce(testing::Return(WebCore::CCInputHandlerClient::ScrollIgnored));
+        .WillOnce(testing::Return(WebCore::CCInputHandlerClient::ScrollFailed));
     EXPECT_CALL(m_mockCCInputHandlerClient, scrollBy(testing::_)).Times(0);
     EXPECT_CALL(m_mockCCInputHandlerClient, scrollEnd()).Times(0);
     m_inputHandler->animate(10.2);

[James Robinson]

(In reply to comment #15)
> (From update of attachment 132683 [details])
> Yeah, as far as the mock EXPECT_CALL and verifyAndClearExpectations issue is concerned, I think this is good now. =)
> 
> Only one other thought:
> 
> View in context: https://bugs.webkit.org/attachment.cgi?id=132683&action=review
> 
> > Source/WebKit/chromium/tests/WebCompositorInputHandlerImplTest.cpp:203
> > +    verifyAndResetMocks();
> 
> If I understand the intention here, maybe we should make the enum a direct arg of verifyAndResetMocks and it probably doesn't need to be stored in the text fixture?  That might make it clearer that we're resetting mocks to the next expectation behavior.

This is not a bad idea, but the typical setup for these tests it that within a single test all of the inputs will have the same disposition (since they normally test the flow through a single logical gesture or sequence of events) so it'd be pretty repetitive.  In the future if we end up resetting this then making it a param would improve things but I'd prefer to do that once we need to.

[Adrienne Walker]

Comment on attachment 132708 [details]
Patch

Ack.  Nice catch.

[James Robinson]

Committed r111270: <http://trac.webkit.org/changeset/111270>