Remove Overflow menu and snackbar when infobar is closing.

When using the new translate infobar, if I navigate away from the web page with the overflow menu remains open, chrome will crash if I click on any menu item (trying to access the native infobar of the previous page which is killed already).

To fix this bug, we need to make sure all the child UI are dismissed when the infobar is being removed.

About the new translate infobar:
- prototype: https://bbergher.googleplex.com/prototypes/translate/easier-1/
- Try it by: open chrome://flags/#translate-compact-infobar , select Enable

BUG=703887
Review-Url: https://codereview.chromium.org/2871483004
Cr-Commit-Position: refs/heads/master@{#471626}
Committed: https://chromium.googlesource.com/chromium/src/+/4ed2e81288b9ec8735be8ac3828817b3d4c56eb3

[Marti Wong, 2017-05-09 06:28:49]

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

[Marti Wong, 2017-05-09 06:28:49]

PTAL. thanks~!

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

https://codereview.chromium.org/2871483004/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/infobar/TranslateCompactInfoBar.java:257:
protected boolean closeInfoBar() {
Please advice if changing InfoBar.closeInfoBar() to protected and override it is
not appropriate.
I thought I could do it at onDetachedFromWindow of TabLayout but it's called
after onNativeDestroyed() so I gave that up.
Thanks!

[Marti Wong, 2017-05-10 08:25:45]

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

[Marti Wong, 2017-05-10 08:25:45]

Hi Matthew,
PTAL.
Dan is OOO so I'd like to ask you for help.
thanks you~!

[mdjones, 2017-05-10 15:32:40]

The description is a little unclear to me. Why do those things being open affect
the infobar? Why does this fix it?

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

https://codereview.chromium.org/2871483004/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/infobar/TranslateCompactInfoBar.java:257:
protected boolean closeInfoBar() {
On 2017/05/09 06:28:49, Marti Wong wrote:
> Please advice if changing InfoBar.closeInfoBar() to protected and override it
is
> not appropriate.
> I thought I could do it at onDetachedFromWindow of TabLayout but it's called
> after onNativeDestroyed() so I gave that up.
> Thanks!

Since infobars have a native component you can't properly close the infobar from
Java, which is what this name implies. Instead, I'd add a new method to InfoBar
called "onHideInfoBar" (or "onStartedHiding" depending on where you put it) that
is called by closeInfoBar() where it would remove the infobar from it's
container.

[Marti Wong, 2017-05-11 04:01:30]

Thanks Matthew for the prompt review~!
PTAL again. thx

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

https://codereview.chromium.org/2871483004/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/infobar/TranslateCompactInfoBar.java:257:
protected boolean closeInfoBar() {
On 2017/05/10 15:32:40, mdjones wrote:
> On 2017/05/09 06:28:49, Marti Wong wrote:
> > Please advice if changing InfoBar.closeInfoBar() to protected and override
it
> is
> > not appropriate.
> > I thought I could do it at onDetachedFromWindow of TabLayout but it's called
> > after onNativeDestroyed() so I gave that up.
> > Thanks!
> 
> Since infobars have a native component you can't properly close the infobar
from
> Java, which is what this name implies. Instead, I'd add a new method to
InfoBar
> called "onHideInfoBar" (or "onStartedHiding" depending on where you put it)
that
> is called by closeInfoBar() where it would remove the infobar from it's
> container.

Done.

[Marti Wong, 2017-05-11 13:06:09]

On 2017/05/10 15:32:40, mdjones wrote:
> The description is a little unclear to me. Why do those things being open
affect
> the infobar? Why does this fix it?
> 
>
https://codereview.chromium.org/2871483004/diff/1/chrome/android/java/src/org...
> File
>
chrome/android/java/src/org/chromium/chrome/browser/infobar/TranslateCompactInfoBar.java
> (right):
> 
>
https://codereview.chromium.org/2871483004/diff/1/chrome/android/java/src/org...
>
chrome/android/java/src/org/chromium/chrome/browser/infobar/TranslateCompactInfoBar.java:257:
> protected boolean closeInfoBar() {
> On 2017/05/09 06:28:49, Marti Wong wrote:
> > Please advice if changing InfoBar.closeInfoBar() to protected and override
it
> is
> > not appropriate.
> > I thought I could do it at onDetachedFromWindow of TabLayout but it's called
> > after onNativeDestroyed() so I gave that up.
> > Thanks!
> 
> Since infobars have a native component you can't properly close the infobar
from
> Java, which is what this name implies. Instead, I'd add a new method to
InfoBar
> called "onHideInfoBar" (or "onStartedHiding" depending on where you put it)
that
> is called by closeInfoBar() where it would remove the infobar from it's
> container.

oh. nearly forgot to reply this:

We are working on the new translate infobar.
(In case you never see it, here is the prototype:
https://bbergher.googleplex.com/prototypes/translate/easier-1/)
(Or you can try it by: open chrome://flags/#translate-compact-infobar , select
Enable)

There is a overflow menu button on the new infobar.
When I navigate away from the web page with the overflow menu remains open,
chrome will crash if I click on any menu item (trying to access the native
infobar of the previous page which is killed already).

To fix this bug, we need to make sure all the child UI are dismissed when the
infobar is being removed.
Hope it makes the thing clear.  thx

[mdjones, 2017-05-11 17:08:25]

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

https://codereview.chromium.org/2871483004/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBar.java:164:
onStartedRemoving();
I'd prefer this be named "onStartedHiding" despite the line below this since the
infobar nearly always animates.

https://codereview.chromium.org/2871483004/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBar.java:173: *
Called when infobar is being removed.  Give the chance to clean up any child UI
that may
"Called just before the Java infobar has begun hiding."

https://codereview.chromium.org/2871483004/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBar.java:181: */
Remove this floating javadoc.

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

https://codereview.chromium.org/2871483004/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/infobar/TranslateCompactInfoBar.java:259:
mOverflowMenuHelper.dismiss();
nit: inline where possible (and below).

[Marti Wong, 2017-05-12 00:04:50]

Thanks Matthew!
PTAL.

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

https://codereview.chromium.org/2871483004/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBar.java:164:
onStartedRemoving();
On 2017/05/11 17:08:24, mdjones wrote:
> I'd prefer this be named "onStartedHiding" despite the line below this since
the
> infobar nearly always animates.

Done.

https://codereview.chromium.org/2871483004/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBar.java:173: *
Called when infobar is being removed.  Give the chance to clean up any child UI
that may
On 2017/05/11 17:08:24, mdjones wrote:
> "Called just before the Java infobar has begun hiding."

Done.

https://codereview.chromium.org/2871483004/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBar.java:181: */
On 2017/05/11 17:08:24, mdjones wrote:
> Remove this floating javadoc.

Done. sorry for the carelessness.

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

https://codereview.chromium.org/2871483004/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/infobar/TranslateCompactInfoBar.java:259:
mOverflowMenuHelper.dismiss();
On 2017/05/11 17:08:25, mdjones wrote:
> nit: inline where possible (and below).

Done.

[Marti Wong, 2017-05-12 00:07:05]

Description was changed from

==========
Remove Overflow menu and snackbar when infobar is closing.
Chrome will crash if overflow menu or snackbar doesn't dismiss when
the translate infobar is closing.

BUG=703887
==========

to

==========
When using the new translate infobar, if I navigate away from the web page with
the overflow menu remains open, chrome will crash if I click on any menu item
(trying to access the native infobar of the previous page which is killed
already).

To fix this bug, we need to make sure all the child UI are dismissed when the
infobar is being removed.

About the new translate infobar:
- prototype: https://bbergher.googleplex.com/prototypes/translate/easier-1/
- Try it by: open chrome://flags/#translate-compact-infobar , select Enable

BUG=703887
==========

[mdjones, 2017-05-12 16:21:51]

lgtm, thanks!

[Marti Wong, 2017-05-14 02:39:44]

The CQ bit was checked by martiw@chromium.org

[commit-bot: I haz the power, 2017-05-14 02:39:53]

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-05-14 08:26:11]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1494729584916150,
"parent_rev": "6e27c23b503ebc4cef57d104f40950aae9dc1bcc", "commit_rev":
"4ed2e81288b9ec8735be8ac3828817b3d4c56eb3"}

[commit-bot: I haz the power, 2017-05-14 08:26:20]

Description was changed from

==========
When using the new translate infobar, if I navigate away from the web page with
the overflow menu remains open, chrome will crash if I click on any menu item
(trying to access the native infobar of the previous page which is killed
already).

To fix this bug, we need to make sure all the child UI are dismissed when the
infobar is being removed.

About the new translate infobar:
- prototype: https://bbergher.googleplex.com/prototypes/translate/easier-1/
- Try it by: open chrome://flags/#translate-compact-infobar , select Enable

BUG=703887
==========

to

==========
When using the new translate infobar, if I navigate away from the web page with
the overflow menu remains open, chrome will crash if I click on any menu item
(trying to access the native infobar of the previous page which is killed
already).

To fix this bug, we need to make sure all the child UI are dismissed when the
infobar is being removed.

About the new translate infobar:
- prototype: https://bbergher.googleplex.com/prototypes/translate/easier-1/
- Try it by: open chrome://flags/#translate-compact-infobar , select Enable

BUG=703887

Review-Url: https://codereview.chromium.org/2871483004
Cr-Commit-Position: refs/heads/master@{#471626}
Committed:
https://chromium.googlesource.com/chromium/src/+/4ed2e81288b9ec8735be8ac38288...
==========

[commit-bot: I haz the power, 2017-05-14 08:26:21]

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