Replace multiple-tab-closing prompt with the ability to restore multiple tabs

REOPENED
Unassigned

Status

()

Firefox
Menus
REOPENED
4 years ago
2 months ago

People

(Reporter: jaws, Unassigned)

Tracking

(Blocks: 1 bug, {feature})

Trunk
feature
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox25 wontfix, firefox26 wontfix, firefox27 wontfix)

Details

(Whiteboard: [qa-])

Attachments

(1 attachment, 2 obsolete attachments)

STR:
Open 10 tabs
Right click on one of the tabs and choose "Close Other Tabs"

Expected:
All the other tabs close
There exists a menuitem to "Undo Close Tabs"

Actual:
A prompt saying "are you sure you want to close 9 tabs"
After hitting Yes, you would have to hit "Undo Close Tab" in the context menu 9 times.
Created attachment 768457 [details] [diff] [review]
Patch

This patch implements the behavior that we talked about. I'll upload a separate patch later with tests.
Attachment #768457 - Flags: review?(ttaubert)
Comment on attachment 768457 [details] [diff] [review]
Patch

Still needs a little more work.
Attachment #768457 - Flags: review?(ttaubert)
Created attachment 768603 [details] [diff] [review]
Patch v2

... and now with tests :)
Attachment #768457 - Attachment is obsolete: true
Attachment #768603 - Flags: review?(ttaubert)
Comment on attachment 768603 [details] [diff] [review]
Patch v2

> <!ENTITY  undoCloseTab.label                 "Undo Close Tab">
> <!ENTITY  undoCloseTab.accesskey             "U">
>+<!ENTITY  undoCloseTabs.label                "Undo Close Tabs">

This should probably have a localization note about where and when this label appears... and that undoCloseTab.accesskey is the access key regardless of which label is used.
Comment on attachment 768603 [details] [diff] [review]
Patch v2

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

::: browser/base/content/browser.js
@@ +6287,5 @@
>  
>    var tab = null;
>    var ss = Cc["@mozilla.org/browser/sessionstore;1"].
>             getService(Ci.nsISessionStore);
> +  let numberOfTabsToUndoClose = ss.getNumberOfTabsClosedLastTime(window);

aIndex is only given when undoCloseTab() is called from browser-places.js to indicate that we want to undo-close a specific tab. Even if I call undoCloseTab(0) I don't want the last bunch restored but the first tab in the list.

if aIndex is undefined we should go ahead and restore the last bunch of tabs, otherwise only single ones. For that reason we should fix browser-places.js to use undoCloseTab(0) when "Restore all tabs" is clicked:

http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser-places.js#574

(Although the current version wouldn't actually break, it's just a couple of undoCloseTab() calls that will be ignored.)

::: browser/base/content/tabbrowser.xml
@@ +1518,3 @@
>              return true;
>  
> +          if (!Services.prefs.getBoolPref("browser.tabs.warnOnClose"))

Nit: We could include that in the condition above.

::: browser/components/sessionstore/nsISessionStore.idl
@@ +109,5 @@
> +   * Set the number of tabs that was closed during the last close-tabs
> +   * operation. This helps us keep track of batch-close operations so
> +   * we can restore multiple tabs at once.
> +   */
> +  void setNumberOfTabsClosedLastTime(in nsIDOMWindow aWindow, in unsigned long aNumber);

Maybe rather setNumberOfTabsClosedLast()?

::: browser/components/sessionstore/src/SessionStore.jsm
@@ +1472,5 @@
>    },
>  
> +  setNumberOfTabsClosedLastTime: function ssi_setNumberOfTabsClosedLastTime(aWindow, aNumber) {
> +    if (aWindow.__SSi) {
> +      this._windows[aWindow.__SSi]._numberOfTabsClosedLastTime = aNumber;

I'd rather create a new map (using WeakMaps, like DyingWindowCache) instead of introducing another "add-on attack point".

@@ +1488,5 @@
> +  /* Used to undo batch tab-close operations. Defaults to 1. */
> +  getNumberOfTabsClosedLastTime: function ssi_getNumberOfTabsClosedLastTime(aWindow) {
> +    function min(a, b) {
> +      return a < b ? a : b;
> +    }

Math.min()?

::: browser/locales/en-US/chrome/browser/browser.dtd
@@ +45,5 @@
>  <!ENTITY  bookmarkAllTabs.label              "Bookmark All Tabs…">
>  <!ENTITY  bookmarkAllTabs.accesskey          "T">
>  <!ENTITY  undoCloseTab.label                 "Undo Close Tab">
>  <!ENTITY  undoCloseTab.accesskey             "U">
> +<!ENTITY  undoCloseTabs.label                "Undo Close Tabs">

Like Dão said, a comment to help localizers would be great.
Attachment #768603 - Flags: review?(ttaubert) → feedback+
Created attachment 772358 [details] [diff] [review]
Patch v3

Tim, what do you think of this? I don't particularly like the undoCloseTab() function in browser.js, but it was one of the better ways I could think of to reduce duplication.
Attachment #768603 - Attachment is obsolete: true
Attachment #772358 - Flags: review?(ttaubert)
Comment on attachment 772358 [details] [diff] [review]
Patch v3

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

::: browser/base/content/browser.js
@@ +6294,5 @@
>    var ss = Cc["@mozilla.org/browser/sessionstore;1"].
>             getService(Ci.nsISessionStore);
> +  let numberOfTabsToUndoClose = 0;
> +  if (Number.isInteger(aIndex)) {
> +    if (ss.getClosedTabCount(window) > aIndex) {

Shouldn't we bail out if the given index is bigger or equal like we did before?

@@ +6302,5 @@
> +    numberOfTabsToUndoClose = ss.getNumberOfTabsClosedLast(window);
> +    aIndex = 0;
> +  }
> +
> +  while (numberOfTabsToUndoClose--) {

If ss.getNumberOfTabsClosedLast() returns zero or someone passes aIndex > ss.getClosedTabCount() we'd loop forever here. Maybe add "> 0"?

@@ +6310,3 @@
>      if (blankTabToRemove)
>        gBrowser.removeTab(blankTabToRemove);
> +    blankTabToRemove = null;

We could include that in the if branch.

::: browser/components/sessionstore/src/SessionStore.jsm
@@ +4685,5 @@
>      this._data.delete(window);
>    }
>  };
>  
> +let ClosedTabCache = {

I think we can find a better name for this. It's not really a cache as it doesn't cache any computed value. Is 'NumberOfTabsClosedLastPerWindow' too descriptive? I'd rather go with something like that.

@@ +4696,5 @@
> +  get: function (window) {
> +    return this._data.get(window);
> +  },
> +
> +  set: function (window, data) {

'data' should probably be 'number' as we know what value to expect here.
Attachment #772358 - Flags: review?(ttaubert) → review+
Addressed review feedback and pushed to try server,
https://tbpl.mozilla.org/?tree=Try&rev=2aa18344597f

As discussed on IRC, NumberOfTabsClosedLastPerWindow no longer has a wrapper object around the WeakMap, and is itself just a WeakMap.
https://hg.mozilla.org/integration/mozilla-inbound/rev/6d739868bd90

I had to remove the {animate:true} from RemoveAllTabsBut due to the test failures. It wasn't clear why the test was failing considering that the similar test for RemoveTabsToTheEndFrom (browser_removeTabsToTheEndFrom.js) doesn't suffer from the same issue. I filed bug 894048 to investigate this.
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/0f3bc9d8f44e due to causing the following failures:

17:51:33    ERROR - Return code: 1
17:51:34    ERROR - Return code: 1
18:10:22  WARNING -  TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/modules/test/browser_taskbar_preview.js | Browser has one preview per tab - Got 5, expected 1
18:10:22  WARNING -  TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/modules/test/browser_taskbar_preview.js | Browser starts with one preview - Got 5, expected 1
18:10:22  WARNING -  TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/modules/test/browser_taskbar_preview.js | Browser has one preview per tab - Got 8, expected 4
18:10:22  WARNING -  TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/modules/test/browser_taskbar_preview.js | Correct number of previews after adding - Got 8, expected 4
18:10:22  WARNING -  TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/modules/test/browser_taskbar_preview.js | Preview is shown as expected
18:10:22  WARNING -  TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/modules/test/browser_taskbar_preview.js | Preview is shown as expected
18:10:22  WARNING -  TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/modules/test/browser_taskbar_preview.js | Preview is shown as expected
18:10:22  WARNING -  TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/modules/test/browser_taskbar_preview.js | Preview is shown as expected
18:10:22  WARNING -  TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/modules/test/browser_taskbar_preview.js | Browser has one preview per tab - Got 8, expected 4
18:10:22  WARNING -  TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/modules/test/browser_taskbar_preview.js | Previews are unchanged when disabling - Got 8, expected 4
18:10:22  WARNING -  TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/modules/test/browser_taskbar_preview.js | Browser has one preview per tab - Got 8, expected 4
18:10:22  WARNING -  TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/modules/test/browser_taskbar_preview.js | Previews are unchanged when re-enabling - Got 8, expected 4
18:10:22  WARNING -  TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/modules/test/browser_taskbar_preview.js | Preview is shown as expected after re-enabling
18:10:22  WARNING -  TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/modules/test/browser_taskbar_preview.js | Preview is shown as expected after re-enabling
18:10:22  WARNING -  TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/modules/test/browser_taskbar_preview.js | Preview is shown as expected after re-enabling
18:10:22  WARNING -  TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/modules/test/browser_taskbar_preview.js | Preview is shown as expected after re-enabling
18:10:22  WARNING -  TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/modules/test/browser_taskbar_preview.js | Browser has one preview per tab - Got 7, expected 3
18:10:22  WARNING -  TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/modules/test/browser_taskbar_preview.js | Expected number of previews after closing selected tab via controller - Got 7, expected 3
18:10:22  WARNING -  TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/modules/test/browser_taskbar_preview.js | Browser has one preview per tab - Got 6, expected 2
18:10:22  WARNING -  TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/modules/test/browser_taskbar_preview.js | Expected number of previews after closing unselected via browser - Got 6, expected 2
18:10:22  WARNING -  TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/modules/test/browser_taskbar_preview.js | Browser has one preview per tab - Got 5, expected 1
18:10:22  WARNING -  TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/modules/test/browser_taskbar_preview.js | Expected number of previews after closing selected tab via browser - Got 5, expected 1
18:10:22  WARNING -  TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/modules/test/browser_taskbar_preview.js | Browser has one preview per tab - Got 6, expected 2
18:10:22  WARNING -  TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/modules/test/browser_taskbar_preview.js | Got expected number of previews - Got 6, expected 2
18:10:22  WARNING -  TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/modules/test/browser_taskbar_preview.js | Browser has one preview per tab - Got 5, expected 1
18:10:22  WARNING -  TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/modules/test/browser_taskbar_preview.js | Got expected number of previews - Got 5, expected 1
18:16:51  WARNING -  TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/tabview/test/browser_tabview_bug624265_perwindowpb.js | leaked until shutdown [nsGlobalWindow #7083 about:blank]
18:16:51  WARNING -  TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/tabview/test/browser_tabview_bug624265_perwindowpb.js | leaked until shutdown [nsGlobalWindow #7084 about:blank]
18:16:51  WARNING -  TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/tabview/test/browser_tabview_bug624265_perwindowpb.js | leaked until shutdown [nsGlobalWindow #7094 chrome://browser/content/tabview.html]
18:16:51  WARNING -  TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/tabview/test/browser_tabview_bug624265_perwindowpb.js | leaked until shutdown [nsGlobalWindow #7092 chrome://browser/content/tabview.html]
Relanded with changes to the tests,
https://hg.mozilla.org/integration/mozilla-inbound/rev/b94553178dfe

Try push, https://tbpl.mozilla.org/?tree=Try&rev=1df1df973db3

Comment 12

4 years ago
After closing bunch of about:blank or about:newtab tabs ss.getNumberOfTabsClosedLast will always return the wrong number of tabs for undo!

since _shouldSaveTabState exclude about:blank and about:newtab we need to exclude such tabs when counting closed tabs.
we need to call ss.setNumberOfTabsClosedLast with the proper number
(In reply to onemen.one from comment #12)
> After closing bunch of about:blank or about:newtab tabs
> ss.getNumberOfTabsClosedLast will always return the wrong number of tabs for
> undo!
> 
> since _shouldSaveTabState exclude about:blank and about:newtab we need to
> exclude such tabs when counting closed tabs.
> we need to call ss.setNumberOfTabsClosedLast with the proper number

Can you please file a bug for this and assign it to me?

Updated

4 years ago
Depends on: 895436
https://hg.mozilla.org/mozilla-central/rev/b94553178dfe
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 25
Jared,

Should we remove the `browser.tabs.warnOnCloseOtherTabs` hidden pref now? Or should we fix to keep working its pref in continues?

The latter one's merit is that we don't need any network traffic because it ask us before closing multiple tabs.
The approach of this bug need some network traffics when we undo multiple (many) tabs.
And the approach of this bug is depends on `browser.sessionstore.max_tabs_undo`. So it cannot replace the multiple-tab-closing prompt on that case: Firefox closes 100~200 tabs when user does "close other tabs" and `browser.sessionstore.max_tabs_undo` is 10 or less.
Depends on: 896291

Updated

4 years ago
Depends on: 896986
Duplicate of this bug: 582993

Updated

4 years ago
Depends on: 909662

Updated

4 years ago
Depends on: 911241
Depends on: 914258
status-firefox25: --- → disabled
Backed out of Firefox 25 by bug 914258.
status-firefox25: disabled → wontfix
status-firefox26: --- → fixed
Depends on: 931891
Backed out of Firefox 26 (and eventually 27) by bug 931891.
status-firefox26: fixed → wontfix
status-firefox27: --- → wontfix
status-firefox28: --- → fixed
Keywords: feature
Assuming Target Milestone is incorrect here given status flags. 

Ioana, given this is flagged as a "feature", please make sure this gets tested and signed off.
Flags: needinfo?(ioana.budnar)
Target Milestone: Firefox 25 → Firefox 28
Keywords: verifyme

Comment 21

4 years ago
Alexandra will start working in this today and she'll try to sign the feature off before the merge to beta.
Flags: needinfo?(ioana.budnar)
QA Contact: alexandra.lucinet
This feature was backed out of trunk and all branches before reaching the Release builds. There is no need to test this.
Keywords: verifyme
Whiteboard: [qa-]
Target Milestone: Firefox 28 → ---

Updated

4 years ago
Status: RESOLVED → REOPENED
status-firefox28: fixed → ---
Resolution: FIXED → ---
Duplicate of this bug: 609628
Blocks: 565515
See Also: → bug 565771
Unassigning as I haven't been working on this and I don't want to hold people up.
Assignee: jaws → nobody
You need to log in before you can comment on or make changes to this bug.