Prevent Contextual Search from breaking InfoBarContainer visibility

Prevent bottom-of-the-screen Views from breaking InfoBarContainer visibility

* SurfaceViews don't play well with Views on top of them toggling between
  View.VISIBLE and View.INVISIBLE.  Fix this by using View.GONE instead.

* Make logic for hiding the InfoBarContainer when other bottom-of-the-screen
  Views try to take over the screen out of SwipableOverlayView and
  ContextualSearchManager into the InfoBarContainer itself.

* Move logic for hiding the InfoBarContainer when the keyboard is showing
  from SwipableOverlayView#onLayout into InfoBarContainer and make it so
  that the InfoBarContainer listens for global layout changes.

BUG=563947
Committed: https://crrev.com/e0906ef875c59ab9355e44e506b1e08f223844f2
Cr-Commit-Position: refs/heads/master@{#371094}

[gone, 2016-01-21 01:23:46]

dfalcantara@chromium.org changed reviewers:
+ newt@chromium.org

[newt (away), 2016-01-21 05:27:51]

https://codereview.chromium.org/1613513004/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/banners/SwipableOverlayView.java
(right):

https://codereview.chromium.org/1613513004/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/banners/SwipableOverlayView.java:424:
protected boolean isAllowedToAutoHide() {
will things break if the value of isAllowedToAutoHide() changes dynamically?

An alternative solution would be to reset the y-translation to 0 when the view's
visibility is set back to visible, in onVisibilityChanged().

Still another alternative would be to use removeFromParentView() instead of
setting this view to invisible. That seems even better since it means we won't
be wasting CPU cycles moving an invisible InfoBarContainer around. And this
reduces the number of different ways that we hide the InfoBarContainer, which is
a Good Thing.

https://codereview.chromium.org/1613513004/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java
(right):

https://codereview.chromium.org/1613513004/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java:357:
container.setDoStayInvisible(false);
Letting ContextualSearchManager muck around with InfoBarContainer's state so
much frightens me. Let's move this extra logic inside the
InfoBarContainer.setDoStayInvisible() method. We should also consider renaming
setDoStayInvisible()... maybe setObscuredByContextualSearch() or
setObscuredByBottomBar().

Longer term, of course, we should get rid of this setDoStayInvisible() nonsense
and create an object to manage the visibility of the several bottom views.

https://codereview.chromium.org/1613513004/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java:433:
container.requestLayout();
why do we need to request a layout after hiding the InfoBarContainer?

[gone, 2016-01-22 02:07:29]

Description was changed from

==========
Prevent Contextual Search from breaking InfoBarContainer visibility

* Contextual Search hides infobars when it appears, but the
  InfoBarContainer continues listening for scrolls on the page.
  This means that scrolling the page by dragging the Contextual
  Search bar upwards makes the InfoBarContainer hide itself,
  making it hard to find them again when the Contextual Search
  bar is dismissed.  Prevent this by ignoring scrolls triggered
  when the InfoBarContainer/SwipableOverlayView are hidden.

* Request a layout when the InfoBarContainer's visibility is
  changed by Contextual Search.  This fixes an issue where the
  InfoBarContainer would refuse to become visible when
  Contextual Search hides infobars immediately after a page load.

BUG=563947
==========

to

==========
Prevent bottom-of-the-screen Views from breaking InfoBarContainer visibility

* SurfaceViews don't play well with Views on top of them toggling between
  View.VISIBLE and View.INVISIBLE.  Fix this by using View.GONE instead.

* Make logic for hiding the InfoBarContainer when other bottom-of-the-screen
  Views try to take over the screen out of SwipableOverlayView and
  ContextualSearchManager into the InfoBarContainer itself.

* Move logic for hiding the InfoBarContainer when the keyboard is showing
  from SwipableOverlayView#onLayout into InfoBarContainer and make it so
  that the InfoBarContainer listens for global layout changes.

BUG=563947
==========

[gone, 2016-01-22 02:10:20]

https://chromiumcodereview.appspot.com/1613513004/diff/1/chrome/android/java/...
File
chrome/android/java/src/org/chromium/chrome/browser/banners/SwipableOverlayView.java
(right):

https://chromiumcodereview.appspot.com/1613513004/diff/1/chrome/android/java/...
chrome/android/java/src/org/chromium/chrome/browser/banners/SwipableOverlayView.java:424:
protected boolean isAllowedToAutoHide() {
The translation actually wasn't the issue (its y-translation is 0 when it gets
hidden by Contextual Search, so it's still in the right place).  The problem is
that the View wasn't being painted again.  I've reverted this bit because there
would be side-effects if/when some other new bottom-hugging View allows
scrolling the content.

https://chromiumcodereview.appspot.com/1613513004/diff/1/chrome/android/java/...
File
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java
(right):

https://chromiumcodereview.appspot.com/1613513004/diff/1/chrome/android/java/...
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java:357:
container.setDoStayInvisible(false);
On 2016/01/21 05:27:51, newt wrote:
> Letting ContextualSearchManager muck around with InfoBarContainer's state so
> much frightens me. Let's move this extra logic inside the
> InfoBarContainer.setDoStayInvisible() method. We should also consider renaming
> setDoStayInvisible()... maybe setObscuredByContextualSearch() or
> setObscuredByBottomBar().
> 
> Longer term, of course, we should get rid of this setDoStayInvisible()
nonsense
> and create an object to manage the visibility of the several bottom views.

Agreed.  I'll pencil it in for when people figure out that Contextual Search
isn't an infobar.

https://chromiumcodereview.appspot.com/1613513004/diff/1/chrome/android/java/...
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java:433:
container.requestLayout();
On 2016/01/21 05:27:51, newt wrote:
> why do we need to request a layout after hiding the InfoBarContainer?

Wonkiness with how a SurfaceView behaves with other views on top of it:
http://stackoverflow.com/questions/11236336/setvisibilityview-visible-doesnt-...

[newt (away), 2016-01-22 21:43:46]

Thanks. I think this is cleaner now -- all the logic for
hiding/obscuring/removing the InfoBarContainer is now in the same class.

Just one comment about layout listeners.

https://chromiumcodereview.appspot.com/1613513004/diff/40001/chrome/android/j...
File
chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBarContainer.java
(right):

https://chromiumcodereview.appspot.com/1613513004/diff/40001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBarContainer.java:84:
private OnGlobalLayoutListener mGlobalLayoutListener = new
OnGlobalLayoutListener() {
Just realized a downside of using a global layout listener: every tab has an
infobarcontainer, and they'll all be changing their visibility whenever a layout
happens.

How about adding an OnLayoutChangeListener to the parent layout instead?

[gone, 2016-01-22 22:11:41]

https://chromiumcodereview.appspot.com/1613513004/diff/40001/chrome/android/j...
File
chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBarContainer.java
(right):

https://chromiumcodereview.appspot.com/1613513004/diff/40001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBarContainer.java:84:
private OnGlobalLayoutListener mGlobalLayoutListener = new
OnGlobalLayoutListener() {
On 2016/01/22 21:43:46, newt wrote:
> Just realized a downside of using a global layout listener: every tab has an
> infobarcontainer, and they'll all be changing their visibility whenever a
layout
> happens.
> 
> How about adding an OnLayoutChangeListener to the parent layout instead?

Android doesn't like that:
 01-22 14:10:17.992 29076 29076 W View    : requestLayout() improperly called by
org.chromium.chrome.browser.infobar.InfoBarContainer{41fdee88 V.ED.... ......ID
0,759-768,1134} during layout: running second layout pass

Posting the visibility change would get around this, I guess, but is it worth
forcing another layout pass after the post?

[gone, 2016-01-22 22:20:38]

It'd look like this, if we were fine with adding the Handler usage.

[newt (away), 2016-01-22 22:54:15]

lgtm after commetn

https://chromiumcodereview.appspot.com/1613513004/diff/60001/chrome/android/j...
File
chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBarContainer.java
(right):

https://chromiumcodereview.appspot.com/1613513004/diff/60001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBarContainer.java:89:
mHandler.post(new Runnable() {
No need for a Handler. Just use View.post().

Also, I'd add a comment explaining why this is being posted since it's not
obvious.

[gone, 2016-01-22 23:05:34]

https://codereview.chromium.org/1613513004/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBarContainer.java
(right):

https://codereview.chromium.org/1613513004/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBarContainer.java:89:
mHandler.post(new Runnable() {
On 2016/01/22 22:54:15, newt wrote:
> No need for a Handler. Just use View.post().
> 
> Also, I'd add a comment explaining why this is being posted since it's not
> obvious.

Done.

[gone, 2016-01-22 23:05:37]

The CQ bit was checked by dfalcantara@chromium.org

[gone, 2016-01-22 23:05:37]

The patchset sent to the CQ was uploaded after l-g-t-m from newt@chromium.org
Link to the patchset: https://codereview.chromium.org/1613513004/#ps80001
(title: "Final comments")

[commit-bot: I haz the power, 2016-01-22 23:06:22]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1613513004/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1613513004/80001

[commit-bot: I haz the power, 2016-01-23 00:10:42]

Description was changed from

==========
Prevent bottom-of-the-screen Views from breaking InfoBarContainer visibility

* SurfaceViews don't play well with Views on top of them toggling between
  View.VISIBLE and View.INVISIBLE.  Fix this by using View.GONE instead.

* Make logic for hiding the InfoBarContainer when other bottom-of-the-screen
  Views try to take over the screen out of SwipableOverlayView and
  ContextualSearchManager into the InfoBarContainer itself.

* Move logic for hiding the InfoBarContainer when the keyboard is showing
  from SwipableOverlayView#onLayout into InfoBarContainer and make it so
  that the InfoBarContainer listens for global layout changes.

BUG=563947
==========

to

==========
Prevent bottom-of-the-screen Views from breaking InfoBarContainer visibility

* SurfaceViews don't play well with Views on top of them toggling between
  View.VISIBLE and View.INVISIBLE.  Fix this by using View.GONE instead.

* Make logic for hiding the InfoBarContainer when other bottom-of-the-screen
  Views try to take over the screen out of SwipableOverlayView and
  ContextualSearchManager into the InfoBarContainer itself.

* Move logic for hiding the InfoBarContainer when the keyboard is showing
  from SwipableOverlayView#onLayout into InfoBarContainer and make it so
  that the InfoBarContainer listens for global layout changes.

BUG=563947
==========

[commit-bot: I haz the power, 2016-01-23 00:10:42]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-01-23 00:11:41]

Description was changed from

==========
Prevent bottom-of-the-screen Views from breaking InfoBarContainer visibility

* SurfaceViews don't play well with Views on top of them toggling between
  View.VISIBLE and View.INVISIBLE.  Fix this by using View.GONE instead.

* Make logic for hiding the InfoBarContainer when other bottom-of-the-screen
  Views try to take over the screen out of SwipableOverlayView and
  ContextualSearchManager into the InfoBarContainer itself.

* Move logic for hiding the InfoBarContainer when the keyboard is showing
  from SwipableOverlayView#onLayout into InfoBarContainer and make it so
  that the InfoBarContainer listens for global layout changes.

BUG=563947
==========

to

==========
Prevent bottom-of-the-screen Views from breaking InfoBarContainer visibility

* SurfaceViews don't play well with Views on top of them toggling between
  View.VISIBLE and View.INVISIBLE.  Fix this by using View.GONE instead.

* Make logic for hiding the InfoBarContainer when other bottom-of-the-screen
  Views try to take over the screen out of SwipableOverlayView and
  ContextualSearchManager into the InfoBarContainer itself.

* Move logic for hiding the InfoBarContainer when the keyboard is showing
  from SwipableOverlayView#onLayout into InfoBarContainer and make it so
  that the InfoBarContainer listens for global layout changes.

BUG=563947

Committed: https://crrev.com/e0906ef875c59ab9355e44e506b1e08f223844f2
Cr-Commit-Position: refs/heads/master@{#371094}
==========

[commit-bot: I haz the power, 2016-01-23 00:11:42]

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