customtabs: Prevent the app menu window from leaking when finishing the activity.

customtabs: Prevent the app menu window from leaking when finishing the activity.

BUG=606783
Committed: https://crrev.com/02c5a62089bed609d72622aeaec9583c89007f6d
Cr-Commit-Position: refs/heads/master@{#390054}

[Benoit L, 2016-04-26 14:57:25]

lizeb@chromium.org changed reviewers:
+ yfriedman@chromium.org, yusufo@chromium.org

[Yaron, 2016-04-26 15:51:25]

good find!

https://codereview.chromium.org/1922123003/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java
(right):

https://codereview.chromium.org/1922123003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java:540:
// Prevent the menu window from leaking.
What about the case in |onStop| (for herb)?
Also is a null-check necessary? It's there in a few other cases and requires
toolbar to have been inflated

[Yusuf, 2016-04-26 16:36:41]

Great catch Benoit.

lgtm modulo Yaron's comments

https://codereview.chromium.org/1922123003/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java
(right):

https://codereview.chromium.org/1922123003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java:540:
// Prevent the menu window from leaking.
On 2016/04/26 15:51:25, Yaron wrote:
> What about the case in |onStop| (for herb)?
> Also is a null-check necessary? It's there in a few other cases and requires
> toolbar to have been inflated

We initialize AppMenuHandler in postinflationstartup which is during onCreate
which "should" always happen before finish, but I would still nullcheck just to
be safe here.

[Benoit L, 2016-04-26 17:09:55]

Thanks!

Yaron, PTAL.

https://codereview.chromium.org/1922123003/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java
(right):

https://codereview.chromium.org/1922123003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java:540:
// Prevent the menu window from leaking.
On 2016/04/26 15:51:25, Yaron wrote:
> What about the case in |onStop| (for herb)?
> Also is a null-check necessary? It's there in a few other cases and requires
> toolbar to have been inflated

I moved it to finish(). I'm not 100% sure that's the right thing though.
Done.

[Yaron, 2016-04-26 17:21:17]

On 2016/04/26 17:09:55, Benoit L wrote:
> Thanks!
> 
> Yaron, PTAL.
> 
>
https://codereview.chromium.org/1922123003/diff/1/chrome/android/java/src/org...
> File
>
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java
> (right):
> 
>
https://codereview.chromium.org/1922123003/diff/1/chrome/android/java/src/org...
>
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java:540:
> // Prevent the menu window from leaking.
> On 2016/04/26 15:51:25, Yaron wrote:
> > What about the case in |onStop| (for herb)?
> > Also is a null-check necessary? It's there in a few other cases and requires
> > toolbar to have been inflated
> 
> I moved it to finish(). I'm not 100% sure that's the right thing though.
> Done.

lgtm - seems reasonable

[Benoit L, 2016-04-27 08:14:24]

The CQ bit was checked by lizeb@chromium.org

[Benoit L, 2016-04-27 08:14:25]

The patchset sent to the CQ was uploaded after l-g-t-m from yusufo@chromium.org
Link to the patchset: https://codereview.chromium.org/1922123003/#ps20001
(title: "Address comments.")

[commit-bot: I haz the power, 2016-04-27 08:14:34]

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

[commit-bot: I haz the power, 2016-04-27 09:24:10]

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

[commit-bot: I haz the power, 2016-04-27 09:24:11]

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

[Benoit L, 2016-04-27 11:58:26]

The CQ bit was checked by lizeb@chromium.org

[Benoit L, 2016-04-27 11:58:26]

The patchset sent to the CQ was uploaded after l-g-t-m from yusufo@chromium.org,
yfriedman@chromium.org
Link to the patchset: https://codereview.chromium.org/1922123003/#ps40001
(title: "Tests.")

[commit-bot: I haz the power, 2016-04-27 11:58:35]

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

[commit-bot: I haz the power, 2016-04-27 12:42:30]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-04-27 12:43:42]

Description was changed from

==========
customtabs: Prevent the app menu window from leaking when finishing the
activity.

BUG=606783
==========

to

==========
customtabs: Prevent the app menu window from leaking when finishing the
activity.

BUG=606783

Committed: https://crrev.com/02c5a62089bed609d72622aeaec9583c89007f6d
Cr-Commit-Position: refs/heads/master@{#390054}
==========

[commit-bot: I haz the power, 2016-04-27 12:43:43]

Patchset 3 (id:??) landed as
https://crrev.com/02c5a62089bed609d72622aeaec9583c89007f6d
Cr-Commit-Position: refs/heads/master@{#390054}