Unify Android Webview and Chrome's fling

Unify Android Webview and Chrome's fling

There are two ways to generate flings in Android WebView: through user
finger gesture or through calling WebView API flingScroll(int vx, int
vy).  This CL unifies Android WebView's fling code with Chrome. This
means Android WebView will use the same fling curve in InputHandlerProxy
as Chrome now does.

Also note "smooth scrolling" (which is used by WebView#pageUp(),
WebView#pageDown() and WebView#requestChildRectangleOnScreen(child,
rect, false)) logic stays unchanged in this CL and does not use the same
fling curve as mentioned above.  Instead, it uses the underlying curve
in android.widget.OverScroller.startScroll().

Corner cases:
1. What happens when a smooth scroll immediately follows a fling before the
fling finishes?
The fling will be stopped and smooth scroll takes control of WebView's
scroll offset.

2. What happens when a fling immediately follows a smooth scroll before it
finishes?
The smooth scroll will be stopped and fling takes control of WebView's
scroll offset.

Next steps:
1. crbug.com/493765: make the API WebView#flingScroll() always target
root layer.
2. Smooth scroll is still handled by WebView. Expose
ui::Scroller::StartScroll to support smooth scroll in chromium
compositor.

BUG=378984
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/b0ee8dc431d6d871f4001c9eb924d33a33d9e874
Cr-Commit-Position: refs/heads/master@{#333626}

[hush (inactive), 2015-05-04 21:05:37]

hush@chromium.org changed reviewers:
+ jdduke@chromium.org

[hush (inactive), 2015-05-04 21:05:38]

Hi Jared,
PTAL PS6!
The tests are passing.. I haven't added a unit test in LTHI for testing root
layer fling yet.

[jdduke (slow), 2015-05-04 22:02:10]

Thanks for tackling this.

https://codereview.chromium.org/1063853005/diff/100001/android_webview/java/s...
File android_webview/java/src/org/chromium/android_webview/AwContents.java
(right):

https://codereview.chromium.org/1063853005/diff/100001/android_webview/java/s...
android_webview/java/src/org/chromium/android_webview/AwContents.java:580:
public void onUnhandledFlingStartEvent(int velocityX, int velocityY) {
Looks like we can completely eliminate this method from the GestureStateListener
interface.

https://codereview.chromium.org/1063853005/diff/100001/content/renderer/input...
File content/renderer/input/input_handler_proxy.cc (right):

https://codereview.chromium.org/1063853005/diff/100001/content/renderer/input...
content/renderer/input/input_handler_proxy.cc:419: if
(gesture_scroll_on_impl_thread_)
I'm surprised this was necessary, was there a particular gesture sequence where
we didn't receive a GestureFlingCancel first?

[jdduke (slow), 2015-05-04 22:11:48]

https://codereview.chromium.org/1063853005/diff/100001/android_webview/java/s...
File android_webview/java/src/org/chromium/android_webview/AwContents.java
(right):

https://codereview.chromium.org/1063853005/diff/100001/android_webview/java/s...
android_webview/java/src/org/chromium/android_webview/AwContents.java:2525: //
TODO(jdduke): Skip this for components with non-zero velocity?
How important is this todo? Can you verify that the glow works if you can
overscroll both horizontally and vertically from a fling (i.e., start the fling
when both axes are scrollable and overscroll into the corner)?

https://codereview.chromium.org/1063853005/diff/100001/cc/input/layer_scroll_...
File cc/input/layer_scroll_offset_delegate.h (right):

https://codereview.chromium.org/1063853005/diff/100001/cc/input/layer_scroll_...
cc/input/layer_scroll_offset_delegate.h:49: // TODO(jdduke): Remove?
Should we rename this to |IsExternalScrollActive()| (and so on up the chain)?

[hush (inactive), 2015-05-05 21:15:32]

https://codereview.chromium.org/1063853005/diff/100001/android_webview/java/s...
File android_webview/java/src/org/chromium/android_webview/AwContents.java
(right):

https://codereview.chromium.org/1063853005/diff/100001/android_webview/java/s...
android_webview/java/src/org/chromium/android_webview/AwContents.java:580:
public void onUnhandledFlingStartEvent(int velocityX, int velocityY) {
On 2015/05/04 22:02:10, jdduke wrote:
> Looks like we can completely eliminate this method from the
GestureStateListener
> interface.

Done.

https://codereview.chromium.org/1063853005/diff/100001/android_webview/java/s...
android_webview/java/src/org/chromium/android_webview/AwContents.java:2525: //
TODO(jdduke): Skip this for components with non-zero velocity?
On 2015/05/04 22:11:48, jdduke wrote:
> How important is this todo? Can you verify that the glow works if you can
> overscroll both horizontally and vertically from a fling (i.e., start the
fling
> when both axes are scrollable and overscroll into the corner)?

Glow works with the current logic.
I actually just did this TODO and glow works as well (both in flinging near the
corner and scrolling at the corner)

https://codereview.chromium.org/1063853005/diff/100001/cc/input/layer_scroll_...
File cc/input/layer_scroll_offset_delegate.h (right):

https://codereview.chromium.org/1063853005/diff/100001/cc/input/layer_scroll_...
cc/input/layer_scroll_offset_delegate.h:49: // TODO(jdduke): Remove?
On 2015/05/04 22:11:48, jdduke wrote:
> Should we rename this to |IsExternalScrollActive()| (and so on up the chain)?

Yes. Will do. I will also remove it when smooth scrolling logic is in chrome.

https://codereview.chromium.org/1063853005/diff/100001/content/renderer/input...
File content/renderer/input/input_handler_proxy.cc (right):

https://codereview.chromium.org/1063853005/diff/100001/content/renderer/input...
content/renderer/input/input_handler_proxy.cc:419: if
(gesture_scroll_on_impl_thread_)
On 2015/05/04 22:02:10, jdduke wrote:
> I'm surprised this was necessary, was there a particular gesture sequence
where
> we didn't receive a GestureFlingCancel first?

This is not necessary in chrome code I think.
In Android WebView, you could fling the webview with the finger immediately
after calling the Java API "WebView#flingScroll()", before the synthesized fling
finishes. I hit this DCHECK in that case.

When 2 real flings go back to back, CancelCurrentFlingWithoutNotifyingClient
will always be called before the 2nd fling. But when there is a real fling
immediately following a synthesized fling,
CancelCurrentFlingWithoutNotifyingClient is sometimes not called before the real
fling. And that is the problem. I got around it by calling "CancelCurrentFling"
in this case.
The better solution would be to figure out why
CancelCurrentFlingWithoutNotifyingClient is not called in that case. I don't
know it yet.

[jdduke (slow), 2015-05-05 21:38:01]

jdduke@chromium.org changed reviewers:
+ aelias@chromium.org

[jdduke (slow), 2015-05-05 21:38:02]

+aelias for thoughts on the design (one that mkosiba and I toyed with a long
time ago).

A high level overview: We'd like for composited root-layer fling animation code
on WebView to live in the same place as other platforms. The problem is that we
need a signal different from cc's animation to "tick" the WebView fling (via
compute View.computeScroll). This patch makes the root scroll offset delegate
responsible for this input tick by providing it a (cached) callback each time it
needs another fling/input update. The wiring is a little messy but probably no
more messy than the rest of the cc/WebView interop code.

[aelias_OOO_until_Jul13, 2015-05-06 00:25:00]

Could you remind me why CC's self-scheduling can't be used for them?

[jdduke (slow), 2015-05-06 01:28:45]

On 2015/05/06 00:25:00, aelias wrote:
> Could you remind me why CC's self-scheduling can't be used for them?

Apparently the side effects of the fling animation need to be driven by
View.computeScroll, at least that's the impression I got from mkosiba@. I
haven't a clue why that is the case, but we should definitely make sure (as
otherwise this should be a pretty trivial change).

[jdduke (slow), 2015-05-06 01:30:04]

+cc boliu who might know more about how fling timing should behave.

[boliu, 2015-05-06 02:22:14]

On 2015/05/06 01:30:04, jdduke wrote:
> +cc boliu who might know more about how fling timing should behave.

From me looking around framework code, computeScroll normally happens in draw
step, but of course apps can call it directly if they want, not sure why though.
All the scroller implementations use the system clock rather than frame time
from choreographer.

Not sure what else you are looking for here. Maybe talk tomorrow?

[jdduke (slow), 2015-05-06 18:25:22]

On 2015/05/06 02:22:14, boliu wrote:
> On 2015/05/06 01:30:04, jdduke wrote:
> > +cc boliu who might know more about how fling timing should behave.
> 
> From me looking around framework code, computeScroll normally happens in draw
> step, but of course apps can call it directly if they want, not sure why
though.
> All the scroller implementations use the system clock rather than frame time
> from choreographer.
> 
> Not sure what else you are looking for here. Maybe talk tomorrow?

Hmm, chatting with Bo, he seems to believe that we don't actually need
View.computeScroll to drive the fling at all. I'm still a bit concerned as I
recall this being the big issue in the first place (maybe with respect to custom
scroll effects or overscroll behavior with embedding apps?), but maybe we can
proceed and wait until we actually run into issues?

In that case, I guess we can just get rid of all of the plumbing for animate
input between cc and WebView, and just use the regular cc animation tick to
drive the fling as we do on other platforms. View.computeScroll will then only
drive the smooth scrolling stuff in Java, and we'll still need the
|IsExternalScrollActive| hook.

[hush (inactive), 2015-05-08 17:52:40]

On 2015/05/06 18:25:22, jdduke wrote:
> On 2015/05/06 02:22:14, boliu wrote:
> > On 2015/05/06 01:30:04, jdduke wrote:
> > > +cc boliu who might know more about how fling timing should behave.
> > 
> > From me looking around framework code, computeScroll normally happens in
draw
> > step, but of course apps can call it directly if they want, not sure why
> though.
> > All the scroller implementations use the system clock rather than frame time
> > from choreographer.
> > 
> > Not sure what else you are looking for here. Maybe talk tomorrow?
> 
> Hmm, chatting with Bo, he seems to believe that we don't actually need
> View.computeScroll to drive the fling at all. I'm still a bit concerned as I
> recall this being the big issue in the first place (maybe with respect to
custom
> scroll effects or overscroll behavior with embedding apps?), but maybe we can
> proceed and wait until we actually run into issues?
> 
> In that case, I guess we can just get rid of all of the plumbing for animate
> input between cc and WebView, and just use the regular cc animation tick to
> drive the fling as we do on other platforms. View.computeScroll will then only
> drive the smooth scrolling stuff in Java, and we'll still need the
> |IsExternalScrollActive| hook.

sorry for the delayed response (there was another fire in webview). I think
WebView#computeScroll is still important for apps that want to have custom
scroll behavior. See the comment section in
https://codereview.chromium.org/274323004/

The apps can override computeScroll to nothing so that they can do their own
scrolling. For example, Inbox app overrides computeScroll to an empty function.
See this internal link
https://cs.corp.google.com/#piper///depot/google3/java/com/google/android/app...

[boliu, 2015-05-08 22:32:27]

On 2015/05/08 17:52:40, hush wrote:
> On 2015/05/06 18:25:22, jdduke wrote:
> > On 2015/05/06 02:22:14, boliu wrote:
> > > On 2015/05/06 01:30:04, jdduke wrote:
> > > > +cc boliu who might know more about how fling timing should behave.
> > > 
> > > From me looking around framework code, computeScroll normally happens in
> draw
> > > step, but of course apps can call it directly if they want, not sure why
> > though.
> > > All the scroller implementations use the system clock rather than frame
time
> > > from choreographer.
> > > 
> > > Not sure what else you are looking for here. Maybe talk tomorrow?
> > 
> > Hmm, chatting with Bo, he seems to believe that we don't actually need
> > View.computeScroll to drive the fling at all. I'm still a bit concerned as I
> > recall this being the big issue in the first place (maybe with respect to
> custom
> > scroll effects or overscroll behavior with embedding apps?), but maybe we
can
> > proceed and wait until we actually run into issues?
> > 
> > In that case, I guess we can just get rid of all of the plumbing for animate
> > input between cc and WebView, and just use the regular cc animation tick to
> > drive the fling as we do on other platforms. View.computeScroll will then
only
> > drive the smooth scrolling stuff in Java, and we'll still need the
> > |IsExternalScrollActive| hook.
> 
> sorry for the delayed response (there was another fire in webview). I think
> WebView#computeScroll is still important for apps that want to have custom
> scroll behavior. See the comment section in
> https://codereview.chromium.org/274323004/
> 
> The apps can override computeScroll to nothing so that they can do their own
> scrolling. For example, Inbox app overrides computeScroll to an empty
function.
> See this internal link
>
https://cs.corp.google.com/#piper///depot/google3/java/com/google/android/app...

Hmm, so apps rely on computeScroll being the only place that scroll offsets are
updated? Is that actually true? If not, I don't think overriding computeScroll
works in general?

[hush (inactive), 2015-05-09 00:19:49]

On 2015/05/08 22:32:27, boliu wrote:
> On 2015/05/08 17:52:40, hush wrote:
> > On 2015/05/06 18:25:22, jdduke wrote:
> > > On 2015/05/06 02:22:14, boliu wrote:
> > > > On 2015/05/06 01:30:04, jdduke wrote:
> > > > > +cc boliu who might know more about how fling timing should behave.
> > > > 
> > > > From me looking around framework code, computeScroll normally happens in
> > draw
> > > > step, but of course apps can call it directly if they want, not sure why
> > > though.
> > > > All the scroller implementations use the system clock rather than frame
> time
> > > > from choreographer.
> > > > 
> > > > Not sure what else you are looking for here. Maybe talk tomorrow?
> > > 
> > > Hmm, chatting with Bo, he seems to believe that we don't actually need
> > > View.computeScroll to drive the fling at all. I'm still a bit concerned as
I
> > > recall this being the big issue in the first place (maybe with respect to
> > custom
> > > scroll effects or overscroll behavior with embedding apps?), but maybe we
> can
> > > proceed and wait until we actually run into issues?
> > > 
> > > In that case, I guess we can just get rid of all of the plumbing for
animate
> > > input between cc and WebView, and just use the regular cc animation tick
to
> > > drive the fling as we do on other platforms. View.computeScroll will then
> only
> > > drive the smooth scrolling stuff in Java, and we'll still need the
> > > |IsExternalScrollActive| hook.
> > 
> > sorry for the delayed response (there was another fire in webview). I think
> > WebView#computeScroll is still important for apps that want to have custom
> > scroll behavior. See the comment section in
> > https://codereview.chromium.org/274323004/
> > 
> > The apps can override computeScroll to nothing so that they can do their own
> > scrolling. For example, Inbox app overrides computeScroll to an empty
> function.
> > See this internal link
> >
>
https://cs.corp.google.com/#piper///depot/google3/java/com/google/android/app...
> 
> Hmm, so apps rely on computeScroll being the only place that scroll offsets
are
> updated? Is that actually true? If not, I don't think overriding computeScroll
> works in general?

Scrolloffsets will be synced onDraw.
https://code.google.com/p/chromium/codesearch#chromium/src/android_webview/ja...
So if an app overrides "computeScroll()" to an empty function, you won't be able
to fling the webview. But you will still be able to scroll the webview if the
finger keeps pressed on screen.
Inbox overrides computeScroll() to an empty function so that it can use its own
fling logic.
I don't think we can make computeScroll no-op because apps need it to customize
fling.

[boliu, 2015-05-09 01:25:59]

On 2015/05/09 00:19:49, hush wrote:
> Scrolloffsets will be synced onDraw.
>
https://code.google.com/p/chromium/codesearch#chromium/src/android_webview/ja...
> So if an app overrides "computeScroll()" to an empty function, you won't be
able
> to fling the webview. But you will still be able to scroll the webview if the
> finger keeps pressed on screen.

Logic doesn't follow there, but I get your point:

Touch scroll is driven by the compositor. Fling scroll is driven by Scroller in
computeScroll, which is then synced to native in draw.

> Inbox overrides computeScroll() to an empty function so that it can use its
own
> fling logic.
> I don't think we can make computeScroll no-op because apps need it to
customize
> fling.

Hmmm.... in theory, they can just intercept the input stream (which I assume
they do already), and do *everything* with scrollTos. But that's a lot harder I
guess. What they really need is a way to intercept gestures before they go into
the compositor I guess.

Anyway, I guess conclusion here is computeScroll can't be no-op in webview.
Sorry Jared, I was wrong.

[aelias_OOO_until_Jul13, 2015-05-09 03:32:05]

https://codereview.chromium.org/1063853005/diff/120001/android_webview/browse...
File android_webview/browser/browser_view_renderer.cc (right):

https://codereview.chromium.org/1063853005/diff/120001/android_webview/browse...
android_webview/browser/browser_view_renderer.cc:616: // TODO(jdduke): Skip
invalidate if animation already pending?
Resolve this TODO one way or the other?  Personally, I think there's probably no
harm in extra invalidates.

https://codereview.chromium.org/1063853005/diff/120001/android_webview/java/s...
File android_webview/java/src/org/chromium/android_webview/AwContents.java
(right):

https://codereview.chromium.org/1063853005/diff/120001/android_webview/java/s...
android_webview/java/src/org/chromium/android_webview/AwContents.java:2520: if
((velocityX != 0 && firstOverscrollX) || (velocityY != 0 && firstOverscrollY)) {
Could you explain why the first overscroll needs to be special?

https://codereview.chromium.org/1063853005/diff/120001/cc/input/layer_scroll_...
File cc/input/layer_scroll_offset_delegate.h (right):

https://codereview.chromium.org/1063853005/diff/120001/cc/input/layer_scroll_...
cc/input/layer_scroll_offset_delegate.h:49: // TODO(jdduke): Remove?
Seems we can't yet because of the other smooth scrolls, is that right?

https://codereview.chromium.org/1063853005/diff/120001/cc/trees/layer_tree_ho...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/1063853005/diff/120001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_impl.cc:2968: // TODO(jdduke): Skip when scrolling the
root layer with an offset delegate?
Maybe unnecessary optimization?

[hush (inactive), 2015-05-12 19:08:46]

https://codereview.chromium.org/1063853005/diff/120001/android_webview/browse...
File android_webview/browser/browser_view_renderer.cc (right):

https://codereview.chromium.org/1063853005/diff/120001/android_webview/browse...
android_webview/browser/browser_view_renderer.cc:616: // TODO(jdduke): Skip
invalidate if animation already pending?
On 2015/05/09 03:32:04, aelias wrote:
> Resolve this TODO one way or the other?  Personally, I think there's probably
no
> harm in extra invalidates.

I will just remove this TODO.

https://codereview.chromium.org/1063853005/diff/120001/android_webview/java/s...
File android_webview/java/src/org/chromium/android_webview/AwContents.java
(right):

https://codereview.chromium.org/1063853005/diff/120001/android_webview/java/s...
android_webview/java/src/org/chromium/android_webview/AwContents.java:2520: if
((velocityX != 0 && firstOverscrollX) || (velocityY != 0 && firstOverscrollY)) {
On 2015/05/09 03:32:04, aelias wrote:
> Could you explain why the first overscroll needs to be special?

I guess the original idea to absorb the glow if it is the first overscroll.
However, I just found WebView's overScroll glow is not working properly,
including version m39:
The overscroll glow does not *always* show when you fling the webview towards
the corners. You have to be very close to the corner when you start the fling
for the overscroll glow to appear. This is different from Chrome on Android,
where you always have the edge glow effects when the velocity is non-zero at the
corner.


I am thinking of fixing this by removing WebView's own "OverScrollGlow" if
possible and just Chrome's overscroll glow edge effect. Like what Marcin said in

https://code.google.com/p/chromium/issues/detail?id=378984#c2

https://codereview.chromium.org/1063853005/diff/120001/cc/input/layer_scroll_...
File cc/input/layer_scroll_offset_delegate.h (right):

https://codereview.chromium.org/1063853005/diff/120001/cc/input/layer_scroll_...
cc/input/layer_scroll_offset_delegate.h:49: // TODO(jdduke): Remove?
On 2015/05/09 03:32:05, aelias wrote:
> Seems we can't yet because of the other smooth scrolls, is that right?

Right. We will still need to keep it until we can unify smooth scrolls with
Chrome. I will upload a followup CL after this CL is submitted.
Jared and I had a very brief discussion about how to unify smooth scrolls a
while back.
The high level plan for that is we will introduce 2 new gesture events on
ContentViewCore(maybe call them SmoothScrollStart/Cancel), do the plumbing up
until web_gesture_curve_impl.cc to expose Scroller::startScroll.

[jdduke (slow), 2015-05-12 19:34:11]

https://codereview.chromium.org/1063853005/diff/120001/android_webview/java/s...
File android_webview/java/src/org/chromium/android_webview/AwContents.java
(right):

https://codereview.chromium.org/1063853005/diff/120001/android_webview/java/s...
android_webview/java/src/org/chromium/android_webview/AwContents.java:2520: if
((velocityX != 0 && firstOverscrollX) || (velocityY != 0 && firstOverscrollY)) {
On 2015/05/12 19:08:45, hush wrote:
> I am thinking of fixing this by removing WebView's own "OverScrollGlow" if
> possible and just Chrome's overscroll glow edge effect. Like what Marcin said
in
> 
> https://code.google.com/p/chromium/issues/detail?id=378984#c2

I would love for this to be the case, but Chrome's effect uses the browser
compositor so it won't be as simple as a drop-in replacement for what WebView
has.

In theory we could abstract the rendering so that it uses the same logic as
Chrome, but does its rendering differently (similar to selection handles,
CompositedTouchHandleDrawable vs PopupTouchHandleDrawable), but that's probably
best carved out as a separate project (if at all).

[jdduke (slow), 2015-05-12 20:46:07]

https://codereview.chromium.org/1063853005/diff/120001/android_webview/java/s...
File android_webview/java/src/org/chromium/android_webview/AwContents.java
(right):

https://codereview.chromium.org/1063853005/diff/120001/android_webview/java/s...
android_webview/java/src/org/chromium/android_webview/AwContents.java:2519: //
TODO(jdduke): Skip this for components with non-zero velocity?
It's quite possible we don't even need these checks for
|firstOverscrollX|/|firstOverscrollY|. It looks like OverscrollGlow.java may
already have the necessary data to make the call on whether to process the glow,
so maybe try removing this guard and see if it works OK?

If so, we can probably get rid of the |firstOverscrollX|/|firstOverscrollY|
computation altogether. I think I may have introduced that b/c we have to do
something with Chrome's version of the overscroll glow, but again, looking at
OverscrollGlow.java I think it may already have the data it needs to know when
an overscroll actually starts.

[hush (inactive), 2015-05-13 01:34:53]

Patchset #8 (id:140001) has been deleted

[hush (inactive), 2015-05-13 01:39:29]

Patchset #9 (id:180001) has been deleted

[hush (inactive), 2015-05-13 23:26:20]

https://codereview.chromium.org/1063853005/diff/120001/android_webview/java/s...
File android_webview/java/src/org/chromium/android_webview/AwContents.java
(right):

https://codereview.chromium.org/1063853005/diff/120001/android_webview/java/s...
android_webview/java/src/org/chromium/android_webview/AwContents.java:2519: //
TODO(jdduke): Skip this for components with non-zero velocity?
On 2015/05/12 20:46:06, jdduke wrote:
> It's quite possible we don't even need these checks for
> |firstOverscrollX|/|firstOverscrollY|. It looks like OverscrollGlow.java may
> already have the necessary data to make the call on whether to process the
glow,
> so maybe try removing this guard and see if it works OK?
> 
> If so, we can probably get rid of the |firstOverscrollX|/|firstOverscrollY|
> computation altogether. I think I may have introduced that b/c we have to do
> something with Chrome's version of the overscroll glow, but again, looking at
> OverscrollGlow.java I think it may already have the data it needs to know when
> an overscroll actually starts.

OverScrollGlow class has all the information it needs. I will remove
firstOverScrollX/Y.

https://codereview.chromium.org/1063853005/diff/120001/android_webview/java/s...
android_webview/java/src/org/chromium/android_webview/AwContents.java:2520: if
((velocityX != 0 && firstOverscrollX) || (velocityY != 0 && firstOverscrollY)) {
On 2015/05/12 19:34:11, jdduke wrote:
> On 2015/05/12 19:08:45, hush wrote:
> > I am thinking of fixing this by removing WebView's own "OverScrollGlow" if
> > possible and just Chrome's overscroll glow edge effect. Like what Marcin
said
> in
> > 
> > https://code.google.com/p/chromium/issues/detail?id=378984#c2
> 
> I would love for this to be the case, but Chrome's effect uses the browser
> compositor so it won't be as simple as a drop-in replacement for what WebView
> has.
> 
> In theory we could abstract the rendering so that it uses the same logic as
> Chrome, but does its rendering differently (similar to selection handles,
> CompositedTouchHandleDrawable vs PopupTouchHandleDrawable), but that's
probably
> best carved out as a separate project (if at all).

Okay.. I will just continue using WebView's custom overscroll glow class.

https://codereview.chromium.org/1063853005/diff/120001/android_webview/java/s...
android_webview/java/src/org/chromium/android_webview/AwContents.java:2524:
final int y = oldX + deltaY;
It should have been oldY + deltaY.

https://codereview.chromium.org/1063853005/diff/120001/cc/trees/layer_tree_ho...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/1063853005/diff/120001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_impl.cc:2968: // TODO(jdduke): Skip when scrolling the
root layer with an offset delegate?
On 2015/05/09 03:32:05, aelias wrote:
> Maybe unnecessary optimization?

Actually I think we can't skip even when scrolling the root layer with an offset
delegate. In line 462, we will register the root_layer_animation_callback_ as
LTHI::AnimateInput when the layer is the root layer.

So I will just remove this TODO.

[hush (inactive), 2015-05-13 23:27:11]

Hello Alex and Jared,
PTAL patch set 11

[hush (inactive), 2015-05-15 20:52:52]

Hi Alex, any comments on this CL before you go on a vacation?

[aelias_OOO_until_Jul13, 2015-05-16 10:09:46]

lgtm, please wait for jdduke's review as well though.

[jdduke (slow), 2015-05-18 21:18:34]

content/ basically lgtm, just a couple questions and a small test request.

My final question is with respect to the GestureFlingStart hit test. In theory,
the fling might lock to a non-root layer, in which case the behavior would be
different than what we have now. Does that matter in practice?

https://codereview.chromium.org/1063853005/diff/300001/android_webview/browse...
File android_webview/browser/browser_view_renderer.h (right):

https://codereview.chromium.org/1063853005/diff/300001/android_webview/browse...
android_webview/browser/browser_view_renderer.h:111: void
SetNeedsAnimateFling(const AnimationCallback& fling_animation) override;
Let's go ahead and rename this to |SetNeedsAnimateScroll|, |scroll_animation|
and also below |FlushPendingScrollAnimation|.

https://codereview.chromium.org/1063853005/diff/300001/android_webview/java/s...
File android_webview/java/src/org/chromium/android_webview/AwContents.java
(right):

https://codereview.chromium.org/1063853005/diff/300001/android_webview/java/s...
android_webview/java/src/org/chromium/android_webview/AwContents.java:2935: if
(mScrollOffsetManager.willComputeScroll()) {
Are there cases where a fling would be active and also a smooth scroll? I guess
there's no right thing to do there other than perhaps cancelling the fling, is
it worth allowing both codepaths to animate? Don't have a strong opinion here.

https://codereview.chromium.org/1063853005/diff/300001/content/public/browser...
File content/public/browser/android/synchronous_compositor_client.h (right):

https://codereview.chromium.org/1063853005/diff/300001/content/public/browser...
content/public/browser/android/synchronous_compositor_client.h:41: virtual void
SetNeedsAnimateFling(const AnimationCallback& animation) = 0;
If we ever move smooth scrolling to the compositor, would it be helpful to have
a more generic name for this? |SetNeedsAnimateInput| or |SetNeedsAnimateScroll|?

https://codereview.chromium.org/1063853005/diff/300001/content/renderer/input...
File content/renderer/input/input_handler_proxy.cc (right):

https://codereview.chromium.org/1063853005/diff/300001/content/renderer/input...
content/renderer/input/input_handler_proxy.cc:421: if
(gesture_scroll_on_impl_thread_)
This probably deserves a small test.

[hush (inactive), 2015-05-19 18:07:11]

Patchset #15 (id:320001) has been deleted

[hush (inactive), 2015-05-19 18:14:59]

https://codereview.chromium.org/1063853005/diff/300001/android_webview/browse...
File android_webview/browser/browser_view_renderer.h (right):

https://codereview.chromium.org/1063853005/diff/300001/android_webview/browse...
android_webview/browser/browser_view_renderer.h:111: void
SetNeedsAnimateFling(const AnimationCallback& fling_animation) override;
On 2015/05/18 21:18:34, jdduke wrote:
> Let's go ahead and rename this to |SetNeedsAnimateScroll|, |scroll_animation|
> and also below |FlushPendingScrollAnimation|.

Done.

https://codereview.chromium.org/1063853005/diff/300001/android_webview/java/s...
File android_webview/java/src/org/chromium/android_webview/AwContents.java
(right):

https://codereview.chromium.org/1063853005/diff/300001/android_webview/java/s...
android_webview/java/src/org/chromium/android_webview/AwContents.java:2935: if
(mScrollOffsetManager.willComputeScroll()) {
On 2015/05/18 21:18:34, jdduke wrote:
> Are there cases where a fling would be active and also a smooth scroll? I
guess
> there's no right thing to do there other than perhaps cancelling the fling, is
> it worth allowing both codepaths to animate? Don't have a strong opinion here.

That's a good point. I think whichever happens later should stop the other.

When a smooth scroll happens after a fling has started, the fling will be
ignored, because the fling animated is ticked by "nativeOnComputeScroll", which
won't be called until the smooth scroll animation is done.

On the other hand, when a fling happens after smoothscroll, the smooth scroll
(currently driven by android frameworks OverScroller) will stop. I don't know
android frameworks code to pin point the logic but I tested by flinging right
after a very long smoothscrolling.

https://codereview.chromium.org/1063853005/diff/300001/content/public/browser...
File content/public/browser/android/synchronous_compositor_client.h (right):

https://codereview.chromium.org/1063853005/diff/300001/content/public/browser...
content/public/browser/android/synchronous_compositor_client.h:41: virtual void
SetNeedsAnimateFling(const AnimationCallback& animation) = 0;
On 2015/05/18 21:18:34, jdduke wrote:
> If we ever move smooth scrolling to the compositor, would it be helpful to
have
> a more generic name for this? |SetNeedsAnimateInput| or
|SetNeedsAnimateScroll|?

Yes. I will rename this function to SetNeedsAnimateScroll.

https://codereview.chromium.org/1063853005/diff/300001/content/renderer/input...
File content/renderer/input/input_handler_proxy.cc (right):

https://codereview.chromium.org/1063853005/diff/300001/content/renderer/input...
content/renderer/input/input_handler_proxy.cc:421: if
(gesture_scroll_on_impl_thread_)
On 2015/05/18 21:18:34, jdduke wrote:
> This probably deserves a small test.

Done.

[hush (inactive), 2015-05-19 18:18:37]

>> My final question is with respect to the GestureFlingStart hit test. In
theory,
the fling might lock to a non-root layer, in which case the behavior would be
different than what we have now. Does that matter in practice?

How is the expected behavior different from what we have now? I will find out a
test page with scrollable child layer.

[jdduke (slow), 2015-05-19 18:45:00]

On 2015/05/19 18:18:37, hush wrote:
> >> My final question is with respect to the GestureFlingStart hit test. In
> theory,
> the fling might lock to a non-root layer, in which case the behavior would be
> different than what we have now. Does that matter in practice?
> 
> How is the expected behavior different from what we have now? I will find out
a
> test page with scrollable child layer.

I'm thinking with an explicit fling from the embedder, doesn't it always target
the root layer (using the Scroller)? Now it could target a sublayer? Here's a
page you might test: http://jsbin.com/qunoju/1/quiet.

[hush (inactive), 2015-05-27 23:11:38]

https://codereview.chromium.org/1063853005/diff/340001/android_webview/java/s...
File android_webview/java/src/org/chromium/android_webview/AwContents.java
(right):

https://codereview.chromium.org/1063853005/diff/340001/android_webview/java/s...
android_webview/java/src/org/chromium/android_webview/AwContents.java:1882:
mContentViewCore.fling(SystemClock.uptimeMillis(), 0, 0, -velocityX,
-velocityY);
Regarding the possibility of scrolling a scrollable sublayer instead of
scrolling just the root layer:

How about replacing 0, 0 with a magic number like -100, -100 so that the hit
test will always fall onto the root layer?
I tried -100, -100 with the test page you provided and flingScroll() correctly
scrolls the root layer.

[jdduke (slow), 2015-05-28 15:18:09]

+bokan for any thoughts on naming for a bit on GestureScrollBegin that would
make it target the root scrolling layer.

https://codereview.chromium.org/1063853005/diff/340001/android_webview/java/s...
File android_webview/java/src/org/chromium/android_webview/AwContents.java
(right):

https://codereview.chromium.org/1063853005/diff/340001/android_webview/java/s...
android_webview/java/src/org/chromium/android_webview/AwContents.java:1882:
mContentViewCore.fling(SystemClock.uptimeMillis(), 0, 0, -velocityX,
-velocityY);
On 2015/05/27 23:11:38, hush wrote:
> Regarding the possibility of scrolling a scrollable sublayer instead of
> scrolling just the root layer:
> 
> How about replacing 0, 0 with a magic number like -100, -100 so that the hit
> test will always fall onto the root layer?
> I tried -100, -100 with the test page you provided and flingScroll() correctly
> scrolls the root layer.

That's a thought, though it might just be that it happens to work with that test
page. If you scrolled the page down/right by 100, in theory the hit test might
go back to the sublayer.

Another thought would be to make it explicit, so we add a bit to the
GestureScrollBegin (here
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...)
which says "targetRoot" or "targetViewport" or something like that. Then we just
set that bit for flings, and perhaps rename CVC.fling() to CVC.flingRoot()?

[hush (inactive), 2015-05-28 23:49:51]

https://codereview.chromium.org/1063853005/diff/340001/android_webview/java/s...
File android_webview/java/src/org/chromium/android_webview/AwContents.java
(right):

https://codereview.chromium.org/1063853005/diff/340001/android_webview/java/s...
android_webview/java/src/org/chromium/android_webview/AwContents.java:1882:
mContentViewCore.fling(SystemClock.uptimeMillis(), 0, 0, -velocityX,
-velocityY);
On 2015/05/28 15:18:08, jdduke wrote:
> On 2015/05/27 23:11:38, hush wrote:
> > Regarding the possibility of scrolling a scrollable sublayer instead of
> > scrolling just the root layer:
> > 
> > How about replacing 0, 0 with a magic number like -100, -100 so that the hit
> > test will always fall onto the root layer?
> > I tried -100, -100 with the test page you provided and flingScroll()
correctly
> > scrolls the root layer.
> 
> That's a thought, though it might just be that it happens to work with that
test
> page. If you scrolled the page down/right by 100, in theory the hit test might
> go back to the sublayer.
> 
> Another thought would be to make it explicit, so we add a bit to the
> GestureScrollBegin (here
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...)
> which says "targetRoot" or "targetViewport" or something like that. Then we
just
> set that bit for flings, and perhaps rename CVC.fling() to CVC.flingRoot()?
> 

Yeah... using -100, -100 was too brittle and too much of a hack. I agree with
adding a bit to WebInputEvent, scrollBegin

[hush (inactive), 2015-05-29 00:12:18]

On 2015/05/28 23:49:51, hush wrote:
>
https://codereview.chromium.org/1063853005/diff/340001/android_webview/java/s...
> File android_webview/java/src/org/chromium/android_webview/AwContents.java
> (right):
> 
>
https://codereview.chromium.org/1063853005/diff/340001/android_webview/java/s...
> android_webview/java/src/org/chromium/android_webview/AwContents.java:1882:
> mContentViewCore.fling(SystemClock.uptimeMillis(), 0, 0, -velocityX,
> -velocityY);
> On 2015/05/28 15:18:08, jdduke wrote:
> > On 2015/05/27 23:11:38, hush wrote:
> > > Regarding the possibility of scrolling a scrollable sublayer instead of
> > > scrolling just the root layer:
> > > 
> > > How about replacing 0, 0 with a magic number like -100, -100 so that the
hit
> > > test will always fall onto the root layer?
> > > I tried -100, -100 with the test page you provided and flingScroll()
> correctly
> > > scrolls the root layer.
> > 
> > That's a thought, though it might just be that it happens to work with that
> test
> > page. If you scrolled the page down/right by 100, in theory the hit test
might
> > go back to the sublayer.
> > 
> > Another thought would be to make it explicit, so we add a bit to the
> > GestureScrollBegin (here
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...)
> > which says "targetRoot" or "targetViewport" or something like that. Then we
> just
> > set that bit for flings, and perhaps rename CVC.fling() to CVC.flingRoot()?
> > 
> 
> Yeah... using -100, -100 was too brittle and too much of a hack. I agree with
> adding a bit to WebInputEvent, scrollBegin

Since adding the extra bit to WebInputEvent takes another CL in blink, how about
submitting this CL now?

[jdduke (slow), 2015-05-29 00:52:15]

On 2015/05/29 00:12:18, hush wrote:
> On 2015/05/28 23:49:51, hush wrote:
> >
>
https://codereview.chromium.org/1063853005/diff/340001/android_webview/java/s...
> > File android_webview/java/src/org/chromium/android_webview/AwContents.java
> > (right):
> > 
> >
>
https://codereview.chromium.org/1063853005/diff/340001/android_webview/java/s...
> > android_webview/java/src/org/chromium/android_webview/AwContents.java:1882:
> > mContentViewCore.fling(SystemClock.uptimeMillis(), 0, 0, -velocityX,
> > -velocityY);
> > On 2015/05/28 15:18:08, jdduke wrote:
> > > On 2015/05/27 23:11:38, hush wrote:
> > > > Regarding the possibility of scrolling a scrollable sublayer instead of
> > > > scrolling just the root layer:
> > > > 
> > > > How about replacing 0, 0 with a magic number like -100, -100 so that the
> hit
> > > > test will always fall onto the root layer?
> > > > I tried -100, -100 with the test page you provided and flingScroll()
> > correctly
> > > > scrolls the root layer.
> > > 
> > > That's a thought, though it might just be that it happens to work with
that
> > test
> > > page. If you scrolled the page down/right by 100, in theory the hit test
> might
> > > go back to the sublayer.
> > > 
> > > Another thought would be to make it explicit, so we add a bit to the
> > > GestureScrollBegin (here
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...)
> > > which says "targetRoot" or "targetViewport" or something like that. Then
we
> > just
> > > set that bit for flings, and perhaps rename CVC.fling() to
CVC.flingRoot()?
> > > 
> > 
> > Yeah... using -100, -100 was too brittle and too much of a hack. I agree
with
> > adding a bit to WebInputEvent, scrollBegin
> 
> Since adding the extra bit to WebInputEvent takes another CL in blink, how
about
> submitting this CL now?

Up to you, if you're OK with a temporary deviation in behavior, that's probably
fine as long as we make sure it's fixed before M45 branch. If you decided to
land as-is, please add a TODO where WebView calls CVC.fling() to make the change
(filing a new bug or linking against this bug). You'll need an android_webview
owner to review that section.

[hush (inactive), 2015-05-29 17:33:01]

hush@chromium.org changed reviewers:
+ boliu@chromium.org

[hush (inactive), 2015-05-29 17:33:02]

Hi Bo!
PTAL android_webview

[bokan, 2015-05-29 19:32:47]

bokan@chromium.org changed reviewers:
+ bokan@chromium.org

[bokan, 2015-05-29 19:32:49]

https://codereview.chromium.org/1063853005/diff/340001/android_webview/java/s...
File android_webview/java/src/org/chromium/android_webview/AwContents.java
(right):

https://codereview.chromium.org/1063853005/diff/340001/android_webview/java/s...
android_webview/java/src/org/chromium/android_webview/AwContents.java:1882:
mContentViewCore.fling(SystemClock.uptimeMillis(), 0, 0, -velocityX,
-velocityY);
On 2015/05/28 23:49:51, hush wrote:
> On 2015/05/28 15:18:08, jdduke wrote:
> > On 2015/05/27 23:11:38, hush wrote:
> > > Regarding the possibility of scrolling a scrollable sublayer instead of
> > > scrolling just the root layer:
> > > 
> > > How about replacing 0, 0 with a magic number like -100, -100 so that the
hit
> > > test will always fall onto the root layer?
> > > I tried -100, -100 with the test page you provided and flingScroll()
> correctly
> > > scrolls the root layer.
> > 
> > That's a thought, though it might just be that it happens to work with that
> test
> > page. If you scrolled the page down/right by 100, in theory the hit test
might
> > go back to the sublayer.
> > 
> > Another thought would be to make it explicit, so we add a bit to the
> > GestureScrollBegin (here
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...)
> > which says "targetRoot" or "targetViewport" or something like that. Then we
> just
> > set that bit for flings, and perhaps rename CVC.fling() to CVC.flingRoot()?
> > 
> 
> Yeah... using -100, -100 was too brittle and too much of a hack. I agree with
> adding a bit to WebInputEvent, scrollBegin

Sorry for late reply, "targetViewport" sgtm. "targetRoot" is ok but maybe a bit
misleading since the viewport may not actually be the root layer. (There's two
viewport scroll layers so they can't both be root, also, above even the top one
is the elastic overscroll layer...)

[boliu, 2015-05-29 19:42:47]

https://codereview.chromium.org/1063853005/diff/380001/android_webview/browse...
File android_webview/browser/browser_view_renderer.cc (right):

https://codereview.chromium.org/1063853005/diff/380001/android_webview/browse...
android_webview/browser/browser_view_renderer.cc:617: client_->PostInvalidate();
Conceptually this feels wrong. What does "SetNeedAnimate" have to do with
invalidates? You are waiting for OnComputeScroll, and PostInvalidate is for
OnDraws.

Have we considered moving driving animations to the animate step? Is that how
chrome works, etc? How does this work before this CL?

And more practically, does this need to be integrated into
PostInvalidateWithFallback? ie pending_fling_animation_.is_null() will be one
more state that controls whether fallback ticks should be scheduled or
cancelled.

https://codereview.chromium.org/1063853005/diff/380001/android_webview/browse...
android_webview/browser/browser_view_renderer.cc:632: 
nit: blank lines

https://codereview.chromium.org/1063853005/diff/380001/android_webview/browse...
android_webview/browser/browser_view_renderer.cc:713: void
BrowserViewRenderer::FlushPendingScrollAnimation(
There is only one call site. Just inline this?

https://codereview.chromium.org/1063853005/diff/380001/android_webview/browse...
File android_webview/browser/browser_view_renderer.h (right):

https://codereview.chromium.org/1063853005/diff/380001/android_webview/browse...
android_webview/browser/browser_view_renderer.h:190: // as a  single-shot
animation; the compositor will continually post an
nit: extra space

https://codereview.chromium.org/1063853005/diff/380001/android_webview/java/s...
File android_webview/java/src/org/chromium/android_webview/AwContents.java
(right):

https://codereview.chromium.org/1063853005/diff/380001/android_webview/java/s...
android_webview/java/src/org/chromium/android_webview/AwContents.java:2576:
@SuppressLint("NewApi")
Add a comment which api

https://codereview.chromium.org/1063853005/diff/380001/android_webview/java/s...
android_webview/java/src/org/chromium/android_webview/AwContents.java:3002: if
(mScrollOffsetManager.willComputeScroll()) {
What does it mean when this is true, but compositor also wants updates via
BVR::OnComputeScroll?

https://codereview.chromium.org/1063853005/diff/380001/android_webview/java/s...
File
android_webview/java/src/org/chromium/android_webview/AwScrollOffsetManager.java
(right):

https://codereview.chromium.org/1063853005/diff/380001/android_webview/java/s...
android_webview/java/src/org/chromium/android_webview/AwScrollOffsetManager.java:29:
private boolean mIsAnimatingScroll = false;
what's the difference between mIsAnimatingScroll and mWasSmoothScrolling?

https://codereview.chromium.org/1063853005/diff/380001/android_webview/java/s...
android_webview/java/src/org/chromium/android_webview/AwScrollOffsetManager.java:268:
if (mScroller.isFinished()) mIsAnimatingScroll = false;
not quite the same as mIsAnimatingScroll = mScroller.isFinished(); but is that
what you are going for here?

https://codereview.chromium.org/1063853005/diff/380001/android_webview/native...
File android_webview/native/aw_contents.cc (right):

https://codereview.chromium.org/1063853005/diff/380001/android_webview/native...
android_webview/native/aw_contents.cc:947:
base::TimeDelta::FromMilliseconds(animation_time_millis));
Does this addition make sense?

Documentation on currentAnimationTimeMillis says it is *not* the "wall" clock.

https://codereview.chromium.org/1063853005/diff/380001/content/browser/androi...
File content/browser/android/in_process/synchronous_compositor_impl.cc (right):

https://codereview.chromium.org/1063853005/diff/380001/content/browser/androi...
content/browser/android/in_process/synchronous_compositor_impl.cc:340: if
(compositor_client_)
This is a call from cc, right? In that case, DCHECK(compositor_client_) should
be fine.

[hush (inactive), 2015-06-05 21:47:22]

https://codereview.chromium.org/1063853005/diff/380001/android_webview/browse...
File android_webview/browser/browser_view_renderer.cc (right):

https://codereview.chromium.org/1063853005/diff/380001/android_webview/browse...
android_webview/browser/browser_view_renderer.cc:617: client_->PostInvalidate();
> Conceptually this feels wrong. What does "SetNeedAnimate" have to do with
> invalidates? You are waiting for OnComputeScroll, and PostInvalidate is for
> OnDraws.
> Have we considered moving driving animations to the animate step? Is that how
> chrome works, etc? How does this work before this CL?

We've discussed this in person.
The fling animation is driven by "computeScroll" call which is an Android view
api. And we have to hook fling with it because there are applications (Inbox for
example) that rely on that fact to overide computeScroll to nothing to achieve
their own custom fling.
computeScroll is called within View#updateDisplayListIfDirty, which is during
the draw step, instead of animation step, from the choreorgrapher's point of
view.

I will update description about how this works before and after this CL. Sorry
for the short description there..

> And more practically, does this need to be integrated into
> PostInvalidateWithFallback? ie pending_fling_animation_.is_null() will be one
> more state that controls whether fallback ticks should be scheduled or
> cancelled.
I think you mean when pending_fling_animation_ is not null, there is no point in
rescheduling fallback ticks because there is definitely a PostInvalidate in
flight. I will do it.

https://codereview.chromium.org/1063853005/diff/380001/android_webview/browse...
android_webview/browser/browser_view_renderer.cc:632: 
On 2015/05/29 19:42:47, boliu wrote:
> nit: blank lines

Done.

https://codereview.chromium.org/1063853005/diff/380001/android_webview/browse...
android_webview/browser/browser_view_renderer.cc:713: void
BrowserViewRenderer::FlushPendingScrollAnimation(
On 2015/05/29 19:42:47, boliu wrote:
> There is only one call site. Just inline this?

Done.

https://codereview.chromium.org/1063853005/diff/380001/android_webview/java/s...
File android_webview/java/src/org/chromium/android_webview/AwContents.java
(right):

https://codereview.chromium.org/1063853005/diff/380001/android_webview/java/s...
android_webview/java/src/org/chromium/android_webview/AwContents.java:2576:
@SuppressLint("NewApi")
On 2015/05/29 19:42:47, boliu wrote:
> Add a comment which api

Done.

https://codereview.chromium.org/1063853005/diff/380001/android_webview/java/s...
android_webview/java/src/org/chromium/android_webview/AwContents.java:3002: if
(mScrollOffsetManager.willComputeScroll()) {
On 2015/05/29 19:42:47, boliu wrote:
> What does it mean when this is true, but compositor also wants updates via
> BVR::OnComputeScroll?

That's a good observation.
This means "pending_fling_animation_" in BVR will stay not null until
mScrollOffsetManager.willComputeScroll() returns false.
So LTHI::AnimateInput will not be called, which means InputHandlerProxy::Animate
won't be called.
From inspecting the code of these mentioned functions, I don't think there is
anything waiting on InputHandlerProxy::Animate to be called eventually. 

That is the case until the current smooth scrolling (caused by pageUp, pageDown)
finishes.
I've found a problem here:
Suppose there is a fling that lasts for 10 seconds and scrolls the webview by
1000 pixels intercepted by a pageDown that scrolls the webview by 100 pixels and
lasts for 1 second. Right after pageDown finishes and scrolls the webview to its
location,mScrollOffsetManager.willComputeScroll will return false, the fling
curve takes over to update the scroll offset of webview. This will probably make
the WebView's scroll position "teleport" to a different position as calculated
by the fling curve.

I think the correct behavior (and it is also the behavior before this CL) should
be the in-flight fling will be canceled instead of being temporarily ignored, if
there comes another smooth scroll. Jared, sorry about not getting this right
when you first commented about it..

https://codereview.chromium.org/1063853005/diff/380001/android_webview/java/s...
File
android_webview/java/src/org/chromium/android_webview/AwScrollOffsetManager.java
(right):

https://codereview.chromium.org/1063853005/diff/380001/android_webview/java/s...
android_webview/java/src/org/chromium/android_webview/AwScrollOffsetManager.java:29:
private boolean mIsAnimatingScroll = false;
On 2015/05/29 19:42:47, boliu wrote:
> what's the difference between mIsAnimatingScroll and mWasSmoothScrolling?
I was silly there.
mIsAnimatingScroll is equal to mScroller.computeScrollOffset(). I will just
remove this field.

https://codereview.chromium.org/1063853005/diff/380001/android_webview/java/s...
android_webview/java/src/org/chromium/android_webview/AwScrollOffsetManager.java:71:
private boolean mWasSmoothScrolling;
This field is not needed any more. It is used to guard early exit
computeScrollAndAbsorbGlow, which should always be called when
isSmoothScrolling()

https://codereview.chromium.org/1063853005/diff/380001/android_webview/java/s...
android_webview/java/src/org/chromium/android_webview/AwScrollOffsetManager.java:268:
if (mScroller.isFinished()) mIsAnimatingScroll = false;
On 2015/05/29 19:42:47, boliu wrote:
> not quite the same as mIsAnimatingScroll = mScroller.isFinished(); but is that
> what you are going for here?

Sorry. Same as I said above. This function should just be
"mScroller.computeScrollOffset()" And it is the same as isSmoothScrollingActive.

https://codereview.chromium.org/1063853005/diff/380001/android_webview/native...
File android_webview/native/aw_contents.cc (right):

https://codereview.chromium.org/1063853005/diff/380001/android_webview/native...
android_webview/native/aw_contents.cc:947:
base::TimeDelta::FromMilliseconds(animation_time_millis));
On 2015/05/29 19:42:47, boliu wrote:
> Does this addition make sense?
> 
> Documentation on currentAnimationTimeMillis says it is *not* the "wall" clock.

I am not exactly sure here. We have confusing usage of time related functions,
some of which are wrong in my opinion.

For example, I looked at some example in ContentViewCore.java where we pass
SystemClock.uptimeMillis() to construct a WebInputEvent. According to
documentation here:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
You are supposed to pass the time since epoch, so SystemClock.currentTimeMillis
is the right one.
I filed crbug.com/497323 for it.

For the comment about currentAnimationTimeMillis, I think it is right by looking
at the code. What's important for the animation tick is the difference between
each tick, but not the absolute value of each timer tick. We're consistently
passing currentAnimationTimeMillis to InputHandlerProxy to tick the animation.
So we're fine.
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/i...

https://codereview.chromium.org/1063853005/diff/380001/content/browser/androi...
File content/browser/android/in_process/synchronous_compositor_impl.cc (right):

https://codereview.chromium.org/1063853005/diff/380001/content/browser/androi...
content/browser/android/in_process/synchronous_compositor_impl.cc:340: if
(compositor_client_)
On 2015/05/29 19:42:47, boliu wrote:
> This is a call from cc, right? In that case, DCHECK(compositor_client_) should
> be fine.

Yes. This is a new function added to LayerScrollOffsetDelegate and it is only
called from LTHI. Done.

[boliu, 2015-06-08 16:54:52]

https://codereview.chromium.org/1063853005/diff/420001/android_webview/browse...
File android_webview/browser/browser_view_renderer.cc (right):

https://codereview.chromium.org/1063853005/diff/420001/android_webview/browse...
android_webview/browser/browser_view_renderer.cc:421:
"BrowserViewRenderer::FlushPendingScrollAnimation");
Fix trace event name

https://codereview.chromium.org/1063853005/diff/420001/android_webview/browse...
android_webview/browser/browser_view_renderer.cc:623:
PostInvalidateWithFallback();
Actually I'm eating my words. You said it's ok if compositor is not ticked. Also
fallback tick doesn't call computeScroll either. In that case, just call
client->PostInvalidate for now, and add a note why fallback tick is not needed.

We are close to removing the fallback tick altogether soon, so don't want to add
any more dependencies on it. Sounds like it's not necessary here anyway.

https://codereview.chromium.org/1063853005/diff/420001/android_webview/browse...
android_webview/browser/browser_view_renderer.cc:665:
!pending_fling_animation_.is_null())
don't forget to remove this

(Seems to be backwards anyway. The idea is cc *wants* fallback ticks if there is
a pending animation, in case the real draw doesn't come back.)

https://codereview.chromium.org/1063853005/diff/420001/android_webview/browse...
File android_webview/browser/browser_view_renderer.h (right):

https://codereview.chromium.org/1063853005/diff/420001/android_webview/browse...
android_webview/browser/browser_view_renderer.h:186: // as a  single-shot
animation; the compositor will continually post an
carried over from before, there's an extra space before 'single-shot'

https://codereview.chromium.org/1063853005/diff/420001/android_webview/java/s...
File
android_webview/java/src/org/chromium/android_webview/AwScrollOffsetManager.java
(right):

https://codereview.chromium.org/1063853005/diff/420001/android_webview/java/s...
android_webview/java/src/org/chromium/android_webview/AwScrollOffsetManager.java:261:
public void computeScrollAndAbsorbGlow(OverScrollGlow overScrollGlow) {
safe to assert isSmoothScrollingActive? It's not a clear no-op though

https://codereview.chromium.org/1063853005/diff/420001/content/browser/androi...
File content/browser/android/in_process/synchronous_compositor_impl.cc (right):

https://codereview.chromium.org/1063853005/diff/420001/content/browser/androi...
content/browser/android/in_process/synchronous_compositor_impl.cc:357:
DCHECK(compositor_client_);
Should check for registered_with_client_ after rebase

[hush (inactive), 2015-06-09 19:11:34]

Try succeeded. PTAL!

https://codereview.chromium.org/1063853005/diff/420001/android_webview/browse...
File android_webview/browser/browser_view_renderer.cc (right):

https://codereview.chromium.org/1063853005/diff/420001/android_webview/browse...
android_webview/browser/browser_view_renderer.cc:421:
"BrowserViewRenderer::FlushPendingScrollAnimation");
On 2015/06/08 16:54:52, boliu wrote:
> Fix trace event name

Done.

https://codereview.chromium.org/1063853005/diff/420001/android_webview/browse...
android_webview/browser/browser_view_renderer.cc:623:
PostInvalidateWithFallback();
On 2015/06/08 16:54:52, boliu wrote:
> Actually I'm eating my words. You said it's ok if compositor is not ticked.
Also
> fallback tick doesn't call computeScroll either. In that case, just call
> client->PostInvalidate for now, and add a note why fallback tick is not
needed.
> 
> We are close to removing the fallback tick altogether soon, so don't want to
add
> any more dependencies on it. Sounds like it's not necessary here anyway.

Okay. I will just postInvalidate and add a comment there.

https://codereview.chromium.org/1063853005/diff/420001/android_webview/browse...
android_webview/browser/browser_view_renderer.cc:665:
!pending_fling_animation_.is_null())
On 2015/06/08 16:54:52, boliu wrote:
> don't forget to remove this
> 
> (Seems to be backwards anyway. The idea is cc *wants* fallback ticks if there
is
> a pending animation, in case the real draw doesn't come back.)

okay,.. removed this. And yes. The logic here is backward :/

https://codereview.chromium.org/1063853005/diff/420001/android_webview/browse...
File android_webview/browser/browser_view_renderer.h (right):

https://codereview.chromium.org/1063853005/diff/420001/android_webview/browse...
android_webview/browser/browser_view_renderer.h:186: // as a  single-shot
animation; the compositor will continually post an
On 2015/06/08 16:54:52, boliu wrote:
> carried over from before, there's an extra space before 'single-shot'

Done.

https://codereview.chromium.org/1063853005/diff/420001/android_webview/java/s...
File
android_webview/java/src/org/chromium/android_webview/AwScrollOffsetManager.java
(right):

https://codereview.chromium.org/1063853005/diff/420001/android_webview/java/s...
android_webview/java/src/org/chromium/android_webview/AwScrollOffsetManager.java:261:
public void computeScrollAndAbsorbGlow(OverScrollGlow overScrollGlow) {
On 2015/06/08 16:54:52, boliu wrote:
> safe to assert isSmoothScrollingActive? It's not a clear no-op though

Yes, it is safe to assert so, because this function is only called inside the if
statement.
By the way, I decide to change isSmoothScrollingActive to returning
!mScroller.isFinished() which is lighter weight and only call
mScroller.computeScrollOffset() here.
The source code of OverScroller.java shows that "isFinished" and
"computeScrollOffset" always return the opposite values while the latter does
some computations and changes the "finished" state.

https://codereview.chromium.org/1063853005/diff/420001/content/browser/androi...
File content/browser/android/in_process/synchronous_compositor_impl.cc (right):

https://codereview.chromium.org/1063853005/diff/420001/content/browser/androi...
content/browser/android/in_process/synchronous_compositor_impl.cc:357:
DCHECK(compositor_client_);
On 2015/06/08 16:54:52, boliu wrote:
> Should check for registered_with_client_ after rebase

Done.

[boliu, 2015-06-09 19:15:43]

lgtm

[hush (inactive), 2015-06-09 20:47:27]

hush@chromium.org changed reviewers:
+ yfriedman@chromium.org

[hush (inactive), 2015-06-09 20:47:29]

Hi Yaron,
PTAL chrome/android/ and content/public

[hush (inactive), 2015-06-09 20:49:01]

On 2015/06/09 20:47:29, hush wrote:
> Hi Yaron,
> PTAL chrome/android/ and content/public

These files to be specific:
    chrome/android/java_staging/proguard.flags
    chrome/android/javatests/src/org/chromium/chrome/browser/TabsTest.java
   
content/public/android/java/src/org/chromium/content_public/browser/GestureStateListener.java
    content/public/browser/android/synchronous_compositor_client.h

[Yaron, 2015-06-09 21:24:29]

lgtm

[hush (inactive), 2015-06-09 21:37:22]

The CQ bit was checked by hush@chromium.org

[hush (inactive), 2015-06-09 21:37:23]

The patchset sent to the CQ was uploaded after l-g-t-m from aelias@chromium.org,
jdduke@chromium.org
Link to the patchset: https://codereview.chromium.org/1063853005/#ps480001
(title: "add proguard")

[commit-bot: I haz the power, 2015-06-09 21:39:39]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1063853005/480001

[commit-bot: I haz the power, 2015-06-10 00:49:06]

Committed patchset #22 (id:480001)

[commit-bot: I haz the power, 2015-06-10 00:51:00]

Patchset 22 (id:??) landed as
https://crrev.com/b0ee8dc431d6d871f4001c9eb924d33a33d9e874
Cr-Commit-Position: refs/heads/master@{#333626}

[Torne, 2015-06-11 10:32:04]

android.util.FloatMath was deprecated in L, we shouldn't be introducing calls to
it. I'll write a CL to replace this with Math.