Hide PopupZoomer when the container view or window loses focus

Hide PopupZoomer when the container view or window loses focus

Currently there is a method which hides all the popups but call to 
hide PopupZoomer is missing. Added the call to hide PopupZoomer inside 
hidePopups and hiding the zoomer when the view loses focus or the 
window loses focus. Also added unit test code.

BUG=405477
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=291359

[raghu, 2014-08-20 15:31:02]

Hi Jdduke,
Can you please review my patch. 
Please let me know if there is some issue with the test code. I have changed the
PopupZoomerTest class to extend ContentShellTestBase instead of
InstrumentationTestCase, but I am not sure that is the right thing to do.

[jdduke (slow), 2014-08-20 15:47:47]

This seems reasonable, but I'm not sure about the test change. Adding aruslan@
(per git blame :) for any additional thoughts.

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

https://codereview.chromium.org/470193005/diff/20001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:524:
if (mPopupZoomer.isShowing()) {
Can you also just make this |mPopupZoomer.hide(true)| and remove the
conditional? The showing condition is checked in the |hide()| method so this is
just extra work/noise.

https://codereview.chromium.org/470193005/diff/20001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1652:
hidePopups();
Will a select popup and/or action bar action cause the focus to change? This
change doesn't seem completely necessary to me, unless you're seeing that the
zoomer continues to steal focus after the focus change event.

[raghu, 2014-08-21 06:23:59]

Adding aruslan@
PTAL

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

https://codereview.chromium.org/470193005/diff/20001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:524:
if (mPopupZoomer.isShowing()) {
On 2014/08/20 15:47:46, jdduke wrote:
> Can you also just make this |mPopupZoomer.hide(true)| and remove the
> conditional? The showing condition is checked in the |hide()| method so this
is
> just extra work/noise.

Done. I have done it in |onKeyUp| also.

Also, I feel, we should swap the order of |getContentViewClient().onImeEvent()|
and |hideTextHandles()| calls in this method. The client should be notified
after we are done with handling the ImeEvent. Please let me know your opinion
about it.

https://codereview.chromium.org/470193005/diff/20001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1652:
hidePopups();
On 2014/08/20 15:47:46, jdduke wrote:
> Will a select popup and/or action bar action cause the focus to change? This
> change doesn't seem completely necessary to me, unless you're seeing that the
> zoomer continues to steal focus after the focus change event.

No, they are not causing the focus to change. But anyways I have now changed it
to just hide the zoomer

[jdduke (slow), 2014-08-21 14:46:42]

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

https://codereview.chromium.org/470193005/diff/60001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1645:
mPopupZoomer.hide(false);
When is this method called (without onHide also being called)?

https://codereview.chromium.org/470193005/diff/60001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1655:
mPopupZoomer.hide(false);
Just curious, is this a source of bugs? Is there harm in keeping it visible if
we lose focus (i.e., does it conflict with other behavior?)?

https://codereview.chromium.org/470193005/diff/60001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1664:
if (keyCode == KeyEvent.KEYCODE_BACK) {
Actually, this isn't quite right, as now we'll never forward BACK key events,
when we really only want to suppress them if it's visible. You should restore
the original code here.

[raghu, 2014-08-21 16:12:32]

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

https://codereview.chromium.org/470193005/diff/60001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1645:
mPopupZoomer.hide(false);
On 2014/08/21 14:46:41, jdduke wrote:
> When is this method called (without onHide also being called)?

Acknowledged. Its not necessary after hiding the zoomer in |hidePopups|

https://codereview.chromium.org/470193005/diff/60001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1655:
mPopupZoomer.hide(false);
On 2014/08/21 14:46:41, jdduke wrote:
> Just curious, is this a source of bugs? Is there harm in keeping it visible if
> we lose focus (i.e., does it conflict with other behavior?)?

I didnt see any harm in keeping it visible, but I think the zoomer should hide
when the user is doing something else.

https://codereview.chromium.org/470193005/diff/60001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1664:
if (keyCode == KeyEvent.KEYCODE_BACK) {
On 2014/08/21 14:46:41, jdduke wrote:
> Actually, this isn't quite right, as now we'll never forward BACK key events,
> when we really only want to suppress them if it's visible. You should restore
> the original code here.

Done. It was a mistake to overlook the return statement.

[jdduke (slow), 2014-08-21 16:16:29]

lgtm

[raghu, 2014-08-21 18:43:39]

On 2014/08/21 16:16:29, jdduke wrote:
> lgtm

Thanks.
How does the test code look? Should I wait for aruslan@ to get back or is it
fine to land the patch?

[jdduke (slow), 2014-08-21 18:47:09]

On 2014/08/21 18:43:39, rvg wrote:
> On 2014/08/21 16:16:29, jdduke wrote:
> > lgtm
> 
> Thanks.
> How does the test code look? Should I wait for aruslan@ to get back or is it
> fine to land the patch?

I think the change looks fine but let's wait to confirm with aruslan@.

[aruslan, 2014-08-21 19:07:01]

Yes, the patch looks fine. I had only one comment about incorrect OnKeyUp stuff,
but that's already fixed, so lgtm, thanks!

[raghu, 2014-08-22 03:10:47]

The CQ bit was checked by r.ghatage@samsung.com

[commit-bot: I haz the power, 2014-08-22 03:12:19]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/r.ghatage@samsung.com/470193005/80001

[commit-bot: I haz the power, 2014-08-22 04:07:42]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_chromium_gn_compile_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)

[commit-bot: I haz the power, 2014-08-22 04:10:33]

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

[commit-bot: I haz the power, 2014-08-22 04:10:33]

Try jobs failed on following builders:
  android_chromium_gn_compile_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)

[raghu, 2014-08-22 05:29:54]

The CQ bit was checked by r.ghatage@samsung.com

[commit-bot: I haz the power, 2014-08-22 05:31:45]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/r.ghatage@samsung.com/470193005/80001

[commit-bot: I haz the power, 2014-08-22 07:43:35]

Committed patchset #5 (80001) as 291359