Refactor the view hierarchy of snackbars and infobars

Refactor the view hierarchy of snackbars and infobars

To make snackbars and infobars animate, snackbars were moved to be child
of CompositorViewHolder, and TabContentViewParent was introduced to handle the
animation. One side effect of such design is that many Android views in
CompositorViewHolder now rely on a behavior to receive touch event.

This CL removes such behavior, as it is not the standard Android routine and it
makes our view hierarchy no longer extensible. Instead, a parent of
CompositorViewHolder is introduced to harbor all Android views that may display
on top of content.

This CL also moves infobars to be a sibling of snackbar, and their parent will
be a new FrameLayout called bottom_container.

BUG=640710
Review-Url: https://codereview.chromium.org/2623493003
Cr-Commit-Position: refs/heads/master@{#442733}
Committed: https://chromium.googlesource.com/chromium/src/+/7a609035f046b007eb1b0d8302776aeae3b2e174

[Ian Wen, 2017-01-09 18:55:00]

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

[Ian Wen, 2017-01-09 18:55:21]

ianwen@chromium.org changed reviewers:
+ mdjones@chromium.org, tedchoc@chromium.org

[commit-bot: I haz the power, 2017-01-09 18:55:41]

Dry run: 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, 2017-01-09 20:53:10]

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

[commit-bot: I haz the power, 2017-01-09 20:53:11]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Ian Wen, 2017-01-09 22:19:24]

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

[Ian Wen, 2017-01-09 22:20:01]

The regression about InfoBar has been eliminated. PTAL. :)

[commit-bot: I haz the power, 2017-01-09 22:20:21]

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

[mdjones, 2017-01-09 22:58:25]

lgtm!

[gone, 2017-01-09 23:20:55]

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

[gone, 2017-01-09 23:20:56]

The commit message should reflect info about how the InfoBarContainer parent has
been changed so that it sits as a child of the bottom bar container.

https://codereview.chromium.org/2623493003/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorViewHolder.java
(right):

https://codereview.chromium.org/2623493003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorViewHolder.java:356:
super.onInterceptTouchEvent(e);
Shouldn't you be worried about what the parent is doing with the return value?

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

https://codereview.chromium.org/2623493003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBarContainer.java:106:
* Adds/removes the infobar container when the tab's view is attached/detached.
This is mostly
InfoBarContainer

https://codereview.chromium.org/2623493003/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/snackbar/SnackbarView.java
(right):

https://codereview.chromium.org/2623493003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/snackbar/SnackbarView.java:188:
FrameLayout.LayoutParams lp = new FrameLayout.LayoutParams(getLayoutParams());
This is a little weird... Could you document what's going on here?  I'm confused
about why the view is copying the exact same parameters from the
InfoBarContainer and am wondering if it makes more sense to just create new ones
from scratch.

[Ted C, 2017-01-09 23:35:06]

lgtm

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

https://codereview.chromium.org/2623493003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBarContainer.java:99:
mTabView.removeOnAttachStateChangeListener(mAttachedStateListener);
I don't actually recall if onContentChanged ensures the mTabView is different. 
Maybe add:

if (mTabView == tab.getView()) return;

to avoid the churn?

https://codereview.chromium.org/2623493003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBarContainer.java:100:
mTabView = tab.getView();
should we be calling the equivalent to onParentViewChanged at this point? 
Assuming mTabView is attached currently

[Ian Wen, 2017-01-10 02:19:15]

Description was changed from

==========
Refactor the view hierarchy of snackbars and infobars

To make snackbars and infobars work, snackbars were moved to be child of
CompositorViewHolder, and TabContentViewParent was introduced to handle the
animation. One side effect of such design is that many Android views in
CompositorViewHolder now rely on a behavior to receive touch event.

This CL removes such behavior, as it is not the standard Android routine and it
makes our view hierarchy no longer extensible. Instead, a parent of
CompositorViewHolder is introduced to harbor all Android views that may display
on top of content.

BUG=640710
==========

to

==========
Refactor the view hierarchy of snackbars and infobars

To make snackbars and infobars animate, snackbars were moved to be child
of CompositorViewHolder, and TabContentViewParent was introduced to handle the
animation. One side effect of such design is that many Android views in
CompositorViewHolder now rely on a behavior to receive touch event.

This CL removes such behavior, as it is not the standard Android routine and it
makes our view hierarchy no longer extensible. Instead, a parent of
CompositorViewHolder is introduced to harbor all Android views that may display
on top of content.

This CL also moves infobars to be a sibling of snackbar, and their parent will
be a new FrameLayout called bottom_container.

BUG=640710
==========

[Ian Wen, 2017-01-10 02:19:58]

Thanks for the prompt review. All addressed.

https://codereview.chromium.org/2623493003/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorViewHolder.java
(right):

https://codereview.chromium.org/2623493003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorViewHolder.java:356:
super.onInterceptTouchEvent(e);
On 2017/01/09 23:20:56, dfalcantara (check queue) wrote:
> Shouldn't you be worried about what the parent is doing with the return value?

When the CVH was a CoordinatorLayout, it was necessary to check the returned
value because we might intercept touch in behaviors. Yet since now it's a
FrameLayout, we no longer allow the FrameLayout base implementation to eat any
touch event.

This is mostly reverting my change in https://codereview.chromium.org/1983353002

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

https://codereview.chromium.org/2623493003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBarContainer.java:99:
mTabView.removeOnAttachStateChangeListener(mAttachedStateListener);
On 2017/01/09 23:35:06, Ted C wrote:
> I don't actually recall if onContentChanged ensures the mTabView is different.

> Maybe add:
> 
> if (mTabView == tab.getView()) return;
> 
> to avoid the churn?

Done.

https://codereview.chromium.org/2623493003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBarContainer.java:100:
mTabView = tab.getView();
On 2017/01/09 23:35:06, Ted C wrote:
> should we be calling the equivalent to onParentViewChanged at this point? 
> Assuming mTabView is attached currently

No I don't think we need it. After this patch the parent of infobar container
will never change. What we care is not the parent view, but the newest view of a
tab.

https://codereview.chromium.org/2623493003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBarContainer.java:106:
* Adds/removes the infobar container when the tab's view is attached/detached.
This is mostly
On 2017/01/09 23:20:56, dfalcantara (check queue) wrote:
> InfoBarContainer

Done.

https://codereview.chromium.org/2623493003/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/snackbar/SnackbarView.java
(right):

https://codereview.chromium.org/2623493003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/snackbar/SnackbarView.java:188:
FrameLayout.LayoutParams lp = new FrameLayout.LayoutParams(getLayoutParams());
On 2017/01/09 23:20:56, dfalcantara (check queue) wrote:
> This is a little weird... Could you document what's going on here?  I'm
confused
> about why the view is copying the exact same parameters from the
> InfoBarContainer and am wondering if it makes more sense to just create new
ones
> from scratch.

Good catch. This was needed when we want to move the snackbar from a different
type of ViewGroup to FrameLayout. Now it's not needed anymore. I replaced
#188-#190 with mParent.addView(mView);

[Ian Wen, 2017-01-10 02:20:04]

The CQ bit was checked by ianwen@chromium.org

[Ian Wen, 2017-01-10 02:20:04]

The patchset sent to the CQ was uploaded after l-g-t-m from
mdjones@chromium.org, tedchoc@chromium.org
Link to the patchset: https://codereview.chromium.org/2623493003/#ps40001
(title: "comments")

[commit-bot: I haz the power, 2017-01-10 02:21:21]

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, 2017-01-10 04:42:37]

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

[commit-bot: I haz the power, 2017-01-10 04:42:38]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[gone, 2017-01-10 23:34:38]

lgtm

[Ian Wen, 2017-01-10 23:40:22]

The CQ bit was checked by ianwen@chromium.org

[commit-bot: I haz the power, 2017-01-10 23:41:07]

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, 2017-01-10 23:46:32]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1484091622025270,
"parent_rev": "8a4fdfe3c1fa302318dc3a68a332915e529f2e99", "commit_rev":
"7a609035f046b007eb1b0d8302776aeae3b2e174"}

[commit-bot: I haz the power, 2017-01-10 23:47:06]

Description was changed from

==========
Refactor the view hierarchy of snackbars and infobars

To make snackbars and infobars animate, snackbars were moved to be child
of CompositorViewHolder, and TabContentViewParent was introduced to handle the
animation. One side effect of such design is that many Android views in
CompositorViewHolder now rely on a behavior to receive touch event.

This CL removes such behavior, as it is not the standard Android routine and it
makes our view hierarchy no longer extensible. Instead, a parent of
CompositorViewHolder is introduced to harbor all Android views that may display
on top of content.

This CL also moves infobars to be a sibling of snackbar, and their parent will
be a new FrameLayout called bottom_container.

BUG=640710
==========

to

==========
Refactor the view hierarchy of snackbars and infobars

To make snackbars and infobars animate, snackbars were moved to be child
of CompositorViewHolder, and TabContentViewParent was introduced to handle the
animation. One side effect of such design is that many Android views in
CompositorViewHolder now rely on a behavior to receive touch event.

This CL removes such behavior, as it is not the standard Android routine and it
makes our view hierarchy no longer extensible. Instead, a parent of
CompositorViewHolder is introduced to harbor all Android views that may display
on top of content.

This CL also moves infobars to be a sibling of snackbar, and their parent will
be a new FrameLayout called bottom_container.

BUG=640710

Review-Url: https://codereview.chromium.org/2623493003
Cr-Commit-Position: refs/heads/master@{#442733}
Committed:
https://chromium.googlesource.com/chromium/src/+/7a609035f046b007eb1b0d830277...
==========

[commit-bot: I haz the power, 2017-01-10 23:47:07]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/7a609035f046b007eb1b0d830277...