Plumb smooth scrolling in Chromium compositor

Plumb smooth scrolling in Chromium compositor

Android WebView uses android.widget.OverScroller#startScroll
implement pageUp/pageDown. Since blink  internally knows how
to do a smooth scroll via page scale animation, WebView just
need to send an IPC to blink with the target position and
duration.

The blink change to support smooth scroll:
https://codereview.chromium.org/1251473003

BUG=378984
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/18c284495573fe54a6cbaad16beeb24029add9c4
Cr-Commit-Position: refs/heads/master@{#347770}

[hush (inactive), 2015-07-23 02:02:09]

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

[hush (inactive), 2015-07-23 02:02:10]

PTAL!
Sorry for the gigantic CL... don't know how to split it properly.

[jdduke (slow), 2015-07-23 15:32:50]

Thanks for pushing forward on this.

Question before I dive into further review, is this only used for
pageup/pagedown? I ask because there's already some basic plumbing for that on
WheelEvents (scrollByPage), and there's also basic support for smooth scrolling
with wheel events. I suspect the animation curves are subtly different, and
currently we don't support scrollByPage on the compositor, so we still wouldn't
get the necessary smooth scroll support "for free".

Still, it is somewhat of a shame to have such different inertial scrolling
paths. I guess they already exist and have for some time, and this new change
isn't adding that much complexity.

A part of wonders if we could replace InputHandler::ScrollAnimated with a smooth
fling curve, where the non-Android implementation is taken from the current
LayerTreeHostImpl::ScrollAnimated code. That would certainly happen as a
follow-up, but might cut down on some of the divergence. skobes@: Any thoughts?

https://codereview.chromium.org/1251323002/diff/1/content/renderer/input/inpu...
File content/renderer/input/input_handler_proxy.cc (right):

https://codereview.chromium.org/1251323002/diff/1/content/renderer/input/inpu...
content/renderer/input/input_handler_proxy.cc:701:
ShouldBoostFling(current_fling_velocity_, gesture_event);
We should make sure smooth scroll flings don't boost.

https://codereview.chromium.org/1251323002/diff/1/ui/events/gestures/blink/we...
File ui/events/gestures/blink/web_gesture_curve_impl.cc (right):

https://codereview.chromium.org/1251323002/diff/1/ui/events/gestures/blink/we...
ui/events/gestures/blink/web_gesture_curve_impl.cc:58: float vx = dx /
duration.InSecondsF();
I'd almost rather we NOTREACHED() here and return nullptr so other platforms
don't inadvertently think this will do smooth scrolling. In theory Scroller's
smooth scrolling would work just fine for other platfoms, but for now I'd prefer
we just not support the path until we need to.

[hush (inactive), 2015-07-25 00:29:12]

okay... We've decided in https://codereview.chromium.org/1251473003/ to use page
scale animation.

ptal ps2

[hush (inactive), 2015-07-25 00:59:29]

On 2015/07/25 00:29:12, hush wrote:
> okay... We've decided in https://codereview.chromium.org/1251473003/ to use
page
> scale animation.
> 
> ptal ps2

Apologies about the huge CL.
Most of the changes are mechanic.
The interesting parts are in ContentViewCore, WebContentsImpl,
web_contents_android and render_view_impl

[jdduke (slow), 2015-07-27 17:15:38]

I still think it's a shame we have to hit the main thread to scroll to a fixed
target, but we can probably optimize that in a follow-up patch if necessary.

Did you get a feel for how smooth the animation is despite our not changing the
page scale? The curve is likely quite different than the curve from the Scroller
object.

https://codereview.chromium.org/1251323002/diff/20001/content/browser/web_con...
File content/browser/web_contents/web_contents_android.cc (right):

https://codereview.chromium.org/1251323002/diff/20001/content/browser/web_con...
content/browser/web_contents/web_contents_android.cc:399: jint targetY,
Nit: target_x,target_y,duration_ms

https://codereview.chromium.org/1251323002/diff/20001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://codereview.chromium.org/1251323002/diff/20001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1248:
return mTouchScrollInProgress || mPotentiallyActiveFlingCount > 0;
Hmm... it's a little unfortunate to lose the accounting here for the new scroll
cases, as we won't be reusing the fling architecture :(.

In practice there will be a few minor side effects:
1) the selection handles will remain visible/janky while the smooth scroll
occurs
2) tap suppression won't work if the user taps the screen while the smooth
scroll is active.

We have the same problems with the double-tap zoom animation, so it's not the
end of the world.

[skobes, 2015-07-27 20:33:15]

On 2015/07/23 15:32:50, jdduke wrote:
> A part of wonders if we could replace InputHandler::ScrollAnimated with a
smooth
> fling curve, where the non-Android implementation is taken from the current
> LayerTreeHostImpl::ScrollAnimated code. That would certainly happen as a
> follow-up, but might cut down on some of the divergence. skobes@: Any
thoughts?

Sorry for slow response, I was on vacation last week.  I agree it would be good
to avoid adding new ways of doing animated scrolls.  I'm not really familiar
with the requirements of the android webview api... is there a reason it can't
use InputHandler::ScrollAnimated in its current form?

Also, if you're doing it on the main thread, wouldn't it make more sense to use
the programmatic smooth scroll codepath (ScrollableArea::setScrollPosition with
ScrollBehaviorSmooth) instead of page scale animation?  That seems more
intuitive, and maybe you can extend WebFrame::setScrollOffset with a
ScrollBehavior parameter instead of adding a new method to blink::WebView.

[aelias_OOO_until_Jul13, 2015-07-27 23:03:43]

On 2015/07/27 at 20:33:15, skobes wrote:
> On 2015/07/23 15:32:50, jdduke wrote:
> > A part of wonders if we could replace InputHandler::ScrollAnimated with a
smooth
> > fling curve, where the non-Android implementation is taken from the current
> > LayerTreeHostImpl::ScrollAnimated code. That would certainly happen as a
> > follow-up, but might cut down on some of the divergence. skobes@: Any
thoughts?
> 
> Sorry for slow response, I was on vacation last week.  I agree it would be
good to avoid adding new ways of doing animated scrolls.  I'm not really
familiar with the requirements of the android webview api... is there a reason
it can't use InputHandler::ScrollAnimated in its current form?
> 
> Also, if you're doing it on the main thread, wouldn't it make more sense to
use the programmatic smooth scroll codepath (ScrollableArea::setScrollPosition
with ScrollBehaviorSmooth) instead of page scale animation?  That seems more
intuitive, and maybe you can extend WebFrame::setScrollOffset with a
ScrollBehavior parameter instead of adding a new method to blink::WebView.

Android has always PageScaleAnimation for every kind of embedder-invoked
root-layer animated scroll and/or page scale change (for example find-in-page
uses it), so I recommended using that and in my mind at least it's intuitive. 
(Scroll animations are a special case of page scale animations, after all.)

I think it would be easier to do it this way, and reserve the
ScrollBehaviorSmooth stuff for the web-platform-facing spec, to avoid
web-spec-oriented behaviors causing surprise problems on WebView.  One issue I
see right now is that WebView wants a specific delay in milliseconds, but the
CSS smooth scroll stuff doesn't provide a way to provide that from Blink, and
makes the decision in CC in scroll_offset_animation_curve.cc.

[skobes, 2015-07-27 23:25:53]

On 2015/07/27 23:03:43, aelias wrote:
> On 2015/07/27 at 20:33:15, skobes wrote:
> > On 2015/07/23 15:32:50, jdduke wrote:
> > > A part of wonders if we could replace InputHandler::ScrollAnimated with a
> smooth
> > > fling curve, where the non-Android implementation is taken from the
current
> > > LayerTreeHostImpl::ScrollAnimated code. That would certainly happen as a
> > > follow-up, but might cut down on some of the divergence. skobes@: Any
> thoughts?
> > 
> > Sorry for slow response, I was on vacation last week.  I agree it would be
> good to avoid adding new ways of doing animated scrolls.  I'm not really
> familiar with the requirements of the android webview api... is there a reason
> it can't use InputHandler::ScrollAnimated in its current form?
> > 
> > Also, if you're doing it on the main thread, wouldn't it make more sense to
> use the programmatic smooth scroll codepath (ScrollableArea::setScrollPosition
> with ScrollBehaviorSmooth) instead of page scale animation?  That seems more
> intuitive, and maybe you can extend WebFrame::setScrollOffset with a
> ScrollBehavior parameter instead of adding a new method to blink::WebView.
> 
> Android has always PageScaleAnimation for every kind of embedder-invoked
> root-layer animated scroll and/or page scale change (for example find-in-page
> uses it), so I recommended using that and in my mind at least it's intuitive.

Ok, if we are already doing this for other embedder-triggered scrolls then I am
convinced.

It would be nice to consolidate the scroll animation code in a single component
that is generic enough to handle scrolls from all sources (input, webview, and
web platform).  But certainly that is out of scope for this patch.

[hush (inactive), 2015-07-27 23:36:23]

On 2015/07/27 17:15:38, jdduke wrote:
> I still think it's a shame we have to hit the main thread to scroll to a fixed
> target, but we can probably optimize that in a follow-up patch if necessary.
> 
> Did you get a feel for how smooth the animation is despite our not changing
the
> page scale? The curve is likely quite different than the curve from the
Scroller
> object.

The animation driven by page scale animation is less reactive than the one
driven by Scroller object. You can feel the page stays still for a brief moment
before it scrolls up/down.

[hush (inactive), 2015-07-27 23:41:57]

> 
> I think it would be easier to do it this way, and reserve the
> ScrollBehaviorSmooth stuff for the web-platform-facing spec, to avoid
> web-spec-oriented behaviors causing surprise problems on WebView.  One issue I
> see right now is that WebView wants a specific delay in milliseconds, but the
> CSS smooth scroll stuff doesn't provide a way to provide that from Blink, and
> makes the decision in CC in scroll_offset_animation_curve.cc.

I'm not too concerned with the specific delay here. We don't have a WebView API
contract of how much time pageUp/Down animation takes. WebView internally
calculates the duration of the animation based on the distance:
https://code.google.com/p/chromium/codesearch#chromium/src/android_webview/ja...

I actually think this code could be out of date because
STD_SCROLL_ANIMATION_SPEED_PIX_PER_SEC might not make sense, now that we've
switch to page scale animation, which provides a different animation curve from
the one in ui::Scroller::startScroll.

[hush (inactive), 2015-07-28 00:10:43]

https://codereview.chromium.org/1251323002/diff/20001/content/browser/web_con...
File content/browser/web_contents/web_contents_android.cc (right):

https://codereview.chromium.org/1251323002/diff/20001/content/browser/web_con...
content/browser/web_contents/web_contents_android.cc:399: jint targetY,
On 2015/07/27 17:15:38, jdduke wrote:
> Nit: target_x,target_y,duration_ms

Done.

https://codereview.chromium.org/1251323002/diff/20001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://codereview.chromium.org/1251323002/diff/20001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1248:
return mTouchScrollInProgress || mPotentiallyActiveFlingCount > 0;
On 2015/07/27 17:15:38, jdduke wrote:
> Hmm... it's a little unfortunate to lose the accounting here for the new
scroll
> cases, as we won't be reusing the fling architecture :(.
> 
> In practice there will be a few minor side effects:
> 1) the selection handles will remain visible/janky while the smooth scroll
> occurs
> 2) tap suppression won't work if the user taps the screen while the smooth
> scroll is active.
> 
> We have the same problems with the double-tap zoom animation, so it's not the
> end of the world.

yeah.. I guess the same set of problems apply to "FindInPage" use case.

[jdduke (slow), 2015-07-28 15:54:32]

I don't own most of this code, but in general this looks good per offline
discussion.

https://codereview.chromium.org/1251323002/diff/60001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://codereview.chromium.org/1251323002/diff/60001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1357:
public void smoothScroll(int targetX, int targetY, long durationMs) {
Nit: Javadoc.

https://codereview.chromium.org/1251323002/diff/60001/content/public/android/...
File
content/public/android/java/src/org/chromium/content_public/browser/WebContents.java
(right):

https://codereview.chromium.org/1251323002/diff/60001/content/public/android/...
content/public/android/java/src/org/chromium/content_public/browser/WebContents.java:177:
*/
Nit: Javadoc for params

[hush (inactive), 2015-07-28 21:36:47]

https://codereview.chromium.org/1251323002/diff/60001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://codereview.chromium.org/1251323002/diff/60001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1357:
public void smoothScroll(int targetX, int targetY, long durationMs) {
On 2015/07/28 15:54:31, jdduke wrote:
> Nit: Javadoc.

Done.

https://codereview.chromium.org/1251323002/diff/60001/content/public/android/...
File
content/public/android/java/src/org/chromium/content_public/browser/WebContents.java
(right):

https://codereview.chromium.org/1251323002/diff/60001/content/public/android/...
content/public/android/java/src/org/chromium/content_public/browser/WebContents.java:177:
*/
On 2015/07/28 15:54:31, jdduke wrote:
> Nit: Javadoc for params

Done.

[hush (inactive), 2015-07-28 21:54:06]

hush@chromium.org changed reviewers:
+ aelias@chromium.org, boliu@chromium.org, sievers@chromium.org,
tedchoc@chromium.org

[hush (inactive), 2015-07-28 21:54:07]

Okay.. adding more reviewers:
boliu@: android_webview/
aelias@: cc/
sievers@: content/renderer/ and content/common/
tedchoc@: other things in content/

This CL adds an IPC to blink to do a PageScaleAnimation that changes the scroll
position but not the page scale.
This IPC will be sent when WebView#pageUp, WebView#pageDown or
WebView#requestChildRectangleOnScreen is called.

I also removed "IsExternalScrollActive" because there won't be any externally
controlled scrolls any more.

The interesting parts are in ContentViewCore, webContentsImpl,
web_contents_android and render_view_impl.

Please take a look, thanks!

[boliu, 2015-07-28 22:44:25]

On 2015/07/28 21:54:07, hush wrote:
> Okay.. adding more reviewers:
> boliu@: android_webview/
> aelias@: cc/
> sievers@: content/renderer/ and content/common/
> tedchoc@: other things in content/
> 
> This CL adds an IPC to blink to do a PageScaleAnimation that changes the
scroll
> position but not the page scale.
> This IPC will be sent when WebView#pageUp, WebView#pageDown or
> WebView#requestChildRectangleOnScreen is called.
> 
> I also removed "IsExternalScrollActive" because there won't be any externally
> controlled scrolls any more.
> 
> The interesting parts are in ContentViewCore, webContentsImpl,
> web_contents_android and render_view_impl.
> 
> Please take a look, thanks!

If all the content additions is adding code path just for the IPC and call into
WebViewImpl, you can do that in android_webview code. See all the stuff in
AwRenderViewExt::OnMessageReceived

[hush (inactive), 2015-07-29 23:15:30]

Patchset #8 (id:130033) has been deleted

[hush (inactive), 2015-07-29 23:16:58]

On 2015/07/28 22:44:25, boliu wrote:
> On 2015/07/28 21:54:07, hush wrote:
> > Okay.. adding more reviewers:
> > boliu@: android_webview/
> > aelias@: cc/
> > sievers@: content/renderer/ and content/common/
> > tedchoc@: other things in content/
> > 
> > This CL adds an IPC to blink to do a PageScaleAnimation that changes the
> scroll
> > position but not the page scale.
> > This IPC will be sent when WebView#pageUp, WebView#pageDown or
> > WebView#requestChildRectangleOnScreen is called.
> > 
> > I also removed "IsExternalScrollActive" because there won't be any
externally
> > controlled scrolls any more.
> > 
> > The interesting parts are in ContentViewCore, webContentsImpl,
> > web_contents_android and render_view_impl.
> > 
> > Please take a look, thanks!
> 
> If all the content additions is adding code path just for the IPC and call
into
> WebViewImpl, you can do that in android_webview code. See all the stuff in
> AwRenderViewExt::OnMessageReceived

PTAL PS8

[boliu, 2015-07-29 23:32:31]

lgtm

Although probably should hold this off until crbug.com/514783 is fixed.

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

https://codereview.chromium.org/1251323002/diff/150001/android_webview/java/s...
android_webview/java/src/org/chromium/android_webview/AwScrollOffsetManager.java:269:
mDelegate.smoothScroll(scrollX + dx, scrollY + dy, computeDurationInMilliSec(dx,
dy));
nit: x, y

[Ted C, 2015-07-29 23:40:06]

On 2015/07/29 23:32:31, boliu wrote:
> lgtm
> 
> Although probably should hold this off until crbug.com/514783 is fixed.
> 
>
https://codereview.chromium.org/1251323002/diff/150001/android_webview/java/s...
> File
>
android_webview/java/src/org/chromium/android_webview/AwScrollOffsetManager.java
> (right):
> 
>
https://codereview.chromium.org/1251323002/diff/150001/android_webview/java/s...
>
android_webview/java/src/org/chromium/android_webview/AwScrollOffsetManager.java:269:
> mDelegate.smoothScroll(scrollX + dx, scrollY + dy,
computeDurationInMilliSec(dx,
> dy));
> nit: x, y

content - lgtm

[no sievers, 2015-07-30 00:22:40]

https://codereview.chromium.org/1251323002/diff/150001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://codereview.chromium.org/1251323002/diff/150001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1252:
public boolean isScrollInProgress() {
Is this broken now for smooth scrolls?

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

https://codereview.chromium.org/1251323002/diff/150001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://codereview.chromium.org/1251323002/diff/150001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1252:
public boolean isScrollInProgress() {
On 2015/07/30 00:22:40, sievers wrote:
> Is this broken now for smooth scrolls?

Yup, the same point was raised earlier. We have the same issue with double-tap
as well.

Currently the only WebView side effect will be that the selection handles don't
hide during the smooth scroll.

[aelias_OOO_until_Jul13, 2015-07-30 02:49:33]

https://codereview.chromium.org/1251323002/diff/20001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://codereview.chromium.org/1251323002/diff/20001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1248:
return mTouchScrollInProgress || mPotentiallyActiveFlingCount > 0;
On 2015/07/27 at 17:15:38, jdduke wrote:
> Hmm... it's a little unfortunate to lose the accounting here for the new
scroll cases, as we won't be reusing the fling architecture :(.
> 
> In practice there will be a few minor side effects:
> 1) the selection handles will remain visible/janky while the smooth scroll
occurs
> 2) tap suppression won't work if the user taps the screen while the smooth
scroll is active.
> 
> We have the same problems with the double-tap zoom animation, so it's not the
end of the world.

Well, as you say it's the same problem with double-tap zoom, and the bugs are
probably more impactful there.  These issues could probably be fixed since CC is
aware of page scale animation.

https://codereview.chromium.org/1251323002/diff/150001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://codereview.chromium.org/1251323002/diff/150001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1252:
public boolean isScrollInProgress() {
On 2015/07/30 at 01:04:04, jdduke wrote:
> On 2015/07/30 00:22:40, sievers wrote:
> > Is this broken now for smooth scrolls?
> 
> Yup, the same point was raised earlier. We have the same issue with double-tap
as well.
> 
> Currently the only WebView side effect will be that the selection handles
don't hide during the smooth scroll.

This doesn't need to be fixed in this patch, but I think we should probably
switch to using a field in CompositorFrameMetadata about whether a scroll is
currently active.  Then we can avoid doing things based on "potentially" active
flings, which is another questionable part of this method.

[jdduke (slow), 2015-07-30 23:32:58]

On 2015/07/30 02:49:33, aelias wrote:
> This doesn't need to be fixed in this patch, but I think we should probably
> switch to using a field in CompositorFrameMetadata about whether a scroll is
> currently active.  Then we can avoid doing things based on "potentially"
active
> flings, which is another questionable part of this method.

Yup, agreed. I considered something like that in the past, but we need
additional plumbing bits from the main thread (to indicate main thread
scrolling) and it wasn't urgent at the time. This change is probably
justification enough.

[hush (inactive), 2015-08-03 19:03:36]

The CQ bit was checked by hush@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-08-03 19:05:38]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1251323002/150001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1251323002/150001

[commit-bot: I haz the power, 2015-08-03 20:17:29]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-08-03 20:17:30]

Dry run: This issue passed the CQ dry run.

[boliu, 2015-08-12 18:43:46]

https://codereview.chromium.org/1251323002/diff/150001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://codereview.chromium.org/1251323002/diff/150001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1252:
public boolean isScrollInProgress() {
On 2015/07/30 02:49:33, aelias wrote:
> On 2015/07/30 at 01:04:04, jdduke wrote:
> > On 2015/07/30 00:22:40, sievers wrote:
> > > Is this broken now for smooth scrolls?
> > 
> > Yup, the same point was raised earlier. We have the same issue with
double-tap
> as well.
> > 
> > Currently the only WebView side effect will be that the selection handles
> don't hide during the smooth scroll.
> 
> This doesn't need to be fixed in this patch, but I think we should probably
> switch to using a field in CompositorFrameMetadata about whether a scroll is
> currently active.  Then we can avoid doing things based on "potentially"
active
> flings, which is another questionable part of this method.

Just noticed, I think this is already broken for majority of webview flings in
m44. isExternalScrollActive is not returning true for majority of root layer
scrolls in webview, in case anyone complains..

[hush (inactive), 2015-09-01 17:11:35]

https://codereview.chromium.org/1251323002/diff/150001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://codereview.chromium.org/1251323002/diff/150001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1252:
public boolean isScrollInProgress() {
On 2015/08/12 18:43:46, boliu wrote:
> On 2015/07/30 02:49:33, aelias wrote:
> > On 2015/07/30 at 01:04:04, jdduke wrote:
> > > On 2015/07/30 00:22:40, sievers wrote:
> > > > Is this broken now for smooth scrolls?
> > > 
> > > Yup, the same point was raised earlier. We have the same issue with
> double-tap
> > as well.
> > > 
> > > Currently the only WebView side effect will be that the selection handles
> > don't hide during the smooth scroll.
> > 
> > This doesn't need to be fixed in this patch, but I think we should probably
> > switch to using a field in CompositorFrameMetadata about whether a scroll is
> > currently active.  Then we can avoid doing things based on "potentially"
> active
> > flings, which is another questionable part of this method.
> 
> Just noticed, I think this is already broken for majority of webview flings in
> m44. isExternalScrollActive is not returning true for majority of root layer
> scrolls in webview, in case anyone complains..

Sorry how is this broken?
I just tried m44 and m46 with this change and isScrollInProgress works as
expected. If I fling when I select some texts, the selection handles will be
hidden until the fling stops.

[boliu, 2015-09-01 20:58:46]

https://codereview.chromium.org/1251323002/diff/150001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://codereview.chromium.org/1251323002/diff/150001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1252:
public boolean isScrollInProgress() {
On 2015/09/01 17:11:35, hush wrote:
> On 2015/08/12 18:43:46, boliu wrote:
> > On 2015/07/30 02:49:33, aelias wrote:
> > > On 2015/07/30 at 01:04:04, jdduke wrote:
> > > > On 2015/07/30 00:22:40, sievers wrote:
> > > > > Is this broken now for smooth scrolls?
> > > > 
> > > > Yup, the same point was raised earlier. We have the same issue with
> > double-tap
> > > as well.
> > > > 
> > > > Currently the only WebView side effect will be that the selection
handles
> > > don't hide during the smooth scroll.
> > > 
> > > This doesn't need to be fixed in this patch, but I think we should
probably
> > > switch to using a field in CompositorFrameMetadata about whether a scroll
is
> > > currently active.  Then we can avoid doing things based on "potentially"
> > active
> > > flings, which is another questionable part of this method.
> > 
> > Just noticed, I think this is already broken for majority of webview flings
in
> > m44. isExternalScrollActive is not returning true for majority of root layer
> > scrolls in webview, in case anyone complains..
> 
> Sorry how is this broken?
> I just tried m44 and m46 with this change and isScrollInProgress works as
> expected. If I fling when I select some texts, the selection handles will be
> hidden until the fling stops.

I thought root layer flings in webview no longer go through the java scroller,
so isScrollInProgress would return false in that case here?

[hush (inactive), 2015-09-01 22:43:36]

https://codereview.chromium.org/1251323002/diff/150001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://codereview.chromium.org/1251323002/diff/150001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1252:
public boolean isScrollInProgress() {
On 2015/09/01 20:58:46, boliu wrote:
> On 2015/09/01 17:11:35, hush wrote:
> > On 2015/08/12 18:43:46, boliu wrote:
> > > On 2015/07/30 02:49:33, aelias wrote:
> > > > On 2015/07/30 at 01:04:04, jdduke wrote:
> > > > > On 2015/07/30 00:22:40, sievers wrote:
> > > > > > Is this broken now for smooth scrolls?
> > > > > 
> > > > > Yup, the same point was raised earlier. We have the same issue with
> > > double-tap
> > > > as well.
> > > > > 
> > > > > Currently the only WebView side effect will be that the selection
> handles
> > > > don't hide during the smooth scroll.
> > > > 
> > > > This doesn't need to be fixed in this patch, but I think we should
> probably
> > > > switch to using a field in CompositorFrameMetadata about whether a
scroll
> is
> > > > currently active.  Then we can avoid doing things based on "potentially"
> > > active
> > > > flings, which is another questionable part of this method.
> > > 
> > > Just noticed, I think this is already broken for majority of webview
flings
> in
> > > m44. isExternalScrollActive is not returning true for majority of root
layer
> > > scrolls in webview, in case anyone complains..
> > 
> > Sorry how is this broken?
> > I just tried m44 and m46 with this change and isScrollInProgress works as
> > expected. If I fling when I select some texts, the selection handles will be
> > hidden until the fling stops.
> 
> I thought root layer flings in webview no longer go through the java scroller,
> so isScrollInProgress would return false in that case here?

It will be true during a fling because the GestureFlingStart event will be
handled by InputHandlerProxy. So a "INPUT_EVENT_ACK_STATE_CONSUMED" will be sent
to InputRouterImpl.
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re...
This will eventually call the java callback in
ContentViewCore#onFlingStartEventConsumed.

So yeah, the mPotentiallyActiveFlingCount is correct.

[hush (inactive), 2015-09-03 00:35:00]

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

[hush (inactive), 2015-09-03 00:35:01]

dcheng@: PTAL android_webview/common/render_view_messages.h

aelias@: PTAL cc/

[hush (inactive), 2015-09-03 00:37:20]

On 2015/09/03 00:35:01, hush wrote:
> dcheng@: PTAL android_webview/common/render_view_messages.h
> 
> aelias@: PTAL cc/

sorry no cc change here...
I just need android_webview/common/render_view_messages.h reviewed. Thanks!

[hush (inactive), 2015-09-03 00:37:26]

The CQ bit was checked by hush@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-09-03 00:37:41]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1251323002/190001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1251323002/190001

[commit-bot: I haz the power, 2015-09-03 02:22:35]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-09-03 02:22:36]

Dry run: This issue passed the CQ dry run.

[dcheng, 2015-09-08 18:25:24]

ipc changes lgtm

[hush (inactive), 2015-09-08 18:28:45]

The CQ bit was checked by hush@chromium.org

[hush (inactive), 2015-09-08 18:28:45]

The patchset sent to the CQ was uploaded after l-g-t-m from
tedchoc@chromium.org, boliu@chromium.org
Link to the patchset: https://codereview.chromium.org/1251323002/#ps190001
(title: "rebase again")

[commit-bot: I haz the power, 2015-09-08 18:28:55]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1251323002/190001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1251323002/190001

[commit-bot: I haz the power, 2015-09-08 19:44:48]

Committed patchset #10 (id:190001)

[commit-bot: I haz the power, 2015-09-08 19:45:22]

Patchset 10 (id:??) landed as
https://crrev.com/18c284495573fe54a6cbaad16beeb24029add9c4
Cr-Commit-Position: refs/heads/master@{#347770}