Bug 81479 – [chromium] Transfer wheel flings to main thread if we hit a region that can't be scrolled from the compositor thread

• Component: New Bugs
• Status: RESOLVED FIXED

[James Robinson]

[chromium] Transfer wheel flings to main thread if we hit a region that can't be scrolled from the compositor thread

[James Robinson]

Created attachment 132520 [details]
Patch

[James Robinson]

This patch depends on https://bugs.webkit.org/show_bug.cgi?id=81458 and https://bugs.webkit.org/show_bug.cgi?id=81462 and has an old version of https://bugs.webkit.org/show_bug.cgi?id=81458 included as part of it.

This is needed for the case where we start a wheel fling on the compositor thread but scroll over an area that needs to process wheel events on the main thread, or we receive a tree update that requires us to process wheels on the main thread.  In this case, the curve is cancelled on the compositor thread and the in-progress animation parameters are copied over to the main thread.  The rest of the wheel fling is then evaluated on the main thread by WebViewImpl.

This touches a lot of files and interfaces, unfortunately.  There is no good way to pass full events from the compositor thread to the main thread without risking serious damage to the input acking system since acks from the main thread don't go back through the compositor thread.  I think we definitely want to handle flings on the compositor thread whenever possible so this complexity is worth it.

The only other way I can think of to avoid this is if we handled the curves somewhere external to both the compositor thread and webkit main thread.

There's one known potential bug - I'm not passing the modifiers from the GestureFlingStart through, so they won't end up on the synthetic wheel events generated by the main thread.  I need to bundle up all the transfer..() parameters into a struct before adding that or I'll go crazy modifying 20000 interfaces.

[James Robinson]

Created attachment 132542 [details]
Patch

[James Robinson]

Created attachment 132651 [details]
Patch

[James Robinson]

Created attachment 132658 [details]
Patch

[James Robinson]

Created attachment 132731 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail] or [e-mail] before submitting because this patch contains changes to the Chromium platform API.

[WebKit Review Bot]

Attachment 132731 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/Platform/ChangeLog', u'Source/Platf..." exit_code: 1
Source/WebCore/platform/CrossThreadCopier.h:44:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 37 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[James Robinson]

Comment on attachment 132731 [details]
Patch

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

> Source/WebKit/chromium/src/WebCompositorInputHandlerImpl.cpp:267
> +        OwnPtr<PlatformGestureCurve> flingCurve = TouchFlingPlatformGestureCurve::create(FloatPoint(gestureEvent.deltaX, gestureEvent.deltaY));

note about directions:

Before this patch, the curve was created in the -deltaX / -deltaY direction and the scrollBy() increment was flipped again in WebCompositorInputHandlerImpl::scrollBy() (see line 315 pre-patch).  I changed this here to match what WebViewImpl does - the curve is generated in the +deltaX / +deltaY direction and the scrollBy applies it without inverting so I wouldn't have to invert again when passing out to the main thread.

> Source/WebKit/chromium/tests/WebCompositorInputHandlerImplTest.cpp:369
> +    // The second call should start scrolling in the -X direction.

this flipped because we don't invert inside scrollBy() any more, see comment above.

[James Robinson]

Created attachment 132733 [details]
Patch

[WebKit Review Bot]

Attachment 132733 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/Platform/ChangeLog', u'Source/Platf..." exit_code: 1
Source/WebCore/platform/CrossThreadCopier.h:44:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 37 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Nat Duca]

Comment on attachment 132733 [details]
Patch

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

LGTM given that we're hurrying. :)

> Source/WebCore/platform/PlatformGestureCurve.h:44
> +    virtual IntSize cumulativeScroll() const = 0;

How will this work for a pinch zoom animation? We had gone down the apply(target) route to avoid the interface specifying the properties that the curve manipulated... this having been said, maybe its okay to mix and match design patterns?

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:417
>  

So, thinking about it, CC doesn't actually want to know a thing about transfer logic, right? This is literally reaching into CC simply because thats the only way to çross the threads. It seems to me that if we had a main-thread counterpart to WCIH [e.g. CCLayerTreeHostClient], then all CC would need to provide is a mechanism for posting a task against the main-thread WCIH counterpart. Am I making sense? Fine if we hold this for later, implementation wise.

> Source/WebKit/chromium/src/WebViewImpl.cpp:635
> +        TRACE_EVENT_INSTANT2("webkit", "WebViewImpl::gestureEvent::GestureFlingStart", "deltaX", event.deltaX, "deltaY", event.deltaY);

These are good candidates for TRACE_EVENT_START/TRACE_EVENT_FINISH. You just need an ID for the gesture (which could be the active gesture animaiton pointer). Not a blocker for the reivew ofc.

> Source/WebKit/chromium/tests/WebCompositorInputHandlerImplTest.cpp:415
>  

Do we need a CCLayerTreeHost test to test the proxy features?

[James Robinson]

(In reply to comment #12)
> (From update of attachment 132733 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=132733&action=review
> 
> LGTM given that we're hurrying. :)
> 
> > Source/WebCore/platform/PlatformGestureCurve.h:44
> > +    virtual IntSize cumulativeScroll() const = 0;
> 
> How will this work for a pinch zoom animation? We had gone down the apply(target) route to avoid the interface specifying the properties that the curve manipulated... this having been said, maybe its okay to mix and match design patterns?
>

It doesn't really work for any sort of curve other than a wheel fling curve.  Although, now that you bring it up this could all be tracked inside of WebCompositorInputHandlerImpl by tracking the callbacks from the curve.  Hmmmmm....
 
> > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:417
> >  
> 
> So, thinking about it, CC doesn't actually want to know a thing about transfer logic, right? This is literally reaching into CC simply because thats the only way to çross the threads. It seems to me that if we had a main-thread counterpart to WCIH [e.g. CCLayerTreeHostClient], then all CC would need to provide is a mechanism for posting a task against the main-thread WCIH counterpart. Am I making sense? Fine if we hold this for later, implementation wise.
> 

Yeah, we could pass this out in another way.  One way would be to pass it out through the WebCompositorInputHandlerClient and have the chromium side get it over to the main thread on the chromium side, then pass it back into WebKit via WebView or WebWidget or something.  That might make more sense from a layering perspective.


> > Source/WebKit/chromium/tests/WebCompositorInputHandlerImplTest.cpp:415
> >  
> 
> Do we need a CCLayerTreeHost test to test the proxy features?

It's mostly pass through (now at least), but more tests couldn't hurt!

[Adrienne Walker]

(In reply to comment #13)
> (In reply to comment #12)
> > (From update of attachment 132733 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=132733&action=review
>
> > > Source/WebKit/chromium/tests/WebCompositorInputHandlerImplTest.cpp:415
> > >  
> > 
> > Do we need a CCLayerTreeHost test to test the proxy features?
> 
> It's mostly pass through (now at least), but more tests couldn't hurt!

It's mostly pass through, but I'm more worried about the synchronization.  Does applyScrollAndScale get called at the right time?

I worry about a potential ordering issue applying scrolling and transferring the animation, maybe causing some discrepancy between "global point" and the current scroll position?  I'd feel better with a proxy test to verify that a scroll on the impl thread is equal to the scroll transferred to the main thread, similar to how we have some tests to make sure that scroll positions copied back end up being ultimately consistent.

[James Robinson]

Created attachment 132969 [details]
Patch

[James Robinson]

I think we're OK w.r.t. applyScrollAndScale since they are both compositor thread -> main thread PostTask()s, so they'll be processed in the correct order on the main thread.

Updated this patch with the feedback so far.  I also implemented the transfer via WebCompositorInputHandlerClient, and it's a whole lot cleaner - but it requires a chromium-side patch that I haven't even compile checked yet.  So this is here for people to test with and play with, and in case my chromium side changes aren't quite right.  Given infinite time, I'd rather do the routing on the chromium side so that the core compositor code doesn't need to know anything about this.  In general I think the less parts of the compositor that have to be aware of input stuff the better. 

Here's the alternative:
https://bugs.webkit.org/show_bug.cgi?id=81740
https://chromiumcodereview.appspot.com/9802006

[Adrienne Walker]

(In reply to comment #16)
> I think we're OK w.r.t. applyScrollAndScale since they are both compositor thread -> main thread PostTask()s, so they'll be processed in the correct order on the main thread.

I stared at this some more, and I think we're ok, mostly because CCLTH::applyScrollAndScale is called before CCLTH::updateAnimations.  So, even though the gesture gets transferred and set prior to the BFAC with the croll updates, the scroll position is applied before we start sending synthetic wheel events, so everything should end up synced properly.

[Adrienne Walker]

*** This bug has been marked as a duplicate of bug 81740 ***

[Nat Duca]

Picking this back up.

[Nat Duca]

Created attachment 133630 [details]
Patch

[Nat Duca]

Hey james, one open issue --- the timeshift that we do from monotonic to regular time then mucks up the tests.

Two options:
1. option not listed --- something cool that you think up
2. I get monotonic time on the main thread and we make webviewimpl tick its wheel fling gestures using monotonic time [even though raf uses "regular" time.

Wdyt?

[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.

[Build Bot]

Comment on attachment 133630 [details]
Patch

Attachment 133630 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12132038

[Nat Duca]

Will fix the unit testing by routing monotonic clock up to WebViewImpl and then making fling animations always use that clock.

[Nat Duca]

Created attachment 133710 [details]
Rebase on top of 82154

[James Robinson]

r112364 fixed this