[Android] Allow text handles to observe position of "parent" view

content/public/android/java/src/org/chromium/content/browser/PositionObserver.java

+// Copyright 2013 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+package org.chromium.content.browser;
+
+import android.view.View;
+import android.view.ViewTreeObserver;
+
+import java.util.ArrayList;
+
+/**
+ * Used to register listeners that can be notified of changes to the position of a view.
+ */
+public class PositionObserver implements PositionObserverInterface {

[Ted C, 2013/10/10 18:16:32]

As for naming, I would call this ViewPositionObserver and keep the interface as
PositionObserver

[cjhopman, 2013/10/10 21:35:23]

I like that. Done.

+    private View mView;
+    // Absolute position of the container view relative to its parent window.
+    private int mPositionX, mPositionY;

[Ted C, 2013/10/10 18:16:32]

why do we need to keep position x and y?  can't we always use mTempPos?

[cjhopman, 2013/10/10 21:35:23]

Done.

+
+    private final int[] mTempPos = new int[2];
+
+    private ArrayList<Listener> mListeners;

[Ted C, 2013/10/10 18:16:32]

make this final to ensure it can't ever be set to null.

[cjhopman, 2013/10/10 21:35:23]

Done.

+    private ViewTreeObserver.OnPreDrawListener mPreDrawListener;
+
+    /**
+     * @param view The view to observe.
+     */
+    public PositionObserver(View view) {
+        mView = view;
+        mListeners = new ArrayList<Listener>();
+        updatePosition();
+        mPreDrawListener = new ViewTreeObserver.OnPreDrawListener() {
+            @Override
+            public boolean onPreDraw() {
+                if (updatePosition()) {
+                    for (Listener l : mListeners) {

[Ted C, 2013/10/10 18:16:32]

this will allocate an Iterator on each predraw, which is probably bad.  We
should use the for (int i = 0; blah blah) version instead.

[cjhopman, 2013/10/10 21:35:23]

Done.

+                        l.onPositionChanged(mPositionX, mPositionY);
+                    }
+                }
+                return true;
+            }
+        };
+    }
+
+    /**
+     * @return The current x position of the observed view.
+     */
+    public int getPositionX() {
+        // The stored position may be out-of-date. Get the real current position.
+        updateTempPosition();
+        return mTempPos[0];
+    }
+
+    /**
+     * @return The current y position of the observed view.
+     */
+    public int getPositionY() {
+        // The stored position may be out-of-date. Get the real current position.
+        updateTempPosition();
+        return mTempPos[1];
+    }
+
+    /**
+     * Register a listener to be called when the position of the underlying view changes.
+     */
+    public void addListener(Listener listener) {
+        if (mListeners.contains(listener)) {

[Ted C, 2013/10/10 18:16:32]

the statement and condition all fit on one line, no need for braces

[cjhopman, 2013/10/10 21:35:23]

Done.

+            return;
+        }
+
+        if (mListeners.isEmpty()) {
+            mView.getViewTreeObserver().addOnPreDrawListener(mPreDrawListener);
+            updatePosition();
+        }
+
+        mListeners.add(listener);
+    }
+
+    /**
+     * Remove a previously installed listener.
+     */
+    public void removeListener(Listener listener) {
+        if (!mListeners.contains(listener)) {
+            return;
+        }
+
+        mListeners.remove(listener);
+
+        if (mListeners.isEmpty()) {
+            mView.getViewTreeObserver().removeOnPreDrawListener(mPreDrawListener);
+        }
+    }
+
+    private void updateTempPosition() {
+        mView.getLocationInWindow(mTempPos);
+    }
+
+    private boolean updatePosition() {
+        updateTempPosition();

[Ted C, 2013/10/10 18:16:32]

if we got rid of mPositionX and Y can we just keep the previous x and y values
before calling updateTempPosition and return whether they have changed?

Also, we could probably just get rid of updateTempPosition entirely and have
everyone call this and the interested parties could just the return value.

[cjhopman, 2013/10/10 21:35:23]

So, the issue is that if there is a listener, we want to tell it that the
position has changed. If getPositionX() actually updates the stored position
then on preDraw we might not properly notify that listener.

There are several options:
1. Have getPositionX() not update the stored position
2. getPositionx() updates the stored position and notifies listeners if it
changed
3. getPositionX() stores the updated position and we use some other way of
knowing when to notify listeners during preDraw (a flag, or storing the last
position the listeners were notified of).
4. notify the listeners every frame even if the position hasn't changed.

I've changed this to do (2). The listeners shouldn't really care if they get
notified before preDraw, or if they get multiple notifications.

+        boolean positionChanged = mPositionX != mTempPos[0] || mPositionY != mTempPos[1];
+        mPositionX = position[0];

[Ted C, 2013/10/10 18:16:32]

hmm...position doesn't seem to exist anywhere (literally)

[cjhopman, 2013/10/10 21:35:23]

Yeah, this must have been confusing.

+        mPositionY = position[1];
+        return positionChanged;
+    }
+}
+