Bug 95322 – [chromium] Fix double call to scrollBegin() when handling scroll gesture

• Component: New Bugs
• Status: RESOLVED FIXED

[Iain Merrick]

[chromium] Fix double call to scrollBegin() when handling scroll gesture

[Iain Merrick]

Created attachment 161178 [details]
Patch

[WebKit Review Bot]

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

[Iain Merrick]

Comment on attachment 161178 [details]
Patch

Some failing unit tests, will fix

[Iain Merrick]

Created attachment 161251 [details]
Patch

[Iain Merrick]

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.

[Iain Merrick]

Anyone got time to take a look at this?

Android content shell is still hitting ASSERTs.

[James Robinson]

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()

[Iain Merrick]

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

[Iain Merrick]

Created attachment 162253 [details]
Patch

[Iain Merrick]

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.

[James Robinson]

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.

[Iain Merrick]

(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

[Iain Merrick]

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

[James Robinson]

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.

[Iain Merrick]

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

[Iain Merrick]

Still happening. We've now resolved all the Android diffs, but that just means the same bug crops up in our Android branch too.

[Iain Merrick]

Created attachment 163365 [details]
Patch

[Iain Merrick]

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.

[James Robinson]

Comment on attachment 163365 [details]
Patch

Thanks, this is exactly what I had in mind.

[WebKit Review Bot]

Comment on attachment 163365 [details]
Patch

Clearing flags on attachment: 163365

Committed r128199: <http://trac.webkit.org/changeset/128199>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.