Update selection handle bitmaps in AW if DIP scale changes

Update selection handle bitmaps in AW if DIP scale changes

In android webview the selection handles are implemented in
PopupTouchHandleDrawable.java

In this CL we make this class an observer that listens to
display changes and keep the current dip scale as a member
instead of retrieving it from ContentViewCore.

We cannot update the bitmaps by the DisplayAndroid signal
(i.e. in onDIPScaleChanged()) because the resources are not
yet updated at the moment. Instead, we update the bitmap in the subsequent
onConfigurationChanged() that should follow the display change.

To make sure that the configuration corresponds to the display change
we check that the display density in resources is equal to the expected
value.

BUG=620929
Committed: https://crrev.com/2d5570dfe70975e2697eeced6402430316572f80
Cr-Commit-Position: refs/heads/master@{#435025}

[Tima Vaisburd, 2016-11-22 03:12:29]

timav@chromium.org changed reviewers:
+ boliu@chromium.org

[Tima Vaisburd, 2016-11-22 03:12:30]

Bo, this is my "on demand" idea to re-fetch the bitmap brought to the state that
it somewhat works.

Unfortunately, it does not work _always_: sometimes only one of two handles is
updated, although the coordinates correspond to the would-be-correct size.

If you have some ideas or if you see that the approach is downright wrong please
let me know, otherwise just ignore.

[boliu, 2016-11-22 18:34:53]

On 2016/11/22 03:12:30, Tima Vaisburd wrote:
> Bo, this is my "on demand" idea to re-fetch the bitmap brought to the state
that
> it somewhat works.
> 
> Unfortunately, it does not work _always_: sometimes only one of two handles is
> updated, although the coordinates correspond to the would-be-correct size.
> 
> If you have some ideas or if you see that the approach is downright wrong
please
> let me know, otherwise just ignore.

the reason it sometimes doesn't work is because onConfigurationChanged could be
called for reasons other than the density change, so if you happen to hit that,
then the drawable won't be updated, so you need check if density has changed,
and then update

[Tima Vaisburd, 2016-11-24 04:37:14]

On 2016/11/22 18:34:53, boliu_back_dec wrote:
> On 2016/11/22 03:12:30, Tima Vaisburd wrote:
> > Bo, this is my "on demand" idea to re-fetch the bitmap brought to the state
> that
> > it somewhat works.
> > 
> > Unfortunately, it does not work _always_: sometimes only one of two handles
is
> > updated, although the coordinates correspond to the would-be-correct size.
> > 
> > If you have some ideas or if you see that the approach is downright wrong
> please
> > let me know, otherwise just ignore.
> 
> the reason it sometimes doesn't work is because onConfigurationChanged could
be
> called for reasons other than the density change, so if you happen to hit
that,
> then the drawable won't be updated, so you need check if density has changed,
> and then update

I do not quite understand that: even if some onConfigurationChanged comes before
onDipScaleChanged(), there should be another one that comes after and it should
pick the flag up.

I found a bug that can explain what I saw: I forgot to call requestLayout().
The code uses getLeft(), getRight() etc. for drawing and positioning PopupWindow
and invalidate() does not change these values.

After I called requestLayout() the code started t work, as it seems.

[boliu, 2016-11-28 15:06:37]

On 2016/11/24 04:37:14, Tima Vaisburd wrote:
> On 2016/11/22 18:34:53, boliu_back_dec wrote:
> > On 2016/11/22 03:12:30, Tima Vaisburd wrote:
> > > Bo, this is my "on demand" idea to re-fetch the bitmap brought to the
state
> > that
> > > it somewhat works.
> > > 
> > > Unfortunately, it does not work _always_: sometimes only one of two
handles
> is
> > > updated, although the coordinates correspond to the would-be-correct size.
> > > 
> > > If you have some ideas or if you see that the approach is downright wrong
> > please
> > > let me know, otherwise just ignore.
> > 
> > the reason it sometimes doesn't work is because onConfigurationChanged could
> be
> > called for reasons other than the density change, so if you happen to hit
> that,
> > then the drawable won't be updated, so you need check if density has
changed,
> > and then update
> 
> I do not quite understand that: even if some onConfigurationChanged comes
before
> onDipScaleChanged(), there should be another one that comes after and it
should
> pick the flag up.

But the first onConfigurationChanged will unset mNeedsUpdateDrawable, so any
subsequent onConfigurationChangeds are just ignored?

I dunno if that's the actual issue. I'm just guessing here.

> 
> I found a bug that can explain what I saw: I forgot to call requestLayout().
> The code uses getLeft(), getRight() etc. for drawing and positioning
PopupWindow
> and invalidate() does not change these values.

huh..why did only one of the handles update before then? odd..

> 
> After I called requestLayout() the code started t work, as it seems.

[Tima Vaisburd, 2016-11-28 18:08:01]

On 2016/11/28 15:06:37, boliu_back_dec wrote:
> But the first onConfigurationChanged will unset mNeedsUpdateDrawable, so any
> subsequent onConfigurationChangeds are just ignored?

Yes, that's what I had in mind: ignored until the next signal from
DisplayObserver.
Maybe it would be better to keep the value of dip scale inside the class and
compare?
It should be more explicit, although repetitive to what DisplayAndroid does.

> > I found a bug that can explain what I saw: I forgot to call requestLayout().
> > The code uses getLeft(), getRight() etc. for drawing and positioning
> PopupWindow
> > and invalidate() does not change these values.
> 
> huh..why did only one of the handles update before then? odd..

I do not know. My guess is that re-layout happens sometimes even without
explicit request.
There is one split screen case (WebView in the top half) where PS1 never worked
and
requestLayout() in PS2 seemed to fix it.

[boliu, 2016-11-28 18:26:42]

On 2016/11/28 18:08:01, Tima Vaisburd wrote:
> On 2016/11/28 15:06:37, boliu_back_dec wrote:
> > But the first onConfigurationChanged will unset mNeedsUpdateDrawable, so any
> > subsequent onConfigurationChangeds are just ignored?
> 
> Yes, that's what I had in mind: ignored until the next signal from
> DisplayObserver.

I meant that's an actual bug. mNeedsUpdateDrawable should not be unset until
onConfigurationChanged for the *density* change has happened

> Maybe it would be better to keep the value of dip scale inside the class and
> compare?
> It should be more explicit, although repetitive to what DisplayAndroid does.
> 
> > > I found a bug that can explain what I saw: I forgot to call
requestLayout().
> > > The code uses getLeft(), getRight() etc. for drawing and positioning
> > PopupWindow
> > > and invalidate() does not change these values.
> > 
> > huh..why did only one of the handles update before then? odd..
> 
> I do not know. My guess is that re-layout happens sometimes even without
> explicit request.
> There is one split screen case (WebView in the top half) where PS1 never
worked
> and
> requestLayout() in PS2 seemed to fix it.

[Tima Vaisburd, 2016-11-28 18:47:19]

On 2016/11/28 18:26:42, boliu_back_dec wrote:
> I meant that's an actual bug. mNeedsUpdateDrawable should not be unset until
> onConfigurationChanged for the *density* change has happened

Maybe I got this. Just to double check, do you say that the code allows for the
following wrong sequence:
1. Android's DisplayManager.DisplayListener detects the display property change.
2. The view receives onConfigurationChanged() about something else. I reload
bitmap but pick up wrong resources 
3. Another onConfigurationChanged() comes with updates resources but is ignored.

I've never seen this (I printed the size of the bitmap), but I guess it's
theoretically possible.

[boliu, 2016-11-28 18:56:57]

On 2016/11/28 18:47:19, Tima Vaisburd wrote:
> On 2016/11/28 18:26:42, boliu_back_dec wrote:
> > I meant that's an actual bug. mNeedsUpdateDrawable should not be unset until
> > onConfigurationChanged for the *density* change has happened
> 
> Maybe I got this. Just to double check, do you say that the code allows for
the
> following wrong sequence:
> 1. Android's DisplayManager.DisplayListener detects the display property
change.
> 2. The view receives onConfigurationChanged() about something else. I reload
> bitmap but pick up wrong resources 
> 3. Another onConfigurationChanged() comes with updates resources but is
ignored.

Yep, that case. Should fix that regardless.

> 
> I've never seen this (I printed the size of the bitmap), but I guess it's
> theoretically possible.

Oh so your issue is not this edge case? Ok..

[Tima Vaisburd, 2016-11-28 19:11:56]

On 2016/11/28 18:56:57, boliu_back_dec wrote:
> Oh so your issue is not this edge case? Ok..

Right now I believe it's the lack of the layout pass.

I got several onDraw()'s, but mDrawable.setBounds() did not reflect the new size
here:
https://cs.chromium.org/chromium/src/android_webview/java/src/org/chromium/an...

and onMeasure() was never called.

[boliu, 2016-11-28 19:15:06]

On 2016/11/28 18:56:57, boliu_back_dec wrote:
> On 2016/11/28 18:47:19, Tima Vaisburd wrote:
> > On 2016/11/28 18:26:42, boliu_back_dec wrote:
> > > I meant that's an actual bug. mNeedsUpdateDrawable should not be unset
until
> > > onConfigurationChanged for the *density* change has happened
> > 
> > Maybe I got this. Just to double check, do you say that the code allows for
> the
> > following wrong sequence:
> > 1. Android's DisplayManager.DisplayListener detects the display property
> change.
> > 2. The view receives onConfigurationChanged() about something else. I reload
> > bitmap but pick up wrong resources 
> > 3. Another onConfigurationChanged() comes with updates resources but is
> ignored.
> 
> Yep, that case. Should fix that regardless.

Fix this and I'll approve

> 
> > 
> > I've never seen this (I printed the size of the bitmap), but I guess it's
> > theoretically possible.
> 
> Oh so your issue is not this edge case? Ok..

[Tima Vaisburd, 2016-11-29 00:43:25]

The CQ bit was checked by timav@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-29 00:44:34]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Tima Vaisburd, 2016-11-29 00:54:22]

Description was changed from

==========
Update selection handles bitmaps in AW if DIP scale changes

In android webview the selection handles are implemented in
PopupTouchHandleDrawable.java

In this CL we make this class an observer that listens to
display changes and keep the current dip scale as a member
intead of retrieving it from ContentViewCore.

We cannot update the bitmaps by the DisplayAndroid signal
(i.e. in onDIPScaleChanged()) because the resources are not
yet updated at the moment. Instead, we postpone the update
till onConfigurationChanged().

BUG=620929
==========

to

==========
Update selection handle bitmaps in AW if DIP scale changes

In android webview the selection handles are implemented in
PopupTouchHandleDrawable.java

In this CL we make this class an observer that listens to
display changes and keep the current dip scale as a member
instead of retrieving it from ContentViewCore.

We cannot update the bitmaps by the DisplayAndroid signal
(i.e. in onDIPScaleChanged()) because the resources are not
yet updated at the moment. Instead, we update the bitmap in the subsequent
onConfigurationChanged() that should follow the display change.

To make sure that the configuration corresponds to the display change
we check that the display density in resources is equal to the expected
value.

BUG=620929
==========

[Tima Vaisburd, 2016-11-29 01:01:23]

On 2016/11/28 19:15:06, boliu_back_dec wrote:
> On 2016/11/28 18:56:57, boliu_back_dec wrote:
> > On 2016/11/28 18:47:19, Tima Vaisburd wrote:
> > > On 2016/11/28 18:26:42, boliu_back_dec wrote:
> > > > I meant that's an actual bug. mNeedsUpdateDrawable should not be unset
> until
> > > > onConfigurationChanged for the *density* change has happened
> > > 
> > > Maybe I got this. Just to double check, do you say that the code allows
for
> > the
> > > following wrong sequence:
> > > 1. Android's DisplayManager.DisplayListener detects the display property
> > change.
> > > 2. The view receives onConfigurationChanged() about something else. I
reload
> > > bitmap but pick up wrong resources 
> > > 3. Another onConfigurationChanged() comes with updates resources but is
> > ignored.
> > 
> > Yep, that case. Should fix that regardless.
> 
> Fix this and I'll approve

Done, PTAL.

[commit-bot: I haz the power, 2016-11-29 02:03:37]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-29 02:03:37]

Dry run: This issue passed the CQ dry run.

[boliu, 2016-11-29 02:26:12]

https://codereview.chromium.org/2515343004/diff/60001/android_webview/java/sr...
File
android_webview/java/src/org/chromium/android_webview/PopupTouchHandleDrawable.java
(right):

https://codereview.chromium.org/2515343004/diff/60001/android_webview/java/sr...
android_webview/java/src/org/chromium/android_webview/PopupTouchHandleDrawable.java:87:
private float mDeviceScale;
set this to in constructor, if the real value is not available, then just set it
to 1, default initialized 0 is definitely not ok

[Tima Vaisburd, 2016-11-29 02:55:59]

https://codereview.chromium.org/2515343004/diff/60001/android_webview/java/sr...
File
android_webview/java/src/org/chromium/android_webview/PopupTouchHandleDrawable.java
(right):

https://codereview.chromium.org/2515343004/diff/60001/android_webview/java/sr...
android_webview/java/src/org/chromium/android_webview/PopupTouchHandleDrawable.java:87:
private float mDeviceScale;
On 2016/11/29 02:26:12, boliu_back_dec wrote:
> set this to in constructor, if the real value is not available, then just set
it
> to 1, default initialized 0 is definitely not ok

I set the value in constructor from contentViewCore.getWindowAndroid(), which
apparently is not null because it was already used directly.

However, the scale is assigned in onAttachedToWindow(), is it not enough?

https://codereview.chromium.org/2515343004/diff/100001/android_webview/java/s...
File
android_webview/java/src/org/chromium/android_webview/PopupTouchHandleDrawable.java
(right):

https://codereview.chromium.org/2515343004/diff/100001/android_webview/java/s...
android_webview/java/src/org/chromium/android_webview/PopupTouchHandleDrawable.java:526:
mDeviceScale = windowAndroid.getDisplay().getDipScale();
Here I just assigned the value directly instead of calling onDIPScaleChanged().

[boliu, 2016-11-29 03:35:43]

https://codereview.chromium.org/2515343004/diff/60001/android_webview/java/sr...
File
android_webview/java/src/org/chromium/android_webview/PopupTouchHandleDrawable.java
(right):

https://codereview.chromium.org/2515343004/diff/60001/android_webview/java/sr...
android_webview/java/src/org/chromium/android_webview/PopupTouchHandleDrawable.java:87:
private float mDeviceScale;
On 2016/11/29 02:55:58, Tima Vaisburd wrote:
> On 2016/11/29 02:26:12, boliu_back_dec wrote:
> > set this to in constructor, if the real value is not available, then just
set
> it
> > to 1, default initialized 0 is definitely not ok
> 
> I set the value in constructor from contentViewCore.getWindowAndroid(), which
> apparently is not null because it was already used directly.
> 
> However, the scale is assigned in onAttachedToWindow(), is it not enough?

You have to guarantee mDeviceScale won't be used by anything before attach, and
verifying that is a lot harder than just be safe

https://codereview.chromium.org/2515343004/diff/100001/android_webview/java/s...
File
android_webview/java/src/org/chromium/android_webview/PopupTouchHandleDrawable.java
(right):

https://codereview.chromium.org/2515343004/diff/100001/android_webview/java/s...
android_webview/java/src/org/chromium/android_webview/PopupTouchHandleDrawable.java:132:
WindowAndroid windowAndroid = mContentViewCore.getWindowAndroid();
need null check for windowAndroid

[Tima Vaisburd, 2016-11-29 17:14:52]

https://codereview.chromium.org/2515343004/diff/100001/android_webview/java/s...
File
android_webview/java/src/org/chromium/android_webview/PopupTouchHandleDrawable.java
(right):

https://codereview.chromium.org/2515343004/diff/100001/android_webview/java/s...
android_webview/java/src/org/chromium/android_webview/PopupTouchHandleDrawable.java:132:
WindowAndroid windowAndroid = mContentViewCore.getWindowAndroid();
On 2016/11/29 03:35:43, boliu_back_dec wrote:
> need null check for windowAndroid

Without windowAndroid we apparently cannot create |mContainer|. Would it be ok
to just return here and leave the object not fully constructed? It seems the
original code assumes that |windowAndroid| can not be null at this point?

[boliu, 2016-11-29 17:22:52]

lgtm

https://codereview.chromium.org/2515343004/diff/100001/android_webview/java/s...
File
android_webview/java/src/org/chromium/android_webview/PopupTouchHandleDrawable.java
(right):

https://codereview.chromium.org/2515343004/diff/100001/android_webview/java/s...
android_webview/java/src/org/chromium/android_webview/PopupTouchHandleDrawable.java:132:
WindowAndroid windowAndroid = mContentViewCore.getWindowAndroid();
On 2016/11/29 17:14:52, Tima Vaisburd wrote:
> On 2016/11/29 03:35:43, boliu_back_dec wrote:
> > need null check for windowAndroid
> 
> Without windowAndroid we apparently cannot create |mContainer|. Would it be ok
> to just return here and leave the object not fully constructed? It seems the
> original code assumes that |windowAndroid| can not be null at this point?

No, the better thing would be to not create the object at all.

But otoh, this file has been moved to webview code, and getWindowAndroid will
never be null. So never mind about the null check

[Tima Vaisburd, 2016-11-29 17:24:47]

The CQ bit was checked by timav@chromium.org

[commit-bot: I haz the power, 2016-11-29 17:25:11]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-29 17:44:17]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1480440287483390,
"parent_rev": "faf2aada0c168f221cd67d3146eca15de3a94796", "commit_rev":
"f0527b8589c4b3c17ef99a0d3ec064ec3bc68c99"}

[commit-bot: I haz the power, 2016-11-29 17:44:50]

Description was changed from

==========
Update selection handle bitmaps in AW if DIP scale changes

In android webview the selection handles are implemented in
PopupTouchHandleDrawable.java

In this CL we make this class an observer that listens to
display changes and keep the current dip scale as a member
instead of retrieving it from ContentViewCore.

We cannot update the bitmaps by the DisplayAndroid signal
(i.e. in onDIPScaleChanged()) because the resources are not
yet updated at the moment. Instead, we update the bitmap in the subsequent
onConfigurationChanged() that should follow the display change.

To make sure that the configuration corresponds to the display change
we check that the display density in resources is equal to the expected
value.

BUG=620929
==========

to

==========
Update selection handle bitmaps in AW if DIP scale changes

In android webview the selection handles are implemented in
PopupTouchHandleDrawable.java

In this CL we make this class an observer that listens to
display changes and keep the current dip scale as a member
instead of retrieving it from ContentViewCore.

We cannot update the bitmaps by the DisplayAndroid signal
(i.e. in onDIPScaleChanged()) because the resources are not
yet updated at the moment. Instead, we update the bitmap in the subsequent
onConfigurationChanged() that should follow the display change.

To make sure that the configuration corresponds to the display change
we check that the display density in resources is equal to the expected
value.

BUG=620929
==========

[commit-bot: I haz the power, 2016-11-29 17:44:51]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-11-29 17:47:05]

Description was changed from

==========
Update selection handle bitmaps in AW if DIP scale changes

In android webview the selection handles are implemented in
PopupTouchHandleDrawable.java

In this CL we make this class an observer that listens to
display changes and keep the current dip scale as a member
instead of retrieving it from ContentViewCore.

We cannot update the bitmaps by the DisplayAndroid signal
(i.e. in onDIPScaleChanged()) because the resources are not
yet updated at the moment. Instead, we update the bitmap in the subsequent
onConfigurationChanged() that should follow the display change.

To make sure that the configuration corresponds to the display change
we check that the display density in resources is equal to the expected
value.

BUG=620929
==========

to

==========
Update selection handle bitmaps in AW if DIP scale changes

In android webview the selection handles are implemented in
PopupTouchHandleDrawable.java

In this CL we make this class an observer that listens to
display changes and keep the current dip scale as a member
instead of retrieving it from ContentViewCore.

We cannot update the bitmaps by the DisplayAndroid signal
(i.e. in onDIPScaleChanged()) because the resources are not
yet updated at the moment. Instead, we update the bitmap in the subsequent
onConfigurationChanged() that should follow the display change.

To make sure that the configuration corresponds to the display change
we check that the display density in resources is equal to the expected
value.

BUG=620929

Committed: https://crrev.com/2d5570dfe70975e2697eeced6402430316572f80
Cr-Commit-Position: refs/heads/master@{#435025}
==========

[commit-bot: I haz the power, 2016-11-29 17:47:09]

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