360408 - [SessionStore] Add 'Recently Closed Windows'/'Undo Close Window' (or make API easier on extensions)

Status: VERIFIED FIXED

(Firefox :: Session Restore, enhancement)

Description

[John Mellor (Jomel)]

It would be useful to have a 'Recently Closed Windows' menu that works as for 'Recently Closed Tabs' (and maybe an Undo Close Window keyboard shortcut too).

While you're less likely to accidentally close a window than a tab, it can still be useful to go back to an earlier window, and it would be cheap to implement.

Implementation
--------------
Bug 344642 removed the Undo Close Window API from the SessionStore API because it wasn't being used in Fx2 (and there wasn't time to test it).
Reverting that would provide the necessary backend, and it would just need a menu and shortcut hooking up.

Alternative
-----------
If this isn't implemented, then it'd be good to change the SessionStore API so extensions can at least do this themselves without horrific hacks. At the moment one would expect the following code to dump data on closed windows, but getWindowState throws an exception because in the nsSessionStore service's own "domwindowclosed" observer it voids the window data (dietrich confirmed the problem on IRC).

Components
.classes["@mozilla.org/observer-service;1"]
.getService(Components.interfaces.nsIObserverService)
.addObserver(
    {
        observe: function(aSubject, aTopic, aData) {
            var state = Components
                        .classes["@mozilla.org/browser/sessionstore;1"]
                        .getService(Components.interfaces.nsISessionStore)
                        .getWindowState(aSubject);
            dump(state);
        }
    },
    "domwindowclosed",
    false
);

Note: the window state actually gets invalidated at http://lxr.mozilla.org/mozilla1.8/source/browser/components/sessionstore/src/nsSessionStore.js#481

Comment 1

[Simon Bünzli]

With the current implementation, you're actually supposed to get the closed window's state on unload and not on domwindowclosed.

Comment 2

[John Mellor (Jomel)]

(In reply to comment #1)
> With the current implementation, you're actually supposed to get the closed
> window's state on unload and not on domwindowclosed.
> 

Yes but, somewhat counter-intuitively, unload gets called after domwindowclosed does.

For example:
    window.addEventListener("unload", function() { alert(window.__SSi); }, false);
will alert "undefined", because the __SSi will have already been deleted from the window.

Hence calling getWindowState from an unload handler...

window.addEventListener("unload", function() {
    try {
        alert(Components.classes["@mozilla.org/browser/sessionstore;1"]
                        .getService(Components.interfaces.nsISessionStore)
                        .getWindowState(window));
    } catch (ex) {
        alert(ex);
    }
}, false);

... will always give you an error like this:

[Exception... "'[JavaScript Error: "aWindows[i] has no properties" {file: "file:///C:/Program%20Files/Mozilla%20Firefox%202/components/nsSessionStore.js" line: 1013}]' when calling method: [nsISessionStore::getWindowState]"  nsresult: "0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)"  location: "JS frame :: chrome://quickprompt/content/quickprompt.js :: anonymous :: line 224"  data: yes]

Comment 3

[Michael Kraft [:morac]]

I can confirm that by the time the unload event fires SessionStores window state has already been wiped and calling getWindowState at that point results in an error.

Comment 4

[Simon Bünzli]

Work-around for reference: In an overlay to browser.xul call

eval("closeWindow = " + closeWindow.toString().replace("if (aClose)", "mySaveWindowState(); $&"));

where |mySaveWindowState| is a function calling nsISesionStore::getWindowState for |window| in order to further process that state.

Comment 5

[Simon Bünzli]

Attachment: suggested fix, enabling getWindowState and getWindowValue for closing windows

One quite cheap solution would be to simply cache the closing window's state inside the window itself and fall back to that one where it makes sense (IMO mainly getWindowState and getWindowValue - and getTabValue should still work anyway).

Such a patch should allow extensions to use the two patterns suggested in comment #0 and comment #2.

Comment 6

[John Mellor (Jomel)]

The patch looks good and the approach sensible, though I haven't tested it in action.
For consistency it might make sense to do the same for getClosedTabCount and getClosedTabData, just in case people try and read those.

Comment 7

[Simon Bünzli]

Attachment: enable all read-only APIs for closing windows

Comment 8

[Simon Bünzli]

Dietrich: any chance you might get to review this before it's too late?

Comment 9

[Dietrich Ayala (:dietrich)]

Comment on attachment 265000 [details] [diff] [review]
enable all read-only APIs for closing windows

i'd prefer either a new api, or if you re-use the existing apis add an explicit parameter or use a parameter value that indicates that the caller wants the last-closed-window. either way, please update the idl docs as well.

Comment 10

[Simon Bünzli]

Comment on attachment 265000 [details] [diff] [review]
enable all read-only APIs for closing windows

(In reply to comment #9)
> add an explicit parameter or use a parameter value that indicates
> that the caller wants the last-closed-window.

Uhm, the caller doesn't want the last-closed-window, she wants the window she passes anyway (aWindow) - and that's what she gets. All this patch does is unbreak one edge-case we so far haven't handled well at all - there are no API changes or anything else.

As for the (somewhat unfortunately out-of-context) change referring to _lastWindowClosed - we've just set that value 10 lines above to contain the data for the current window (AKA this._windows[aWindow.__SSi] ), you might thus want to consider the complete function as changed and not just what the patch shows you.

Care to have a second look? Thanks.

Comment 11

[Dietrich Ayala (:dietrich)]

Comment on attachment 265000 [details] [diff] [review]
enable all read-only APIs for closing windows

ugh, my misunderstanding. yes, this looks fine for fixing the closing windows problem.

Comment 12

[Simon Bünzli]

Thanks.

Comment 13

[Dietrich Ayala (:dietrich)]

Checking in browser/components/sessionstore/src/nsSessionStore.js;
/cvsroot/mozilla/browser/components/sessionstore/src/nsSessionStore.js,v  <--  nsSessionStore.js
new revision: 1.70; previous revision: 1.69
done

Comment 14

[Igor Velkov]

Bug closed as fixed, but where is "undo window close" button?

Comment 15

[Michael Kraft [:morac]]

This bug didn't add a "undo window close" button.  It just made it so the window state can still be read while the window is closing.  Basically it changed it so the "Alternate" code from comment #0 works.

You'll need to use an extension like Tab Mix Plus or Session Manager if you want to be able to restore closed windows.

Or you can file another bug requesting the feature be added to Firefox itself.

Comment 16

[Michael Kohler [:mkohler]]

Verified with Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.2a1pre) Gecko/20090617 Minefield/3.6a1pre ID:20090617031528

Comment 17

[u279076]

in-litmus+
https://litmus.mozilla.org/show_test.cgi?id=7794