Make infobars hide on scroll

Make infobars hide on scroll

* Makes it so that infobars hide when scrolling the page and
  return at the top of the page.

* SwipableOverlayView is now the base class of InfoBarContainer
  and now extends from ScrollLayout.

* Adds way to disable the SwipableOverlayView from being swiped
  away.

* AppBannerView (which is going away soon) is adjusted to account
  for the new ScrollLayout requirement of having only one child.

BUG=453170
Committed: https://crrev.com/4ee2b04213697a13fa7efba80434435529d3ec90
Cr-Commit-Position: refs/heads/master@{#313987}

[gone, 2015-01-30 02:05:58]

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

[newt (away), 2015-01-30 18:56:10]

My main concern: I'm not convinced that SwipableOverlayView will unregister
itself as an observer correctly in all cases. Maybe this isn't an issue because
there's only one SwipableOverlayView per tab, but it seems like something we
should be more careful with.

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

https://codereview.chromium.org/893483002/diff/1/chrome/android/java/src/org/...
chrome/android/java/src/org/chromium/chrome/browser/banners/AppBannerView.java:169:
banner.addToParentView(contentViewCore.getContainerView());
Looks like app banners don't currently work in TalkBack mode. Luckily, we're
fixing that by moving them to the infobar container :)

https://codereview.chromium.org/893483002/diff/1/chrome/android/java/src/org/...
chrome/android/java/src/org/chromium/chrome/browser/banners/AppBannerView.java:721:
mContainerView.layout(0, 0, getMeasuredWidth(), getMeasuredHeight());
this seems like it would cause infinite recursion.

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

https://codereview.chromium.org/893483002/diff/1/chrome/android/java/src/org/...
chrome/android/java/src/org/chromium/chrome/browser/banners/SwipableOverlayView.java:200:
mContentViewCore.addGestureStateListener(mGestureStateListener);
Should we add and remove the gesture state listener in addToParentView() and
removeFromParentView()?

Seems like we have a potential leak here with the SwipableOverlayView still
being registered as a GestureStateListener on the ContentViewCore even after the
SwipableOverlayView has been removed and forgotten.

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

https://codereview.chromium.org/893483002/diff/1/chrome/android/java/src/org/...
chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBarContainer.java:215:
protected TabObserver createTabObserver() {
This tab observer is only used for the doStayInvisible case, and yet we always
want to listen to onPageLoadStarted() events, right?

[gone, 2015-01-30 19:01:24]

Yeah, it shouldn't be any different from the InfoBarContainer, though, where
there's one per tab.  AFAICT the TabObserver removal logic hasn't changed there.

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

https://codereview.chromium.org/893483002/diff/1/chrome/android/java/src/org/...
chrome/android/java/src/org/chromium/chrome/browser/banners/AppBannerView.java:721:
mContainerView.layout(0, 0, getMeasuredWidth(), getMeasuredHeight());
On 2015/01/30 18:56:10, newt wrote:
> this seems like it would cause infinite recursion.

Probably not the best named View, but the ContainerView isn't the parent.  It's
a child of the AppBannerView that holds all of the other children.  One of the
caveats of moving to a ScrollLayout.

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

https://codereview.chromium.org/893483002/diff/1/chrome/android/java/src/org/...
chrome/android/java/src/org/chromium/chrome/browser/banners/SwipableOverlayView.java:200:
mContentViewCore.addGestureStateListener(mGestureStateListener);
On 2015/01/30 18:56:10, newt wrote:
> Should we add and remove the gesture state listener in addToParentView() and
> removeFromParentView()?
> 
> Seems like we have a potential leak here with the SwipableOverlayView still
> being registered as a GestureStateListener on the ContentViewCore even after
the
> SwipableOverlayView has been removed and forgotten.

The parent view isn't always a ContentViewCore, which is the problem.  The main
unknown is the reader mode banner, which is the only other place this class is
used (aside from AppBannerView, which is going away).

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

https://codereview.chromium.org/893483002/diff/1/chrome/android/java/src/org/...
chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBarContainer.java:215:
protected TabObserver createTabObserver() {
On 2015/01/30 18:56:10, newt wrote:
> This tab observer is only used for the doStayInvisible case, and yet we always
> want to listen to onPageLoadStarted() events, right?

The parent's TabObserver, is yeah.  This one adds another method on top of it. 
Comment clarity issue?

[newt (away), 2015-01-30 19:13:04]

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

https://codereview.chromium.org/893483002/diff/1/chrome/android/java/src/org/...
chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBarContainer.java:215:
protected TabObserver createTabObserver() {
On 2015/01/30 19:01:24, dfalcantara wrote:
> On 2015/01/30 18:56:10, newt wrote:
> > This tab observer is only used for the doStayInvisible case, and yet we
always
> > want to listen to onPageLoadStarted() events, right?
> 
> The parent's TabObserver, is yeah.  This one adds another method on top of it.

> Comment clarity issue?

The TabObserver is only registered as an observer when the
ContextualSearchManager calls setDoStayInvisible(). *BUT* we want to receive
onPageLoadStarted() callbacks all the time (not just when contextual search has
been triggered). So, you should call Tab.addObserver() in the SOV's constructor,
or somewhere else that always gets called. Right?

[gone, 2015-01-30 19:33:03]

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

https://codereview.chromium.org/893483002/diff/1/chrome/android/java/src/org/...
chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBarContainer.java:215:
protected TabObserver createTabObserver() {
On 2015/01/30 19:13:04, newt wrote:
> On 2015/01/30 19:01:24, dfalcantara wrote:
> > On 2015/01/30 18:56:10, newt wrote:
> > > This tab observer is only used for the doStayInvisible case, and yet we
> always
> > > want to listen to onPageLoadStarted() events, right?
> > 
> > The parent's TabObserver, is yeah.  This one adds another method on top of
it.
> 
> > Comment clarity issue?
> 
> The TabObserver is only registered as an observer when the
> ContextualSearchManager calls setDoStayInvisible(). *BUT* we want to receive
> onPageLoadStarted() callbacks all the time (not just when contextual search
has
> been triggered). So, you should call Tab.addObserver() in the SOV's
constructor,
> or somewhere else that always gets called. Right?

Yeah, point noted.  Check if this new scheme makes sense... I just create the
observer and never remove it.

[newt (away), 2015-01-30 19:44:40]

Happier :)

lgtm modulo my comment in DomDistillerFeedbackReportingView.java

https://codereview.chromium.org/893483002/diff/20001/chrome/android/java/src/...
File
chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/DomDistillerFeedbackReportingView.java
(right):

https://codereview.chromium.org/893483002/diff/20001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/DomDistillerFeedbackReportingView.java:71:
view.observeTab(tab);
I'd remove this. observing the tab doesn't matter since setDoStayInvisible() is
never called on the DomDistillerFeedbackReportingView, and it'll leak this view
as long as the tab exists.

[gone, 2015-01-30 20:04:42]

Yeah, the Contextual Search thing doesn't interact well at all with the banners.
 I changed it so that we kept the TabObserver around in case we ever do need to
remove it, though.

PTAsmallL

https://codereview.chromium.org/893483002/diff/20001/chrome/android/java/src/...
File
chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/DomDistillerFeedbackReportingView.java
(right):

https://codereview.chromium.org/893483002/diff/20001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/DomDistillerFeedbackReportingView.java:71:
view.observeTab(tab);
On 2015/01/30 19:44:40, newt wrote:
> I'd remove this. observing the tab doesn't matter since setDoStayInvisible()
is
> never called on the DomDistillerFeedbackReportingView, and it'll leak this
view
> as long as the tab exists.

Removed this, changed it so that we keep the Observer around in the
SwipableOverlayView in case we ever do need to remove it.

[newt (away), 2015-01-30 20:07:42]

lgtm

[gone, 2015-01-30 20:08:35]

The CQ bit was checked by dfalcantara@chromium.org

[commit-bot: I haz the power, 2015-01-30 20:09:52]

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

[commit-bot: I haz the power, 2015-01-30 20:47:20]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-01-30 20:49:53]

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