Dismiss menu when infobar's host tab is hidden. (new translate infobar)

Dismiss menu when infobar's host tab is hidden. (new translate infobar)

When host tab of infobar is being hidden (this occurs when Chrome app
lost focus or the tab is being switched out), dismiss all existing
overflow menus and snackbars.
It is to prevent the overflow menu (or snackbar) from keep showing when
the chrome app is re-open with a new tab.

BUG=734656
Review-Url: https://codereview.chromium.org/2952553002
Cr-Commit-Position: refs/heads/master@{#484775}
Committed: https://chromium.googlesource.com/chromium/src/+/a1d3a65da2d4e255a001fbcbdde583938d919fc3

[Marti Wong, 2017-06-20 08:57:45]

Description was changed from

==========
Dismiss option menu during activity.onStop (new translate infobar)

BUG=734656
==========

to

==========
Dismiss menu when infobar's host tab is hidden. (new translate infobar)
It is to prevent the overflow menu from keep showing when it's parent infobar is
hidden (when the host tab is hidden).

BUG=734656
==========

[Marti Wong, 2017-06-20 09:02:28]

martiw@chromium.org changed reviewers:
+ googleo@chromium.org

[Marti Wong, 2017-06-20 09:02:28]

Hi Leo, may I have your review first?  thx~!

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

https://codereview.chromium.org/2952553002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBarContainer.java:126:
public void onHidden(Tab tab) {
This is called when:
1. Chrome app losts window focus. 
   (Tab.java#onActivityHidden() <-- ChromeActivity#onStopWithNative())

2. This Tab is being switched out.
   (TabModelSelectionImpl#requestToShowTab())

[Marti Wong, 2017-06-21 08:11:12]

martiw@chromium.org changed reviewers:
+ mdjones@chromium.org

[Marti Wong, 2017-06-21 08:11:12]

Hi Matthew, PTAL. thx~!

[mdjones, 2017-06-21 20:18:11]

Fix your commit message:
https://www.chromium.org/developers/contributing-code#TOC-Writing-change-list...

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

https://codereview.chromium.org/2952553002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBarContainer.java:126:
public void onHidden(Tab tab) {
On 2017/06/20 09:02:28, Marti Wong wrote:
> This is called when:
> 1. Chrome app losts window focus. 
>    (Tab.java#onActivityHidden() <-- ChromeActivity#onStopWithNative())
> 
> 2. This Tab is being switched out.
>    (TabModelSelectionImpl#requestToShowTab())

Does the visibility of the infobar not change? I would try listening to that
(might involve making a custom view).

[Marti Wong, 2017-06-22 00:12:03]

Description was changed from

==========
Dismiss menu when infobar's host tab is hidden. (new translate infobar)
It is to prevent the overflow menu from keep showing when it's parent infobar is
hidden (when the host tab is hidden).

BUG=734656
==========

to

==========
Dismiss menu when infobar's host tab is hidden. (new translate infobar)

When host tab of infobar is being hidden (this occurs when Chrome app
lost focus or the tab is being switched out), dismiss all existing
overflow menus and snackbars.
It is to prevent the overflow menu (or snackbar) from keep showing when
the chrome app is re-open with a new tab.

BUG=734656
==========

[Marti Wong, 2017-06-22 01:41:50]

PTAL again. thx!

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

https://codereview.chromium.org/2952553002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBarContainer.java:126:
public void onHidden(Tab tab) {
On 2017/06/21 20:18:11, mdjones wrote:
> On 2017/06/20 09:02:28, Marti Wong wrote:
> > This is called when:
> > 1. Chrome app losts window focus. 
> >    (Tab.java#onActivityHidden() <-- ChromeActivity#onStopWithNative())
> > 
> > 2. This Tab is being switched out.
> >    (TabModelSelectionImpl#requestToShowTab())
> 
> Does the visibility of the infobar not change? I would try listening to that
> (might involve making a custom view).

Thanks for the review Matthew!

I am not sure if I understand your suggestion about "listen to the visibility
change" and "making a custom view".
But after I looked into the visibility change of the container.
I changed my code to use "InfoBarContainer#removeFromParentView" (instead of
Tab#onHidden) which is more directly related to the visibility change of the
infobar container.
Could you PTAL again?  thanks!

[mdjones, 2017-06-22 19:00:08]

On 2017/06/22 01:41:50, Marti Wong wrote:
> PTAL again. thx!
> 
>
https://codereview.chromium.org/2952553002/diff/1/chrome/android/java/src/org...
> File
>
chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBarContainer.java
> (right):
> 
>
https://codereview.chromium.org/2952553002/diff/1/chrome/android/java/src/org...
>
chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBarContainer.java:126:
> public void onHidden(Tab tab) {
> On 2017/06/21 20:18:11, mdjones wrote:
> > On 2017/06/20 09:02:28, Marti Wong wrote:
> > > This is called when:
> > > 1. Chrome app losts window focus. 
> > >    (Tab.java#onActivityHidden() <-- ChromeActivity#onStopWithNative())
> > > 
> > > 2. This Tab is being switched out.
> > >    (TabModelSelectionImpl#requestToShowTab())
> > 
> > Does the visibility of the infobar not change? I would try listening to that
> > (might involve making a custom view).
> 
> Thanks for the review Matthew!
> 
> I am not sure if I understand your suggestion about "listen to the visibility
> change" and "making a custom view".
> But after I looked into the visibility change of the container.
> I changed my code to use "InfoBarContainer#removeFromParentView" (instead of
> Tab#onHidden) which is more directly related to the visibility change of the
> infobar container.
> Could you PTAL again?  thanks!

You shouldn't need to make any changes to the InfoBarContainer or InfoBar
classes. Your infobar is made of a number of views that have a setVisibility
function. You can have a custom view (something like 'public class
TranslateContentContainer extends LinearLayout...' and use that in place of
https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr...)
that overrides the setVisibility function to send a signal to your infobar
(since your infobar will be invisible in the cases you outlined).

Alternatively, if you really need a tab object, native should be able to provide
it.

[mdjones, 2017-06-22 19:00:16]

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

https://codereview.chromium.org/2952553002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBarContainer.java:267:
mInfoBars.get(0).onInfoBarContainerStartedHiding();
Why only index 0?

[Marti Wong, 2017-06-23 01:37:04]

On 2017/06/22 19:00:08, mdjones wrote:
> On 2017/06/22 01:41:50, Marti Wong wrote:
> > PTAL again. thx!
> > 
> >
>
https://codereview.chromium.org/2952553002/diff/1/chrome/android/java/src/org...
> > File
> >
>
chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBarContainer.java
> > (right):
> > 
> >
>
https://codereview.chromium.org/2952553002/diff/1/chrome/android/java/src/org...
> >
>
chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBarContainer.java:126:
> > public void onHidden(Tab tab) {
> > On 2017/06/21 20:18:11, mdjones wrote:
> > > On 2017/06/20 09:02:28, Marti Wong wrote:
> > > > This is called when:
> > > > 1. Chrome app losts window focus. 
> > > >    (Tab.java#onActivityHidden() <-- ChromeActivity#onStopWithNative())
> > > > 
> > > > 2. This Tab is being switched out.
> > > >    (TabModelSelectionImpl#requestToShowTab())
> > > 
> > > Does the visibility of the infobar not change? I would try listening to
that
> > > (might involve making a custom view).
> > 
> > Thanks for the review Matthew!
> > 
> > I am not sure if I understand your suggestion about "listen to the
visibility
> > change" and "making a custom view".
> > But after I looked into the visibility change of the container.
> > I changed my code to use "InfoBarContainer#removeFromParentView" (instead of
> > Tab#onHidden) which is more directly related to the visibility change of the
> > infobar container.
> > Could you PTAL again?  thanks!
> 
> You shouldn't need to make any changes to the InfoBarContainer or InfoBar
> classes. Your infobar is made of a number of views that have a setVisibility
> function. You can have a custom view (something like 'public class
> TranslateContentContainer extends LinearLayout...' and use that in place of
>
https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr...)
> that overrides the setVisibility function to send a signal to your infobar
> (since your infobar will be invisible in the cases you outlined).
> 
> Alternatively, if you really need a tab object, native should be able to
provide
> it.

Just did some tests.
setVisibility(View.INVISIBLE) (of InfobarContainer, and customer views under
TranslateCompactInfoBar.java) won't be triggered when tab is being hidden (when
Chrome app loses focus or tab being switched out).

Could I continue to use container#removeFromParentView or you may have other
advice?

[Marti Wong, 2017-06-23 01:39:10]

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

https://codereview.chromium.org/2952553002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBarContainer.java:267:
mInfoBars.get(0).onInfoBarContainerStartedHiding();
On 2017/06/22 19:00:16, mdjones wrote:
> Why only index 0?

(0) means the top of the infobar stack.
only the top infobar has the chance to own any child UIs and need to be taken
care of.
I will add a comment to explain this here.

[Marti Wong, 2017-06-30 00:50:41]

On 2017/06/23 01:39:10, Marti Wong wrote:
>
https://codereview.chromium.org/2952553002/diff/20001/chrome/android/java/src...
> File
>
chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBarContainer.java
> (right):
> 
>
https://codereview.chromium.org/2952553002/diff/20001/chrome/android/java/src...
>
chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBarContainer.java:267:
> mInfoBars.get(0).onInfoBarContainerStartedHiding();
> On 2017/06/22 19:00:16, mdjones wrote:
> > Why only index 0?
> 
> (0) means the top of the infobar stack.
> only the top infobar has the chance to own any child UIs and need to be taken
> care of.
> I will add a comment to explain this here.

Hi Matthew,
PTAL.  I just tried using OnDetachedFromWindow()
InfoBar is not a View so I have to do that on InfoBarContainer.
Thanks~!

[Marti Wong, 2017-06-30 00:50:51]

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

[commit-bot: I haz the power, 2017-06-30 00:51:09]

Dry run: CQ is trying da patch.

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

[Marti Wong, 2017-06-30 00:54:35]

https://codereview.chromium.org/2952553002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/infobar/TranslateCompactInfoBar.java
(right):

https://codereview.chromium.org/2952553002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/infobar/TranslateCompactInfoBar.java:436:
if (getSnackbarManager() != null) getSnackbarManager().dismissAllSnackbars();
crrev.com/2965503002 is changing this part of code
would need to merge later.

[commit-bot: I haz the power, 2017-06-30 02:18:01]

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

[commit-bot: I haz the power, 2017-06-30 02:18:02]

Dry run: This issue passed the CQ dry run.

[mdjones, 2017-06-30 18:51:57]

On 2017/06/30 00:50:41, Marti Wong wrote:
> On 2017/06/23 01:39:10, Marti Wong wrote:
> >
>
https://codereview.chromium.org/2952553002/diff/20001/chrome/android/java/src...
> > File
> >
>
chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBarContainer.java
> > (right):
> > 
> >
>
https://codereview.chromium.org/2952553002/diff/20001/chrome/android/java/src...
> >
>
chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBarContainer.java:267:
> > mInfoBars.get(0).onInfoBarContainerStartedHiding();
> > On 2017/06/22 19:00:16, mdjones wrote:
> > > Why only index 0?
> > 
> > (0) means the top of the infobar stack.
> > only the top infobar has the chance to own any child UIs and need to be
taken
> > care of.
> > I will add a comment to explain this here.
> 
> Hi Matthew,
> PTAL.  I just tried using OnDetachedFromWindow()
> InfoBar is not a View so I have to do that on InfoBarContainer.
> Thanks~!

You don't. The infobar is made up of views you control. I did a test with a
different infobar:

    myInfoBarView.addOnAttachStateChangeListener(new
View.OnAttachStateChangeListener() {
        @Override
        public void onViewAttachedToWindow(View view) {
            android.util.Log.w("mdjones", "attached");
        }

        @Override
        public void onViewDetachedFromWindow(View view) {
            android.util.Log.w("mdjones", "detached");
            // Dismiss open menus here.
        }
    });

onViewDetachedFromWindow is called when you switch tabs or the infobar container
hides.

[Marti Wong, 2017-07-02 06:39:34]

Thanks for the tip Matthew!

I have added onViewDetachedFromWindow in InfoBarCompactLayout.java.
(I wanted to edit TranslateCompactInfoBar only but failed.  I cannot find a good
place to put getView().addOnAttachStateChangeListener in
TranslateCompactInfoBar.  It seems mView is always null inside create and
createCompactLayoutContent.)

PTAL.
thanks again!

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

https://codereview.chromium.org/2952553002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBarCompactLayout.java:45:
if (mInfoBarView instanceof TranslateCompactInfoBar) {
Please advise if I could skip the instanceof checking here.
And to onParentTabSwitchedOut to infoBar.java as well.

[mdjones, 2017-07-05 22:06:27]

On 2017/07/02 06:39:34, Marti Wong wrote:
> Thanks for the tip Matthew!
> 
> I have added onViewDetachedFromWindow in InfoBarCompactLayout.java.
> (I wanted to edit TranslateCompactInfoBar only but failed.  I cannot find a
good
> place to put getView().addOnAttachStateChangeListener in
> TranslateCompactInfoBar.  It seems mView is always null inside create and
> createCompactLayoutContent.)

Were you unable to use "content" in TranslateCompactInfoBar here?:
https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr...

Since it is a LinearLayout (a View), and you have direct control over it, I
don't see why it couldn't be added there. The root view isn't the only View that
gets a notification that it was attached or detached from the window.

> 
> PTAL.
> thanks again!
> 
>
https://codereview.chromium.org/2952553002/diff/60001/chrome/android/java/src...
> File
>
chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBarCompactLayout.java
> (right):
> 
>
https://codereview.chromium.org/2952553002/diff/60001/chrome/android/java/src...
>
chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBarCompactLayout.java:45:
> if (mInfoBarView instanceof TranslateCompactInfoBar) {
> Please advise if I could skip the instanceof checking here.
> And to onParentTabSwitchedOut to infoBar.java as well.

[Marti Wong, 2017-07-06 08:04:48]

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

[commit-bot: I haz the power, 2017-07-06 08:04:55]

Dry run: CQ is trying da patch.

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

[Marti Wong, 2017-07-06 08:20:39]

Patchset #5 (id:80001) has been deleted

[Marti Wong, 2017-07-06 08:21:31]

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

[commit-bot: I haz the power, 2017-07-06 08:21:38]

Dry run: CQ is trying da patch.

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

[Marti Wong, 2017-07-06 08:22:46]

On 2017/07/05 22:06:27, mdjones wrote:
> On 2017/07/02 06:39:34, Marti Wong wrote:
> > Thanks for the tip Matthew!
> > 
> > I have added onViewDetachedFromWindow in InfoBarCompactLayout.java.
> > (I wanted to edit TranslateCompactInfoBar only but failed.  I cannot find a
> good
> > place to put getView().addOnAttachStateChangeListener in
> > TranslateCompactInfoBar.  It seems mView is always null inside create and
> > createCompactLayoutContent.)
> 
> Were you unable to use "content" in TranslateCompactInfoBar here?:
>
https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr...
> 
> Since it is a LinearLayout (a View), and you have direct control over it, I
> don't see why it couldn't be added there. The root view isn't the only View
that
> gets a notification that it was attached or detached from the window.
> 
> > 
> > PTAL.
> > thanks again!
> > 
> >
>
https://codereview.chromium.org/2952553002/diff/60001/chrome/android/java/src...
> > File
> >
>
chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBarCompactLayout.java
> > (right):
> > 
> >
>
https://codereview.chromium.org/2952553002/diff/60001/chrome/android/java/src...
> >
>
chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBarCompactLayout.java:45:
> > if (mInfoBarView instanceof TranslateCompactInfoBar) {
> > Please advise if I could skip the instanceof checking here.
> > And to onParentTabSwitchedOut to infoBar.java as well.

Hi Matthew,  got it.
It's much simplified now.  PTAL.
thanks for your patience!

[commit-bot: I haz the power, 2017-07-06 09:13:05]

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

[commit-bot: I haz the power, 2017-07-06 09:13:06]

Dry run: This issue passed the CQ dry run.

[mdjones, 2017-07-06 15:54:44]

lgtm!

[Marti Wong, 2017-07-07 00:00:18]

On 2017/07/06 15:54:44, mdjones wrote:
> lgtm!

thanks Matthew! committing.

[Marti Wong, 2017-07-07 00:00:30]

The CQ bit was checked by martiw@chromium.org

[commit-bot: I haz the power, 2017-07-07 00:00:42]

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-07-07 00:04:38]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1499385630930910,
"parent_rev": "5c33235ae031c7b03910b17173abba2b9f031d6a", "commit_rev":
"6e92c95a67f2b2cf9eb61de8fad9cd119fbd3c70"}

[commit-bot: I haz the power, 2017-07-07 00:04:56]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1499385630930910,
"parent_rev": "39e1b7761b76d793ae81db196279ebe43c44f1e6", "commit_rev":
"a1d3a65da2d4e255a001fbcbdde583938d919fc3"}

[commit-bot: I haz the power, 2017-07-07 00:05:09]

Description was changed from

==========
Dismiss menu when infobar's host tab is hidden. (new translate infobar)

When host tab of infobar is being hidden (this occurs when Chrome app
lost focus or the tab is being switched out), dismiss all existing
overflow menus and snackbars.
It is to prevent the overflow menu (or snackbar) from keep showing when
the chrome app is re-open with a new tab.

BUG=734656
==========

to

==========
Dismiss menu when infobar's host tab is hidden. (new translate infobar)

When host tab of infobar is being hidden (this occurs when Chrome app
lost focus or the tab is being switched out), dismiss all existing
overflow menus and snackbars.
It is to prevent the overflow menu (or snackbar) from keep showing when
the chrome app is re-open with a new tab.

BUG=734656

Review-Url: https://codereview.chromium.org/2952553002
Cr-Commit-Position: refs/heads/master@{#484775}
Committed:
https://chromium.googlesource.com/chromium/src/+/a1d3a65da2d4e255a001fbcbdde5...
==========

[commit-bot: I haz the power, 2017-07-07 00:05:10]

Committed patchset #5 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/a1d3a65da2d4e255a001fbcbdde5...