WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
81479
[chromium] Transfer wheel flings to main thread if we hit a region that can't be scrolled from the compositor thread
https://bugs.webkit.org/show_bug.cgi?id=81479
Summary
[chromium] Transfer wheel flings to main thread if we hit a region that can't...
James Robinson
Reported
2012-03-18 18:43:57 PDT
[chromium] Transfer wheel flings to main thread if we hit a region that can't be scrolled from the compositor thread
Attachments
Patch
(51.39 KB, patch)
2012-03-18 18:45 PDT
,
James Robinson
no flags
Details
Formatted Diff
Diff
Patch
(57.53 KB, patch)
2012-03-18 21:58 PDT
,
James Robinson
no flags
Details
Formatted Diff
Diff
Patch
(55.20 KB, patch)
2012-03-19 13:33 PDT
,
James Robinson
no flags
Details
Formatted Diff
Diff
Patch
(55.22 KB, patch)
2012-03-19 13:47 PDT
,
James Robinson
no flags
Details
Formatted Diff
Diff
Patch
(54.54 KB, patch)
2012-03-19 17:29 PDT
,
James Robinson
no flags
Details
Formatted Diff
Diff
Patch
(54.54 KB, patch)
2012-03-19 17:35 PDT
,
James Robinson
no flags
Details
Formatted Diff
Diff
Patch
(70.91 KB, patch)
2012-03-20 22:09 PDT
,
James Robinson
no flags
Details
Formatted Diff
Diff
Patch
(87.62 KB, patch)
2012-03-24 04:16 PDT
,
Nat Duca
no flags
Details
Formatted Diff
Diff
Rebase on top of 82154
(111.00 KB, patch)
2012-03-25 18:31 PDT
,
Nat Duca
no flags
Details
Formatted Diff
Diff
Show Obsolete
(8)
View All
Add attachment
proposed patch, testcase, etc.
James Robinson
Comment 1
2012-03-18 18:45:02 PDT
Created
attachment 132520
[details]
Patch
James Robinson
Comment 2
2012-03-18 18:51:37 PDT
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
Comment 3
2012-03-18 21:58:10 PDT
Created
attachment 132542
[details]
Patch
James Robinson
Comment 4
2012-03-19 13:33:48 PDT
Created
attachment 132651
[details]
Patch
James Robinson
Comment 5
2012-03-19 13:47:44 PDT
Created
attachment 132658
[details]
Patch
James Robinson
Comment 6
2012-03-19 17:29:27 PDT
Created
attachment 132731
[details]
Patch
WebKit Review Bot
Comment 7
2012-03-19 17:33:33 PDT
Please wait for approval from
fishd@chromium.org
,
abarth@webkit.org
or
jamesr@chromium.org
before submitting because this patch contains changes to the Chromium platform API.
WebKit Review Bot
Comment 8
2012-03-19 17:34:01 PDT
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 9
2012-03-19 17:34:09 PDT
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
Comment 10
2012-03-19 17:35:12 PDT
Created
attachment 132733
[details]
Patch
WebKit Review Bot
Comment 11
2012-03-19 17:37:22 PDT
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 12
2012-03-19 19:50:23 PDT
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
Comment 13
2012-03-19 20:47:34 PDT
(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
Comment 14
2012-03-19 21:37:01 PDT
(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
Comment 15
2012-03-20 22:09:36 PDT
Created
attachment 132969
[details]
Patch
James Robinson
Comment 16
2012-03-20 22:14:46 PDT
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
Comment 17
2012-03-21 09:04:22 PDT
(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
Comment 18
2012-03-21 11:10:24 PDT
*** This bug has been marked as a duplicate of
bug 81740
***
Nat Duca
Comment 19
2012-03-23 17:54:26 PDT
Picking this back up.
Nat Duca
Comment 20
2012-03-24 04:16:44 PDT
Created
attachment 133630
[details]
Patch
Nat Duca
Comment 21
2012-03-24 04:18:34 PDT
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
Comment 22
2012-03-24 04:19:53 PDT
Please wait for approval from
abarth@webkit.org
,
dglazkov@chromium.org
,
fishd@chromium.org
,
jamesr@chromium.org
or
tkent@chromium.org
before submitting, as this patch contains changes to the Chromium public API. See also
https://trac.webkit.org/wiki/ChromiumWebKitAPI
.
Build Bot
Comment 23
2012-03-24 04:49:16 PDT
Comment on
attachment 133630
[details]
Patch
Attachment 133630
[details]
did not pass mac-ews (mac): Output:
http://queues.webkit.org/results/12132038
Nat Duca
Comment 24
2012-03-25 17:33:54 PDT
Will fix the unit testing by routing monotonic clock up to WebViewImpl and then making fling animations always use that clock.
Nat Duca
Comment 25
2012-03-25 18:31:13 PDT
Created
attachment 133710
[details]
Rebase on top of 82154
James Robinson
Comment 26
2012-03-29 16:30:16 PDT
r112364
fixed this
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug