aw: Ensure synchronization of PopupWindow handle visibility/position

aw: Ensure synchronization of PopupWindow handle visibility/position

Setting the visibility and position of a PopupWindow does not guarantee
that the new visibility/position will take effect in the same frame.
This leads to cases where the PopupWindow touch handles can appear out
of position for a single frame.

Prevent this by delaying visibility updates by one frame when the handles
transition from hidden to visible.

Also avoid issues where Android rejects one-dimensional selection
rect regions, as well as issues where selection bounds queried from
inside the selection event callback were stale.

BUG=490800
Committed: https://crrev.com/b3446ff33425d6a1944087fdb342cdc8959e7d99
Cr-Commit-Position: refs/heads/master@{#331703}

[jdduke (slow), 2015-05-21 19:23:01]

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

[jdduke (slow), 2015-05-21 19:23:01]

+sievers, FYI I pulled this part out from
https://codereview.chromium.org/1087893003/ so we can cherry-pick to M44.

[no sievers, 2015-05-21 21:16:17]

sievers@chromium.org changed reviewers:
+ tedchoc@chromium.org

[no sievers, 2015-05-21 21:17:12]

+Ted to see if he has any idea why this wouldn't be in the right position the
moment you make it visible.

FWIU this code is used in Webview only.

[jdduke (slow), 2015-05-21 21:19:28]

On 2015/05/21 21:17:12, sievers wrote:
> +Ted to see if he has any idea why this wouldn't be in the right position the
> moment you make it visible.
> 
> FWIU this code is used in Webview only.

Yeah, there's basically one frame where the position/visibility are not in sync.
I had hoped that by doing this in the animation frame callback we'd get some
kind of synchronization, but PopupWindows appear to be special little
snowflakes.

[jdduke (slow), 2015-05-27 18:06:51]

On 2015/05/21 21:19:28, jdduke wrote:
> On 2015/05/21 21:17:12, sievers wrote:
> > +Ted to see if he has any idea why this wouldn't be in the right position
the
> > moment you make it visible.
> > 
> > FWIU this code is used in Webview only.
> 
> Yeah, there's basically one frame where the position/visibility are not in
sync.
> I had hoped that by doing this in the animation frame callback we'd get some
> kind of synchronization, but PopupWindows appear to be special little
> snowflakes.

Ted any ideas here?

[jdduke (slow), 2015-05-27 19:39:11]

Patchset #3 (id:40001) has been deleted

[jdduke (slow), 2015-05-27 19:49:41]

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

[jdduke (slow), 2015-05-27 19:49:42]

Per offline discussion, there's a much better solution for the stale visibility
(a little embarrassing honestly =/). Ted could you take a look at the
PopupTouchHandleDrawable change?

+mohsen: Could you take a look at the TouchSelectionController changes? I've
modified it so we get the |TSCClient::OnSelectionEvent| call for new/updated
selections only at the *end* of the selection update, ensuring any public data
exposed by the TSC is up-to-date within the scope of the callback.

[Ted C, 2015-05-27 20:02:03]

lgtm

https://codereview.chromium.org/1129193007/diff/80001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/input/PopupTouchHandleDrawable.java
(right):

https://codereview.chromium.org/1129193007/diff/80001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/input/PopupTouchHandleDrawable.java:231:
updatePosition();
Should we add an assert in updatePosition that ensures it is visible to make
this easier to diagnose in the future?

[jdduke (slow), 2015-05-27 20:05:56]

https://codereview.chromium.org/1129193007/diff/80001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/input/PopupTouchHandleDrawable.java
(right):

https://codereview.chromium.org/1129193007/diff/80001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/input/PopupTouchHandleDrawable.java:231:
updatePosition();
On 2015/05/27 20:02:03, Ted C wrote:
> Should we add an assert in updatePosition that ensures it is visible to make
> this easier to diagnose in the future?

Hmm, we also use this method when we transition from visible -> invisible, in
which case the position update is benign. I guess we could try to make this
method only apply for the case where the handle is visible?

[jdduke (slow), 2015-05-27 20:07:18]

https://codereview.chromium.org/1129193007/diff/80001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/input/PopupTouchHandleDrawable.java
(right):

https://codereview.chromium.org/1129193007/diff/80001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/input/PopupTouchHandleDrawable.java:231:
updatePosition();
On 2015/05/27 20:05:56, jdduke wrote:
> On 2015/05/27 20:02:03, Ted C wrote:
> > Should we add an assert in updatePosition that ensures it is visible to make
> > this easier to diagnose in the future?
> 
> Hmm, we also use this method when we transition from visible -> invisible, in
> which case the position update is benign. I guess we could try to make this
> method only apply for the case where the handle is visible?

Or I could just conditionally update the position so it's clear from *this* code
when we'll actually get a position update? Maybe I'll go ahead and do that.

[mohsen, 2015-05-27 20:13:14]

TouchSelectionController changes LGTM.

[Ted C, 2015-05-27 20:14:20]

On 2015/05/27 20:07:18, jdduke wrote:
>
https://codereview.chromium.org/1129193007/diff/80001/content/public/android/...
> File
>
content/public/android/java/src/org/chromium/content/browser/input/PopupTouchHandleDrawable.java
> (right):
> 
>
https://codereview.chromium.org/1129193007/diff/80001/content/public/android/...
>
content/public/android/java/src/org/chromium/content/browser/input/PopupTouchHandleDrawable.java:231:
> updatePosition();
> On 2015/05/27 20:05:56, jdduke wrote:
> > On 2015/05/27 20:02:03, Ted C wrote:
> > > Should we add an assert in updatePosition that ensures it is visible to
make
> > > this easier to diagnose in the future?
> > 
> > Hmm, we also use this method when we transition from visible -> invisible,
in
> > which case the position update is benign. I guess we could try to make this
> > method only apply for the case where the handle is visible?
> 
> Or I could just conditionally update the position so it's clear from *this*
code
> when we'll actually get a position update? Maybe I'll go ahead and do that.

We could also rename the method updatePositionIfVisible to make it easier to
see.

[jdduke (slow), 2015-05-27 20:30:40]

https://codereview.chromium.org/1129193007/diff/60002/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/input/PopupTouchHandleDrawable.java
(right):

https://codereview.chromium.org/1129193007/diff/60002/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/input/PopupTouchHandleDrawable.java:229:
// updates until it has been marked visible.
I did some more testing, turns out this doesn't actually work :( There's got to
be a simpler way to fix this though...

[jdduke (slow), 2015-05-27 21:21:39]

OK, per offline discussion Ted I've gone back to a modified version of the
original wokaround. The new version is a good deal more self-contained, though,
and works great in testing.

[Ted C, 2015-05-27 21:37:18]

lgtm

[jdduke (slow), 2015-05-27 22:08:19]

The CQ bit was checked by jdduke@chromium.org

[jdduke (slow), 2015-05-27 22:08:20]

The patchset sent to the CQ was uploaded after l-g-t-m from mohsen@chromium.org
Link to the patchset: https://codereview.chromium.org/1129193007/#ps110001
(title: "Slightly simpler visibility fix")

[commit-bot: I haz the power, 2015-05-27 22:10:32]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1129193007/110001

[commit-bot: I haz the power, 2015-05-28 00:22:07]

Committed patchset #6 (id:110001)

[commit-bot: I haz the power, 2015-05-28 00:23:51]

Patchset 6 (id:??) landed as
https://crrev.com/b3446ff33425d6a1944087fdb342cdc8959e7d99
Cr-Commit-Position: refs/heads/master@{#331703}