Bug 96403 – [chromium] Flings halt immediately on Android content shell

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Iain Merrick]

[chromium] Flings halt immediately on Android content shell

[Iain Merrick]

Created attachment 163392 [details]
Patch

[WebKit Review Bot]

Comment on attachment 163392 [details]
Patch

Attachment 163392 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13824323

[James Robinson]

We already have a bug to calculate proper range, what about doing that?

[Iain Merrick]

Is there code around for that, or just a bug? I was hoping just to do the minimal fix here rather than taking the time to understand and extend the code.

[Iain Merrick]

Er, my laziness aside -- our test infrastructure isn't fully set up to cover this case upstream as far as I can see, so just getting it working and getting fully upstreamed ASAP seems useful.

[James Robinson]

https://bugs.webkit.org/show_bug.cgi?id=93514

That bug has been kicked around for (literally) months.  I'd prefer not to kick it down further

[Iain Merrick]

Do we definitely need it? I guess we'll need to take the range into account somewhere in order to do some kind of overscroll animation (brief highlight or rebound). Do you see that happening in the platform UI layer, or directly in the compositor?

https://bugs.webkit.org/show_bug.cgi?id=93514 is the bug I'm aware of where this has been discussed, but it looks like it didn't get very far.

[James Robinson]

My understanding is the fling range is required to build the correct fling curve on android - is that not true?  If it isn't, why do we need a range at all?

[Iain Merrick]

Here's the Android API that we're calling through to:

http://developer.android.com/reference/android/widget/OverScroller.html#fling(int, int, int, int, int, int, int, int)

If we don't set minX/maxX/minY/maxY, the OverScroller object won't know the bounds of the layer being scrolled, so the gesture adapter will just keep calling scrollBy() until the curve finishes. The layer will just ignore any scrollBy() it can't handle.

If we have nested scrollable layers, this does mean that a single fling gesture could spill over into the outer layer. I think we don't want that on Android -- it should stop the fling when the current layer is exhausted, then scroll the outer layer on the *next* fling -- but it's a pretty subtle distinction.

[James Robinson]

Does that change the curve itself, though?  I thought it did, but if it doesn't then maybe we should just drop the whole range business and just call forceFinished() when we can't take any more scrollBy()s on the currently scrollable layer?

[Iain Merrick]

(In reply to comment #10)
> Does that change the curve itself, though?

Apparently not -- docs say "The distance traveled will depend on the initial velocity of the fling"

> I thought it did, but if it doesn't then maybe we should just drop the whole range business and just call forceFinished() when we can't take any more scrollBy()s on the currently scrollable layer?

That sounds good to me. That's a platform API change, so it needs to be done in a couple of steps, right?

[James Robinson]

Yeah, it might be multi-sided.  I would start by ignoring the parameter on the implementation side, making sure that works, and then working backwards from there.

[James Robinson]

Comment on attachment 163392 [details]
Patch

r- until we can decide if we're ditching this range completely or if we need to calculate it correctly in the short term to get content_shell working, feel free to ignore this value on the chromium side.

[Iain Merrick]

(In reply to comment #13)
> (From update of attachment 163392 [details])
> r- until we can decide if we're ditching this range completely or if we need to calculate it correctly in the short term to get content_shell working, feel free to ignore this value on the chromium side.

Chromium patch: http://codereview.chromium.org/10913218/

[Sami Kyöstilä]

The fling range is useful for two things on Android. First, the fling spline calculated by OverScroller.java[1] varies depending on the target distance. Second,  eventually I think we'll want to render the blue glow and bounce back effects when a fling hits the end of its range.

For the short term I'm fine with hard coding an infinite range for ContentShell, but eventually we should fix this properly.

[1] https://github.com/android/platform_frameworks_base/blob/master/core/java/android/widget/OverScroller.java

[Iain Merrick]

Fixed on the Chromium side.