[chromium] Transfer wheel fling via WebCompositorInputHandlerClient
Created attachment 132970 [details] Patch
Did the cc input handler changes not make it into this patch?
(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.
Created attachment 132986 [details] Patch
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.
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
(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+.
Comment on attachment 132986 [details] Patch I think everything looks fine. I expect this will have more merge conflicts on landing, though.
*** Bug 81479 has been marked as a duplicate of this bug. ***
(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.
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.
Reopening to attach new patch.
Created attachment 133629 [details] Patch
Comment on attachment 133629 [details] Patch Darnit, wrong bug #
Created attachment 133913 [details] Patch
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.
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?
(In reply to comment #18) > (From update of attachment 133913 [details]) I'm fine with this approach in the interests of getting this landed.
(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.
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.
Created attachment 134213 [details] Patch for landing
Comment on attachment 134213 [details] Patch for landing Clearing flags on attachment: 134213 Committed r112364: <http://trac.webkit.org/changeset/112364>
All reviewed patches have been landed. Closing bug.