Summary says all I did back end part, some questions about interface part, will add it later.

Comment on attachment 394813 [details] [diff] [review] Sessionstore backend part Looks reasonable, although I haven't tested it in any way (such as ensuring that session restore still works).

Well, I've made interface part like our restoreTab works, not sure it right, but working. Please comment if you want it be done like in Firefox. Tests on their way too.

Comment on attachment 395056 [details] [diff] [review] Interface part UI looks great. Just some code nits: >+ recentWindowsItem.setAttribute("disabled", !browser || browser.getClosedWindowsList().length == 0); What happens on the Mac when you have no windows? I'm thinking that getClosedWindowsList has to be a global function so that you can undo close window on the Mac even when no windows are open. (I actually wanted it to be a global function for other reasons anyway.) >+ var ss = Components.classes["@mozilla.org/suite/sessionstore;1"]. >+ getService(Components.interfaces.nsISessionStore); Nit: var ss = Components.classes["@mozilla.org/suite/sessionstore;1"] .getService(Components.interfaces.nsISessionStore); >+ >+ var undoItems = JSON.parse(ss.getClosedWindowData()); I think updateRecentWindows should just call this directly. >+ if (ss.getClosedWindowCount() > (aIndex || 0)) >+ window = ss.undoCloseWindow(aIndex || 0); I think this check should be in the session store code, not in the caller.

How hard would it be to return an array of just the closed window titles?

Comment on attachment 395551 [details] [diff] [review] Interface part, nits fixed >+ var ss = Components.classes["@mozilla.org/suite/sessionstore;1"]. >+ getService(Components.interfaces.nsISessionStore); [I'm sure I said the . ^ goes here, not at the end of the previous line...] >+ recentWindowsItem.setAttribute("disabled", !browser || ss.getClosedWindowCount() == 0); You don't need the !browser check here. >+ var browser = getBrowser(); Not used. >+ var list = []; Delete this. >+ var ss = Components.classes["@mozilla.org/suite/sessionstore;1"]. >+ getService(Components.interfaces.nsISessionStore); As above. >+ list.push(undoItems[i].title); >+ } >+ >+ for (var i = 0; i < list.length; i++) { Delete this. >+ var label = list[i]; var label = undoItems[i].title; >+ <method name="undoCloseWindow"> This goes in navigator.js too. >+ let ss = Components.classes["@mozilla.org/suite/sessionstore;1"]. >+ getService(Components.interfaces.nsISessionStore); As above. >+ let window = null; >+ window = ss.undoCloseWindow(aIndex); >+ >+ return window; You can do all this in one line of code!

Sorry :(

(In reply to comment #6) > >+ let ss = Components.classes["@mozilla.org/suite/sessionstore;1"]. > >+ getService(Components.interfaces.nsISessionStore); > As above. > > >+ let window = null; > >+ window = ss.undoCloseWindow(aIndex); > >+ > >+ return window; > You can do all this in one line of code! One statement to rule them all ;-)

Comment on attachment 394813 [details] [diff] [review] Sessionstore backend part >+ // default to the most-recently closed window >+ aIndex = aIndex || 0; This doesn't actually make any difference here because xpconnect ensures that aIndex is already an unsigned long, so it can be removed. >+ if (!aIndex in this._closedWindows) This test is incorrect; ! has higher precedence than in so you need ()s. I blame Sun for this; they just weren't thinking when they gave "in" such a ridiculously low precedence :-( sr=me with this fixed. >+ throw (Components.returnCode = Cr.NS_ERROR_INVALID_ARG); Please return null instead so that Ctrl+Shift+Y won't error on an empty list.

Comment on attachment 395555 [details] [diff] [review] Interface part, more nits fixed >+ let ss = Components.classes["@mozilla.org/suite/sessionstore;1"] >+ .getService(Components.interfaces.nsISessionStore); Would you mind changing this to be a "var" for consistency? I think it would silly if we "let" this one through ;-)

Carrying forward r+ and sr+ from Neil.

Carrying forward r+ and sr+ from Neil

This is a test part, it times out now, can't understand why. Help needed.

Comment on attachment 395567 [details] [diff] [review] test > _BROWSER_FILES = browser_bug463504.js \ >+ browser_510890.js \ browser_bug510890.js >+ /** Test for Bug 394759 **/ Oops. >+ newWin.addEventListener("load", function(aEvent) { This outer load is OK, but... >+ newWin.gBrowser.addEventListener("load", function(aEvent) { >+ newWin.gBrowser.removeEventListener("load", arguments.callee, true); This is no good, because you listen to the "load" of the tab drag indicator. Try using "DOMContentLoaded" and/or "pageshow" instead. Note: these events bubble, so you can change the "true" to "false" as well if you like. >+ let window = openDialog(location, "_blank", settings, url); Rename this variable, it's confusing. >+ window.gBrowser.addEventListener("load", function(aEvent) { >+ // the window _should_ have state with a tab of url, but it doesn't >+ // always happend before window.close(). addTab ensure we don't treat >+ // this window as a stateless window >+ window.gBrowser.addTab(); >+ window.gBrowser.removeEventListener("load", arguments.callee, true); I think removeEventListener should come first. Also, which event? (I tried everything but I still got errors or timeouts...) >+ }, true); This is probably incorrect, and should be false.

(In reply to comment #14) > >+ }, true); > This is probably incorrect, and should be false. In fact this is correct, but gBrowser should be replaced by getBrowser() (Firefox uses a custom gBrowser dynamic property).

this one passes on my Linux box, not sure i understood and did everything correct though.

Comment on attachment 395785 [details] [diff] [review] fixed test >+ newWin.addEventListener("load", function(aEvent) { >+ newWin.getBrowser().addEventListener("pageshow", function(aEvent) { >+ newWin.getBrowser().removeEventListener("pageshow", arguments.callee, false); >+ >+ executeSoon(function() { >+ newWin.getBrowser().addTab(); >+ newWin3.addEventListener("load", function(aEvent) { >+ newWin3.getBrowser().addEventListener("pageshow", function(aEvent) { >+ // the window _should_ have state with a tab of url, but it doesn't >+ // always happend before window.close(). addTab ensure we don't treat >+ // this window as a stateless window >+ newWin3.getBrowser().addTab(); >+ newWin3.getBrowser().removeEventListener("pageshow", arguments.callee, true); If it can be done without making the test go wrong, please: a) move removing the event listener to the beginning of the function (as above) b) use bubbling instead of capturing event listeners (as above)

Fixed nits, carrying forward r+ from Neil. Test passes on my Linux box.

Pushed to comm-central: attachment 395565 [details] [diff] [review]: http://hg.mozilla.org/comm-central/rev/c43e364dec91 attachment 395566 [details] [diff] [review]: http://hg.mozilla.org/comm-central/rev/0fe6bf0ddb61 attachment 396178 [details] [diff] [review]: http://hg.mozilla.org/comm-central/rev/cea01fa911fb Misak, feel free to resolve this bug as fixed if nothing more is to be done in here.

Misak, it looks like a good part of browser_bug510890.js is running OK, but then it runs into a timeout on our unit test boxes. Could you look into that?

I restored original test code for one function and deleted another - it seems fully related to Private Browsing, which we don't have. The test passes on my linux buid.

(In reply to comment #20) Ftr: { mochitest-browser-chrome: browser_bug510890.js | Timed out browser_Browser.js | Timed out }

I'll check in attachment 396727 [details] [diff] [review] as a bustage fix, in terms of tests it is that.

Comment on attachment 396727 [details] [diff] [review] modified test Pushed this as http://hg.mozilla.org/comm-central/rev/058aa5379fc7 - I took it as a test bustage fix, which it actually is, and bustage fixes don't necessarily need reviews.

I quoted the wrong bug number in the checkin comment, but the bustage fix has made tests pass again, it seems. Marking FIXED again.

Adding fixed-seamonkey2.0 to make it go away from "unfixed wanted" query