The current group probably shouldn't be discarded if the last tab is closed but the user still has a tab open as an app tab. This can happen in a couple situations: - Select a non-app tab then close all non-app tabs. Tabs from another group appear immediately. - Select an app tab and click the close button of all the non-app tabs. Tab strip will now only contain app tabs but opening a new tab will cause another group's tabs to appear.

At minimum, the 2 different initial behaviors is confusing (other group's tabs appear immediately vs appearing only after creating a new tab). What I was actually trying to do was for my "restaurants" group, I was getting rid of all the tabs with cmd-w then hit cmd-t to open yelp. But at that point, I was in some other random group.

I can't reproduce this on trunk. Does this still happen for you?

Still happens for me. With both STRs.

bugspam (moving b9 to b10)

bugspam (removing b9)

softblocker = critical

(In reply to comment #0) > The current group probably shouldn't be discarded if the last tab is closed but > the user still has a tab open as an app tab. This can happen in a couple > situations: > > - Select a non-app tab then close all non-app tabs. Tabs from another group > appear immediately This makes sense to me. > > - Select an app tab and click the close button of all the non-app tabs. Tab > strip will now only contain app tabs but opening a new tab will cause another > group's tabs to appear. Even there are app tabs, I think when the last non-app tab is closed in a group, we should show other group's tabs to make things consistent. Another suggestions?

Bug 624939 is a dupe of this bug, with a patch but no blocking status. So, is it better to move the patch to the blocker, or move the blocking status to the bug with the patch?

(In reply to comment #8) > Bug 624939 is a dupe of this bug, with a patch but no blocking status. So, is > it better to move the patch to the blocker, or move the blocking status to the > bug with the patch? I think it's better to move the patch to this blocker. Assigning to pwalton@mozilla.com

Here's a new version of the patch that rebases to tip, adds a test, and addresses Ian's review comments.

(In reply to comment #7) > (In reply to comment #0) > Even there are app tabs, I think when the last non-app tab is closed in a > group, we should show other group's tabs to make things consistent. > > Another suggestions? It isnt't nice! I dont want my group to close. It could have an "confirm" alert, at least... But I noticed that if the group has a name, it won't close even if all the non-app tabs are closed! Its good :)

Comment on attachment 505321 [details] [diff] [review] Proposed patch. >- if (!GroupItems.getUnclosableGroupItemId()) { We do still need this; it makes sure we never close the last group if there are app tabs. It probably belongs in closeIfEmpty(). Adding Raymond, who wrote that code, to make sure I've got it right. >+++ b/browser/base/content/test/tabview/browser_tabview_bug612470.js >@@ -0,0 +1,76 @@ >+/* Any copyright is dedicated to the Public Domain. >+ http://creativecommons.org/publicdomain/zero/1.0/ */ Please use the standard license block we've been using in the rest of the tests. Otherwise looks great!

Comment on attachment 505321 [details] [diff] [review] Proposed patch. Agreed with Ian, we need should close the last group if it has one or more app tabs.

Patch version 2 addresses review comments.

Comment on attachment 505651 [details] [diff] [review] Proposed patch, version 2. Thanks for making the changes! One thing: > >- if (!GroupItems.getUnclosableGroupItemId()) { > > We do still need this; it makes sure we never close the last group if there are > app tabs. It probably belongs in closeIfEmpty(). On further reflection, it definitely needs to go in closeIfEmpty(); otherwise there'll be a loophole that will allow you to end up with no groups if you have app tabs. R+ with that fixed.

i am not sure if this is a separate bug or not: after closing the last non-app tab as described in this bug, focus moves to the app-tab in the switched-to group, instead of to the last focused tab in that group. is the tab focus after closing a group an issue, or should the view always go back to tabcandy anyway?

(In reply to comment #17) > i am not sure if this is a separate bug or not: > after closing the last non-app tab as described in this bug, focus moves to the > app-tab in the switched-to group, instead of to the last focused tab in that > group. > is the tab focus after closing a group an issue, or should the view always go > back to tabcandy anyway? This patch should fix that, as you'll remain the same group rather than switching.

bugspam. Moving b10 to b11

bugspam. Removing b10

Patch version 3 fixes test bustage.

Comment on attachment 506032 [details] [diff] [review] Proposed patch, version 3. Looks great! Verified that this passes try for me locally... have you pushed to try?

(In reply to comment #22) > Comment on attachment 506032 [details] [diff] [review] > Proposed patch, version 3. > > Looks great! Verified that this passes try for me locally... have you pushed to > try? Yup, it works.

Comment on attachment 506032 [details] [diff] [review] Proposed patch, version 3. >+ let dontClose = !item.closedManually && gBrowser._numPinnedTabs > 0; >+ self.remove(item, { dontClose: dontClose }); I imagine you don't need that _numPinnedTabs check now that closeIfEmpty is calling getUnclosableGroupItemId, yes? Anyway, r+

(In reply to comment #24) > Comment on attachment 506032 [details] [diff] [review] > Proposed patch, version 3. > > >+ let dontClose = !item.closedManually && gBrowser._numPinnedTabs > 0; > >+ self.remove(item, { dontClose: dontClose }); > > I imagine you don't need that _numPinnedTabs check now that closeIfEmpty is > calling getUnclosableGroupItemId, yes? > > Anyway, r+ We definitely need it; I just checked and removing that check reintroduces the bug.

Causing oranges on Windows only: TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/tabview/browser_tabview_bug625424.js | there are 2 tabs in this groupItem - Got 1, expected 2 TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/tabview/browser_tabview_bug625424.js | Found an unexpected tab at the end of test run: about:blank Backing out.

Backed out: http://hg.mozilla.org/mozilla-central/rev/ba6017b613c8

You weren't actually at fault, we just failed to notice that the push right before you landed that test, but the hateful buildbot decided not to bother running Windows Moth on that push, so it looked like it started from your push because yours was the first that actually ran the test. It kept failing after your backout, wound up fixed by http://hg.mozilla.org/mozilla-central/rev/5a0f46c26d12

Backed out again: http://hg.mozilla.org/mozilla-central/rev/14c9cceb7649

The test for private browsing passes. I made some little changes, so requesting review again: 1.) groupItem.close() can now be called with an options object. The only supported option is {immediately: true} so that the groupItem is not closed animatedly. This was the timing problem with the private browsing test. The only part where this argument is given is in GroupItems.reconstitute() where unknown groups are deleted when the tabview state is restored from session. 2.) Another test started failing (that one restored a tab). So I merged the tabview part from bug 628270 to fix this. Pushed to try. http://tbpl.mozilla.org/?tree=MozillaTry&pusher=tim.taubert@gmx.de&rev=38a71e2896d0

Passed try.

Comment on attachment 507374 [details] [diff] [review] patch v4 Looks great

This patch badly fails to apply.

Already checked in: http://hg.mozilla.org/mozilla-central/rev/ea8bf490e66d Sorry for neglecting to update this.

verfieid with Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b12pre) Gecko/20110204 Firefox/4.0b12pre