896291 - "Close other tabs" should show the confirmation dialog if the number of tabs is greater than browser.sessionstore.max_tabs_undo

Status: VERIFIED FIXED

(Firefox :: Tabbed Browser, defect)

Description

[Olli Pettay [:smaug][[e-mail]]]

about:config says I have browser.tabs.warnOnCloseOtherTabs set to default which is true.
and I'm 100% "Close other tabs" used to have confirmation dialog with a number
of to-be-closed tabs, since I used that to check how many tabs I have open.
I just lost around 150 tabs because there isn't the dialog anymore :/

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

Just reproduced with another profile.

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

Note, I haven't figured out a way to restore all the closed tabs.
FF seem to restore only some tabs. Max 10 or so.

Comment 3

[Michael]

Would creating a sqlite database of closed tabs be feasible in this case?

Comment 4

[:Gavin Sharp [email: [e-mail]]]

See bug 887515. Apparently there's a menu item that you can use to restore all of the closed tabs (either the same as the existing menu but its behavior changed, or its a different menu option? I'm not sure).

Comment 5

[Jared Wein [:jaws] (please needinfo? me)]

Yes, you can click on "Undo Closed Tabs" (note the plurality) to undo the action. When only one tab has been closed it should read "Undo Closed Tab".

Comment 6

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Olli Pettay [:smaug] from comment #2)
> Note, I haven't figured out a way to restore all the closed tabs.
> FF seem to restore only some tabs. Max 10 or so.

This is a problem. Sorry for your loss of tabs. The dialog should be shown for more than our limit of tabs to undo-closing.

Comment 7

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: 896291.patch

I'm not sure if we should show the prompt if the user has browser.tabs.warnOnClose set to false. I think it might be worth it since they won't have a way to restore those tabs otherwise, but they've also opted in to not being warned.

Comment 8

[Jared Wein [:jaws] (please needinfo? me)]

Review ping for Tim or Dao?

Comment 9

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 779316 [details] [diff] [review]
896291.patch

Review of attachment 779316 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/tabbrowser.xml
@@ +1566,5 @@
>            }
>  
>            if (tabsToClose <= 1 ||
> +              (aCloseTabs != this.closingTabsEnum.ALL &&
> +               tabsToClose <= Services.prefs.getIntPref("browser.sessionstore.max_tabs_undo")) ||

Shouldn't the middle of the condition be (modified for readability):

let maxUndo = Services.prefs.getIntPref("browser.sessionstore.max_tabs_undo");
let warnOther = Services.prefs.getBoolPref("browser.tabs.warnOnCloseOtherTabs");

(aCloseTabs != this.closingTabsEnum.ALL && (tabsToClose <= maxUndo || !warnOther))

Comment 10

[Tim Taubert [:ttaubert] (inactive)]

Oh, wait. I think that's what you meant in your comment but you probably wanted to say 'warnOnCloseOtherTabs'. I think we should respect that setting, if not the pref is completely unused and could be removed.

Comment 11

[Tim Taubert [:ttaubert] (inactive)]

If we don't respect that setting and Olli has =< 10 tabs he'll be mad at us for closing these ;)

Comment 12

[Jared Wein [:jaws] (please needinfo? me)]

Hm, trying to remember what I was thinking back then is tough. Maybe I did mean warnOnCloseOtherTabs? I don't see a problem with keeping that pref around, it's probably what I should have used when writing this patch.

Should I upload a new patch that references the different pref now or is there anything else that I should change while I'm updating the patch?

Comment 13

[Tim Taubert [:ttaubert] (inactive)]

So... this feels like the condition is a little more complicated now. We don't want to show the confirmation dialog if:

1) tabsToClose <= 1

If there's only one tab to close we'll never warn you about that.

2) aCloseTabs == this.closingTabsEnum.ALL && !browser.tabs.warnOnClose

If the whole window is going to be closed we never warn with warnOnClose=false.

3) aCloseTabs != this.closingTabsEnum.ALL &&
   (!browser.tabs.warnOnCloseOtherTabs ||
    tabsToClose <= browser.sessionstore.max_tabs_undo)

If "other tabs" or "tabs to the right" are closed we never warn with warnOnCloseOtherTabs=false. We also don't warn if we're closing less than max_tabs_undo tabs.

So basically, if (1 || 2 || 3) return true; Did I miss anything? The test lgtm.

(Please disregard comment #11, I forgot that we want to explicitly remove the warning dialog for users as long as we can restore all those tabs.)

Comment 14

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Tim Taubert [:ttaubert] from comment #13)
> So... this feels like the condition is a little more complicated now. We
> don't want to show the confirmation dialog if:
> 
> 1) tabsToClose <= 1
> 
> If there's only one tab to close we'll never warn you about that.
> 
> 2) aCloseTabs == this.closingTabsEnum.ALL && !browser.tabs.warnOnClose
> 
> If the whole window is going to be closed we never warn with
> warnOnClose=false.
> 
> 3) aCloseTabs != this.closingTabsEnum.ALL &&
>    (!browser.tabs.warnOnCloseOtherTabs ||
>     tabsToClose <= browser.sessionstore.max_tabs_undo)
> 
> If "other tabs" or "tabs to the right" are closed we never warn with
> warnOnCloseOtherTabs=false. We also don't warn if we're closing less than
> max_tabs_undo tabs.
> 
> So basically, if (1 || 2 || 3) return true; Did I miss anything? The test
> lgtm.
> 
> (Please disregard comment #11, I forgot that we want to explicitly remove
> the warning dialog for users as long as we can restore all those tabs.)

This is almost correct.

For case 3, we need to disregard the warnOnCloseOtherTabs preference when tabsToClose > max_tabs_undo and always show the warning in this case since there is no undo.

Comment 15

[Tim Taubert [:ttaubert] (inactive)]

Hmm. I'm not sure if we should do that, we certainly didn't do it before. On the one hand the user opted into not being warned about closing other tabs. OTOH, they might expect that to be revertible if done by mistake.

I see where you're coming from but we'd change the current behavior by doing that. And while there certainly are users that are lucky to be notified about an irreversible change, there will be users that are annoyed we don't respect their preferences as we did before.

IMO, the right thing with (warnOnCloseOther=false && tabsToClose > max_tabs_undo) would probably be to not warn and temporarily raise the max_tabs_undo limit, but that might not be totally trivial to implement.

Comment 16

[Dão Gottwald [:dao]]

(In reply to Jared Wein [:jaws] from comment #14)
> For case 3, we need to disregard the warnOnCloseOtherTabs preference when
> tabsToClose > max_tabs_undo and always show the warning in this case since
> there is no undo.

The whole point of the (hidden) warnOnCloseOtherTabs pref is to allow users to disable that warning.

Comment 17

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch v2

Comment 18

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 787143 [details] [diff] [review]
Patch v2

Review of attachment 787143 [details] [diff] [review]:
-----------------------------------------------------------------

The test unfortunately fails when run in single-mode on my machine:

TEST-UNEXPECTED-FAIL | browser_bug896291.js | All other tabs should be closed when the 'OK' option on the prompt is chosen - Got 4, expected 1
TEST-UNEXPECTED-FAIL | browser_bug896291.js | Found an unexpected tab at the end of test run: http://mochi.test:8888/
TEST-UNEXPECTED-FAIL | browser_bug896291.js | Found an unexpected tab at the end of test run: http://mochi.test:8888/
TEST-UNEXPECTED-FAIL | browser_bug896291.js | Found an unexpected tab at the end of test run: http://mochi.test:8888/

::: browser/base/content/tabbrowser.xml
@@ +1639,5 @@
>            }
>  
> +          let maxUndo =
> +            Services.prefs.getIntPref("browser.sessionstore.max_tabs_undo");
> +          let warnOnOtherTabs =

Nit: maybe warnOnCloseOtherTabs?

@@ +1641,5 @@
> +          let maxUndo =
> +            Services.prefs.getIntPref("browser.sessionstore.max_tabs_undo");
> +          let warnOnOtherTabs =
> +            Services.prefs.getBoolPref("browser.tabs.warnOnCloseOtherTabs");
> +          let warnOnWindow =

Nit: maybe warnOnCloseWindow?

@@ +1649,4 @@
>            if (tabsToClose <= 1 ||
> +              (aCloseTabs == closingAll && !warnOnWindow) ||
> +              (aCloseTabs != closingAll && (!warnOnOtherTabs ||
> +                                            tabsToClose <= maxUndo)))

Can you please add a comment that breaks this condition down and explains the rationale behind it? I just had to re-read my comment because even with your changes it's not quite readable enough IMO, especially if one hasn't touched that code for a while.

Comment 19

[tabmix.onemen]

Don't forget to update the preference

>
>-    if (reallyClose && !warnOnClose.value)
>+    if (reallyClose && !warnOnClose.value) {
>+      let pref = aCloseTabs == this.closingTabsEnum.ALL ?
>+                 "browser.tabs.warnOnClose" : "browser.tabs.warnOnCloseOtherTabs"
>       Services.prefs.setBoolPref(pref, false);
>+    }
>

Comment 20

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch

The test was passing when I uploaded the patch. Bug 894048 landed in the interim, I've disabled the tab animation preference in the test now to get the test passing.

(In reply to onemen.one from comment #19)
> Don't forget to update the preference
> 
> >
> >-    if (reallyClose && !warnOnClose.value)
> >+    if (reallyClose && !warnOnClose.value) {
> >+      let pref = aCloseTabs == this.closingTabsEnum.ALL ?
> >+                 "browser.tabs.warnOnClose" : "browser.tabs.warnOnCloseOtherTabs"
> >       Services.prefs.setBoolPref(pref, false);
> >+    }
> >

This was fixed in bug 909662.

Comment 21

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 799701 [details] [diff] [review]
Patch

Felipe, can you review this since Tim is on PTO?

Comment 22

[tabmix.onemen]

(In reply to Jared Wein [:jaws] from comment #20)
> Created attachment 799701 [details] [diff] [review]
> Patch
> 
> The test was passing when I uploaded the patch. Bug 894048 landed in the
> interim, I've disabled the tab animation preference in the test now to get
> the test passing.
> 
> (In reply to onemen.one from comment #19)
> > Don't forget to update the preference
> > 
> > >
> > >-    if (reallyClose && !warnOnClose.value)
> > >+    if (reallyClose && !warnOnClose.value) {
> > >+      let pref = aCloseTabs == this.closingTabsEnum.ALL ?
> > >+                 "browser.tabs.warnOnClose" : "browser.tabs.warnOnCloseOtherTabs"
> > >       Services.prefs.setBoolPref(pref, false);
> > >+    }
> > >
> 
> This was fixed in bug 909662.

NO.
bug 909662 only deal with "browser.tabs.warnOnClose"
with this bug, user also can see the confirmation dialog when closing other tab
and when user un-check the box in the dialog you need to set "browser.tabs.warnOnCloseOtherTabs" to false not "browser.tabs.warnOnClose"

Comment 23

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 799701 [details] [diff] [review]
Patch

Ok, sorry for the misunderstanding. I see what you mean here.

Comment 24

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch v1.1

Now updating the preference correctly.

Comment 25

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 800907 [details] [diff] [review]
Patch v1.1

As Tim is back, sending it to him since he has already thought more about this change

Comment 26

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 800907 [details] [diff] [review]
Patch v1.1

Review of attachment 800907 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/tabbrowser.xml
@@ +1647,5 @@
> +          let warnOnCloseOtherTabs =
> +            Services.prefs.getBoolPref("browser.tabs.warnOnCloseOtherTabs");
> +          let warnOnCloseWindow =
> +            Services.prefs.getBoolPref("browser.tabs.warnOnClose");
> +          let closingAll = this.closingTabsEnum.ALL;

Looks like instead of having a shortcut to the const we could save (aCloseTabs == this.closingTabsEnum.ALL) to a variable (maybe isWindowClosing) and use that in all the conditions.

::: browser/base/content/test/browser_bug896291.js
@@ +75,5 @@
> +
> +  originalTab = gBrowser.selectedTab;
> +  gBrowser.selectedBrowser.loadURI("http://mochi.test:8888/");
> +
> +  maxTabsUndo = Services.prefs.getIntPref("browser.sessionstore.max_tabs_undo");

We need to clear this pref after the test finishes as well.

Comment 27

[Jared Wein [:jaws] (please needinfo? me)]

This should no longer track Firefox 25 since the patches that caused this regression were backed out in bug 914258. Moving tracking to Firefox 26.

Comment 28

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch v1.2

(In reply to Tim Taubert [:ttaubert] from comment #26)
> Comment on attachment 800907 [details] [diff] [review]
> Patch v1.1
> 
> Review of attachment 800907 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/base/content/tabbrowser.xml
> @@ +1647,5 @@
> > +          let warnOnCloseOtherTabs =
> > +            Services.prefs.getBoolPref("browser.tabs.warnOnCloseOtherTabs");
> > +          let warnOnCloseWindow =
> > +            Services.prefs.getBoolPref("browser.tabs.warnOnClose");
> > +          let closingAll = this.closingTabsEnum.ALL;
> 
> Looks like instead of having a shortcut to the const we could save
> (aCloseTabs == this.closingTabsEnum.ALL) to a variable (maybe
> isWindowClosing) and use that in all the conditions.

Agreed, change made.

> ::: browser/base/content/test/browser_bug896291.js
> @@ +75,5 @@
> > +
> > +  originalTab = gBrowser.selectedTab;
> > +  gBrowser.selectedBrowser.loadURI("http://mochi.test:8888/");
> > +
> > +  maxTabsUndo = Services.prefs.getIntPref("browser.sessionstore.max_tabs_undo");
> 
> We need to clear this pref after the test finishes as well.

This pref isn't changed by the test, so no need to reset it when the test finishes.

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/0da508ef253c

Comment 30

[Ryan VanderMeulen [:RyanVM]]

Backed out for mochitest-bc failures.
https://hg.mozilla.org/integration/fx-team/rev/ef744779aca2

https://tbpl.mozilla.org/php/getParsedLog.php?id=27982536&tree=Fx-Team

Comment 31

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch v1.21

Fixed test, it was broken when run after a test that closed multiple tabs.

Comment 32

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/2e69de47685e

Comment 33

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/fx-team/rev/b27ea519eebe since something in that wad of checkin-needed caused browser_homeDrop.js to time out, and according to that log in comment 30, you've already done that once.

Comment 34

[Jared Wein [:jaws] (please needinfo? me)]

I have fixed the browser_homeDrop.js hang in bug 918069.

The patch in that bug and the patch that I'll land for this bug depend on bug 917887 which moves the test directory.

Comment 35

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch v2

This patch avoids the issues that I was having with the mock object factory by using the WindowListener pattern. This will also be a safer patch to uplift to Aurora.

Pushed to tryserver: https://tbpl.mozilla.org/?tree=Try&rev=6283aede0c9c

Comment 36

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 813956 [details] [diff] [review]
Patch v2

The production code changes were already r+'d by Tim, so this patch only contains changes to the test code. I talked with Matt about this earlier, so he may be able to review it as well.

Comment 37

[Matthew N. [:MattN]]

Comment on attachment 813956 [details] [diff] [review]
Patch v2

Review of attachment 813956 [details] [diff] [review]:
-----------------------------------------------------------------

r=me on the test

::: browser/base/content/test/general/browser.ini
@@ +198,5 @@
>  [browser_bug839103.js]
>  [browser_bug880101.js]
>  [browser_bug882977.js]
>  [browser_bug887515.js]
> +[browser_bug896291.js]

Better test filename please

::: browser/base/content/test/general/browser_bug896291.js
@@ +7,5 @@
> +let maxTabsUndo;
> +let maxTabsUndoPlusOne;
> +
> +function verifyUndoMultipleClose() {
> +  let cancelRemoveAllTabsDialogListener = new WindowListener("chrome://global/content/commonDialog.xul", function (domwindow) {

Nit: s/domwindow/domWindow/g

@@ +12,5 @@
> +    ok(true, "dialog appeared in response to multiple tab close action");
> +    domwindow.document.documentElement.cancelDialog();
> +    Services.wm.removeListener(cancelRemoveAllTabsDialogListener);
> +
> +    let acceptRemoveAllTabsDialogListener = new WindowListener("chrome://global/content/commonDialog.xul", function (domwindow) {

Nit: name the anonymous functions (4x) with non-trivial bodies.

@@ +13,5 @@
> +    domwindow.document.documentElement.cancelDialog();
> +    Services.wm.removeListener(cancelRemoveAllTabsDialogListener);
> +
> +    let acceptRemoveAllTabsDialogListener = new WindowListener("chrome://global/content/commonDialog.xul", function (domwindow) {
> +      ok(true, "dialog appeared in response to multiple tab close action");

Nit: It would be a bit easier to read if the acceptRemoveAllTabsDialogListener sub-test was a separate top-level function.

@@ +19,5 @@
> +      Services.wm.removeListener(acceptRemoveAllTabsDialogListener);
> +
> +      waitForCondition(function () gBrowser.tabs.length == 1, function () {
> +        is(gBrowser.tabs.length, 1,
> +           "All other tabs should be closed when the 'OK' option on the prompt is chosen");

s/when/after/

@@ +25,5 @@
> +      }, "Waited too long for the other tabs to be closed.");
> +    });
> +
> +    Services.wm.addListener(acceptRemoveAllTabsDialogListener);
> +    waitForCondition(function () gBrowser.tabs.length == 1 + maxTabsUndoPlusOne, function () {

Is the waitForCondition really necessary here?

@@ +87,5 @@
> +    var self = this;
> +    domwindow.addEventListener("load", function() {
> +      domwindow.removeEventListener("load", arguments.callee, false);
> +
> +      ok(true, "domwindow.document.location.href: " + domwindow.document.location.href);

This seems mostly informational so it can be an info() instead.

Comment 38

[Jared Wein [:jaws] (please needinfo? me)]

Thanks, fixed the nits and pushed to fx-team:
https://hg.mozilla.org/integration/fx-team/rev/1dd4cd4890fc

Comment 39

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1dd4cd4890fc

Comment 40

[Jared Wein [:jaws] (please needinfo? me)]

I've filed bug 931891 to back out the patches that caused this regression from Fx26 and Fx27. This is similar to what we did for bug 914258.

Comment 41

[Jared Wein [:jaws] (please needinfo? me)]

Removing tracking from Firefox 26 since the patches that caused this regression have been backed out of Beta 26 in bug 931891. The patches are also expected to be backed out of Fx27 along with the rest of this feature. The feature is planned to be ride the trains once bug 895436 is fixed.

Comment 42

[Bogdan Maris, Desktop Test Engineering]

I have reproduced the issue that Olli had: Pref "browser.tabs.warnOnCloseOtherTabs" set to true, opened more then 10 tabs, click on "Close other tabs" (no confirmation dialog was prompted), used "Undo closed tabs" (only 10 tabs are reopened).

With latest Aurora the confirmation dialog is prompted but using "Undo closed tab" I can only reopen a max of 10 tabs. Is this expected, or there is already a bug on this?

This bug is verified as fixed since this is about Close other tabs not displaying the confirmation dialog.

Comment 43

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Bogdan Maris, QA [:bogdan_maris] from comment #42)
> I have reproduced the issue that Olli had: Pref
> "browser.tabs.warnOnCloseOtherTabs" set to true, opened more then 10 tabs,
> click on "Close other tabs" (no confirmation dialog was prompted), used
> "Undo closed tabs" (only 10 tabs are reopened).

I am assuming here you are talking about using a beta1 build? This feature will be backed out in beta2.

> With latest Aurora the confirmation dialog is prompted but using "Undo
> closed tab" I can only reopen a max of 10 tabs. Is this expected, or there
> is already a bug on this?

This is expected. We show the confirmation dialog when we will be unable to restore all of the closed tabs if the action proceeds any further. After accepting the dialog and closing the group of tabs, Undo Close Tabs will only restore as many as we can. This is not a regression from before, where the user could choose the "Undo Close Tab" menuitem 10 times but no more (this simply groups that operation in to one action instead of 10).