[Android] Allow text handles to observe position of "parent" view
This adds the class PositionObserver. This class provides a way to
listen for changes to the position of a particular view (and to query
that views current position).
Then, HandleView is updated to use a PositionObserver to keep its
PopupWindow's position up-to-date with the logical "parent" view (
HandleView is logically a child of some other view, but has to use
a PopupWindow outside of the logical parent's view hierarchy).
This is similar to, and influenced by, Android's Editor.java
(https://android.googlesource.com/platform/frameworks/base/+/android-4.3_r3.1/core/java/android/widget/Editor.java).
BUG=b/10562149
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=228594
Added tedchoc@ since he's a bit familiar with this code. "How does this stuff get ...
7 years, 2 months ago
(2013-10-04 18:07:29 UTC)
#3
Added tedchoc@ since he's a bit familiar with this code.
"How does this stuff get tested?"
Mostly, it doesn't. There are some tests in
SelectionHandleTest.java/InertionHandleTest.java but they are far from
comprehensive and many have been disabled forever as they are flaky.
I've long wanted to do something about that, but I've been told not too spend
too much time on these since the plan is to throw all this code out and do text
handles in the renderer.
I actually have no idea how I would have tested this new added behavior without
a big refactoring (maybe some kind of android_webview test could have done it).
So I've done a bit of refactoring to make the selection handles and insertion
handles testable without relying on a ContentView and all the other things that
go with it. I've then added a short test for this particular new behavior.
https://codereview.chromium.org/24449007/diff/8001/base/android/java/src/org/...
File base/android/java/src/org/chromium/base/PositionObserver.java (right):
https://codereview.chromium.org/24449007/diff/8001/base/android/java/src/org/...
base/android/java/src/org/chromium/base/PositionObserver.java:1: // Copyright
(c) 2012 The Chromium Authors. All rights reserved.
On 2013/09/27 22:23:20, joth wrote:
> this yr
Done.
https://codereview.chromium.org/24449007/diff/8001/base/android/java/src/org/...
base/android/java/src/org/chromium/base/PositionObserver.java:5: package
org.chromium.base;
On 2013/09/27 22:23:20, joth wrote:
> This is only needed in content/browser so I don't think it meets the bar for
> needing to go in base. (ui/ maybe, if there were other users for it)
Done.
https://codereview.chromium.org/24449007/diff/8001/base/android/java/src/org/...
base/android/java/src/org/chromium/base/PositionObserver.java:54: // The stored
position may be out-of-date. Get the real current position.
On 2013/09/27 22:23:20, joth wrote:
> the position will be correct during a frame draw, and will always correspond
to
> the most recent frame between frames.
> Is that not good enough?
The preDrawListener is only registered on the view tree if there is a listener
on this observer. So, the stored position can become stale while nobody is
listening for changes.
This comes up when showing the text handles. They check if the selection point
is actually visible on-screen before being shown and so they need to get the
current position.
https://codereview.chromium.org/24449007/diff/8001/base/android/java/src/org/...
base/android/java/src/org/chromium/base/PositionObserver.java:98: int[] position
= new int[2];
On 2013/09/27 22:23:20, joth wrote:
> this will cause garbage allocations on every frame drawn. I think you can use
a
> long lived int[2] and pass into getLocationInWindow() to avoid this
Done.
cjhopman
Also, this should probably be two changes so feel free to suggest that.
7 years, 2 months ago
(2013-10-04 18:08:45 UTC)
#4
Also, this should probably be two changes so feel free to suggest that.
joth
On 2013/10/04 18:08:45, cjhopman wrote: > Also, this should probably be two changes so feel ...
7 years, 2 months ago
(2013-10-08 20:17:31 UTC)
#5
On 2013/10/04 18:08:45, cjhopman wrote:
> Also, this should probably be two changes so feel free to suggest that.
Yes please - it would be better to have the bug fix and a followup to refactor
and add the test, as it will keep the former more cherry-pickable.
@tedchoc did you have any comments? or wait for the CL to be split first.
Thanks
Ted C
I'm fine splitting it up, but here are my comments on this (if this needs ...
7 years, 2 months ago
(2013-10-10 18:16:32 UTC)
#6
Removed the testability refactoring and addressed those comments still relevant. Also, I think it compiles ...
7 years, 2 months ago
(2013-10-10 21:35:23 UTC)
#7
Removed the testability refactoring and addressed those comments still relevant.
Also, I think it compiles this time.
https://codereview.chromium.org/24449007/diff/22001/content/public/android/ja...
File
content/public/android/java/src/org/chromium/content/browser/PositionObserver.java
(right):
https://codereview.chromium.org/24449007/diff/22001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/PositionObserver.java:15:
public class PositionObserver implements PositionObserverInterface {
On 2013/10/10 18:16:32, Ted C wrote:
> As for naming, I would call this ViewPositionObserver and keep the interface
as
> PositionObserver
I like that. Done.
https://codereview.chromium.org/24449007/diff/22001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/PositionObserver.java:18:
private int mPositionX, mPositionY;
On 2013/10/10 18:16:32, Ted C wrote:
> why do we need to keep position x and y? can't we always use mTempPos?
Done.
https://codereview.chromium.org/24449007/diff/22001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/PositionObserver.java:22:
private ArrayList<Listener> mListeners;
On 2013/10/10 18:16:32, Ted C wrote:
> make this final to ensure it can't ever be set to null.
Done.
https://codereview.chromium.org/24449007/diff/22001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/PositionObserver.java:36:
for (Listener l : mListeners) {
On 2013/10/10 18:16:32, Ted C wrote:
> this will allocate an Iterator on each predraw, which is probably bad. We
> should use the for (int i = 0; blah blah) version instead.
Done.
https://codereview.chromium.org/24449007/diff/22001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/PositionObserver.java:67:
if (mListeners.contains(listener)) {
On 2013/10/10 18:16:32, Ted C wrote:
> the statement and condition all fit on one line, no need for braces
Done.
https://codereview.chromium.org/24449007/diff/22001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/PositionObserver.java:99:
updateTempPosition();
On 2013/10/10 18:16:32, Ted C wrote:
> 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.
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.
https://codereview.chromium.org/24449007/diff/22001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/PositionObserver.java:101:
mPositionX = position[0];
On 2013/10/10 18:16:32, Ted C wrote:
> hmm...position doesn't seem to exist anywhere (literally)
Yeah, this must have been confusing.
https://codereview.chromium.org/24449007/diff/22001/content/public/android/ja...
File
content/public/android/java/src/org/chromium/content/browser/PositionObserverInterface.java
(right):
https://codereview.chromium.org/24449007/diff/22001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/PositionObserverInterface.java:10:
On 2013/10/10 18:16:32, Ted C wrote:
> remove extra blank line
Done.
Ted C
lgtm w/ nits https://chromiumcodereview.appspot.com/24449007/diff/64001/content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://chromiumcodereview.appspot.com/24449007/diff/64001/content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java#newcode2007 content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2007: mPositionObserver) { these params now fit ...
7 years, 2 months ago
(2013-10-14 23:46:44 UTC)
#8
Issue 24449007: [Android] Allow text handles to observe position of "parent" view
(Closed)
Created 7 years, 2 months ago by cjhopman
Modified 7 years, 2 months ago
Reviewers: joth, Ted C
Base URL: svn://svn.chromium.org/chrome/trunk/src
Comments: 34
Chromium Code Reviews
Issue
24449007:
[Android] Allow text handles to observe position of "parent" view (Closed)
