Expose more gestures to ContentViewCore.GestureStateListeners

Expose more gestures to ContentViewCore.GestureStateListeners

* Let ContentViewCore manage more than one GestureStateListener.
* Change GestureStateListener to be an empty implementation.
* ContentViewCore alerts its GestureStateListeners of any scrolls
  or flings, and alerts it of changes in the vertical scroll offset
  or extent.  That last bit is admittedly a bit strange to put in
  the GestureStateListener, but it's going to be listening to starts
  and stops already anyway...

Downstream CL that uses it: https://chrome-internal-review.googlesource.com/#/c/152640/
BUG=332281
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=247825

[gone, 2014-01-28 19:45:20]

Hey guys, AFAICT this CL doesn't change the current behavior of how the
GestureStateListener works for the things that currently implement the class:
the fling end, and scroll start/stop/update calls are all new, and I don't think
I've changed when or how the GestureStateListener is alerted about fling starts.
 Running things through the unit tests to make sure.

[gone, 2014-01-28 19:46:07]

+bo for android webview

[Ted C, 2014-01-28 21:36:39]

https://chromiumcodereview.appspot.com/145953005/diff/1/content/public/androi...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewClient.java
(right):

https://chromiumcodereview.appspot.com/145953005/diff/1/content/public/androi...
content/public/android/java/src/org/chromium/content/browser/ContentViewClient.java:194:
public void onFlingStopped() {
can we not remove these as well?

https://chromiumcodereview.appspot.com/145953005/diff/1/content/public/androi...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://chromiumcodereview.appspot.com/145953005/diff/1/content/public/androi...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:188:
public static class GestureStateListener {
I would pull this out to a separate class.  Maybe even to content_public!  :-)

https://chromiumcodereview.appspot.com/145953005/diff/1/content/public/androi...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1319:
for (GestureStateListener listener : mGestureStateListeners) {
this foreach approach will result in a new Iterator object everytime you do
this.  Since this can happen a lot for gestures, I think we should either do:

for (int i = 0; i < ...)

Or use the ObserverList from base/ and keep a rewindableIterator reference in
this class and do:

for (mGestureListenersIterator.rewind(); mGestureListenersIterator.hasNext();) {
}

Using this approach might be safer as it would allow callers to remove
themselves in one of the callbacks without messing up the further iterations (or
you would need to iterate backwards using the above).

https://chromiumcodereview.appspot.com/145953005/diff/1/content/public/androi...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1454:
public void addGestureStateListener(GestureStateListener listener) {
Javadocs plz.

[jdduke (slow), 2014-01-28 21:36:45]

https://chromiumcodereview.appspot.com/145953005/diff/1/content/public/androi...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://chromiumcodereview.appspot.com/145953005/diff/1/content/public/androi...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:189:
/**

In general, it might be somewhat confusing (and racy) that the gesture state
listener is notified of gesture inputs (events being sent) as well as gesture
outputs (ack'ed events, or the scroll offset changing). 

I do like the idea of consolidating methods for gesture listening, but I wonder
if we can get away with listening *only* to event acks, rather than both event
acks and sent events? Fling may be the exception here (I'd have to think about
it).

mkosiba@: Would it be sufficient for WebView to listen to acks for
Pinch{Begin,End}?
wangxianzhu@: If we listen to the GestureFlingStart ack, rather than the sent 
GestureFlingStart, would that be enough to suppress the resize?

[Xianzhu, 2014-01-28 21:43:47]

https://chromiumcodereview.appspot.com/145953005/diff/1/content/public/androi...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://chromiumcodereview.appspot.com/145953005/diff/1/content/public/androi...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:189:
/**
On 2014/01/28 21:36:45, jdduke wrote:
> wangxianzhu@: If we listen to the GestureFlingStart ack, rather than the sent 
> GestureFlingStart, would that be enough to suppress the resize?

Would be good in most cases, but if the renderer is slow in returning the ack,
the browser may send resize before the fling actually starts or after the fling
just starts, making the slow condition worse.

[mkosiba (inactive), 2014-01-28 21:49:20]

On 2014/01/28 21:36:45, jdduke wrote:
> mkosiba@: Would it be sufficient for WebView to listen to acks for
> Pinch{Begin,End}?

most likely yes.

[gone, 2014-01-28 22:26:37]

Addressing Ted's comments, still discussing with Jared about what to do for his.

https://chromiumcodereview.appspot.com/145953005/diff/1/content/public/androi...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewClient.java
(right):

https://chromiumcodereview.appspot.com/145953005/diff/1/content/public/androi...
content/public/android/java/src/org/chromium/content/browser/ContentViewClient.java:194:
public void onFlingStopped() {
On 2014/01/28 21:36:40, Ted C wrote:
> can we not remove these as well?

Huh, apparently we can.

https://chromiumcodereview.appspot.com/145953005/diff/1/content/public/androi...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://chromiumcodereview.appspot.com/145953005/diff/1/content/public/androi...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1319:
for (GestureStateListener listener : mGestureStateListeners) {
On 2014/01/28 21:36:40, Ted C wrote:
> this foreach approach will result in a new Iterator object everytime you do
> this.  Since this can happen a lot for gestures, I think we should either do:
> 
> for (int i = 0; i < ...)
> 
> Or use the ObserverList from base/ and keep a rewindableIterator reference in
> this class and do:
> 
> for (mGestureListenersIterator.rewind(); mGestureListenersIterator.hasNext();)
{
> }
> 
> Using this approach might be safer as it would allow callers to remove
> themselves in one of the callbacks without messing up the further iterations
(or
> you would need to iterate backwards using the above).

Done.

https://chromiumcodereview.appspot.com/145953005/diff/1/content/public/androi...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1454:
public void addGestureStateListener(GestureStateListener listener) {
On 2014/01/28 21:36:40, Ted C wrote:
> Javadocs plz.

Done.

[jdduke (slow), 2014-01-28 23:20:10]

I spoke offline with dfalcantara@.  We decided it's probably simplest if this
lands, and I'll then create a follow-up patch to have all listeners only listen
to gesture acks.  For the rest I'll defer to tedchoc@ and boliu@ for owner
review.

[boliu, 2014-01-28 23:56:51]

aw lgtm

https://codereview.chromium.org/145953005/diff/40001/content/public/android/j...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://codereview.chromium.org/145953005/diff/40001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:397:
private final RewindableIterator<GestureStateListener>
mGestureStateListenersIterator;
I looked at 2 usages of ObserverList, below should work, no need for saving the
iterator.

for (GestureStateListener listener : mGestureStateListeners) {
}

[gone, 2014-01-28 23:59:14]

https://codereview.chromium.org/145953005/diff/40001/content/public/android/j...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://codereview.chromium.org/145953005/diff/40001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:397:
private final RewindableIterator<GestureStateListener>
mGestureStateListenersIterator;
On 2014/01/28 23:56:51, boliu wrote:
> I looked at 2 usages of ObserverList, below should work, no need for saving
the
> iterator.
> 
> for (GestureStateListener listener : mGestureStateListeners) {
> }

I had it this way in the earlier patch sets, but Ted suggested using the
iterator instead.  No preference on my end.

[boliu, 2014-01-29 00:00:57]

https://codereview.chromium.org/145953005/diff/40001/content/public/android/j...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://codereview.chromium.org/145953005/diff/40001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:397:
private final RewindableIterator<GestureStateListener>
mGestureStateListenersIterator;
On 2014/01/28 23:59:14, dfalcantara wrote:
> On 2014/01/28 23:56:51, boliu wrote:
> > I looked at 2 usages of ObserverList, below should work, no need for saving
> the
> > iterator.
> > 
> > for (GestureStateListener listener : mGestureStateListeners) {
> > }
> 
> I had it this way in the earlier patch sets, but Ted suggested using the
> iterator instead.  No preference on my end.

Oh, need a comment then :)

[gone, 2014-01-29 00:04:16]

https://codereview.chromium.org/145953005/diff/40001/content/public/android/j...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://codereview.chromium.org/145953005/diff/40001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:397:
private final RewindableIterator<GestureStateListener>
mGestureStateListenersIterator;
On 2014/01/29 00:00:58, boliu wrote:
> On 2014/01/28 23:59:14, dfalcantara wrote:
> > On 2014/01/28 23:56:51, boliu wrote:
> > > I looked at 2 usages of ObserverList, below should work, no need for
saving
> > the
> > > iterator.
> > > 
> > > for (GestureStateListener listener : mGestureStateListeners) {
> > > }
> > 
> > I had it this way in the earlier patch sets, but Ted suggested using the
> > iterator instead.  No preference on my end.
> 
> Oh, need a comment then :)

I am wondering, however, if it's possible for the iterator to be used on two
different threads.  Ted: ideas?

[jdduke (slow), 2014-01-29 00:08:10]

On 2014/01/29 00:04:16, dfalcantara wrote:
>
https://codereview.chromium.org/145953005/diff/40001/content/public/android/j...
> File
>
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
> (right):
> 
>
https://codereview.chromium.org/145953005/diff/40001/content/public/android/j...
>
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:397:
> private final RewindableIterator<GestureStateListener>
> mGestureStateListenersIterator;
> On 2014/01/29 00:00:58, boliu wrote:
> > On 2014/01/28 23:59:14, dfalcantara wrote:
> > > On 2014/01/28 23:56:51, boliu wrote:
> > > > I looked at 2 usages of ObserverList, below should work, no need for
> saving
> > > the
> > > > iterator.
> > > > 
> > > > for (GestureStateListener listener : mGestureStateListeners) {
> > > > }
> > > 
> > > I had it this way in the earlier patch sets, but Ted suggested using the
> > > iterator instead.  No preference on my end.
> > 
> > Oh, need a comment then :)
> 
> I am wondering, however, if it's possible for the iterator to be used on two
> different threads.  Ted: ideas?

Hmm, if anything other than the main browser thread is hitting these methods,
something's horribly wrong.

[gone, 2014-01-29 00:20:17]

Xianzhu: the main (only?) difference in behavior from the previous patch is
removing where ContentViewClient.onFlingStarted() is called in
ContentViewCore.java:1382.  It is replaced by the updateGestureStateListener()
call, which alerts the listener through onFlingStartGesture().  The
corresponding patch in
https://chrome-internal-review.googlesource.com/#/c/153076/ shows that the Tab
is still alerted, but I'm not sure what effect it would have to do it this way
instead.  Do you think it'll be fine?

[Xianzhu, 2014-01-29 00:29:21]

On 2014/01/29 00:20:17, dfalcantara wrote:
> Xianzhu: the main (only?) difference in behavior from the previous patch is
> removing where ContentViewClient.onFlingStarted() is called in
> ContentViewCore.java:1382.  It is replaced by the updateGestureStateListener()
> call, which alerts the listener through onFlingStartGesture().  The
> corresponding patch in
> https://chrome-internal-review.googlesource.com/#/c/153076/ shows that the Tab
> is still alerted, but I'm not sure what effect it would have to do it this way
> instead.  Do you think it'll be fine?

lgtm.

[Ted C, 2014-01-29 00:50:40]

On 2014/01/29 00:29:21, Xianzhu wrote:
> On 2014/01/29 00:20:17, dfalcantara wrote:
> > Xianzhu: the main (only?) difference in behavior from the previous patch is
> > removing where ContentViewClient.onFlingStarted() is called in
> > ContentViewCore.java:1382.  It is replaced by the
updateGestureStateListener()
> > call, which alerts the listener through onFlingStartGesture().  The
> > corresponding patch in
> > https://chrome-internal-review.googlesource.com/#/c/153076/ shows that the
Tab
> > is still alerted, but I'm not sure what effect it would have to do it this
way
> > instead.  Do you think it'll be fine?
> 
> lgtm.

lgtm

[mkosiba (inactive), 2014-01-29 18:07:48]

https://codereview.chromium.org/145953005/diff/1/android_webview/java/src/org...
File android_webview/java/src/org/chromium/android_webview/AwContents.java
(right):

https://codereview.chromium.org/145953005/diff/1/android_webview/java/src/org...
android_webview/java/src/org/chromium/android_webview/AwContents.java:529:
contentViewCore.addGestureStateListener(pinchGestureStateListener);
nit: s/pinchGestureStateListener/gestureStateListener/

[gone, 2014-01-29 21:51:59]

https://codereview.chromium.org/145953005/diff/1/android_webview/java/src/org...
File android_webview/java/src/org/chromium/android_webview/AwContents.java
(right):

https://codereview.chromium.org/145953005/diff/1/android_webview/java/src/org...
android_webview/java/src/org/chromium/android_webview/AwContents.java:529:
contentViewCore.addGestureStateListener(pinchGestureStateListener);
On 2014/01/29 18:07:49, mkosiba wrote:
> nit: s/pinchGestureStateListener/gestureStateListener/

Done.

[commit-bot: I haz the power, 2014-01-29 21:56:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dfalcantara@chromium.org/145953005/100001

[commit-bot: I haz the power, 2014-01-30 03:04:23]

CQ bit was unchecked on CL. Ignoring.

[commit-bot: I haz the power, 2014-01-30 03:11:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dfalcantara@chromium.org/145953005/100001

[commit-bot: I haz the power, 2014-01-30 04:02:11]

Change committed as 247825