Bug 104947 – Send a message from WebViewImpl to the compositor to inform about end of flings

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

Send a message from WebViewImpl to the compositor to inform about end of flings

Created attachment 179329 [details]
Patch

Created attachment 179339 [details]
Patch

[James Robinson]

Comment on attachment 179329 [details]
Patch

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

I swear I put this feedback up earlier but must have imagined it or a gremlin ate it.  Logic looks sound, needs a bit of naming love methinks

> Source/Platform/chromium/public/WebLayerTreeView.h:162
> +    virtual void mainThreadHasStoppedFlinging() { };

no trailing ;

the "mainThread" part of this isn't very informative - this API (WebLayerTreeView) is single threaded, so all calls on it are on the main thread.

maybe "didStopFlinging" or something like that?

> Source/WebKit/chromium/src/WebCompositorInputHandlerImpl.h:102
> +    bool m_flingActiveOnMainThread;

you should note either in the variable name or some clear comments that this is conservative - i.e. if this is false, we know definitely that the main thread is not flinging, but if it is true the main thread may or may not be flinging

How about using didStopFlinging for WebLayerTreeView and web_layer_tree_view_impl and using either mainThreadHasStoppedFLinging or mainHasStoppedFlinging starting from and including layer_tree_host? I feel like when it gets to the input handler we should have some mention of it not being a fling on the compositor, but not sure where to make the name switch.

[James Robinson]

(In reply to comment #4)
> How about using didStopFlinging for WebLayerTreeView 

Sounds good

> and web_layer_tree_view_impl

soungs good

>  and using either mainThreadHasStoppedFLinging or mainHasStoppedFlinging starting from and including layer_tree_host? I feel like when it gets to the input handler we should have some mention of it not being a fling on the compositor, but not sure where to make the name switch.

Yeah, we should definitely flip names once we switch threads.  Maybe have the call to the proxy be the switching point?

Created attachment 179351 [details]
Patch

[James Robinson]

Comment on attachment 179351 [details]
Patch

R=me

[James Robinson]

Guess this means we have to iterate again on the chromium side - sorry about that.

Uploaded the chromium side changes. Looks like I will have to wait for the other change in WebCompositorInputHandlerImpl to be gardened in anyway.

Created attachment 179358 [details]
Patch

I think my local repo is up to date now. jamesr@ would you mind giving this a final review and pushing it to the CQ whenever possible?

[James Robinson]

Comment on attachment 179358 [details]
Patch

R+ CQ- for now, will set CQ once the chromium-side implementation of WebInputHandler has landed (and please remind me if I forget).

(In reply to comment #12)
> (From update of attachment 179358 [details])
> R+ CQ- for now, will set CQ once the chromium-side implementation of WebInputHandler has landed (and please remind me if I forget).

I think this one has to go in first? The change to WebInputHandler won't be an issue, since WebCompositorInputHandlerImpl is the one extending that. WebToCCInputHandlerAdapter extends InputHandler and own a WebInputHandler. So I can't land the chromium side if WebInputHandler mainThreadHasStoppedFLinginng is not there yet.

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[WebKit Review Bot]

Comment on attachment 179358 [details]
Patch

Attachment 179358 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/15311478

New failing tests:
accessibility/aria-checkbox-checked.html
animations/3d/matrix-transform-type-animation.html
http/tests/appcache/cyrillic-uri.html
accessibility/anchor-linked-anonymous-block-crash.html
animations/animation-add-events-in-handler.html
http/tests/appcache/deferred-events-delete-while-raising.html
animations/additive-transform-animations.html
animations/3d/replace-filling-transform.html
http/tests/appcache/deferred-events-delete-while-raising-timer.html
accessibility/aria-disabled.html
http/tests/appcache/crash-when-navigating-away-then-back.html
animations/animation-border-overflow.html
animations/animation-direction-alternate-reverse.html
accessibility/accessibility-object-detached.html
http/tests/appcache/abort-cache-ondownloading-manifest-404.html
http/tests/appcache/credential-url.html
animations/animation-controller-drt-api.html
animations/3d/change-transform-in-end-event.html
canvas/philip/tests/2d.canvas.readonly.html
animations/3d/transform-perspective.html
accessibility/accessibility-node-reparent.html
http/tests/appcache/access-via-redirect.php
http/tests/appcache/deferred-events.html
animations/3d/state-at-end-event-transform.html
animations/animation-direction-normal.html
animations/3d/transform-origin-vs-functions.html
accessibility/accessibility-node-memory-management.html
animations/animation-css-rule-types.html
accessibility/adjacent-continuations-cause-assertion-failure.html
accessibility/aria-describedby-on-input.html

Yes, I just verified locally that this one is building by itself(the linux failure for tests is a flake?) and the chromium side one doesn't pass without the WebInputHandler.h change here.

[James Robinson]

Comment on attachment 179358 [details]
Patch

Oh yes!  Quite right.

[WebKit Review Bot]

Comment on attachment 179358 [details]
Patch

Rejecting attachment 179358 [details] from commit-queue.

New failing tests:
accessibility/aria-checkbox-checked.html
animations/3d/matrix-transform-type-animation.html
http/tests/appcache/cyrillic-uri.html
accessibility/anchor-linked-anonymous-block-crash.html
animations/animation-add-events-in-handler.html
http/tests/appcache/deferred-events-delete-while-raising.html
animations/additive-transform-animations.html
animations/3d/replace-filling-transform.html
http/tests/appcache/deferred-events-delete-while-raising-timer.html
accessibility/aria-disabled.html
http/tests/appcache/crash-when-navigating-away-then-back.html
animations/animation-border-overflow.html
animations/animation-direction-alternate-reverse.html
accessibility/accessibility-object-detached.html
http/tests/appcache/abort-cache-ondownloading-manifest-404.html
http/tests/appcache/credential-url.html
animations/animation-controller-drt-api.html
animations/3d/change-transform-in-end-event.html
canvas/philip/tests/2d.canvas.readonly.html
animations/3d/transform-perspective.html
accessibility/accessibility-node-reparent.html
http/tests/appcache/access-via-redirect.php
http/tests/appcache/deferred-events.html
animations/3d/state-at-end-event-transform.html
animations/animation-direction-normal.html
animations/3d/transform-origin-vs-functions.html
accessibility/accessibility-node-memory-management.html
animations/animation-css-rule-types.html
accessibility/adjacent-continuations-cause-assertion-failure.html
accessibility/aria-describedby-on-input.html
Full output: http://queues.webkit.org/results/15311546

Can't really tell if the issue is a bot failure/tree breakage but I see "DumpRenderTree crash" on all test failures. Maybe this one will also need to be manually landed?

[WebKit Review Bot]

Comment on attachment 179358 [details]
Patch

Rejecting attachment 179358 [details] from commit-queue.

New failing tests:
accessibility/aria-checkbox-checked.html
animations/3d/matrix-transform-type-animation.html
http/tests/appcache/cyrillic-uri.html
accessibility/anchor-linked-anonymous-block-crash.html
animations/animation-add-events-in-handler.html
http/tests/appcache/deferred-events-delete-while-raising.html
animations/additive-transform-animations.html
animations/3d/replace-filling-transform.html
http/tests/appcache/deferred-events-delete-while-raising-timer.html
accessibility/aria-disabled.html
http/tests/appcache/crash-when-navigating-away-then-back.html
animations/animation-border-overflow.html
animations/animation-direction-alternate-reverse.html
accessibility/accessibility-object-detached.html
http/tests/appcache/abort-cache-ondownloading-manifest-404.html
http/tests/appcache/credential-url.html
http/tests/appcache/access-via-redirect.php
animations/3d/change-transform-in-end-event.html
animations/animation-controller-drt-api.html
animations/3d/transform-perspective.html
accessibility/accessibility-node-reparent.html
http/tests/appcache/deferred-events.html
animations/3d/state-at-end-event-transform.html
animations/animation-direction-normal.html
animations/3d/transform-origin-vs-functions.html
accessibility/accessibility-node-memory-management.html
animations/animation-css-rule-types.html
animations/animation-direction-reverse-fill-mode-hardware.html
accessibility/adjacent-continuations-cause-assertion-failure.html
accessibility/aria-describedby-on-input.html
Full output: http://queues.webkit.org/results/15316789

[James Robinson]

Comment on attachment 179358 [details]
Patch

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

> Source/WebKit/chromium/src/WebViewImpl.cpp:710
> +            m_layerTreeView->didStopFlinging();

or maybe DRT is actually crashing - you need to null check m_layerTreeView here and other places before you deref it

(In reply to comment #21)
> (From update of attachment 179358 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=179358&action=review
> 
> > Source/WebKit/chromium/src/WebViewImpl.cpp:710
> > +            m_layerTreeView->didStopFlinging();
> 
> or maybe DRT is actually crashing - you need to null check m_layerTreeView here and other places before you deref it

Ooops! Yes, I think the one in didCommitLoad was the one biting me. Uploaded another patch with null checks. But it is possible for it to be non-null in didCommitLoad right? Or should I not even bother calling it there?

Created attachment 179505 [details]
Patch

[James Robinson]

Comment on attachment 179505 [details]
Patch

I think it's possible for m_layerTreeView to be null in any of these calls.

[WebKit Review Bot]

Comment on attachment 179505 [details]
Patch

Clearing flags on attachment: 179505

Committed r137765: <http://trac.webkit.org/changeset/137765>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[noel gordon]

webkit-unit-tests began failing with the patch I think.

http://build.webkit.org/builders/Chromium%20Linux%20Release%20%28Tests%29/builds/42406

[----------] Global test environment tear-down
[==========] 490 tests from 63 test cases ran. (7061 ms total)
[  PASSED  ] 485 tests.
[  FAILED  ] 5 tests, listed below:
[  FAILED  ] WebCompositorInputHandlerImplTest.gestureFlingIgnoredTouchpad
[  FAILED  ] WebCompositorInputHandlerImplTest.gestureFlingAnimatesTouchpad
[  FAILED  ] WebCompositorInputHandlerImplTest.gestureFlingTransferResetsTouchpad
[  FAILED  ] WebCompositorInputHandlerImplTest.gestureFlingIgnoredTouchscreen
[  FAILED  ] WebCompositorInputHandlerImplTest.lastInputEventForVSync

Expected?

(In reply to comment #27)
> webkit-unit-tests began failing with the patch I think.
> 
> http://build.webkit.org/builders/Chromium%20Linux%20Release%20%28Tests%29/builds/42406
> 
> [----------] Global test environment tear-down
> [==========] 490 tests from 63 test cases ran. (7061 ms total)
> [  PASSED  ] 485 tests.
> [  FAILED  ] 5 tests, listed below:
> [  FAILED  ] WebCompositorInputHandlerImplTest.gestureFlingIgnoredTouchpad
> [  FAILED  ] WebCompositorInputHandlerImplTest.gestureFlingAnimatesTouchpad
> [  FAILED  ] WebCompositorInputHandlerImplTest.gestureFlingTransferResetsTouchpad
> [  FAILED  ] WebCompositorInputHandlerImplTest.gestureFlingIgnoredTouchscreen
> [  FAILED  ] WebCompositorInputHandlerImplTest.lastInputEventForVSync
> 
> Expected?

It might be. This has a chromium side change that is waiting for this to be gardened in. https://codereview.chromium.org/11565022/

(In reply to comment #27)
> webkit-unit-tests began failing with the patch I think.
> 
> http://build.webkit.org/builders/Chromium%20Linux%20Release%20%28Tests%29/builds/42406
> 
> [----------] Global test environment tear-down
> [==========] 490 tests from 63 test cases ran. (7061 ms total)
> [  PASSED  ] 485 tests.
> [  FAILED  ] 5 tests, listed below:
> [  FAILED  ] WebCompositorInputHandlerImplTest.gestureFlingIgnoredTouchpad
> [  FAILED  ] WebCompositorInputHandlerImplTest.gestureFlingAnimatesTouchpad
> [  FAILED  ] WebCompositorInputHandlerImplTest.gestureFlingTransferResetsTouchpad
> [  FAILED  ] WebCompositorInputHandlerImplTest.gestureFlingIgnoredTouchscreen
> [  FAILED  ] WebCompositorInputHandlerImplTest.lastInputEventForVSync
> 
> Expected?

Yes, it looks like I missed to update expected results on some of the tests. Would you like to roll this back or should I upload another one that fixes the test expectations?

[James Robinson]

Hmm, I wonder why cr-linux was green, then.

We need this patch to land the chromium side, so I think updating or suppressing the test failures for now is the only option.

Uploaded https://bugs.webkit.org/process_bug.cgi

Testing locally to verify

Tested and verified https://bugs.webkit.org/show_bug.cgi?id=105059 resolves these issues.

[WebKit Review Bot]

Re-opened since this is blocked by bug 105067

[James Robinson]

There are real bugs uncovered by the tests, we're gonna roll out and try again more carefully.

Created attachment 179560 [details]
Patch

[WebKit Review Bot]

Comment on attachment 179560 [details]
Patch

Clearing flags on attachment: 179560

Committed r137793: <http://trac.webkit.org/changeset/137793>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.