Let the browser know the end of fling

Let the browser know the end of fling

On Android, resize event caused by showing/hiding top controls may be
expensive and cause janky fling if it is sent before the end of fling.
Let the browser know the end of fling so that the browser can delay the
resize event.

As we have not resolved the issue of raciness between the main thread
and the compositor thread, and between the renderer process and the
browser process, about the sequence of fling events and notifications,
for now we don't use the notification to update the fling status in
ContentViewGestureHandler and GestureEventFilter.

The DidStopFlinging notification is sent along the following path:

- Renderer side:
  - compositor-thread-flinging:
      InputHandlerProxy::CancelCurrentFling()
   -> InputHandlerWrapper::DidStopFlinging()
   -> InputHandlerManager::DidStopFlinging()
   -> InputEventFilter::DidStopFlinging()
   -> IPC::Sender::Send(ViewHostMsg_DidStopFlinging)

  - main-thread-flinging (existing except the steps same as compositor-thread-flinging):
      WebViewImpl::endActiveFlingAnimation()
   -> RenderWidgetCompositor::didStopFlinging()
   -> LayerTreeHost::didStopFlinging()
   -> ThreadProxy::MainThreadHasStoppedFlinging()
   -> LayerTreeHostImpl::MainThreadHasStoppedFlinging()
   -> InputHandlerProxy::MainThreadHasStoppedFlinging()
   then same as compositor-thread-flinging[1:]

- Browser side:
     RenderWidgetHostImpl::OnMessageReceived(ViewHostMsg_DidStopFlinging)
  -> RenderWidgetHostImpl::OnFlingingStopped()
     // Android only. No-op on other platforms
  -> RenderWidgetHostViewAndroid::DidStopFlinging()
  -> ContentViewCoreImpl::DidStopFlinging()
  -> ContentViewCore.onNativeFlingStopped() // Java
  -> ContentViewClient.onFlingStopped()
  then downstream embedder code

BUG=244736
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=245624

[Xianzhu, 2014-01-09 19:48:10]

jdduke: Java code, content/renderer/input, content/browser/android,
content/browser/renderer_host/input
aelias: above + content/browser/renderer_host/render_widget_host_view_android.*
jochen: other content files
dcheng: view_messages.h

[Xianzhu, 2014-01-09 19:51:00]

About the naming:
- DidStopFlinging (most places): following existing
LayerTreeHost::DidStopFlinging
- RenderWidgetHostImpl::OnFlingingStopped: following
RenderWidgetHostImpl::OnOverscrolled
- In Java code: following existing code ('Fling' not 'Flinging').

[jdduke (slow), 2014-01-09 20:07:13]

My chief concern here is raciness between flings active on main and impl
threads.  In practice, this shouldn't happen a lot, but it is yet another
argument for consolidating where flings are driven (impl-thread only?).

https://codereview.chromium.org/131373004/diff/1/content/browser/android/in_p...
File content/browser/android/in_process/synchronous_input_event_filter.cc
(right):

https://codereview.chromium.org/131373004/diff/1/content/browser/android/in_p...
content/browser/android/in_process/synchronous_input_event_filter.cc:79: void
SynchronousInputEventFilter::DidStopFlinging(int routing_id) {
Does WebView not care?

https://codereview.chromium.org/131373004/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/input/gesture_event_filter.cc (right):

https://codereview.chromium.org/131373004/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/input/gesture_event_filter.cc:240:
fling_in_progress_ = false;
What if there is a pending GestureFlingStart still in the queue?

https://codereview.chromium.org/131373004/diff/1/content/common/view_messages.h
File content/common/view_messages.h (right):

https://codereview.chromium.org/131373004/diff/1/content/common/view_messages...
content/common/view_messages.h:2075:
IPC_MESSAGE_ROUTED0(ViewHostMsg_DidStopFlinging)
I guess I should have made ViewHostMsg_DidOverscroll an
InputHostMsg_DidOverscroll, but I could probably move these later if necessary.

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

https://codereview.chromium.org/131373004/diff/1/content/renderer/input/input...
content/renderer/input/input_handler_proxy.cc:355: client_->DidStopFlinging();
What if there's a fling active on the impl thread?

https://codereview.chromium.org/131373004/diff/1/content/renderer/input/input...
content/renderer/input/input_handler_proxy.cc:392: client_->DidStopFlinging();
I think we only want to send this if there was a fling curve, no?

[Xianzhu, 2014-01-09 22:28:51]

On 2014/01/09 20:07:13, jdduke wrote:
> My chief concern here is raciness between flings active on main and impl
> threads.  In practice, this shouldn't happen a lot, but it is yet another
> argument for consolidating where flings are driven (impl-thread only?).
> 

Besides this, the raciness between the renderer and the browser seems a bigger
problem and may happen more frequently. Perhaps we can let the renderer send
both DidStartFlinging and DidStopFlinging. This seems good for the purpose of
the bug but not for GestureFlingCancel control because of the delay. The final
solution seems still browser-side fling control. wdyt?

[jdduke (slow), 2014-01-09 22:36:56]

On 2014/01/09 22:28:51, Xianzhu wrote:
> On 2014/01/09 20:07:13, jdduke wrote:
> > My chief concern here is raciness between flings active on main and impl
> > threads.  In practice, this shouldn't happen a lot, but it is yet another
> > argument for consolidating where flings are driven (impl-thread only?).
> > 
> 
> Besides this, the raciness between the renderer and the browser seems a bigger
> problem and may happen more frequently. Perhaps we can let the renderer send
> both DidStartFlinging and DidStopFlinging. This seems good for the purpose of
> the bug but not for GestureFlingCancel control because of the delay. The final
> solution seems still browser-side fling control. wdyt?

Yeah, I was just about to mention the case where a GestureFlingStart goes
unhandled.  For now, I think we might consider using this message only for
notifications to the view, for the sake of fixing the bug.  We can adjust the
GestureEventFilter and ContentViewGestureHandler later on with the right hooks
to leverage these notifications.

So the question is, what about the time gap between sending a MOTION_UP event
and receiving a DidStartFlinging response.  If we allow resizing in that gap,
will it defeat the purpose?  I'm not very familiar with the sequence of events
between fullscreen changes and a resize event, so I can't really comment.

[Xianzhu, 2014-01-10 01:06:20]

On 2014/01/09 22:36:56, jdduke wrote:
> Yeah, I was just about to mention the case where a GestureFlingStart goes
> unhandled.  For now, I think we might consider using this message only for
> notifications to the view, for the sake of fixing the bug.  We can adjust the
> GestureEventFilter and ContentViewGestureHandler later on with the right hooks
> to leverage these notifications.
>

PTAL. New patch set leave original GestureEventFilter and
ContentViewGestureHandler unchanged.
 
> So the question is, what about the time gap between sending a MOTION_UP event
> and receiving a DidStartFlinging response.  If we allow resizing in that gap,
> will it defeat the purpose?  I'm not very familiar with the sequence of events
> between fullscreen changes and a resize event, so I can't really comment.

We can delay the resize for a short time to tolerate the gap. The resize event
isn't required to be timely and can be asynchronized with other events.
Will upload the downstream patch soon.

[jdduke (slow), 2014-01-10 17:45:59]

This should work, but I'd like to hear what aelias@ thinks.

There could still be gaps in the coverage if a fling is queued, however I'm not
super worried about that, as it means we're already in a slow state.  

I wonder if it would be sufficient to have an |active_fling_event| counter in
ContentViewCore.   This would increment each time a fling is sent by
ContentViewCore, and decremented each time we got one of |FlingStopped()| or an
unconsumed fling ack. If the counter increases from zero, we notify the client
"FlingStarted", and if the counter decrements to zero, we notify the client
"FlingStopped".  This safely handles any number of oustanding flings, and
eliminates the need for a speculative "FlingStarted" message from the renderer. 
What do you think?

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

https://codereview.chromium.org/131373004/diff/110001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1435:
// onFlingStopped() will be called from unhandledFlingStartEvent().
So we may get a redundant |onFlingStarted()| call from native? I guess that's OK
as long as there's always a terminal |onFlingStopped()|.

https://codereview.chromium.org/131373004/diff/110001/content/renderer/input/...
File content/renderer/input/input_event_filter.cc (right):

https://codereview.chromium.org/131373004/diff/110001/content/renderer/input/...
content/renderer/input/input_event_filter.cc:71: io_loop_->PostTask(
Now that we have a good number of methods posting to |SendMessageOnUIThread()|,
could we create a |Send()| method that takes a message and does the posting?

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

https://codereview.chromium.org/131373004/diff/110001/content/renderer/input/...
content/renderer/input/input_handler_proxy.cc:307: client_->DidStartFlinging();
So, if the fling goes unhandled by main, the |DidStart()| will be terminated by
the NOT_CONSUMED ack?

[Xianzhu, 2014-01-10 20:16:02]

On 2014/01/10 17:45:59, jdduke wrote:
...

Your comments remind me a way to ensure the status of fling: using the ACK of
GestureStartFling event, which seems better.

Counter would work if we strictly paired onFlingStarted() and onFlingStopped(),
but it couldn't handle the gap well. In Patch Set 3, to handle the gap,
onFlingStarted() is called from both sendGesture() and onFlingStartEventAck() so
the client should manage the fling status as a bool instead of a counter.

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

https://codereview.chromium.org/131373004/diff/1/content/renderer/input/input...
content/renderer/input/input_handler_proxy.cc:392: client_->DidStopFlinging();
On 2014/01/09 20:07:14, jdduke wrote:
> I think we only want to send this if there was a fling curve, no?

Done.

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

https://codereview.chromium.org/131373004/diff/110001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1435:
// onFlingStopped() will be called from unhandledFlingStartEvent().
On 2014/01/10 17:45:59, jdduke wrote:
> So we may get a redundant |onFlingStarted()| call from native? I guess that's
OK
> as long as there's always a terminal |onFlingStopped()|.

Yes, the redundancy is to handle the gap. Added comments about it.

https://codereview.chromium.org/131373004/diff/110001/content/renderer/input/...
File content/renderer/input/input_event_filter.cc (right):

https://codereview.chromium.org/131373004/diff/110001/content/renderer/input/...
content/renderer/input/input_event_filter.cc:71: io_loop_->PostTask(
On 2014/01/10 17:45:59, jdduke wrote:
> Now that we have a good number of methods posting to
|SendMessageOnUIThread()|,
> could we create a |Send()| method that takes a message and does the posting?

Done.

[jdduke (slow), 2014-01-10 21:23:11]

This looks pretty good.  I think the counter would work, you'd just have to
increment the counter when you *send* the GestureFlingStart from
ContentViewCore, listening to the ack's and the "HasStopped" messages to
decrement appropriately.

[Xianzhu, 2014-01-10 22:47:05]

On 2014/01/10 21:23:11, jdduke wrote:
> This looks pretty good.  I think the counter would work, you'd just have to
> increment the counter when you *send* the GestureFlingStart from
> ContentViewCore, listening to the ack's and the "HasStopped" messages to
> decrement appropriately.

Yes, this works :)

New patch set ensures onFlingStarted()/onFlingStopped() calls are paired (but
may be interlaced). Requires client to use a counter (see
https://chrome-internal-review.googlesource.com/151649).

[jdduke (slow), 2014-01-10 23:00:03]

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

https://codereview.chromium.org/131373004/diff/510001/content/renderer/input/...
content/renderer/input/input_handler_proxy.cc:428:
fling_may_be_active_on_main_thread_ = true;

Hmm, I think we need to be careful here.  This will send a |DidStopFlinging()|
message immediately, and another one when the transferred fling terminates on
the main thread.  

You can prevent this by calling |fling_curve_.reset()| before
|CancelCurrentFling()| (that's kind of hacky, but it's valid because fling
transfer only occurs with wheel events).

[jdduke (slow), 2014-01-10 23:03:57]

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

https://codereview.chromium.org/131373004/diff/510001/content/renderer/input/...
content/renderer/input/input_handler_proxy.cc:428:
fling_may_be_active_on_main_thread_ = true;
On 2014/01/10 23:00:03, jdduke wrote:
> 
> Hmm, I think we need to be careful here.  This will send a |DidStopFlinging()|
> message immediately, and another one when the transferred fling terminates on
> the main thread.  
> 
> You can prevent this by calling |fling_curve_.reset()| before
> |CancelCurrentFling()| (that's kind of hacky, but it's valid because fling
> transfer only occurs with wheel events).

If you do go that route, please put a comment explaining why you explicitly
reset the fling before before calling |CancelCurrentFling()|, so somebody
doesn't roll in and clean it up :)

[Xianzhu, 2014-01-10 23:30:44]

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

https://codereview.chromium.org/131373004/diff/510001/content/renderer/input/...
content/renderer/input/input_handler_proxy.cc:428:
fling_may_be_active_on_main_thread_ = true;
On 2014/01/10 23:03:58, jdduke wrote:
> On 2014/01/10 23:00:03, jdduke wrote:
> > 
> > Hmm, I think we need to be careful here.  This will send a
|DidStopFlinging()|
> > message immediately, and another one when the transferred fling terminates
on
> > the main thread.  
> > 
> > You can prevent this by calling |fling_curve_.reset()| before
> > |CancelCurrentFling()| (that's kind of hacky, but it's valid because fling
> > transfer only occurs with wheel events).
> 
> If you do go that route, please put a comment explaining why you explicitly
> reset the fling before before calling |CancelCurrentFling()|, so somebody
> doesn't roll in and clean it up :)

Thanks for the nice finding!

However, this makes me rethink counter vs boolean. I'm afraid there might be
similar issues hidden somewhere in the possible paths of fling handling (here,
cc, WebViewImpl, EventHandler, ScrollBar, ...). This makes the counter fragile.
Any error can break the counter until the tab is closed. A boolean status won't
propagate errors, though there may be short gaps of inaccuracy which could be
tolerated with a delay.

[jdduke (slow), 2014-01-10 23:55:12]

On 2014/01/10 23:30:44, Xianzhu wrote:
>
https://codereview.chromium.org/131373004/diff/510001/content/renderer/input/...
> File content/renderer/input/input_handler_proxy.cc (right):
> 
>
https://codereview.chromium.org/131373004/diff/510001/content/renderer/input/...
> content/renderer/input/input_handler_proxy.cc:428:
> fling_may_be_active_on_main_thread_ = true;
> On 2014/01/10 23:03:58, jdduke wrote:
> > On 2014/01/10 23:00:03, jdduke wrote:
> > > 
> > > Hmm, I think we need to be careful here.  This will send a
> |DidStopFlinging()|
> > > message immediately, and another one when the transferred fling terminates
> on
> > > the main thread.  
> > > 
> > > You can prevent this by calling |fling_curve_.reset()| before
> > > |CancelCurrentFling()| (that's kind of hacky, but it's valid because fling
> > > transfer only occurs with wheel events).
> > 
> > If you do go that route, please put a comment explaining why you explicitly
> > reset the fling before before calling |CancelCurrentFling()|, so somebody
> > doesn't roll in and clean it up :)
> 
> Thanks for the nice finding!
> 
> However, this makes me rethink counter vs boolean. I'm afraid there might be
> similar issues hidden somewhere in the possible paths of fling handling (here,
> cc, WebViewImpl, EventHandler, ScrollBar, ...). This makes the counter
fragile.
> Any error can break the counter until the tab is closed. A boolean status
won't
> propagate errors, though there may be short gaps of inaccuracy which could be
> tolerated with a delay.

Yeah, I don't think either approach is perfect =/

However, if the browser is the only client that does the increment (when sending
a GestureFlingStart), the two bad scenarios are 1) We don't get enough
DidStopFlinging messages, or 2) we get too many.

With (1), both approaches will fail (boolean or counter).  We might have a
timeout that "unlocks" the resize, but there's no clean solution.  With (2),
both approaches are fine so long as we just stop the counter at 0.  This could
be worth a DCHECK/assert (only the browser should generate fling events, so we
have know the ground truth).

[Xianzhu, 2014-01-11 00:11:12]

On 2014/01/10 23:55:12, jdduke wrote:
> On 2014/01/10 23:30:44, Xianzhu wrote:
> >
>
https://codereview.chromium.org/131373004/diff/510001/content/renderer/input/...
> > File content/renderer/input/input_handler_proxy.cc (right):
> > 
> >
>
https://codereview.chromium.org/131373004/diff/510001/content/renderer/input/...
> > content/renderer/input/input_handler_proxy.cc:428:
> > fling_may_be_active_on_main_thread_ = true;
> > On 2014/01/10 23:03:58, jdduke wrote:
> > > On 2014/01/10 23:00:03, jdduke wrote:
> > > > 
> > > > Hmm, I think we need to be careful here.  This will send a
> > |DidStopFlinging()|
> > > > message immediately, and another one when the transferred fling
terminates
> > on
> > > > the main thread.  
> > > > 
> > > > You can prevent this by calling |fling_curve_.reset()| before
> > > > |CancelCurrentFling()| (that's kind of hacky, but it's valid because
fling
> > > > transfer only occurs with wheel events).
> > > 
> > > If you do go that route, please put a comment explaining why you
explicitly
> > > reset the fling before before calling |CancelCurrentFling()|, so somebody
> > > doesn't roll in and clean it up :)
> > 
> > Thanks for the nice finding!
> > 
> > However, this makes me rethink counter vs boolean. I'm afraid there might be
> > similar issues hidden somewhere in the possible paths of fling handling
(here,
> > cc, WebViewImpl, EventHandler, ScrollBar, ...). This makes the counter
> fragile.
> > Any error can break the counter until the tab is closed. A boolean status
> won't
> > propagate errors, though there may be short gaps of inaccuracy which could
be
> > tolerated with a delay.
> 
> Yeah, I don't think either approach is perfect =/
> 
> However, if the browser is the only client that does the increment (when
sending
> a GestureFlingStart), the two bad scenarios are 1) We don't get enough
> DidStopFlinging messages, or 2) we get too many.
> 
> With (1), both approaches will fail (boolean or counter).  We might have a
> timeout that "unlocks" the resize, but there's no clean solution.  With (2),
> both approaches are fine so long as we just stop the counter at 0.  This could
> be worth a DCHECK/assert (only the browser should generate fling events, so we
> have know the ground truth).

I thought the boolean method could do better for (1) because the bad status
could be cleared on the next touch if we call onFlingStopped unconditionally on
GestureFlingCancel. Anyway, with timeout to reset the status, they will be
similar again.

Will continue to use the counter method because it looks clearer in chromium
code, but will add a timeout in downstream client code.

[Xianzhu, 2014-01-11 00:18:51]

Patch Set 5 handles fling transition case. Can't use |fling_curve_.reset()|
because it will break touchscreen's ScrollEnd(). Neither checking
fling_may_be_active_on_main_thread_ because it may be also true when the main
thread is not actually flinging. Added a parameter to CancelCurrentFling()
instead.

[aelias_OOO_until_Jul13, 2014-01-11 00:45:52]

This is a step along the path of suppressing the main-thread to protect the fast
path.  I'm not sold on the idea that we should do this, it adds complexity as a
workaround.  Before doing this, I'd rather we continued to discuss how to reduce
resize cost in Blink.

I recall you ran into some tricky obstacles there, but maybe there are further
options we can discuss.  If needed, for example, Blink could be made explicitly
aware of the concept of top-controls based resize.  Something special could also
be done involving fixed-pos elements since those are one of the main reasons why
the resize is needed.

[aelias_OOO_until_Jul13, 2014-01-11 03:07:02]

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

https://codereview.chromium.org/131373004/diff/580001/content/renderer/input/...
content/renderer/input/input_handler_proxy.cc:349: CancelCurrentFling(true);
Also, a concrete bug here is that this is called when the fling velocity runs
out of steam, which may be several seconds after the scroll limit is hit.

[aelias_OOO_until_Jul13, 2014-01-11 03:13:25]

Another problem is that the notification is racing with tap events.  Let's say
the user taps on a bottom-fixed-pos element right after the fling ends, then
Blink hears about the tap before the resize.  Then the tap won't be registered
on the right element.  I think this problem may be unsolvable short of moving
fling to the browser.

[Xianzhu, 2014-01-13 17:56:00]

On 2014/01/11 03:13:25, aelias wrote:
> Another problem is that the notification is racing with tap events.  Let's say
> the user taps on a bottom-fixed-pos element right after the fling ends, then
> Blink hears about the tap before the resize.  Then the tap won't be registered
> on the right element.  I think this problem may be unsolvable short of moving
> fling to the browser.

I'm wondering how this would happen. After the top controls hide and before the
resize event is sent, the bottom-fixed-pos element will be not at the bottom
because blink still thinks the viewport size is small. Only after the resize and
blink re-layout the page will the element be at the bottom again. There is
inconsistency about viewport size between the browser and blink before the
resize (which also happens during the top-control animation). However, I think
despite the inconsistency, the status of blink itself should be still
consistent. Is there inconsistency between cc and blink causing the wrong target
of touch event?

crbug.com/258219 is about optimization of layout/repaint on resize. However, no
matter how it's optimized, there will still be pages needing complex layout on
resize. So for the patch I'm trying to find a more proper time to send the
resize.

The delay caused by big velocity wouldn't look a big problem to me, if there
were no malfunction in the page caused by the delay (though some page may look
not layouted as expected temporarily).

[Xianzhu, 2014-01-15 18:56:45]

Based on Alex's hint, I found that cc:TopControlsManager seems a better source
of the notification, because it knows not only the end of scroll/fling, but also
the end of top-controls animation which may happen after the end of
scroll/fling. I think we should also delay the resize after the end of animation
if it is later.

Tried to pass an additional field |handling_location_bar_gesture| in
CompositorFrameMetadata, but because it is sent in
ViewHostMsg_SwapCompositorFrame, there may be not immediate notification when
|handling_location_bar_gesture| becomes false. An additional message seems still
needed. Exploring in this way, while welcome your opinions.

There seems real inconsistency between cc and blink about viewport size during
the top-controls animation before blink gets the resize. Also investigating the
issue.

[jdduke (slow), 2014-01-15 21:50:04]

mkosiba@: Would WebView have any interest in this kind of notification? For root
layer flings, you always handle the fling in the browser, no?

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

https://codereview.chromium.org/131373004/diff/580001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1371:
}

I'm only worried about WebView behavior here.  Is it possible that WebView (via
ContentViewClient) would be interested in this? If so,  we might need to look
into how |mGestureStateListener.onUnhandledFlingStartEvent()| would interact
with the |onFlingStopped()| call.

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

https://codereview.chromium.org/131373004/diff/580001/content/renderer/input/...
content/renderer/input/input_handler_proxy.cc:349: CancelCurrentFling(true);
On 2014/01/11 03:07:03, aelias wrote:
> Also, a concrete bug here is that this is called when the fling velocity runs
> out of steam, which may be several seconds after the scroll limit is hit.

Hmm, that should only hold for non-root flings.  For root flings, we'll
terminate the fling as soon as it overscrolls on each axis that was flung. 
Speaking of which, I wonder why we don't use the result of
|InputHandler::ScrollBy| to early-out from a fling that hits the limit. I'd have
to look at the details to make sure that would work...

[aelias_OOO_until_Jul13, 2014-01-15 22:57:08]

WebView has a muych easier time with fling because everything happens on the
same thread, so it passes around perfect information synchronously via
LayerScrollOffsetDelegate.  WebView doesn't have top controls either, so it
doesn't really need to be considered in this discussion.

> because it is sent in
ViewHostMsg_SwapCompositorFrame, there may be not immediate notification when
|handling_location_bar_gesture| becomes false.

One option would be to call SetFullRootLayerDamage to ensure the frame is sent. 
I'm also uncertain whether FrameMetadata or your current approach is cleaner
though.

[jdduke (slow), 2014-01-15 23:23:31]

Having the message does give us a tighter window, in the event that we use this
for other purposes (e.g., dfalcantara may be interested in fling end
notifications as well).  It could also tighten up the bounds for disappearing
selection handles (though that certainly doesn't need to be precise).

[aelias_OOO_until_Jul13, 2014-01-16 00:49:56]

OK.  lgtm to land this as is.  I think it's the best we can do in our current
fling architecture.  (I think it's a bad idea to cause damage when nothing on
the screen has changed so that makes this superior to the
CompositorFrameMetadata approach.)

[Xianzhu, 2014-01-16 01:42:52]

Thanks for review.

FYI, also experimented other 2 ways (for top control resize only, not for other
purposes of fling end notification):

- https://codereview.chromium.org/139383006: use CompositedFrameMetadata
  - extra damage on end of top controls gesture (but the cost may be trivial
because we already did it on each step of the animation);
  - handles the case of bottom-fixed-pos element. If any of them, just send
resize immediately on end of gesture or next touch.

- https://codereview.chromium.org/139563004: similar to this patch, but
  - the message changed to InputHostMsg_DidFinishTopControlsGesture
  - source of the notification is TopControlsManager
  - main thread fling omitted, because we don't trigger top control auto-hide.

[aelias_OOO_until_Jul13, 2014-01-16 04:17:55]

For "I think we should also delay the resize after the end of animation if it is
later.", assuming we go with this fling-end approach, you can probably just look
at the CompositorFrameMetadata and see if the coordinate is at the top or bottom
of the range?  If it's in the middle then we're still animating (or scrolling).

[mkosiba (inactive), 2014-01-16 13:58:45]

On 2014/01/15 21:50:04, jdduke wrote:
> mkosiba@: Would WebView have any interest in this kind of notification? For
root
> layer flings, you always handle the fling in the browser, no?

Yes, we always handle the root layer fling on our own. Can we even get a "stop"
in
this case? We don't care for sublayer fling.

[Xianzhu, 2014-01-16 18:00:08]

On 2014/01/16 13:58:45, mkosiba wrote:
> On 2014/01/15 21:50:04, jdduke wrote:
> > mkosiba@: Would WebView have any interest in this kind of notification? For
> root
> > layer flings, you always handle the fling in the browser, no?
> 
> Yes, we always handle the root layer fling on our own. Can we even get a
"stop"
> in
> this case? We don't care for sublayer fling.

Yes, in the case, the client will get onFlingStarted() on the GestureFlingStart
event and onFlingStopped() on the ACK of the event (as root layer fling is
disabled for WebView). Each call happens after the corresponding
GestureStateListener call.

[Xianzhu, 2014-01-16 18:00:45]

On 2014/01/16 04:17:55, aelias wrote:
> For "I think we should also delay the resize after the end of animation if it
is
> later.", assuming we go with this fling-end approach, you can probably just
look
> at the CompositorFrameMetadata and see if the coordinate is at the top or
bottom
> of the range?  If it's in the middle then we're still animating (or
scrolling).

Thanks for the idea. This can be done in the downstream change.

[Xianzhu, 2014-01-16 18:04:01]

Files still need review:
view_messages.h: dcheng@
render_widget_host_view_port.h: jochen@
content/browser/android and content/public/android: jdduke@

[jdduke (slow), 2014-01-16 18:44:06]

https://codereview.chromium.org/131373004/diff/580001/content/browser/android...
File content/browser/android/in_process/synchronous_input_event_filter.cc
(right):

https://codereview.chromium.org/131373004/diff/580001/content/browser/android...
content/browser/android/in_process/synchronous_input_event_filter.cc:79: void
SynchronousInputEventFilter::DidStopFlinging(int routing_id) {

I'm not totally comfortable leaving this as a dead-end.

Could you extend this to a |SynchronousCompositorImpl::DidStopFlinging()|, where
we can call |RenderWidgetHostViewAndroid::DidStopFlinging()|?  You can look at
the methods here and in SynchronousCompositorImpl to see how that should be
done, it should be fairly straightforward.

[Xianzhu, 2014-01-16 19:02:24]

On 2014/01/16 18:00:08, Xianzhu wrote:
> On 2014/01/16 13:58:45, mkosiba wrote:
> > Yes, we always handle the root layer fling on our own. Can we even get a
> "stop"
> > in
> > this case? We don't care for sublayer fling.
> 
> Yes, in the case, the client will get onFlingStarted() on the
GestureFlingStart
> event and onFlingStopped() on the ACK of the event (as root layer fling is
> disabled for WebView). Each call happens after the corresponding
> GestureStateListener call.

Actually this is true only with the latest Patch Set :)

https://codereview.chromium.org/131373004/diff/580001/content/browser/android...
File content/browser/android/in_process/synchronous_input_event_filter.cc
(right):

https://codereview.chromium.org/131373004/diff/580001/content/browser/android...
content/browser/android/in_process/synchronous_input_event_filter.cc:79: void
SynchronousInputEventFilter::DidStopFlinging(int routing_id) {
On 2014/01/16 18:44:07, jdduke wrote:
> 
> I'm not totally comfortable leaving this as a dead-end.
> 
> Could you extend this to a |SynchronousCompositorImpl::DidStopFlinging()|,
where
> we can call |RenderWidgetHostViewAndroid::DidStopFlinging()|?  You can look at
> the methods here and in SynchronousCompositorImpl to see how that should be
> done, it should be fairly straightforward.

Done.

[jdduke (slow), 2014-01-16 19:19:20]

On 2014/01/16 19:02:24, Xianzhu wrote:
> On 2014/01/16 18:00:08, Xianzhu wrote:
> > On 2014/01/16 13:58:45, mkosiba wrote:
> > > Yes, we always handle the root layer fling on our own. Can we even get a
> > "stop"
> > > in
> > > this case? We don't care for sublayer fling.
> > 
> > Yes, in the case, the client will get onFlingStarted() on the
> GestureFlingStart
> > event and onFlingStopped() on the ACK of the event (as root layer fling is
> > disabled for WebView). Each call happens after the corresponding
> > GestureStateListener call.
> 
> Actually this is true only with the latest Patch Set :)
> 
>
https://codereview.chromium.org/131373004/diff/580001/content/browser/android...
> File content/browser/android/in_process/synchronous_input_event_filter.cc
> (right):
> 
>
https://codereview.chromium.org/131373004/diff/580001/content/browser/android...
> content/browser/android/in_process/synchronous_input_event_filter.cc:79: void
> SynchronousInputEventFilter::DidStopFlinging(int routing_id) {
> On 2014/01/16 18:44:07, jdduke wrote:
> > 
> > I'm not totally comfortable leaving this as a dead-end.
> > 
> > Could you extend this to a |SynchronousCompositorImpl::DidStopFlinging()|,
> where
> > we can call |RenderWidgetHostViewAndroid::DidStopFlinging()|?  You can look
at
> > the methods here and in SynchronousCompositorImpl to see how that should be
> > done, it should be fairly straightforward.
> 
> Done.

content_browser_android/ and content/public/android/ lgtm.

[jdduke (slow), 2014-01-16 19:19:51]

On 2014/01/16 19:19:20, jdduke wrote:
> On 2014/01/16 19:02:24, Xianzhu wrote:
> > On 2014/01/16 18:00:08, Xianzhu wrote:
> > > On 2014/01/16 13:58:45, mkosiba wrote:
> > > > Yes, we always handle the root layer fling on our own. Can we even get a
> > > "stop"
> > > > in
> > > > this case? We don't care for sublayer fling.
> > > 
> > > Yes, in the case, the client will get onFlingStarted() on the
> > GestureFlingStart
> > > event and onFlingStopped() on the ACK of the event (as root layer fling is
> > > disabled for WebView). Each call happens after the corresponding
> > > GestureStateListener call.
> > 
> > Actually this is true only with the latest Patch Set :)
> > 
> >
>
https://codereview.chromium.org/131373004/diff/580001/content/browser/android...
> > File content/browser/android/in_process/synchronous_input_event_filter.cc
> > (right):
> > 
> >
>
https://codereview.chromium.org/131373004/diff/580001/content/browser/android...
> > content/browser/android/in_process/synchronous_input_event_filter.cc:79:
void
> > SynchronousInputEventFilter::DidStopFlinging(int routing_id) {
> > On 2014/01/16 18:44:07, jdduke wrote:
> > > 
> > > I'm not totally comfortable leaving this as a dead-end.
> > > 
> > > Could you extend this to a |SynchronousCompositorImpl::DidStopFlinging()|,
> > where
> > > we can call |RenderWidgetHostViewAndroid::DidStopFlinging()|?  You can
look
> at
> > > the methods here and in SynchronousCompositorImpl to see how that should
be
> > > done, it should be fairly straightforward.
> > 
> > Done.
> 
> content_browser_android/ and content/public/android/ lgtm.

content/browser/android that is...

[Xianzhu, 2014-01-16 20:27:16]

Just found a bug of the previous patch: the top control animation may hang on
start because of a zero scroll_delta. Animate() won't be called as expected
because there is no new frame drawn. Before the patch the issue is hidden
because we send resize on finger up which will causes a redraw and keep the
animation run.

Added SetNeedsRedraw() in AnimateTopControls if TopControlsManager::Animate()
returns a zero scroll delta. PTAL.

[aelias_OOO_until_Jul13, 2014-01-16 20:29:18]

Sounds good, still lgtm.

[Xianzhu, 2014-01-16 22:00:02]

+jln for view_messages.h

[jln (very slow on Chromium), 2014-01-16 22:11:56]

On 2014/01/16 22:00:02, Xianzhu wrote:
> +jln for view_messages.h

content/common/view_messages.h lgtm.

[Xianzhu, 2014-01-17 01:05:18]

+joi for content/port/browser/render_widget_host_view_port.h.

[Jói, 2014-01-17 08:46:28]

//content/port LGTM.

[jochen (gone - plz use gerrit), 2014-01-17 12:28:21]

lgtm

https://codereview.chromium.org/131373004/diff/820028/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_base.h (right):

https://codereview.chromium.org/131373004/diff/820028/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_base.h:88: virtual void
DidStopFlinging() OVERRIDE;
nit. put the empty body here

[Xianzhu, 2014-01-17 17:31:07]

https://codereview.chromium.org/131373004/diff/820028/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_base.h (right):

https://codereview.chromium.org/131373004/diff/820028/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_base.h:88: virtual void
DidStopFlinging() OVERRIDE;
On 2014/01/17 12:28:22, jochen wrote:
> nit. put the empty body here

Done.

[commit-bot: I haz the power, 2014-01-17 17:31:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wangxianzhu@chromium.org/131373004/113...

[commit-bot: I haz the power, 2014-01-17 17:52:58]

Retried try job too often on linux_rel for step(s) base_unittests,
browser_tests, interactive_ui_tests, net_unittests, unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2014-01-17 19:01:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wangxianzhu@chromium.org/131373004/135...

[commit-bot: I haz the power, 2014-01-17 21:31:46]

Change committed as 245624