[chromium] Fix double call to scrollBegin() when handling scroll gesture
Created attachment 161178 [details] Patch
Comment on attachment 161178 [details] Patch Attachment 161178 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13682187 New failing tests: WebCompositorInputHandlerImplTest.gestureFlingAnimates CCLayerTreeHostTestScrollChildLayer.runMultiThread WebCompositorInputHandlerImplTest.gestureFlingTransferResets
Comment on attachment 161178 [details] Patch Some failing unit tests, will fix
Created attachment 161251 [details] Patch
My previous patch removed the double scrollBegin() call, but on second thoughts, exactly that behavior is expected by the unit tests. For now, just remove the offending ASSERT so the tests pass in debug mode.
Anyone got time to take a look at this? Android content shell is still hitting ASSERTs.
Comment on attachment 161251 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=161251&action=review > Source/WebCore/ChangeLog:9 > + client, then if the result is ScrollStarted, it creates a PlatformGestureCurve I think this is wrong - you should instead fix the PlatformGestureCurve stuff to not do a scrollBegin()
(In reply to comment #7) > (From update of attachment 161251 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=161251&action=review > > > Source/WebCore/ChangeLog:9 > > + client, then if the result is ScrollStarted, it creates a PlatformGestureCurve > > I think this is wrong - you should instead fix the PlatformGestureCurve stuff to not do a scrollBegin() OK, will do -- it'll be along the lines of the first patchset, but I need to fix up those tests as well.
Created attachment 162253 [details] Patch
I modified the gestureFlingAnimates test, and removed gestureFlingTransferResets completely. That was a pretty complicated test but I can't see that it represents usage that would occur in practice. This patch performs well when testing manually in the Android content shell. If there are additional unit tests you think I should add, please let me know.
Comment on attachment 162253 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=162253&action=review > Source/WebKit/chromium/ChangeLog:23 > + doesn't seem to represent normal usage -- composited flings don't turn into > + main thread flings halfway through. Sorry, yes they do for wheel flings. This test and the behavior it tests are important. > Source/WebKit/chromium/src/WebCompositorInputHandlerImpl.cpp:333 > + m_inputHandlerClient->scrollBy(IntPoint(m_wheelFlingParameters.point.x, m_wheelFlingParameters.point.y), This doesn't look right. Keep in mind we have 2 kinds of flings - "gesture" flings and "wheel pad" flings. The former is what Chrome on android has. The latter - which is what this code implements - works like OS X fling by generating a sequence of synthetic wheel events. It looks like you've just deleted the code for the second kind of fling completely. That's definitely not the right fix! I think what you really want to do is for wheel fling type scrolls send a matching scrollEnd() to the scrollBegin() when the fling starts.
(In reply to comment #11) > This doesn't look right. Keep in mind we have 2 kinds of flings - "gesture" flings and "wheel pad" flings. The former is what Chrome on android has. The latter - which is what this code implements - works like OS X fling by generating a sequence of synthetic wheel events. > > It looks like you've just deleted the code for the second kind of fling completely. That's definitely not the right fix! It did seem a bit strange, yes. :) Chrome for Android is hitting that code path, though, and it sounds like maybe it shouldn't -- I'll take a closer look. > I think what you really want to do is for wheel fling type scrolls send a matching scrollEnd() to the scrollBegin() when the fling starts. np, will do
(In reply to comment #12) > Chrome for Android is hitting that code path, though Oops, I don't mean Chrome, I mean the content shell.
I'm somewhat dubious of how android is sharing this code right now. I suspect https://bugs.webkit.org/show_bug.cgi?id=95756 will help.
(In reply to comment #14) > I'm somewhat dubious of how android is sharing this code right now. I suspect https://bugs.webkit.org/show_bug.cgi?id=95756 will help. Oho, yes it will! Downstream we have "FlingAnimator" which basically implements PlatformGestureCurve (but with reversed sign) with different plumbing. Once 95756 gets it, we should be able to hook it up to that.
Still happening. We've now resolved all the Android diffs, but that just means the same bug crops up in our Android branch too.
Created attachment 163365 [details] Patch
Another try. This patch just adds an extra scrollEnd() where it's needed, and makes the unit test more strict to hopefully catch future incorrect usage.
Comment on attachment 163365 [details] Patch Thanks, this is exactly what I had in mind.
Comment on attachment 163365 [details] Patch Clearing flags on attachment: 163365 Committed r128199: <http://trac.webkit.org/changeset/128199>
All reviewed patches have been landed. Closing bug.