Tab manager view should hide on press of back key

Tab manager view should hide on press of back key

Currently when tab manager is shown and user presses back key
then instead of hiding tab manager page is navigated to previous
entry. If there is nothing to navigate then application is exited.
Added code to hide tab manager view if back key is pressed and tab
manager view is on.

BUG=

Committed: https://crrev.com/85552242275fd01d18f5e84730da6b5acee35492
Cr-Commit-Position: refs/heads/master@{#302222}

[ankit, 2014-10-29 08:50:51]

ankit2.kumar@samsung.com changed reviewers:
+ bauerb@chromium.org

[ankit, 2014-10-29 08:50:51]

@Bernhard
PTAL

[Bernhard Bauer, 2014-10-29 09:18:33]

https://codereview.chromium.org/689583002/diff/1/chrome/android/shell/java/sr...
File
chrome/android/shell/java/src/org/chromium/chrome/shell/ChromeShellActivity.java
(right):

https://codereview.chromium.org/689583002/diff/1/chrome/android/shell/java/sr...
chrome/android/shell/java/src/org/chromium/chrome/shell/ChromeShellActivity.java:207:
if (mTabManager != null && mTabManager.isTabSwitcherVisible()) {
Can the tab manager actually be null here?

[ankit, 2014-10-29 09:23:43]

PTAL comment and suggest.

https://codereview.chromium.org/689583002/diff/1/chrome/android/shell/java/sr...
File
chrome/android/shell/java/src/org/chromium/chrome/shell/ChromeShellActivity.java
(right):

https://codereview.chromium.org/689583002/diff/1/chrome/android/shell/java/sr...
chrome/android/shell/java/src/org/chromium/chrome/shell/ChromeShellActivity.java:207:
if (mTabManager != null && mTabManager.isTabSwitcherVisible()) {
On 2014/10/29 09:18:33, Bernhard Bauer wrote:
> Can the tab manager actually be null here?

As per my understanding it should not be null at this point. But here only we
are making call for getActiveTab() and in getActiveTab() we are  checking for
tab manager null condition. So I added null condition check.
Please suggest if it's required or not.

[ankit, 2014-10-30 10:44:15]

On 2014/10/29 09:23:43, ankit wrote:
> PTAL comment and suggest.
> 
>
https://codereview.chromium.org/689583002/diff/1/chrome/android/shell/java/sr...
> File
>
chrome/android/shell/java/src/org/chromium/chrome/shell/ChromeShellActivity.java
> (right):
> 
>
https://codereview.chromium.org/689583002/diff/1/chrome/android/shell/java/sr...
>
chrome/android/shell/java/src/org/chromium/chrome/shell/ChromeShellActivity.java:207:
> if (mTabManager != null && mTabManager.isTabSwitcherVisible()) {
> On 2014/10/29 09:18:33, Bernhard Bauer wrote:
> > Can the tab manager actually be null here?
> 
> As per my understanding it should not be null at this point. But here only we
> are making call for getActiveTab() and in getActiveTab() we are  checking for
> tab manager null condition. So I added null condition check.
> Please suggest if it's required or not.

@Bernhard
PTAL and suggest

[Bernhard Bauer, 2014-10-30 11:12:12]

https://codereview.chromium.org/689583002/diff/1/chrome/android/shell/java/sr...
File
chrome/android/shell/java/src/org/chromium/chrome/shell/ChromeShellActivity.java
(right):

https://codereview.chromium.org/689583002/diff/1/chrome/android/shell/java/sr...
chrome/android/shell/java/src/org/chromium/chrome/shell/ChromeShellActivity.java:207:
if (mTabManager != null && mTabManager.isTabSwitcherVisible()) {
On 2014/10/29 09:23:43, ankit wrote:
> On 2014/10/29 09:18:33, Bernhard Bauer wrote:
> > Can the tab manager actually be null here?
> 
> As per my understanding it should not be null at this point. But here only we
> are making call for getActiveTab() and in getActiveTab() we are  checking for
> tab manager null condition. So I added null condition check.
> Please suggest if it's required or not.

Hm... I could imagine that getActiveTab() might be called in a situation where
the tab manager is null (e.g. during initialization). I'm not so sure whether
the same is the case for onKeyUp(). Can we just remove the null-check and see if
something crashes?

[ankit, 2014-10-30 13:59:16]

@Bernhard
PTAL
Verified and removed null check.

https://codereview.chromium.org/689583002/diff/1/chrome/android/shell/java/sr...
File
chrome/android/shell/java/src/org/chromium/chrome/shell/ChromeShellActivity.java
(right):

https://codereview.chromium.org/689583002/diff/1/chrome/android/shell/java/sr...
chrome/android/shell/java/src/org/chromium/chrome/shell/ChromeShellActivity.java:207:
if (mTabManager != null && mTabManager.isTabSwitcherVisible()) {
On 2014/10/30 11:12:12, Bernhard Bauer wrote:
> On 2014/10/29 09:23:43, ankit wrote:
> > On 2014/10/29 09:18:33, Bernhard Bauer wrote:
> > > Can the tab manager actually be null here?
> > 
> > As per my understanding it should not be null at this point. But here only
we
> > are making call for getActiveTab() and in getActiveTab() we are  checking
for
> > tab manager null condition. So I added null condition check.
> > Please suggest if it's required or not.
> 
> Hm... I could imagine that getActiveTab() might be called in a situation where
> the tab manager is null (e.g. during initialization). I'm not so sure whether
> the same is the case for onKeyUp(). Can we just remove the null-check and see
if
> something crashes?

Removed null check as no crash is happening after removing null check.

[Bernhard Bauer, 2014-10-30 16:35:45]

lgtm

[ankit, 2014-10-31 04:07:07]

On 2014/10/30 16:35:45, Bernhard Bauer wrote:
> lgtm

Thanks.

[ankit, 2014-10-31 04:07:12]

The CQ bit was checked by ankit2.kumar@samsung.com

[commit-bot: I haz the power, 2014-10-31 04:08:22]

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

[commit-bot: I haz the power, 2014-10-31 04:51:07]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2014-10-31 04:52:05]

Patchset 2 (id:??) landed as
https://crrev.com/85552242275fd01d18f5e84730da6b5acee35492
Cr-Commit-Position: refs/heads/master@{#302222}