Opening new "about:blank" tab if all tabs are closed in tab switcher mode.

Opening new "about:blank" tab if all tabs are closed in tab switcher mode.

Currently in tab switcher mode if we remove all the tabs and tap on Url
bar to open new link, suggestion popup doesnot appear. In these changes,
after all tabs are closed a new "about:blank" tab will be opened.

Signed-off-by: divyabansal <divya.bansal@samsung.com>;

BUG=433808
Committed: https://crrev.com/bd59f3e5f60a667ab5ee93d074e1b0575a8b54d9
Cr-Commit-Position: refs/heads/master@{#305212}

[divya.bansal, 2014-11-18 06:33:47]

divya.bansal@samsung.com changed reviewers:
+ bauerb@chromium.org

[divya.bansal, 2014-11-18 06:34:10]

@Bernhard PTAL

[Bernhard Bauer, 2014-11-18 09:05:49]

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

https://codereview.chromium.org/738693002/diff/1/chrome/android/shell/java/sr...
chrome/android/shell/java/src/org/chromium/chrome/shell/TabManager.java:60:
setCurrentTab(mCurrentTab);
Wait, why are we doing this?

[divya.bansal, 2014-11-18 09:20:19]

@Bernhard Please suggest.

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

https://codereview.chromium.org/738693002/diff/1/chrome/android/shell/java/sr...
chrome/android/shell/java/src/org/chromium/chrome/shell/TabManager.java:60:
setCurrentTab(mCurrentTab);
On 2014/11/18 09:05:48, Bernhard Bauer wrote:
> Wait, why are we doing this?

To get last tab so that its profile and url info we can use to create suggestion
popup.Otherwise in SuggestionPopUp.java, in afterTextChanged() function, if
mToolbar.getCurrentTab() == null, we return.

[Bernhard Bauer, 2014-11-18 09:27:58]

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

https://codereview.chromium.org/738693002/diff/1/chrome/android/shell/java/sr...
chrome/android/shell/java/src/org/chromium/chrome/shell/TabManager.java:60:
setCurrentTab(mCurrentTab);
On 2014/11/18 09:20:19, divya.bansal wrote:
> On 2014/11/18 09:05:48, Bernhard Bauer wrote:
> > Wait, why are we doing this?
> 
> To get last tab so that its profile and url info we can use to create
suggestion
> popup.Otherwise in SuggestionPopUp.java, in afterTextChanged() function, if
> mToolbar.getCurrentTab() == null, we return.

But this is called right before we close the tab. The caller assumes that the
tab doesn't exist anymore, and it's removed from the model. We can't just keep
the tab around.

We could do something like opening a new tab with about:blank if the last tab
(not necessarily the current one!) is closed.

[divya.bansal, 2014-11-19 09:51:31]

@Bernhard PTAL

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

https://codereview.chromium.org/738693002/diff/1/chrome/android/shell/java/sr...
chrome/android/shell/java/src/org/chromium/chrome/shell/TabManager.java:60:
setCurrentTab(mCurrentTab);
On 2014/11/18 09:27:58, Bernhard Bauer wrote:
> On 2014/11/18 09:20:19, divya.bansal wrote:
> > On 2014/11/18 09:05:48, Bernhard Bauer wrote:
> > > Wait, why are we doing this?
> > 
> > To get last tab so that its profile and url info we can use to create
> suggestion
> > popup.Otherwise in SuggestionPopUp.java, in afterTextChanged() function, if
> > mToolbar.getCurrentTab() == null, we return.
> 
> But this is called right before we close the tab. The caller assumes that the
> tab doesn't exist anymore, and it's removed from the model. We can't just keep
> the tab around.
> 
> We could do something like opening a new tab with about:blank if the last tab
> (not necessarily the current one!) is closed.

Done.

[Bernhard Bauer, 2014-11-19 09:57:34]

https://codereview.chromium.org/738693002/diff/20001/chrome/android/java/src/...
File
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabModelBase.java
(right):

https://codereview.chromium.org/738693002/diff/20001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabModelBase.java:456:
for (TabModelObserver obs : mObservers) obs.didCloseTab(tab);
Why this change? didCloseTab() means that closing the tab is done, but here we
are just starting to close the tab (it's not instantaneous because there's
animations etc.)

[divya.bansal, 2014-11-19 10:04:24]

@Bernhard Please suggest

https://codereview.chromium.org/738693002/diff/20001/chrome/android/java/src/...
File
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabModelBase.java
(right):

https://codereview.chromium.org/738693002/diff/20001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabModelBase.java:456:
for (TabModelObserver obs : mObservers) obs.didCloseTab(tab);
On 2014/11/19 09:57:34, Bernhard Bauer wrote:
> Why this change? didCloseTab() means that closing the tab is done, but here we
> are just starting to close the tab (it's not instantaneous because there's
> animations etc.)

Here after tab is removed didclosetab is called.If didclosetab() in
finalizetabclosure is used, that is not instantaneous and there after tab is
removed focus goes to url bar and then after some time new tab opens.
But here its happening instantaneously.

[Bernhard Bauer, 2014-11-19 10:06:45]

https://codereview.chromium.org/738693002/diff/20001/chrome/android/java/src/...
File
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabModelBase.java
(right):

https://codereview.chromium.org/738693002/diff/20001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabModelBase.java:456:
for (TabModelObserver obs : mObservers) obs.didCloseTab(tab);
On 2014/11/19 10:04:24, divya.bansal wrote:
> On 2014/11/19 09:57:34, Bernhard Bauer wrote:
> > Why this change? didCloseTab() means that closing the tab is done, but here
we
> > are just starting to close the tab (it's not instantaneous because there's
> > animations etc.)
> 
> Here after tab is removed didclosetab is called.If didclosetab() in
> finalizetabclosure is used, that is not instantaneous and there after tab is
> removed focus goes to url bar and then after some time new tab opens.
> But here its happening instantaneously.

Right, but you can't just go ahead and change the semantics of existing
notifications. That is almost guaranteed to break things.

Does using willCloseTab not work?

[divya.bansal, 2014-11-19 10:09:39]

https://codereview.chromium.org/738693002/diff/20001/chrome/android/java/src/...
File
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabModelBase.java
(right):

https://codereview.chromium.org/738693002/diff/20001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabModelBase.java:456:
for (TabModelObserver obs : mObservers) obs.didCloseTab(tab);
On 2014/11/19 10:06:45, Bernhard Bauer wrote:
> On 2014/11/19 10:04:24, divya.bansal wrote:
> > On 2014/11/19 09:57:34, Bernhard Bauer wrote:
> > > Why this change? didCloseTab() means that closing the tab is done, but
here
> we
> > > are just starting to close the tab (it's not instantaneous because there's
> > > animations etc.)
> > 
> > Here after tab is removed didclosetab is called.If didclosetab() in
> > finalizetabclosure is used, that is not instantaneous and there after tab is
> > removed focus goes to url bar and then after some time new tab opens.
> > But here its happening instantaneously.
> 
> Right, but you can't just go ahead and change the semantics of existing
> notifications. That is almost guaranteed to break things.
> 
> Does using willCloseTab not work?

willCloseTab() is set even before tab is removed in starttabclosure(), so there
still count !=0 and if in that i open a new tab, after that tab is removed.

[Bernhard Bauer, 2014-11-19 10:13:53]

https://codereview.chromium.org/738693002/diff/20001/chrome/android/java/src/...
File
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabModelBase.java
(right):

https://codereview.chromium.org/738693002/diff/20001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabModelBase.java:456:
for (TabModelObserver obs : mObservers) obs.didCloseTab(tab);
On 2014/11/19 10:09:39, divya.bansal wrote:
> On 2014/11/19 10:06:45, Bernhard Bauer wrote:
> > On 2014/11/19 10:04:24, divya.bansal wrote:
> > > On 2014/11/19 09:57:34, Bernhard Bauer wrote:
> > > > Why this change? didCloseTab() means that closing the tab is done, but
> here
> > we
> > > > are just starting to close the tab (it's not instantaneous because
there's
> > > > animations etc.)
> > > 
> > > Here after tab is removed didclosetab is called.If didclosetab() in
> > > finalizetabclosure is used, that is not instantaneous and there after tab
is
> > > removed focus goes to url bar and then after some time new tab opens.
> > > But here its happening instantaneously.
> > 
> > Right, but you can't just go ahead and change the semantics of existing
> > notifications. That is almost guaranteed to break things.
> > 
> > Does using willCloseTab not work?
> 
> willCloseTab() is set even before tab is removed in starttabclosure(), so
there
> still count !=0 and if in that i open a new tab, after that tab is removed.

Would it work if you call createNewTab() if the count is 1?

[divya.bansal, 2014-11-19 12:07:57]

PTAL

https://codereview.chromium.org/738693002/diff/20001/chrome/android/java/src/...
File
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabModelBase.java
(right):

https://codereview.chromium.org/738693002/diff/20001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabModelBase.java:456:
for (TabModelObserver obs : mObservers) obs.didCloseTab(tab);
On 2014/11/19 10:13:53, Bernhard Bauer wrote:
> On 2014/11/19 10:09:39, divya.bansal wrote:
> > On 2014/11/19 10:06:45, Bernhard Bauer wrote:
> > > On 2014/11/19 10:04:24, divya.bansal wrote:
> > > > On 2014/11/19 09:57:34, Bernhard Bauer wrote:
> > > > > Why this change? didCloseTab() means that closing the tab is done, but
> > here
> > > we
> > > > > are just starting to close the tab (it's not instantaneous because
> there's
> > > > > animations etc.)
> > > > 
> > > > Here after tab is removed didclosetab is called.If didclosetab() in
> > > > finalizetabclosure is used, that is not instantaneous and there after
tab
> is
> > > > removed focus goes to url bar and then after some time new tab opens.
> > > > But here its happening instantaneously.
> > > 
> > > Right, but you can't just go ahead and change the semantics of existing
> > > notifications. That is almost guaranteed to break things.
> > > 
> > > Does using willCloseTab not work?
> > 
> > willCloseTab() is set even before tab is removed in starttabclosure(), so
> there
> > still count !=0 and if in that i open a new tab, after that tab is removed.
> 
> Would it work if you call createNewTab() if the count is 1?

Done.

[Bernhard Bauer, 2014-11-19 12:13:40]

https://codereview.chromium.org/738693002/diff/60002/chrome/android/shell/jav...
File chrome/android/shell/java/src/org/chromium/chrome/shell/TabManager.java
(right):

https://codereview.chromium.org/738693002/diff/60002/chrome/android/shell/jav...
chrome/android/shell/java/src/org/chromium/chrome/shell/TabManager.java:131: if
(mTabModelSelector.getCurrentModel().getCount() == 1) {
This also changes the semantics of existing code. This method is currently
called at startup to ensure that at least one tab exists. Now, if there is no
tab open, it won't open a new one, and if there is a tab open, it will open
another one.

[divya.bansal, 2014-11-19 12:25:49]

Please suggest

https://codereview.chromium.org/738693002/diff/60002/chrome/android/shell/jav...
File chrome/android/shell/java/src/org/chromium/chrome/shell/TabManager.java
(right):

https://codereview.chromium.org/738693002/diff/60002/chrome/android/shell/jav...
chrome/android/shell/java/src/org/chromium/chrome/shell/TabManager.java:131: if
(mTabModelSelector.getCurrentModel().getCount() == 1) {
On 2014/11/19 12:13:40, Bernhard Bauer wrote:
> This also changes the semantics of existing code. This method is currently
> called at startup to ensure that at least one tab exists. Now, if there is no
> tab open, it won't open a new one, and if there is a tab open, it will open
> another one.

Yes thats true, actually in tabmodelbase.java getcount gives size of |tabs|
arraylist and willCloseTab() is called before removing tab in the arraylist. So
if we want to check for count =0, then after removing only we can check and
create new tab.
That is why , I was calling didclosetab after removing tab. didclosetab() is
also member of tabmodelobserver and that too need to be handled.

[Bernhard Bauer, 2014-11-19 12:54:39]

https://codereview.chromium.org/738693002/diff/60002/chrome/android/shell/jav...
File chrome/android/shell/java/src/org/chromium/chrome/shell/TabManager.java
(right):

https://codereview.chromium.org/738693002/diff/60002/chrome/android/shell/jav...
chrome/android/shell/java/src/org/chromium/chrome/shell/TabManager.java:131: if
(mTabModelSelector.getCurrentModel().getCount() == 1) {
On 2014/11/19 12:25:49, divya.bansal wrote:
> On 2014/11/19 12:13:40, Bernhard Bauer wrote:
> > This also changes the semantics of existing code. This method is currently
> > called at startup to ensure that at least one tab exists. Now, if there is
no
> > tab open, it won't open a new one, and if there is a tab open, it will open
> > another one.
> 
> Yes thats true, actually in tabmodelbase.java getcount gives size of |tabs|
> arraylist and willCloseTab() is called before removing tab in the arraylist.
So
> if we want to check for count =0, then after removing only we can check and
> create new tab.
> That is why , I was calling didclosetab after removing tab. didclosetab() is
> also member of tabmodelobserver and that too need to be handled.

I already told you what to do: in willCloseTab() (i.e. right before the tab is
closed), call createNewTab() if there is exactly one tab (because that means
that we are about to close the last tab).

[divya.bansal, 2014-11-19 14:48:39]

@Bernhard PTAL

https://codereview.chromium.org/738693002/diff/60002/chrome/android/shell/jav...
File chrome/android/shell/java/src/org/chromium/chrome/shell/TabManager.java
(right):

https://codereview.chromium.org/738693002/diff/60002/chrome/android/shell/jav...
chrome/android/shell/java/src/org/chromium/chrome/shell/TabManager.java:131: if
(mTabModelSelector.getCurrentModel().getCount() == 1) {
On 2014/11/19 12:54:39, Bernhard Bauer wrote:
> On 2014/11/19 12:25:49, divya.bansal wrote:
> > On 2014/11/19 12:13:40, Bernhard Bauer wrote:
> > > This also changes the semantics of existing code. This method is currently
> > > called at startup to ensure that at least one tab exists. Now, if there is
> no
> > > tab open, it won't open a new one, and if there is a tab open, it will
open
> > > another one.
> > 
> > Yes thats true, actually in tabmodelbase.java getcount gives size of |tabs|
> > arraylist and willCloseTab() is called before removing tab in the arraylist.
> So
> > if we want to check for count =0, then after removing only we can check and
> > create new tab.
> > That is why , I was calling didclosetab after removing tab. didclosetab() is
> > also member of tabmodelobserver and that too need to be handled.
> 
> I already told you what to do: in willCloseTab() (i.e. right before the tab is
> closed), call createNewTab() if there is exactly one tab (because that means
> that we are about to close the last tab).

Done.

[Bernhard Bauer, 2014-11-19 15:51:18]

https://codereview.chromium.org/738693002/diff/80001/chrome/android/shell/jav...
File chrome/android/shell/java/src/org/chromium/chrome/shell/TabManager.java
(right):

https://codereview.chromium.org/738693002/diff/80001/chrome/android/shell/jav...
chrome/android/shell/java/src/org/chromium/chrome/shell/TabManager.java:60: if
(mTabModelSelector.getCurrentModel().getCount() == 1)
You can only leave out braces if everything fits on a single line (like in the
line above).

[divya.bansal, 2014-11-20 03:54:14]

@Bernhard PTAL

https://codereview.chromium.org/738693002/diff/80001/chrome/android/shell/jav...
File chrome/android/shell/java/src/org/chromium/chrome/shell/TabManager.java
(right):

https://codereview.chromium.org/738693002/diff/80001/chrome/android/shell/jav...
chrome/android/shell/java/src/org/chromium/chrome/shell/TabManager.java:60: if
(mTabModelSelector.getCurrentModel().getCount() == 1)
On 2014/11/19 15:51:18, Bernhard Bauer wrote:
> You can only leave out braces if everything fits on a single line (like in the
> line above).

Done.

[Bernhard Bauer, 2014-11-20 09:57:51]

LGTM

As a general reminder though, ChromeShell is not meant to be a generic browser.
It's a shell for injecting test code, and as such is not even meant to be
directly used by humans! We're trying to avoid unnecessary code in ChromeShell,
and going forward will be pushing back on changes that are not directly related
to running tests in ChromeShell.

[divya.bansal, 2014-11-20 10:35:13]

On 2014/11/20 09:57:51, Bernhard Bauer wrote:
> LGTM
> 
> As a general reminder though, ChromeShell is not meant to be a generic
browser.
> It's a shell for injecting test code, and as such is not even meant to be
> directly used by humans! We're trying to avoid unnecessary code in
ChromeShell,
> and going forward will be pushing back on changes that are not directly
related
> to running tests in ChromeShell.

Ok. Thanks for the review and suggestions.

[divya.bansal, 2014-11-20 10:35:31]

The CQ bit was checked by divya.bansal@samsung.com

[commit-bot: I haz the power, 2014-11-20 10:36:11]

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

[commit-bot: I haz the power, 2014-11-20 11:13:57]

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

[commit-bot: I haz the power, 2014-11-20 11:13:57]

Try jobs failed on following builders:
  android_dbg_tests_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)

[divya.bansal, 2014-11-20 11:48:44]

The CQ bit was checked by divya.bansal@samsung.com

[commit-bot: I haz the power, 2014-11-20 11:49:53]

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

[commit-bot: I haz the power, 2014-11-20 12:32:11]

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

[commit-bot: I haz the power, 2014-11-20 12:32:12]

Try jobs failed on following builders:
  android_dbg_tests_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)

[divya.bansal, 2014-11-21 04:02:50]

The CQ bit was checked by divya.bansal@samsung.com

[commit-bot: I haz the power, 2014-11-21 04:04:28]

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

[commit-bot: I haz the power, 2014-11-21 04:46:40]

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

[commit-bot: I haz the power, 2014-11-21 04:46:41]

Try jobs failed on following builders:
  android_dbg_tests_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)

[divya.bansal, 2014-11-21 09:42:51]

The CQ bit was checked by divya.bansal@samsung.com

[commit-bot: I haz the power, 2014-11-21 09:43:49]

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

[commit-bot: I haz the power, 2014-11-21 10:19:40]

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

[commit-bot: I haz the power, 2014-11-21 10:19:41]

Try jobs failed on following builders:
  android_dbg_tests_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)

[Bernhard Bauer, 2014-11-21 10:27:12]

On 2014/11/21 10:19:41, I haz the power (commit-bot) wrote:
> Try jobs failed on following builders:
>   android_dbg_tests_recipe on tryserver.chromium.linux
>
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)

I suggest you find out why that bot is failing on your change instead before
trying it again?

[divya.bansal, 2014-11-21 11:07:48]

On 2014/11/21 10:27:12, Bernhard Bauer wrote:
> On 2014/11/21 10:19:41, I haz the power (commit-bot) wrote:
> > Try jobs failed on following builders:
> >   android_dbg_tests_recipe on tryserver.chromium.linux
> >
>
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)
> 
> I suggest you find out why that bot is failing on your change instead before
> trying it again?

Actually the test case testDialogDismissedAfterClosingTab() is running for
closeAllTabs() api but currently there is no option to close all the tabs and
browser is closed.
After removing last tab I am adding new tab, so this was failing.

[divya.bansal, 2014-11-21 11:07:59]

On 2014/11/21 11:07:48, divya.bansal wrote:
> On 2014/11/21 10:27:12, Bernhard Bauer wrote:
> > On 2014/11/21 10:19:41, I haz the power (commit-bot) wrote:
> > > Try jobs failed on following builders:
> > >   android_dbg_tests_recipe on tryserver.chromium.linux
> > >
> >
>
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)
> > 
> > I suggest you find out why that bot is failing on your change instead before
> > trying it again?
> 
> Actually the test case testDialogDismissedAfterClosingTab() is running for
> closeAllTabs() api but currently there is no option to close all the tabs and
> browser is closed.
> After removing last tab I am adding new tab, so this was failing.

PTAL

[Bernhard Bauer, 2014-11-21 11:24:08]

lgtm

[divya.bansal, 2014-11-21 12:06:23]

On 2014/11/21 11:24:08, Bernhard Bauer wrote:
> lgtm

@Bernahard Thanks for the review.

[divya.bansal, 2014-11-21 12:06:29]

The CQ bit was checked by divya.bansal@samsung.com

[commit-bot: I haz the power, 2014-11-21 12:07:10]

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

[commit-bot: I haz the power, 2014-11-21 12:15:13]

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

[commit-bot: I haz the power, 2014-11-21 12:15:15]

Try jobs failed on following builders:
  android_chromium_gn_compile_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)

[divya.bansal, 2014-11-21 13:41:14]

The CQ bit was checked by divya.bansal@samsung.com

[commit-bot: I haz the power, 2014-11-21 13:42:06]

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

[commit-bot: I haz the power, 2014-11-21 14:23:08]

Committed patchset #11 (id:180001)

[commit-bot: I haz the power, 2014-11-21 14:24:34]

Patchset 11 (id:??) landed as
https://crrev.com/bd59f3e5f60a667ab5ee93d074e1b0575a8b54d9
Cr-Commit-Position: refs/heads/master@{#305212}