Bug 81740

Summary: [chromium] Transfer wheel fling via WebCompositorInputHandlerClient
Product: WebKit Reporter: James Robinson <jamesr>
Component: New BugsAssignee: James Robinson <jamesr>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, cc-bugs, davemoore, dglazkov, enne, fishd, levin+threading, nduca, rjkroege, tkent, webkit.review.bot, wjmaclean
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 82154    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch for landing none

James Robinson
Reported 2012-03-20 22:13:24 PDT
[chromium] Transfer wheel fling via WebCompositorInputHandlerClient
Attachments
Patch (60.93 KB, patch)
2012-03-20 22:14 PDT, James Robinson
no flags
Patch (62.28 KB, patch)
2012-03-21 01:10 PDT, Nat Duca
no flags
Patch (84.67 KB, patch)
2012-03-24 04:13 PDT, Nat Duca
no flags
Patch (51.82 KB, patch)
2012-03-26 15:51 PDT, James Robinson
no flags
Patch for landing (51.88 KB, patch)
2012-03-27 21:32 PDT, James Robinson
no flags
James Robinson
Comment 1 2012-03-20 22:14:27 PDT
Nat Duca
Comment 2 2012-03-20 22:31:30 PDT
Did the cc input handler changes not make it into this patch?
Nat Duca
Comment 3 2012-03-20 23:43:53 PDT
(In reply to comment #2) > Did the cc input handler changes not make it into this patch? Oh, I'm retarded. Just saw the notes on the previous bug.
Nat Duca
Comment 4 2012-03-21 01:10:51 PDT
WebKit Review Bot
Comment 5 2012-03-21 01:14:35 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.
WebKit Review Bot
Comment 6 2012-03-21 01:53:05 PDT
Comment on attachment 132986 [details] Patch Attachment 132986 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12070273 New failing tests: fast/dom/error-to-string-stack-overflow.html
Adrienne Walker
Comment 7 2012-03-21 11:07:23 PDT
(In reply to comment #6) > (From update of attachment 132986 [details]) > Attachment 132986 [details] did not pass chromium-ews (chromium-xvfb): > Output: http://queues.webkit.org/results/12070273 > > New failing tests: > fast/dom/error-to-string-stack-overflow.html That looks like flake. This all looks fine to me. jamesr, do you have any feedback on Nat's modification of your original patch? Otherwise, I'm inclined to R+.
James Robinson
Comment 8 2012-03-21 11:08:57 PDT
Comment on attachment 132986 [details] Patch I think everything looks fine. I expect this will have more merge conflicts on landing, though.
Adrienne Walker
Comment 9 2012-03-21 11:10:24 PDT
*** Bug 81479 has been marked as a duplicate of this bug. ***
James Robinson
Comment 10 2012-03-21 11:12:12 PDT
(In reply to comment #9) > *** Bug 81479 has been marked as a duplicate of this bug. *** They aren't strictly speaking dupes. https://bugs.webkit.org/show_bug.cgi?id=81479 is a hookup path that works purely in WebKit land, so it's a one-patch fix. This patch depends on chromium-side changes that aren't fully baked yet. So if we want something that works quickly then I think https://bugs.webkit.org/show_bug.cgi?id=81479 is the way to go, despite being uglier.
Nat Duca
Comment 11 2012-03-22 01:56:37 PDT
Yeah, I actually will probably go back to the original patch or a slight variation on it. This patch relies on compositor ids being == routing ids, which isn't the case. We could make it that case but that itself is clunky. I think its fine to go through the proxy. Maybe if this becomes more commonplace, we can figure out the right magic to make this less pipey.
Nat Duca
Comment 12 2012-03-24 04:13:29 PDT
Reopening to attach new patch.
Nat Duca
Comment 13 2012-03-24 04:13:31 PDT
Nat Duca
Comment 14 2012-03-24 04:14:21 PDT
Comment on attachment 133629 [details] Patch Darnit, wrong bug #
James Robinson
Comment 15 2012-03-26 15:50:59 PDT
Reopening to attach new patch.
James Robinson
Comment 16 2012-03-26 15:51:02 PDT
James Robinson
Comment 17 2012-03-26 15:53:52 PDT
Depends on http://codereview.chromium.org/9802006/. I think this hookup is fairly clean. It does depend on https://bugs.webkit.org/show_bug.cgi?id=82154 to get the right clock on the main thread (since the start time passed through is monotonic). Nat pointed out that it might be useful to have a generic "run this task on the main thread" interface from the WebCompositorInputHandler or similar. I think we should consider adding it as soon as we have a 2nd caller that would use it.
Adrienne Walker
Comment 18 2012-03-26 16:45:41 PDT
Comment on attachment 133913 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=133913&action=review The WebKit side looks good to me. A few minor comments: > Source/WebCore/platform/ActivePlatformGestureAnimation.cpp:73 > +{ > +} Can you put a trace event here, like the other constructor? > Source/WebKit/chromium/tests/WebCompositorInputHandlerImplTest.cpp:397 > + // *) cumulativeScroll depends on the curve, but since we've animated in the -X direction the X value should be < 0 These sign flips are all really hard to follow. The delta being positive means animating in the negative direction, and the X value being < 0 means the cumulative scroll is > 0. Is there any way to make these comments or variable names more readable?
Nat Duca
Comment 19 2012-03-26 16:51:56 PDT
(In reply to comment #18) > (From update of attachment 133913 [details]) I'm fine with this approach in the interests of getting this landed.
James Robinson
Comment 20 2012-03-26 19:01:09 PDT
(In reply to comment #18) > (From update of attachment 133913 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=133913&action=review > > The WebKit side looks good to me. A few minor comments: > > > Source/WebCore/platform/ActivePlatformGestureAnimation.cpp:73 > > +{ > > +} > > Can you put a trace event here, like the other constructor? Done > > > Source/WebKit/chromium/tests/WebCompositorInputHandlerImplTest.cpp:397 > > + // *) cumulativeScroll depends on the curve, but since we've animated in the -X direction the X value should be < 0 > > These sign flips are all really hard to follow. The delta being positive means animating in the negative direction, and the X value being < 0 means the cumulative scroll is > 0. Is there any way to make these comments or variable names more readable? I agree - at least now the WebCompositorInputHandlerImpl code is consistent with what WebViewImpl does. I can't think of much else really brilliant to do, tho.
James Robinson
Comment 21 2012-03-26 19:09:58 PDT
Depends on https://bugs.webkit.org/show_bug.cgi?id=82154 to pass the monotonic clock into m_gestureAnimations->animate in WebViewImpl::updateAnimations(). This patch will still compile, but the transferred animations will have a bad timebase before https://bugs.webkit.org/show_bug.cgi?id=82154 lands.
James Robinson
Comment 22 2012-03-27 21:32:59 PDT
Created attachment 134213 [details] Patch for landing
WebKit Review Bot
Comment 23 2012-03-27 23:08:33 PDT
Comment on attachment 134213 [details] Patch for landing Clearing flags on attachment: 134213 Committed r112364: <http://trac.webkit.org/changeset/112364>
WebKit Review Bot
Comment 24 2012-03-27 23:08:40 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.