NOT FOR LANDING - [WebView] Allow fling animation via the LayerScrollOffsetDelegate

NOT FOR LANDING - [WebView] Allow fling animation via the LayerScrollOffsetDelegate

BUG=378984

[jdduke (slow), 2014-05-14 18:53:09]

mkosiba@, here's a very rough sketch of some plumbing for the unified fling
pipeline we discussed.

boliu@ mentioned that embedders can optionally handle or turn off the root layer
fling, in which case this may not work, or would require some further tweaking
(perhaps the scroll delegate yields the root fling event (via
FlingScrollBegin(...)) to a client, and if the client doesn't consume the fling
we start the fling animation as usual in InputHandlerProxy)?

[mkosiba (inactive), 2014-05-15 13:48:42]

On 2014/05/14 18:53:09, jdduke wrote:
> mkosiba@, here's a very rough sketch of some plumbing for the unified fling
> pipeline we discussed.

looks pretty neat!

> boliu@ mentioned that embedders can optionally handle or turn off the root
layer
> fling, in which case this may not work, or would require some further tweaking
> (perhaps the scroll delegate yields the root fling event (via
> FlingScrollBegin(...)) to a client, and if the client doesn't consume the
fling
> we start the fling animation as usual in InputHandlerProxy)?

Embedders have "noticed" that if they suppress calls to computeScroll (which is
the equivalent to the android_webview layer never calling AnimateCallback) the
fling animation never takes place.

I put in some suggestions/questions in my other comment, but in general there is
no clean 'handoff' here - the embedder could do something like:

    @Override
    protected void computeScroll() {
       if ((new Random()).nextBoolean())
           super.computeScroll();
    }

which would currently result in a super-janky fling animation. I'm not saying
that
this is what we need to support, just using this as an example.

[mkosiba (inactive), 2014-05-15 13:48:50]

https://codereview.chromium.org/274323004/diff/1/android_webview/browser/brow...
File android_webview/browser/browser_view_renderer.cc (right):

https://codereview.chromium.org/274323004/diff/1/android_webview/browser/brow...
android_webview/browser/browser_view_renderer.cc:632: // Stash the callback and
wait for a call? Trigger an invalidate?
probably more like TODO(mkosiba): Implement. ;)

The implementation would be to stash the callback, invalidate and wait for a
call.

If a fling is active would we be getting a call to SetNeedsAnimate shortly after
(or immediately after) we call the AnimationCallback? (in other words should we
consider AnimationCallback one-shot?)

Is it bad for AnimationCallback to not be called at all? We could detect that
the embedder is suppressing computeScroll and somehow cancel the fling animation
I guess.

https://codereview.chromium.org/274323004/diff/1/cc/input/layer_scroll_offset...
File cc/input/layer_scroll_offset_delegate.h (right):

https://codereview.chromium.org/274323004/diff/1/cc/input/layer_scroll_offset...
cc/input/layer_scroll_offset_delegate.h:41: // TODO(jdduke): Remove?
maybe. This is still used by stuff like pageUp/pageDown and
requestChildRectangleOnScreen. If we take it out those animations might become
choppy. OTOH those APIs are probably not used that much so it could be fine.

https://codereview.chromium.org/274323004/diff/1/cc/trees/layer_tree_host_imp...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/274323004/diff/1/cc/trees/layer_tree_host_imp...
cc/trees/layer_tree_host_impl.cc:402: AnimateFling(monotonic_time);
I don't think we want to call this at all if layer_scroll_offset_delegate is not
null

[jdduke (slow), 2014-05-15 14:15:42]

https://codereview.chromium.org/274323004/diff/1/android_webview/browser/brow...
File android_webview/browser/browser_view_renderer.cc (right):

https://codereview.chromium.org/274323004/diff/1/android_webview/browser/brow...
android_webview/browser/browser_view_renderer.cc:632: // Stash the callback and
wait for a call? Trigger an invalidate?
On 2014/05/15 13:48:51, mkosiba wrote:
> probably more like TODO(mkosiba): Implement. ;)
> 
> The implementation would be to stash the callback, invalidate and wait for a
> call.
> 
> If a fling is active would we be getting a call to SetNeedsAnimate shortly
after
> (or immediately after) we call the AnimationCallback? (in other words should
we
> consider AnimationCallback one-shot?)
> 

Yes, you get the |SetNeedsAnimate()| call either 1) When the fling is first
received and needs scheduling, or 2) You tick the fling (via the animation
callback), and the fling is still active, so it requests another update (in
which case |SetNeedsAnimate()| is called synchronously with respect to
animation.Run(...)). 

Also, we don't necessarily need to use an actual Callback, we could as easily
use an interface that we provide the offset delegate, but the callback approach
is pretty simple (though we'll want to make sure the weak ptr is still valid
before running? or does it to that automatically?).

> Is it bad for AnimationCallback to not be called at all? We could detect that
> the embedder is suppressing computeScroll and somehow cancel the fling
animation
> I guess.

The consequences of not calling this would simply be that the fling animation
has no effect.  There won't be any additional ticking; the fling will just lie
dormant in the InputHandlerProxy over its lifetime, and will get canceled by a
GestureFlingCancel at some point.  I guess the only reason to early-cancel would
be to prevent it being "boosted" with another fling, but in that case you'd be
suppressing the subsequent fling anyway?

https://codereview.chromium.org/274323004/diff/1/cc/input/layer_scroll_offset...
File cc/input/layer_scroll_offset_delegate.h (right):

https://codereview.chromium.org/274323004/diff/1/cc/input/layer_scroll_offset...
cc/input/layer_scroll_offset_delegate.h:41: // TODO(jdduke): Remove?
On 2014/05/15 13:48:51, mkosiba wrote:
> maybe. This is still used by stuff like pageUp/pageDown and
> requestChildRectangleOnScreen. If we take it out those animations might become
> choppy. OTOH those APIs are probably not used that much so it could be fine.

Gotcha, I suppose we could leave in for now.

https://codereview.chromium.org/274323004/diff/1/cc/trees/layer_tree_host_imp...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/274323004/diff/1/cc/trees/layer_tree_host_imp...
cc/trees/layer_tree_host_impl.cc:402: AnimateFling(monotonic_time);
On 2014/05/15 13:48:51, mkosiba wrote:
> I don't think we want to call this at all if layer_scroll_offset_delegate is
not
> null

Oops yeah that's definitely what I had planned.

[mkosiba (inactive), 2014-05-15 17:23:38]

looks cool. I'll try and build the remaining webview plumbing on top of this. I
don't think I'll get around to doing that this week though.

https://codereview.chromium.org/274323004/diff/1/android_webview/browser/brow...
File android_webview/browser/browser_view_renderer.cc (right):

https://codereview.chromium.org/274323004/diff/1/android_webview/browser/brow...
android_webview/browser/browser_view_renderer.cc:632: // Stash the callback and
wait for a call? Trigger an invalidate?
On 2014/05/15 14:15:43, jdduke wrote:
> On 2014/05/15 13:48:51, mkosiba wrote:
> > probably more like TODO(mkosiba): Implement. ;)
> > 
> > The implementation would be to stash the callback, invalidate and wait for a
> > call.
> > 
> > If a fling is active would we be getting a call to SetNeedsAnimate shortly
> after
> > (or immediately after) we call the AnimationCallback? (in other words should
> we
> > consider AnimationCallback one-shot?)
> > 
> 
> Yes, you get the |SetNeedsAnimate()| call either 1) When the fling is first
> received and needs scheduling, or 2) You tick the fling (via the animation
> callback), and the fling is still active, so it requests another update (in
> which case |SetNeedsAnimate()| is called synchronously with respect to
> animation.Run(...)). 

ok, thanks for the explanation. sounds good.

> 
> Also, we don't necessarily need to use an actual Callback, we could as easily
> use an interface that we provide the offset delegate, but the callback
approach
> is pretty simple (though we'll want to make sure the weak ptr is still valid
> before running? or does it to that automatically?).

callback is fine I think. It does the right thing - using weak ptr means the
callback doesn't do anything when you Run() or PostTask it.

> 
> > Is it bad for AnimationCallback to not be called at all? We could detect
that
> > the embedder is suppressing computeScroll and somehow cancel the fling
> animation
> > I guess.
> 
> The consequences of not calling this would simply be that the fling animation
> has no effect.  There won't be any additional ticking; the fling will just lie
> dormant in the InputHandlerProxy over its lifetime, and will get canceled by a
> GestureFlingCancel at some point.  I guess the only reason to early-cancel
would
> be to prevent it being "boosted" with another fling, but in that case you'd be
> suppressing the subsequent fling anyway?

Ok, in that case I don't think we need to do anything special. I was concerned
that if we never "finish" the fling CC will think we're running the fling
forever and thus will always be using bilinear filtering (or whatever we use
when IsExternalFlingActive is true).
It sounds like we'd get a GestureFlingCancel at some point anyway so this is
probably irrelevant.

[jdduke (slow), 2014-05-15 17:24:48]

On 2014/05/15 17:23:38, mkosiba wrote:
> looks cool. I'll try and build the remaining webview plumbing on top of this.
I
> don't think I'll get around to doing that this week though.

Sounds good, yeah I don't think we're in a rush here, I just realized I promised
to whip this up a few weeks ago and I never did :)

[jdduke (slow), 2014-05-15 21:25:45]

On 2014/05/15 17:24:48, jdduke wrote:
> On 2014/05/15 17:23:38, mkosiba wrote:
> > looks cool. I'll try and build the remaining webview plumbing on top of
this.
> I
> > don't think I'll get around to doing that this week though.
> 
> Sounds good, yeah I don't think we're in a rush here, I just realized I
promised
> to whip this up a few weeks ago and I never did :)

I'll go ahead and close this for now.  The key change this would need you've
already noticed (not animating the fling in LTHI::Animate if there's a root
delegate and the root is locked to the scrolling layer).