Bug 360408 added back in the APIs which apparently would make this fairly easy to implement. It's something I've wanted on many occasions and now the new Opera 9.5 alpha is being shown off with this as one of it's new additions. I was mainly thinking just another menu "Recently Closed Windows" off the history menu for parity with the recently closed tabs menu. So would this be taken for Firefox 3?

From Bug 410883 "Session Restore seems to only work when all of FireFox is completely closed down and then restarted, i need it to either allow a session restore immediately or to remember some short session history that i may choose when restarted so that i may recover my working session(s) from accidental mishaps."

See also bug 39165.

I decided on Friday that I was going to do this, so I spent a little while the past couple days and whipped up a basic patch. It's not ready for prime time, but it works. So if people could try it out, look at the patch, let me know of improvements, that would be awesome. The quicker you respond, the better. It would be great to get this into 3.1 and feature freeze is next week...

Paul: Please have a look at the patch in bug 344642 for what we used to have. Your patch currently neither limits the number of closed windows nor does it allow to manually clear the list. Furthermore: Should that list persist through restarts or not? (it currently doesn't)

Simon: I hadn't seen that bug before - it looks like a lot of what I'm doing was taken out. We would definitely need a way to limit the number of windows. As for manually clearing the list - is that really necessary? We don't do that with tabs as far as I know. I talked with Beltzner about this earlier in the summer, and I think we decided we wouldn't want this to persist through shutdown & startup for privacy reasons. It might be good to persist through actual restarts though.

(In reply to comment #7) > As for manually clearing the list - is that really necessary? At least through the Clear Private Data dialog - that's when we clear the Recently Closed Tabs list as well. > we wouldn't want this to persist through shutdown & startup for privacy > reasons. It might be good to persist through actual restarts though. As long as bug 398807 isn't fixed, always persisting that list would actually help preventing dataloss. As for privacy, isn't that what Clear Private Data (which can happen automatically at shutdown) and the pref are for (which I'd limit to about three windows)?

Updated a bit, taking into account much of what Simon suggested. There's still some cruft in there, so ignore that. Highlights: * made number of windows a pref, uses that to limit * fixed he ordering (unshift instead of push) * data is clearable through clear private data Questions: * is "tabs" a localizable string? right now "tabs" is hardcoded into the javascript, but that doesn't seem right (also need to fix singular/plural) * am i putting the entities and other string in the right places? there seems to be a fair amount of repetition with "recently closed tabs" * is the shortcut key ok? I didn't make data persist between browser sessions yet. That will require a little bit more work. I can go either way on this. I guess the privacy issue isn't that great, especially if we get private browsing done sometime soon.

(In reply to comment #9) > * is "tabs" a localizable string? Absolutely. You might want to have a peek at the DownloadMonitorPanel code in browser.js as to how to correctly handle plurals, etc. > * am i putting the entities and other string in the right places? The general patch structure looks alright to me. > * is the shortcut key ok? Is one needed at all? If so, Ctrl+Shift+N would indeed nicely mirror Ctrl+Shift+T for reopening closed tabs.

Now that we've changed things such that: - tabstrip is showing by default - closing the last tab on a tabstrip closes the window this feels even more important to get for Firefox 3.1

Changes in v0.2: * "Tab/Tabs" is now localized * _closedWindows is now saved upon quit and restored at startup I haven't applied any of the patches from bug 248970 (private browsing) to see if they have any effect, but based on comments there, there shouldn't be any problems.

Comment on attachment 339690 [details] [diff] [review] Patch v0.2 Review from Simon canceled - there is still debug, and old commented code in there. Feedback as if it were a review would be nice though.

Comment on attachment 339690 [details] [diff] [review] Patch v0.2 Thanks for the patch. There's mostly a bunch of nits, I'm not quite happy with... General remark: Please include more context in your diffs; at least 5, ideally 8 lines. > // HistoryMenu.toggleRecentlyClosedTabs is defined in browser.js > this.toggleRecentlyClosedTabs(); >+ // HistoryMenu.toggleRecentlyClosedWindows is defined in browser.js Nit: One unified comment for both functions should be enough. >+ <key id="key_undoCloseTab" command="History:UndoCloseTab" key="&tabCmd.commandkey;" modifiers="accel,shift"/> Nit: Unnecessary whitespace changes. >+Cu.import("resource://gre/modules/PluralForm.jsm"); DownloadMonitorPanel imports this on demand. Please do the same (keeping the code tighter together). >+ // get closed-tabs from nsSessionStore >+ // no restorable tabs, so disable menu These comments mention tabs instead of windows (several times below as well). >+ var strings = gNavigatorBundle; This looks wrong. Why not use gNavigatorBundle to start with? Besides: let is the new var. >+ m.setAttribute("label", undoItems[i].title + " (" + numTabsString + ")"); I'd prefer having one localizable string for the whole label, so that translators could e.g. replace the parens or reorder the whole. >+ if (undoItems[i].image) >+ m.setAttribute("value", i); >+ m.addEventListener("click", undoCloseMiddleClick, false); undoCloseMiddleClick restores closed tabs. Since I don't really see the point in middle-clicking in this case, you can just remove these two lines. >+ m.addEventListener("command", function() { >+ for (var i = 0; i < undoItems.length; i++) >+ undoCloseWindow(); >+ }, false); This will unnecessarily keep the undoItems alive in a closure. Why not set an oncommand attribute to |"for (let i = 0; i < " + undoItems.length + "; i++) undoCloseWindow();"| >+ if (ss.getClosedWindowCount() == 0) >+ return; >+ ss.undoCloseWindow(aIndex || 0); Nit: Instead of returning, just reopen a window when there's data for at least one. Then again, you'll either want to check that there are at least |(aIndex || 0) + 1| windows available, or not check at all - so that we either always or never throw. >+ unsigned long getClosedWindowCount(); >+ AString getClosedWindowData(); >+ void undoCloseWindow(in unsigned long aIndex); Please add documentation for the new API and keep the three methods together (i.e. separate from the tab related methods). >+ // states for all recently closed windows >+ _closedWindows: [], Would it be possible to reuse _lastClosedWindows for this, so that we don't have to keep the exact same data twice in memory? >+ if (this._closedWindows.length > maxWindowsUndo) Nit: You've just assigned that length to a local variable... >+ getClosedWindowCount: function sss_getClosedWindowCount() { Please keep the window related functions together here as well. >+ var aState = {windows:[this._closedWindows[aIndex]]}; Nit: Missing whitespace. >+ this._openWindowWithState(aState); >+ var window = this._closedWindows.splice(aIndex, 1); The window is returned by _openWindowWithState! And instead of splicing after the fact, you could use the same |.splice(aIndex, 1).shift()| routine as undoCloseTab for getting the data and removing it from the array at the same time. >+ return { windows: total, selectedWindow: ix + 1, _closedWindows : this._closedWindows }; Since _closedWindows and _lastClosedWindows are different, you might end up having the same window in both the list of open and closed windows... Finally, we also need some tests for this new API.

I saw _lastClosedWindows as I was cleaning up some bitrot - it was added since I last worked on this (well it wasn't an array before) Is there a reason this was changed? I may be able to piggy-back on that so we're not keeping the same thing around twice. It would of course require some rewriting, but as long as it operates the same it shouldn't be too bad. It looks like that array is only stored per session, and never gets resized, so that could be a problem. Thoughts? Otherwise, I've made a number of the changes above and will have a new patch soon.

_lastClosedWindows is used when Firefox is closed with no browser windows open (i.e. when you close the Downloads Manager last) and always contains at least one non-popup window (so closing a popup window just pushes it while closing a non-popup browser window clears the array). If you reuse _lastClosedWindows, you'd thus have to make sure that it contains at least one non-popup window when none is open, even if that means that more than max_windows_undo windows can be reopened. In fact, I'd be fine with just not capping when only popup windows are open. The only other thing you'd then have to do is splitting the _lastClosedWindows in two in _getCurrentState: (1) all windows to be reopened automatically (i.e. everything until the first non-popup window in case where no non-popup window is currently open) and (2) the rest of the list. The windows from (1) will have to be added to |total|, the ones from (2) to |_closedWindows|.

BTW: Please return the reopened window from nsISessionStore::undoCloseWindow. That'll also make it easier to close a reopened window in the unit tests.

(In reply to comment #17) > BTW: Please return the reopened window from nsISessionStore::undoCloseWindow. > That'll also make it easier to close a reopened window in the unit tests. Good idea. Following up on that and your comment from reviewing... In nsISessionStore::undoCloseWindow there is no explicit |throw| happening, instead it's setting |Components.returnCode = Cr.NS_ERROR_INVALID_ARG;| - is this equivalent to throwing? Should I be catching it in the method that calls it? If not, should I return null/false in that case so the caller knows it failed (or is there another method with the returnCode - I haven't used that before). As for tests, can you point me towards the tests for undo close tab so I have something to model from? I'm not seeing them, but maybe I'm blind.

(In reply to comment #18) > (In reply to comment #17) > > BTW: Please return the reopened window from nsISessionStore::undoCloseWindow. I've filed bug 456465 to get undoCloseTab behaving the same way. > In nsISessionStore::undoCloseWindow there is no explicit |throw| happening, > instead it's setting |Components.returnCode = Cr.NS_ERROR_INVALID_ARG;| I hope to change this to actually throwing in bug 345898. > As for tests, can you point me towards the tests for undo close tab All the currently existing tests can be found at <http://mxr.mozilla.org/mozilla-central/source/browser/components/sessionstore/test/browser/>. And see the bugs depending on bug 426045 for tests yet to land.

Comment on attachment 339690 [details] [diff] [review] Patch v0.2 some nits: > +tabString=#1 Tab;#1 Tabs should probably be "tab" and "tabs" (lowercase) We might need some interface tweaks later, but this is a good start. Wanna see if we can get this landed before b1?

(In reply to comment #20) > We might need some interface tweaks later, but this is a good start. Wanna see > if we can get this landed before b1? If yes, we need to land strings today, right?

Well, so this didn't make b1 string freeze. What does that mean as far as this still getting in for b1?

Takes care of all things from comment #14 except: * doesn't use _lastClosedWindows * no tests yet Hopefully I'll get it done for b1, should have some time this weekend.

Comment on attachment 340656 [details] [diff] [review] Patch v0.3 A few more nits on what you've got so far: >+ for (let i = 0; i < undoItems.length; i++) { >+ Cu.import("resource://gre/modules/PluralForm.jsm"); Now that's slightly too tight. ;-) Outside the loop will be fine. >+ let menuLabel = PluralForm.get(undoItems[i].tabs.length, menuLabelString). >+ replace("#1", undoItems[i].title). >+ replace("#2", undoItems[i].tabs.length); I know we're not consistent, but we prefer the dot to go on the new line and for the dots to be aligned (i.e. |.replace| should start at the same level as |.get|). >+} >+function undoCloseWindow(aIndex) { Nit: Blank line between functions. >+ try { ss.undoCloseWindow(aIndex || 0); } catch (e) {} Please see the patch in bug 456465 for how to do this without a try-catch clause. And please also return the reopened window from this function. >+ * @returns a nsIDOMWindow object of the reopened window That should rather be "_the_ nsIDOMWindow object of", although "a reference to" would be fine by me. >+ dump(aMsg + "\n"); Nit: Debugging code. >+ if (this._closedWindows.length > maxWindowsUndo) >+ this._closedWindows.splice(maxWindowsUndo, this._closedWindows.length - maxWindowsUndo); Nit: Overlong line. >+ let aState = { windows: this._closedWindows.splice(aIndex, 1) }; Nit: The "a" prefix stands for function's _a_rgument. So |let state = ...|. >+ return { windows: total, selectedWindow: ix + 1, _closedWindows: this._closedWindows }; I guess we could also fix the fact that this could potentially contain the same window data in two places in a follow-up bug, if you want this to make Beta 1. >+<!ENTITY historyUndoWindowMenu.label "Recently Closed Windows"> >+menuOpenAllInWindows.label=Open All in Windows >+menuOpenAllInWindows.accesskey=o >+# LOCALIZATION NOTE (menuUndoCloseWindowLabel): Semi-colon list of plural forms. >+# See: http://developer.mozilla.org/en/docs/Localization_and_Plurals >+# #1 Window Title, #2 Number of tabs >+menuUndoCloseWindowLabel=#1 (#2 tab);#1 (#2 tabs) These will have to be checked in by tonight, should you want this patch to make Beta 1, though! Except for the tests, this would be r+=me with the above comments fixed. You'll still need review from a browser peer for the changes outside sessionstore, though. Should further review on sessionstore bits be needed, please ask Dietrich, since I'll be unavailable until 10/4.

Paul, could you please give a status update?

Sorry, grad school has been keeping me busy. So I've taken care of the changes for comment #24, but still need to finish looking into using _lastClosedWindows instead. I also need tests, but since I'm short on time, I haven't had much chance to look into those. Would anybody be willing to write some tests for this so that it can get done sooner rather than later? The API won't change from what's in the current patch, just the internals.

Comment on attachment 344645 [details] [diff] [review] tests for the new API Note: A timeout of 0 will work as well (i.e. no real timeout, just yielding some CPU cycles).

Note that we have the executeSoon method now in tests that should probably be used in preference to 0 timeouts

(In reply to comment #29) Interestingly this test passes with a 0 timeout but doesn't with executeSoon...

executeSoon would generally fail when code running immediately after it schedules the event which you are relying on in the test. I think the setTimeout runs differently based on available time. Which might mean while a setTimeout of 0 appears to work it may fail under certain load conditions.

In that case, two |executeSoon|s in a row will replace the timeout. Thanks for the note, Dave.

(In reply to comment #34) > *** Bug 462783 has been marked as a duplicate of this bug. *** I don't think that's a dupe of this bug, actually, as if we did have an undo close window function, I don't think we'd add it to the undo stack for the same reason that we don't have undo close tab in the undo stack. Paul: I know school's got you busy, but is there any hope of this coming soon? I'm thinking we're going to have to punt this to 3.2 :(

Building and looking into it now. I've pulled down the tests from Simon and will look at the _lastClosedWindows now. I'll get this done today. I guess this has missed string & code freezes for b2... so I guess it'll have to be punted :( I'll get it done anyway.

Dusting this off & getting rid of the bit rot so that I can actually work on this and we can get this into 3.5.

So it seems that we have a problem when the selected tab doesn't have a title. For example, a local HTML doc with <title></title>. So I think instead of window.title, I'll have to do something along the lines of selectedTab.title || selectedTab.url (roughly speaking). So the question is, should that be something that sessionstore keeps track of by itself instead of me having to conditionally construct it here? If so, that could be spun off into another bug and probably isn't too hard.

(In reply to comment #39) > instead of window.title Note that the window.title bit isn't currently used at all - I've just overlooked it when ripping out all related code in bug 344642. Feel free to drop that line and do the correct thing instead (see e.g. what we do in onTabClose for tab titles).

Another issue I uncovered... When you close the last/only tab in a window using close tab (or the keyboard shortcut), there's no actual tab data to be restored... We listen for "TabClose" and store the tab data in _closedTabs, and then somewhere along the line, closing the last tab proceeds to cause the window to be closed. So we receieve "domwindowclosed" and I store the window data. So now that gets added to the menu under "recently closed windows", and when we restore it, the window opens with a single empty tab. We can proceed to restore closed tabs from there. Restoring an empty window seems far from ideal, so I wondering what we should do in that case. An idea I had was to check when we catch "domwindowclosed" and if the window has no tabs, 'restore' the last closed tab. That or check when we restore the window and do the same thing.

(In reply to comment #41) See bug 481560.

Why are we talking about tabs vs. windows here? Shouldn't we be talking about documents? When I accidentally close something, what I really care about doing is restoring the WEB PAGE I was on. Windows are just a grouping of tabs. Of course, windows are meaningful groupings of tabs, and if I accidentally close a whole window of tabs (or a window with just one tab), I would like the ability to undo that as well. I guess what I don't understand is why documents/tabs are subordinate to windows. Windows are just groupings of tabs. Since viewing web pages is what matters, I would expect windows to be subordinate.

(In reply to comment #43) > Why are we talking about tabs vs. windows here? Shouldn't we be talking about > documents? When I accidentally close something, what I really care about doing > is restoring the WEB PAGE I was on. Windows are just a grouping of tabs. Of > course, windows are meaningful groupings of tabs, and if I accidentally close a > whole window of tabs (or a window with just one tab), I would like the ability > to undo that as well. I'd argue because although it is about documents, it is about the action that closed the documents. In the case of tabs you can close each with a single click so it makes sense to restore each with a single "undo" action. In the case of windows however you are closing maybe 100 documents with a single click. Do you want to have to perform 100 "undo" actions to get them all back? I doubt it and it doesn't really follow the undo pattern either. Undoing undoes a single action, the single action was closing the window so undoing that should restore the entire window.

I completely agree with you. And upon reflection, I think the point I was making is kinda moot, because I think I misread what it was I was replying to. Sorry about that.

(In reply to comment #42) > See bug 481560. Thanks Dão, and I can confirm that bug 481560 (and in turn bug 462673) do solve the problem without having to hack a solution into sessionstore code, which is ideal.

Ok, next (last?) thing - how should this work with Private Browsing? Right now, it will remember the previously closed windows when you enter private browsing. Windows closed during private browsing will continue to be added to "close recently closed windows..." menu clipping at the pref limit. Upon exiting private browsing, closed windows are forgotten, and the menu only shows windows closed before entering private browsing. Personally, I think that's appropriate, but I'd be willing to hear alternatives. Maybe you could weigh in Ehsan? And the corollary - I assume that would need to be tested, so would we add to browser_248970_a or make browser_248970_c?

(In reply to comment #47) > Personally, I think that's appropriate, but I'd be willing to hear > alternatives. Maybe you could weigh in Ehsan? I think the behavior here should match that of the recently closed tabs menu item. Therefore, what you describe is OK except that upon entering the private browsing mode, the list of recently closed windows should be empty like the list of recently closed tabs. > And the corollary - I assume that would need to be tested, so would we add to > browser_248970_a or make browser_248970_c? I'd rather ask you to do this in a new file, like for example browser_248970_c (or even better browser_bug394759_privatebrowsing).

I don't know HOW you would do it, but I can perhaps offer how I would expect it to behave. I think of private browsing as being analogous to switching to another, temporary, profile that is a clone of SOME parts of the the regular user profile. Upon exiting private browsing, I would expect to return to the original profile, as if you exited the browser (saving open tabs) and then reopened it. Everything should return exactly to the way it was before entering private browsing, which includes restoring undo lists. (Note: If exiting and restarting does not restore undo lists, that's a separate argument to be had. In my world, I like things to be a lot like a Smalltalk VM, where everything is an object in memory in an environment, and quitting and restarting involves saving and restoring the entire VM. But that's just me and a bunch of other fanatics.)

> I think the behavior here should match that of the recently closed tabs menu > item. Therefore, what you describe is OK except that upon entering the private > browsing mode, the list of recently closed windows should be empty like the > list of recently closed tabs. I thought about that, but that's a bad example. The recently closed tabs are confined to a window even in normal browsing, so the fact that we close the open windows immediately makes recently closed tabs null & void. Regardless, it's a simple change to get what you suggest. (In reply to comment #49) > ... I think of private browsing as being analogous to switching to > another, temporary, profile that is a clone of SOME parts of the the regular > user profile. Upon exiting private browsing, I would expect to return to the > original profile, as if you exited the browser (saving open tabs) and then > reopened it. Everything should return exactly to the way it was before > entering private browsing, which includes restoring undo lists. That's the plan, at least in the scope of this undo list.

(In reply to comment #50) > > I think the behavior here should match that of the recently closed tabs menu > > item. Therefore, what you describe is OK except that upon entering the private > > browsing mode, the list of recently closed windows should be empty like the > > list of recently closed tabs. > > I thought about that, but that's a bad example. The recently closed tabs are > confined to a window even in normal browsing, so the fact that we close the > open windows immediately makes recently closed tabs null & void. Regardless, > it's a simple change to get what you suggest. I'm not sure I understand what you mean here. Can you please give an example of the behavior you're suggesting, and why mine was a bad example?

(In reply to comment #51) > I'm not sure I understand what you mean here. Can you please give an example > of the behavior you're suggesting, and why mine was a bad example? I meant that it was a bad example of expected behavior for windows. Recently closed tab data is scoped to a window. So when we close the windows upon entering PB, there is absolutely no closed tab data available to the user. I don't think that there is a good example of what I was thinking in comment #47. The entries on the history menu are the closest I can come up with - the entries shown there don't just disappear when you enter PB. Upon exiting PB, you're left in the same state as when you started. New pages don't get temporarily added to the menu, so that's where the similarities end. I'm fine with either path, and I'm leaning towards what you suggested anyway. It does better at giving the user the impression that they are entering a different state, or "temporary profile" (to steal the term from Timothy).

(In reply to comment #52) > I'm fine with either path, and I'm leaning towards what you suggested anyway. > It does better at giving the user the impression that they are entering a > different state, or "temporary profile" (to steal the term from Timothy). We have tried to impose this "separation" indeed. CCing mconnor to see if he wants to add something to this discussion.

Now with Simon's test (modified) & my own (for PB). It's still not quite done and there's probably a little bit of debug stuff in there, but it's mostly done. This is patched on top of the patch for bug 481560 and bug 462673 (though those just cover the single tab case I talked about before) Simon if you could give it a quick look & see if there are any obvious things (I still didn't reuse _lastClosedWindows). Maybe a browser peer could look at it too if they have time so it can get a final review faster?

Paul, in order to hit string freeze, can we get a strings-only patch here that we can land immediately, along with a localization note that points to the bug (or a screenshot?) of what the feature will look like?

Comment on attachment 368183 [details] [diff] [review] Patch v0.4 (mostly done) >+ // XXXzpao do we need to check the "open" tabs in closed windows too? I'd say, yes we do. > case "enter": > this._inPrivateBrowsing = true; >+ this._closedWindows = []; I'd prefer this to be in the same place where _closedWindows is restored: inside nsPrivateBrowsing.js (just add |_closedWindows: []| to the blank session to be loaded when entering PB mode). >+ // replace "Loading..." with the document title (with minimal side-effects) >+ if (winData.title == tabbrowser.mStringBundle.getString("tabs.loading")) { >+ tabbrowser.setTabTitle(tabbrowser.selectedTab); >+ winData.title = tabbrowser.selectedTab.label; >+ } Please refactor this into its own little helper method instead of copy&pasting. >+ undoCloseWindow: function sss_undoCloseWindow(aIndex) { >+ else { >+ throw (Components.returnCode = Cr.NS_ERROR_INVALID_ARG); >+ } I prefer the code flow we've now got in undoCloseTab: Throw ASAP so that there's no need for big conditional blocks. >+ return { windows: total, selectedWindow: ix + 1, _closedWindows: this._closedWindows }; What to do about _lastClosedWindows...? >+ if (root._closedWindows) >+ this._closedWindows = root._closedWindows; Nit: Not indented by two spaces. And the lines to be removed: >+ // let undoItems = eval("(" + ss.getClosedWindowData() + ")"); >+ dump(aMsg + "\n"); >+ dump("\n\n\n\n394759\n\n\n\n");

Screenshot of UI in action, covering all 3 cases (0, 1, multiple other tabs)

I think this is it for the strings-only patch - with a localization note by each string referencing this bug, which would get removed when the rest of the patch lands.

"Open All in Windows" doesn't make much sense to me. How about "Restore All Windows"?

Comment on attachment 368305 [details] [diff] [review] Strings Only Patch v0.1 Dao's right. a191=beltzner with that change.

beltzner: "you can just carry over the a+"

With an additional change suggested by Dão - changing the entity name & access key to R

Missed updating the localization note with the entity name change.

Landed the strings: http://hg.mozilla.org/mozilla-central/rev/5e53b13462c4 http://hg.mozilla.org/releases/mozilla-1.9.1/rev/2d09ddc3607d

(In reply to comment #56) > >+ // XXXzpao do we need to check the "open" tabs in closed windows too? > > I'd say, yes we do. We don't for open windows currently. I would argue that we should be consistent as far as that goes. I agree with what you're saying that we should check, but perhaps we should be doing it for open windows/tabs too. > >+ return { windows: total, selectedWindow: ix + 1, _closedWindows: this._closedWindows }; > > What to do about _lastClosedWindows...? We were never storing _lastClosedWindows there before. And since I'm not touching _lastClosedWindows at all, why should we start storing that state now?

Should be just about ready to go (depending on how comment #65 pans out) - but any changes for that should be pretty easy. Built on top of the strings that landed & patches for "depends on" bugs.

(In reply to comment #65) > We don't for open windows currently. I would argue that we should be consistent > as far as that goes. The difference between open and closed windows is that you can see the former but not the latter. Not clearing "open" tabs in _closed_ windows is a privacy risk more than the reverse a dataloss one. IOW: "open" tabs in closed windows should be treated consistently like closed tabs in open windows. > perhaps we should be doing it for open windows/tabs too. That's bug 464417. > We were never storing _lastClosedWindows there before. And since I'm not > touching _lastClosedWindows at all, why should we start storing that state now? The state of _lastClosedWindows is already stored, thus potentially leading to a window being both reopened and listed as closed (cf. the last bit of comment #14 and the entire comment #16).

Comment on attachment 368810 [details] [diff] [review] Patch v0.5 The private browsing related changes look fine.

Since bug 367204 touches the transition state in PB, this will have to be updated to reflect those changes.

(In reply to comment #69) > Since bug 367204 touches the transition state in PB, this will have to be > updated to reflect those changes. Sorry, bug 481598.

(In reply to comment #67) > The difference between open and closed windows is that you can see the former > but not the latter. Not clearing "open" tabs in _closed_ windows is a privacy > risk more than the reverse a dataloss one. IOW: "open" tabs in closed windows > should be treated consistently like closed tabs in open windows. Fair enough. Since it seems like there wasn't a clear consensus for bug 464417, should we just destroy the data for the relevent tabs (and update window.selected), or should we "close" the tab? In this case, we'd have to simulate a lot of what is already happening for "close tab." Another option would be to just remove the relevant site from tab.entries. Of the 3, the first option is easiest, but might not line up with whatever happens for bug 464417. I'm leaning towards option 1. I think both features will be relatively unused & in combination, rare. Since bug 464417 isn't done, we can always line this up later. > > We were never storing _lastClosedWindows there before. And since I'm not > > touching _lastClosedWindows at all, why should we start storing that state now? > > The state of _lastClosedWindows is already stored, thus potentially leading to > a window being both reopened and listed as closed (cf. the last bit of comment > #14 and the entire comment #16). Ah, I had forgotten how it's used. Now that I've refreshed myself on it and thought through this a little bit, it's not super complicated. And like you said in comment #16, there is value in just using _lastClosedWindows. This also brings up the point of popup windows. I'm going to assume that we don't treat them any differently and they just get added to the undo close window list as we go. When you restore your session they are added to the undo list. There might be some oddities implementing this, but we shall see.

(In reply to comment #71) > should we just destroy the data for the relevent tabs Yes, as we already completely remove data about closed tabs containing the domain to forget. > This also brings up the point of popup windows. Please just make sure that we always restore at least one non-popup window as we currently do (as on platforms other than OS X, popups might not show the menu bar required for reopening other windows).

(In reply to comment #72) > Please just make sure that we always restore at least one non-popup window as > we currently do (as on platforms other than OS X, popups might not show the > menu bar required for reopening other windows). How will this interact with the undo list (thinking from a user perspective, not implementation)? Take this for example: 1. User closes window X (normal window). 1. User closes window Y (normal window). 2. User closes window Z1 (pop-up window). 2. User closes window Z2 (pop-up window) & quits. At the state of quitting, X, Y, Z1 are on the undo list. What happens when we reopen Firefox?

(In reply to comment #73) > At the state of quitting, X, Y, Z1 are on the undo list. What happens when we > reopen Firefox? As I said: Z2, Z1 and Y should all be reopened, so that the user actually gets at least one window with a menu bar from which she could manually reopen. Also note: that behavior will have to be different on OS X where only the windows open at the time of the user actually quitting (and not just closing windows) should be reopened. In your example, this would thus be no window at all.

I think I was a little unclear in comment #73. I was trying get a better handle on expected behavior regarding the undo menu. Like you said in comment #74, that's fine for the case above. X would be on the undo list at restore. But what if there was a popup Z3 in there? Y, Z1, Z2 would be on the undo list at shutdown, but since Y, Z1, Z2, Z3 would all be restored, the undo list would presumably be empty on system restore. Is that the expected behavior? (In reply to comment #16) > If you reuse _lastClosedWindows, you'd thus have to make sure that it contains > at least one non-popup window when none is open, even if that means that more > than max_windows_undo windows can be reopened. In fact, I'd be fine with just > not capping when only popup windows are open. This is where things get a little tricky and difficult to properly test (in relation to what I was saying earlier in this comment)... Let's take this example: Undo list is [Y, Z1, Z2] and we still have popup windows Z3 and Z4 open. max_windows_undo=3. We close Z3. Now since we have to keep at least one normal window around, so undo list is [Y, Z1, Z2, Z3]. When we show the list to the user, do we stop and just show 3 [Z1, Z2, Z3] or do we show the whole list... Let's say we just show [Z1, Z2, Z3], then the user reopens Z3, does the list keep showing 3 [Y, Z1, Z2]? Will I have to add more ifndef for OS X since the behavior is slightly different (we don't actually need to keep a normal window around if we go above max_windows_undo)? Sorry if this is getting nitty-gritty & redundant, but as they say, the devil's in the details.

(In reply to comment #75) > But what if there was a popup Z3 in there? Y, Z1, Z2 would be on the undo list > at shutdown, but since Y, Z1, Z2, Z3 would all be restored, the undo list would > presumably be empty on system restore. Is that the expected behavior? I guess that'd be acceptable. Might still want to check with an UI guru, though. > Let's take this example: Undo list is [Y, Z1, Z2] and we still have popup > windows Z3 and Z4 open. max_windows_undo=3. We close Z3. Now since we have to > keep at least one normal window around, so undo list is [Y, Z1, Z2, Z3]. When > we show the list to the user, do we stop and just show 3 [Z1, Z2, Z3] or do we > show the whole list... Since we don't expose the .max_windows_undo pref to the user, I'd just show the whole list. Bonus points for more aptly naming max_windows_undo to indicate that it only partially applies to popup windows. > Will I have to add more ifndef for OS X since the behavior is slightly > different? I'm afraid so.

Finished converting to using _lastClosedWindows. This is passing current tests, but it's should get some additional coverage: * Actually test that we're removing the right data in the "forget this site" case * Add tests for the multiple popup window cases & ifdefs for OS X vs the rest

Comment on attachment 370303 [details] [diff] [review] Patch v0.6 A few more drive-by comments: > "_closedTabs": [] >- }] >+ }], >+ "_lastClosedWindows": [] I'd prefer _closedWindows for consistency with _closedTabs. > if (isFullyLoaded) { >- winData.title = aWindow.content.document.title; >+ winData.title = aWindow.content.document.title || tabbrowser.selectedTab.label; >+ winData.title = this._replaceLoadingTitle(winData.title, tabbrowser, >+ tabbrowser.selectedTab); > this._updateCookies(this._lastClosedWindows); On a side note: This might override cookies in already closed windows and could be considered slightly wrong (which'd be my fault). Don't we rather want > this._updateCookies([this._lastClosedWindows[0]]); >+ * Get the number of restore-able windows >+ unsigned long getClosedWindowCount(); >+ getClosedWindowCount: function sss_getClosedWindowCount() { >+#ifdef XP_MACOSX >+ return this._lastClosedWindows.length; >+#else >+ let maxWindowsUndo = this._prefBranch.getIntPref("sessionstore.max_windows_undo"); >+ return this._lastClosedWindows.length > maxWindowsUndo ? maxWindowsUndo >+ : this._lastClosedWindows.length; >+#endif >+ }, API and documentation don't currently match. And as I said: I'd prefer us to always return this._lastClosedWindows.length from this API for transparency reasons (i.e. undoCloseWindow should always fail after calling it getClosedWindowCount() times in a row - it currently doesn't). >+ do { >+ total.unshift(this._lastClosedWindows.shift()) >+ } while (total[0].isPopup) This looks like a destructive action on _lastClosedWindows when it shouldn't be (this method is supposed to be idempotent with no windows opened). You'll rather want to (shallowly) clone _lastClosedWindows first and then shift from the clone. >+ // Resize _lastClosedWindows to comply with Pref >+ // this._lastClosedWindows.splice(this._prefBranch.getIntPref("sessionstore.max_windows_undo")); If the clone from above doesn't have the correct size at this point, something's off with sss_resizeLastClosedWindows. >+ _resizeLastClosedWindows : function ss_resizeLastClosedWindows() { s/ss_/sss_/ ;-) Besides, when I first read that name, I thought that you were going to iterate through all closed windows and change their height and width. Maybe rather _capLastClosedWindowsList ? >+ let normalWindowIndex = 0; >+ for (normalWindowIndex = 0; >+ normalWindowIndex < this._lastClosedWindows.length && >+ this._lastClosedWindows[normalWindowIndex].isPopup; >+ normalWindowIndex++); Nit: No need to set normalWindowIndex to 0 twice. Then the fourth line should be aligned with the third or maybe better, the whole bit should be written as a while loop. >+ // Either we have a normal window in the normal range or we don't have one at all What's the "normal range"? >+ if (normalWindowIndex < maxWindowsUndo || >+ normalWindowIndex > this._lastClosedWindows.length) >+ break; Break from what? s/break/return/ ? >+ >+ this._lastClosedWindows.splice(normalWindowIndex, >+ this._lastClosedWindows.length - normalWindowIndex); Won't that clear out the first non-popup window as well?

(In reply to comment #78) > I'd prefer _closedWindows for consistency with _closedTabs. I thought about this too. Is that something you'd like used internally as well (rename all _lastClosedWindows to _closedWindows), or just for the saved session state? > On a side note: This might override cookies in already closed windows and could > be considered slightly wrong (which'd be my fault). Don't we rather want > > > this._updateCookies([this._lastClosedWindows[0]]); Indeed, I think that is what we want. Or we can just use [winData] which is a little cleaner and fits in better with the code immediately preceding it. > > >+ * Get the number of restore-able windows > >+ unsigned long getClosedWindowCount(); > > >+ getClosedWindowCount: function sss_getClosedWindowCount() { > >+#ifdef XP_MACOSX > >+ return this._lastClosedWindows.length; > >+#else > >+ let maxWindowsUndo = this._prefBranch.getIntPref("sessionstore.max_windows_undo"); > >+ return this._lastClosedWindows.length > maxWindowsUndo ? maxWindowsUndo > >+ : this._lastClosedWindows.length; > >+#endif > >+ }, > > API and documentation don't currently match. And as I said: I'd prefer us to > always return this._lastClosedWindows.length from this API for transparency > reasons (i.e. undoCloseWindow should always fail after calling it > getClosedWindowCount() times in a row - it currently doesn't). My mistake. I'd written this before your previous comment about the behavior here & _resizeLastClosedWindows. Since we resize on closing a window we don't have to do this fancy crap here. > > >+ do { > >+ total.unshift(this._lastClosedWindows.shift()) > >+ } while (total[0].isPopup) > > This looks like a destructive action on _lastClosedWindows when it shouldn't be > (this method is supposed to be idempotent with no windows opened). You'll > rather want to (shallowly) clone _lastClosedWindows first and then shift from > the clone. Good point. > > >+ // Resize _lastClosedWindows to comply with Pref > >+ // this._lastClosedWindows.splice(this._prefBranch.getIntPref("sessionstore.max_windows_undo")); > > If the clone from above doesn't have the correct size at this point, > something's off with sss_resizeLastClosedWindows. > > >+ _resizeLastClosedWindows : function ss_resizeLastClosedWindows() { > > s/ss_/sss_/ ;-) > > Besides, when I first read that name, I thought that you were going to iterate > through all closed windows and change their height and width. Maybe rather > _capLastClosedWindowsList ? Fair enough. > > >+ let normalWindowIndex = 0; > >+ for (normalWindowIndex = 0; > >+ normalWindowIndex < this._lastClosedWindows.length && > >+ this._lastClosedWindows[normalWindowIndex].isPopup; > >+ normalWindowIndex++); > > Nit: No need to set normalWindowIndex to 0 twice. Then the fourth line should > be aligned with the third or maybe better, the whole bit should be written as a > while loop. I had almost written it as a while loop (taking too many shortcuts and ++ing inside the while). I'll convert it to a readable while loop. > > >+ // Either we have a normal window in the normal range or we don't have one at all > > What's the "normal range"? Normal range is the pref setting > > >+ if (normalWindowIndex < maxWindowsUndo || > >+ normalWindowIndex > this._lastClosedWindows.length) > >+ break; > > Break from what? s/break/return/ ? Oops. > > >+ > >+ this._lastClosedWindows.splice(normalWindowIndex, > >+ this._lastClosedWindows.length - normalWindowIndex); > > Won't that clear out the first non-popup window as well? Looking at this again & running a super-simplified extraction, this is actually totally wrong. This is pulling off the end of the array, when we actually want the ones at the front of the array (I'm using |unshift| not |push| in onClose). Tests will ensure that I get this right (I'll have to slice(0, normalWindowIndex + 1))

(In reply to comment #79) > > >+ // Either we have a normal window in the normal range or we don't have one at all > > > > What's the "normal range"? > > Normal range is the pref setting I realized that this comment probably still didn't explain it well... What I meant is that if we have a normal window within in _lCW[0, 1, 2] or we don't have a normal window at all, we don't need to resize the array. if _lCW[3] is normal & _lCW[4] is a popup, we want to trim off _lCW[4].

(In reply to comment #79) > Is that something you'd like used internally as well > (rename all _lastClosedWindows to _closedWindows) Only if it doesn't cause too much patch noise.

(In reply to comment #81) > Only if it doesn't cause too much patch noise. It won't. I'm already making changes around any reference using _lastClosedWindows. (In reply to comment #78) > >+ this._lastClosedWindows.splice(normalWindowIndex, > >+ this._lastClosedWindows.length - normalWindowIndex); > > Won't that clear out the first non-popup window as well? You were absolutely right & I was totally wrong in comment #79

Almost done. Refreshed for recent checkins, makes fixes per comment #78, has better test coverage. It still needs a test for purge domain, but that should be wrapped up today.

Comment on attachment 371261 [details] [diff] [review] Patch v0.7 Looking good. >+ // XXXzpao we should probably handle things more gracefully here & check >+ // if it was the last tab in the window & then remove the window? Yes, please, unless Alex objects... otherwise you'll have to add data for a blank tab, so that the window can still be reopened without throwing errors (see the discussion in bug 480148). That reminds me: The window's title will have to be cleared/replaced, when the selected tab is thrown away. >+ // shallow copy this._closedWindows to preserve current state >+ let lastClosedWindowsCopy = []; >+ for (let i = 0; i < this._closedWindows.length; i++) >+ lastClosedWindowsCopy[i] = this._closedWindows[i]; FYI, arrays do have a clone method, it's just not obviously named: let lastClosedWindowsCopy = this._closedWindows.slice();

(In reply to comment #84) > (From update of attachment 371261 [details] [diff] [review]) > Looking good. > > >+ // XXXzpao we should probably handle things more gracefully here & check > >+ // if it was the last tab in the window & then remove the window? > > Yes, please, unless Alex objects... otherwise you'll have to add data for a > blank tab, so that the window can still be reopened without throwing errors > (see the discussion in bug 480148). Beltzner says we should just "close" the window, since that's the same behavior as open windows. And now another thought... This could potentially mess up the work being done to ensure we have at least 1 normal window in the undo path for that popups on restore case. (grumble) I was also thinking (with the code that's right around this) that I should be moving the "open" tabs that I'm "closing" to the undo close tab list (and then truncating that to match the pref). > That reminds me: The window's title will have to be cleared/replaced, when the > selected tab is thrown away. Indeed. > FYI, arrays do have a clone method, it's just not obviously named: > let lastClosedWindowsCopy = this._closedWindows.slice(); Good call.

(In reply to comment #85) > And now another thought... This could potentially mess up the work being done > to ensure we have at least 1 normal window in the undo path for that popups on > restore case. (grumble) Yeah, theoretically you'd have to keep a blank closed window if all of these conditions are true: 1. This isn't a Mac. 2. No non-popup browser window is open. 3. Your list consists entirely of popup windows. I'd be fine with leaving this edge-case for a follow up bug, though. > I was also thinking (with the code that's right around this) that I should be > moving the "open" tabs that I'm "closing" to the undo close tab list No, they should be gone for good (note: you're clearing out the closed tabs lists of closed windows as well).

Now complete with tests! I also refreshed a couple places so that we're doing the right thing (eg. actually loading the favicon). (In reply to comment #76) > Bonus points for more aptly naming max_windows_undo to indicate > that it only partially applies to popup windows.

Comment on attachment 371966 [details] [diff] [review] Patch v0.8 I'm afraid this will need another round. Most of it are small changes, though. >+// how many windows can be reopened (per session) A note might be helpful that this limit isn't strictly enforced with popup windows on that list. >+ } else if (openTabs.length != openTabCount) { Style nit: else goes on a new line. >+ // some duplication from restoreHistory - make sure we get the correct title >+ let activeIndex = (selectedTab.index || selectedTab.entries.length) - 1; >+ if (activeIndex >= selectedTab.entries.length) >+ activeIndex = selectedTab.entries.length - 1; activeIndex isn't used anywhere - and isn't needed, either, as at this point we control the data and now that it's in a consistent state (as opposed to in restoreHistory where we work on data potentially received through the API. >+ this._closedWindows[ix].title = selectedTab.entries[selectedTab.index || 0].title; Instead of [selectedTab.index || 0], shouldn't this be [selectedTab.index - 1] ? >+ getClosedWindowData: function sss_getClosedWindowData() { >+ return JSON.stringify(this._closedWindows); Please use _toJSONString instead of JSON.stringify (to prevent e.g. bug 485563). >+ // shallow copy this._closedWindows to preserve current state >+ let lastClosedWindowsCopy = this._closedWindows.splice(); s/splice/slice/! >+ /** >+ * Resize this._closedWindows to the value of the pref, except in the case >+ * where we don't have any normal windows on Windows and Linux. Then we must >+ * resize such that we have at least one normal window. For clarity, please replace "normal" with "non-popup" throughout this method, as IMO with "normal" it just isn't obvious enough what the unnormal exception would be. >+ // Either we have a normal window in the normal range or we don't have one at all This comment is (still) more confusing than helpful - and doesn't apply, as we could just as well have a "normal" window outside the "normal" range (that's when we actually have to splice). >+ if (normalWindowIndex < maxWindowsUndo || >+ normalWindowIndex >= this._closedWindows.length) >+ return; >+ >+ this._closedWindows.splice(normalWindowIndex + 1); Please invert the condition so that you can drop the return statement. And also splice at maxWindowsUndo when there are no non-popup windows on the list at all (unless I miss the use case where that indeed makes sense). >+ // open a window and close it once it's completely loaded Nit: Open and close... what for? ;-) >+ let data = JSON.parse(ss.getClosedWindowData())[0]; >+ ok(data.title == testURL && data.toSource().indexOf(uniqueText) > -1, Nit: Instead of parsing and toSourcing, you could directly test the data returned by the API (also further below). >+ try { >+ newWin2 = ss.undoCloseWindow(0); >+ } catch (ex) { } Are we still using this idiom in other tests? Considering that you don't handle the exception case later on (where you rely on newWin2 being a window), you can just let the exception propagate and let the test fail at this point. >+ ok(newWin2.gBrowser.tabContainer.childNodes.length == 2 && >+ newWin2.gBrowser.selectedBrowser.currentURI.spec == testURL, >+ "The window was correctly restored..."); AFAICT we prefer |is| instead of |ok| because it reports the unexpected value in case of failure. >+ gPrefService.setIntPref("browser.sessionstore.max_windows_undo", max_windows_undo); Something that's changed since you've started working on the test: We now prefer clearUserPref for resetting prefs (cf. bug 485088). >+ // executeSoon(function() { >+ // finish(); >+ // }); Nit: Dead code. >+ executeSoon(function() { >+ callback.call(); >+ }); Any reason executeSoon is needed here? Nit: executeSoon(callback); is more concise (also further below). BTW: Instead of |callback.call()|, you could already have just written |callback()| - and then |function() { callback() }| is almost the same as |callback|. >+ let url = "http://window" + count + ".example.com"; Not having tested this myself: Do we really have arbitrary subdomains on example.com? >+ // let obs = Cc["@mozilla.org/observer-service;1"]. >+ // getService(Ci.nsIObserverService); >+ // obs.notifyObservers(null, "browser:purge-domain-data", "mozilla.org"); Dead code? >+ // Make sure that sessionstore.js can be forced to be created by setting >+ // the interval pref to 0 >+ gPrefService.setIntPref("browser.sessionstore.interval", 0); Not sure this has to be ensured again (we already test this in browser_248970_a.js). r+=me with these fixed. You'll then still need review by a browser peer (e.g. Gavin) for the browser.js stuff and AFAICT also superreview for the API change.

> Not having tested this myself: Do we really have arbitrary subdomains on > example.com? Looks like we only have the second level domain without arbitrary subdomains (http://www.rfc-editor.org/rfc/rfc2606.txt). I tested this: > michael@michael:~$ ping window294.example.com > ping: unknown host window294.example.com > michael@michael:~$ ping a.example.com > ping: unknown host a.example.com > michael@michael:~$ ping window294.example.org > ping: unknown host window294.example.org > michael@michael:~$ ping window294.example.net > ping: unknown host window294.example.net

(In reply to comment #89) > Looks like we only have the second level domain without arbitrary subdomains > (http://www.rfc-editor.org/rfc/rfc2606.txt). I was referring to our testing environment, see <http://mxr.mozilla.org/mozilla-central/source/build/pgo/server-locations.txt>.

(In reply to comment #88) > >+ // some duplication from restoreHistory - make sure we get the correct title > >+ let activeIndex = (selectedTab.index || selectedTab.entries.length) - 1; > >+ if (activeIndex >= selectedTab.entries.length) > >+ activeIndex = selectedTab.entries.length - 1; > > activeIndex isn't used anywhere - and isn't needed, either, as at this point we > control the data and now that it's in a consistent state (as opposed to in > restoreHistory where we work on data potentially received through the API. > > >+ this._closedWindows[ix].title = selectedTab.entries[selectedTab.index || 0].title; > > Instead of [selectedTab.index || 0], shouldn't this be [selectedTab.index - 1] > ? This is where I meant to use activeIndex. > selectedTab.entries[activeIndex].title The reason I did this is that we don't necessarily control the data. _closedWindows can be passed into setBrowserState. There's no guarantee that selectedTab.index will be specified (as I discovered while writing the tests for this) > >+ getClosedWindowData: function sss_getClosedWindowData() { > >+ return JSON.stringify(this._closedWindows); > > Please use _toJSONString instead of JSON.stringify (to prevent e.g. bug > 485563). Fair. I was wondering why we were still using _toJSONString. > >+ // shallow copy this._closedWindows to preserve current state > >+ let lastClosedWindowsCopy = this._closedWindows.splice(); > > s/splice/slice/! AH, not sure how I missed that :( > >+ /** > >+ * Resize this._closedWindows to the value of the pref, except in the case > >+ * where we don't have any normal windows on Windows and Linux. Then we must > >+ * resize such that we have at least one normal window. > > For clarity, please replace "normal" with "non-popup" throughout this method, > as IMO with "normal" it just isn't obvious enough what the unnormal exception > would be. Ok > >+ // Either we have a normal window in the normal range or we don't have one at all > > This comment is (still) more confusing than helpful - and doesn't apply, as we > could just as well have a "normal" window outside the "normal" range (that's > when we actually have to splice). Right, so that comment was just supposed to apply to the if statement, which is explicitly exiting early in the cases where splicing is not necessary. > > >+ if (normalWindowIndex < maxWindowsUndo || > >+ normalWindowIndex >= this._closedWindows.length) > >+ return; > >+ > >+ this._closedWindows.splice(normalWindowIndex + 1); > > Please invert the condition so that you can drop the return statement. And also > splice at maxWindowsUndo when there are no non-popup windows on the list at all > (unless I miss the use case where that indeed makes sense). Looks like I did miss that case... (not sure how that happened). I might adjust the tests a little bit to make sure that gets coverage. > >+ // open a window and close it once it's completely loaded > > Nit: Open and close... what for? ;-) Just be more descriptive in the comment? > > >+ let data = JSON.parse(ss.getClosedWindowData())[0]; > >+ ok(data.title == testURL && data.toSource().indexOf(uniqueText) > -1, > > Nit: Instead of parsing and toSourcing, you could directly test the data > returned by the API (also further below). True. > >+ try { > >+ newWin2 = ss.undoCloseWindow(0); > >+ } catch (ex) { } > > Are we still using this idiom in other tests? Considering that you don't handle > the exception case later on (where you rely on newWin2 being a window), you can > just let the exception propagate and let the test fail at this point. I think I had copied this in from the original test you wrote for this. I'll just let that propagate. > >+ ok(newWin2.gBrowser.tabContainer.childNodes.length == 2 && > >+ newWin2.gBrowser.selectedBrowser.currentURI.spec == testURL, > >+ "The window was correctly restored..."); > > AFAICT we prefer |is| instead of |ok| because it reports the unexpected value > in case of failure. True, that is better, especially since we're comparing 2 sets of values. > > >+ gPrefService.setIntPref("browser.sessionstore.max_windows_undo", max_windows_undo); > > Something that's changed since you've started working on the test: We now > prefer clearUserPref for resetting prefs (cf. bug 485088). Easy enough change. > >+ // executeSoon(function() { > >+ // finish(); > >+ // }); > > Nit: Dead code. Indeed. > >+ executeSoon(function() { > >+ callback.call(); > >+ }); > > Any reason executeSoon is needed here? As with bug 483382, it seems that adding an executeSoon wrapper around things lets things "settle down" so to speak before the next test. In the case of the referenced bug, adding executeSoon(finish()) ensures that this test isn't affecting other test files. The same thing here - without the executeSoon (at least around finish) it causes problems with the browser_394759_privatebrowsing test - problems that *should not* be happening. > Nit: executeSoon(callback); is more concise (also further below). > > BTW: Instead of |callback.call()|, you could already have just written > |callback()| - and then |function() { callback() }| is almost the same as > |callback|. Fair > >+ let url = "http://window" + count + ".example.com"; > > Not having tested this myself: Do we really have arbitrary subdomains on > example.com? We don't (like you linked in comment #90), but for the purposes of this test it doesn't matter if it actually resolves - we just need the location. It works here, it works in other tests. > >+ // let obs = Cc["@mozilla.org/observer-service;1"]. > >+ // getService(Ci.nsIObserverService); > >+ // obs.notifyObservers(null, "browser:purge-domain-data", "mozilla.org"); > > Dead code? Indeed. > >+ // Make sure that sessionstore.js can be forced to be created by setting > >+ // the interval pref to 0 > >+ gPrefService.setIntPref("browser.sessionstore.interval", 0); > > Not sure this has to be ensured again (we already test this in > browser_248970_a.js). It doesn't, but I had just copied that from the 248970_a file. I'll take it out.

(In reply to comment #91) > _closedWindows can be passed into setBrowserState. Ah, right, missed that. > > Nit: Open and close... what for? ;-) > Just be more descriptive in the comment? Either that or drop the comment - your choice. > We don't (like you linked in comment #90), but for the purposes of this test it > doesn't matter if it actually resolves - we just need the location. It works > here, it works in other tests. You're waiting for the page to load, but get the load event for the error page instead. I'm fine with this, but please add a comment stating that fact.

This would have been up sooner, but my harddrive failed yesterday & I lost this and other work. I still need to double check this works properly on Win/Linux (it should), but that's a 1 line change if not. Simon - asking for review again to get the official stamp and also because I restructured the tests a little bit to get better coverage.

Comment on attachment 373152 [details] [diff] [review] Patch v0.9 >+ if (normalWindowIndex > maxWindowsUndo) >+ spliceTo = normalWindowIndex + 1; Beware the edge case: When normalWindowIndex == maxWindowsUndo, you'll splice the first non-popup window, even though you shouldn't.

(In reply to comment #94) > (From update of attachment 373152 [details] [diff] [review]) > >+ if (normalWindowIndex > maxWindowsUndo) > >+ spliceTo = normalWindowIndex + 1; > > Beware the edge case: When normalWindowIndex == maxWindowsUndo, you'll splice > the first non-popup window, even though you shouldn't. Yea, you're right (should have listened to you yesterday). I looked at the tryserver builds I put up last night and saw Win/Linux test failure, so I have new ones up with the fix and waiting for those to finish. They _should_ pass this time around.

This _should_ be working now. I had to track down another Win/Linux test failure (which was because of the special handling of popups), so I had to clear the browser state before running my privatebrowsing test. I have this in the tryserver now, so I should have real test results soon.

(In reply to comment #96) > ... so I should have real test results soon. And real test results are in with lots of green. Everything passes & this isn't breaking anything else.

Comment on attachment 373701 [details] [diff] [review] Patch v0.10 >diff --git a/browser/app/profile/firefox.js b/browser/app/profile/firefox.js >--- a/browser/app/profile/firefox.js >+++ b/browser/app/profile/firefox.js >@@ -727,16 +727,19 @@ > // maximum amount of POSTDATA to be saved in bytes per history entry (-1 = all of it) > // (NB: POSTDATA will be saved either entirely or not at all) > pref("browser.sessionstore.postdata", 0); > // on which sites to save text data, POSTDATA and cookies > // 0 = everywhere, 1 = unencrypted sites, 2 = nowhere > pref("browser.sessionstore.privacy_level", 1); > // how many tabs can be reopened (per window) > pref("browser.sessionstore.max_tabs_undo", 10); >+// how many windows can be reopened (per session) - not strictly enforced when >+// dealing with popup-windows on non-OS X platforms >+pref("browser.sessionstore.max_windows_undo", 3); instead of describing what the behavior is not, should instead describe what the behavior actually *is* >+/** >+ * Populate when the history menu is opened >+ */ >+HistoryMenu.populateUndoWindowSubmenu = function PHM_populateUndoWindowSubmenu() { >+ let undoPopup = document.getElementById("historyUndoWindowPopup"); >+ let menuLabelString = gNavigatorBundle.getString("menuUndoCloseWindowLabel"); >+ let menuLabelStringSingleTab = >+ gNavigatorBundle.getString("menuUndoCloseWindowSingleTabLabel"); >+ >+ // remove existing menu items >+ while (undoPopup.hasChildNodes()) >+ undoPopup.removeChild(undoPopup.firstChild); >+ >+ // get closed-windows from nsSessionStore >+ let ss = Cc["@mozilla.org/browser/sessionstore;1"]. >+ getService(Ci.nsISessionStore); could set _ss as a memoized property of HistoryMenu to only get the reference once ever. >+ // no restorable windows, so make sure menu is disabled, and return >+ if (ss.getClosedWindowCount() == 0) { >+ undoPopup.parentNode.setAttribute("disabled", true); >+ return; >+ } >+ >+ // enable menu >+ undoPopup.parentNode.removeAttribute("disabled"); >+ >+ // populate menu >+ let undoItems = JSON.parse(ss.getClosedWindowData()); does JSON.parse() throw if passed a non-valid json string? should we catch errors here? maybe overkill. >+ Cu.import("resource://gre/modules/PluralForm.jsm"); is this included elsewhere in browser.js scope? do you need to re-import here? if there are multiple consumers in browser.js scope, maybe should just make a memoized getter for it. >+ for (let i = 0; i < undoItems.length; i++) { caching undoItems[i] into a local var would make the code inside the loop more compact and readable. >+ >+ // "Open All in Windows" >+ undoPopup.appendChild(document.createElement("menuseparator")); >+ let m = undoPopup.appendChild(document.createElement("menuitem")); >+ m.setAttribute("label", gNavigatorBundle.getString("menuRestoreAllWindows.label")); >+ m.setAttribute("accesskey", gNavigatorBundle.getString("menuRestoreAllWindows.accesskey")); >+ m.setAttribute("oncommand", >+ "for (var i = 0; i < " + undoItems.length + "; i++) undoCloseWindow();"); >+} >+ undoItems.forEach(undoCloseWindow) is more compact >@@ -217,17 +218,18 @@ > let privateBrowsingState = { > "windows": [{ > "tabs": [{ > "entries": [{ > "url": "about:privatebrowsing" > }] > }], > "_closedTabs": [] >- }] >+ }], >+ "_closedWindows": [] > }; > // Transition into private browsing mode > ss.setBrowserState(JSON.stringify(privateBrowsingState)); > } doesn't setBrowserState() overwrite any existing state? wouldn't an absence of _closedTabs/_closedWindows be enough? >+ /** >+ * @param aIndex is the index of the windows to be restored (FIFO ordered). >+ * @returns the nsIDOMWindow object of the reopened window >+ */ >+ nsIDOMWindow undoCloseWindow(in unsigned long aIndex); >+ nit: space after aIndex. >+ // remove all open & closed tabs containing a reference to the given domain in closed windows wrap it >+ for (let ix = this._closedWindows.length - 1; ix >= 0; ix--) { >+ let closedTabs = this._closedWindows[ix]._closedTabs; >+ let openTabs = this._closedWindows[ix].tabs; >+ let openTabCount = openTabs.length; >+ for (let i = closedTabs.length - 1; i >= 0; i--) >+ if (closedTabs[i].state.entries.some(containsDomain, this)) >+ closedTabs.splice(i, 1); >+ for (let j = openTabs.length - 1; j >= 0; j--) { >+ if (openTabs[j].entries.some(containsDomain, this)) { >+ openTabs.splice(j, 1); >+ if (this._closedWindows[ix].selected > j) >+ this._closedWindows[ix].selected--; >+ } >+ } >+ if (openTabs.length == 0) { >+ this._closedWindows.splice(ix, 1); >+ } >+ else if (openTabs.length != openTabCount) { >+ // Adjust the window's title iif we removed and open tab "if we removed an" >@@ -1647,16 +1702,20 @@ > } > } > catch (ex) { // invalid state object - don't restore anything > debug(ex); > this._notifyIfAllWindowsRestored(); > return; > } > >+ // restore this._closedWindows >+ if (root._closedWindows) >+ this._closedWindows = root._closedWindows; >+ nit: whitspace also, the comment seems unnecessary >+function test() { >+ /** Private Browsing Test for Bug 394759 **/ >+ >+ // test setup >+ waitForExplicitFinish(); >+ >+ // private browsing service >+ let pb = Cc["@mozilla.org/privatebrowsing;1"]. >+ getService(Ci.nsIPrivateBrowsingService); >+ let profilePath = Cc["@mozilla.org/file/directory_service;1"]. >+ getService(Ci.nsIProperties). >+ get("ProfD", Ci.nsIFile); >+ >+ function getSessionstorejsModificationTime() { >+ // directory service >+ let file = Cc["@mozilla.org/file/directory_service;1"]. >+ getService(Ci.nsIProperties). >+ get("ProfD", Ci.nsIFile); >+ >+ // access sessionstore.js >+ file.append("sessionstore.js"); >+ >+ if (file.exists()) >+ return file.lastModifiedTime; >+ else >+ return -1; >+ } >+ >+ // sessionstore service >+ let ss = Cc["@mozilla.org/browser/sessionstore;1"]. >+ getService(Ci.nsISessionStore); >+ // Remove the sessionstore.js file before setting the interval to 0 >+ let sessionStoreJS = profilePath.clone(); >+ sessionStoreJS.append("sessionstore.js"); >+ if (sessionStoreJS.exists()) >+ sessionStoreJS.remove(false); test that file doesn't exist? >+ // Make sure that sessionstore.js can be forced to be created by setting >+ // the interval pref to 0 >+ gPrefService.setIntPref("browser.sessionstore.interval", 0); >+ // sessionstore.js should be re-created at this point >+ sessionStoreJS = profilePath.clone(); >+ sessionStoreJS.append("sessionstore.js"); test file exists? >+ // Set up the browser in a blank state. Popup windows in previous tests result >+ // in different states on different platforms. >+ let blankState = JSON.stringify({ >+ windows: [{ >+ tabs: [{ entries: [{ url: "about:blank" }] }], >+ _closedTabs: [] >+ }], >+ _closedWindows: [] >+ }); >+ ss.setBrowserState(blankState); >+ >+ let closedWindowCount = ss.getClosedWindowCount(); >+ >+ let testURL_A = "about:config"; >+ let testURL_B = "about:mozilla"; >+ >+ let uniqueKey_A = "bug 394759 Non-PB"; >+ let uniqueValue_A = "unik" + Date.now(); >+ let uniqueKey_B = "bug 394759 PB"; >+ let uniqueValue_B = "uniq" + Date.now(); >+ >+ >+ // Open a window >+ let newWin = openDialog(location, "_blank", "chrome,all,dialog=no", testURL_A); >+ newWin.addEventListener("load", function(aEvent) { >+ newWin.gBrowser.addEventListener("load", function(aEvent) { >+ executeSoon(function() { >+ newWin.gBrowser.removeEventListener("load", arguments.callee, true); >+ newWin.gBrowser.addTab(); >+ >+ // mark the window with some unique data to be restored later on >+ ss.setWindowValue(newWin, uniqueKey_A, uniqueValue_A); >+ >+ newWin.close(); >+ >+ // ensure that we incremented # of close windows >+ is(ss.getClosedWindowCount(), closedWindowCount + 1, >+ "The closed window was added to the list"); >+ >+ // ensure we added window to undo list >+ let data = JSON.parse(ss.getClosedWindowData())[0]; >+ ok(data.toSource().indexOf(uniqueValue_A) > -1, >+ "The closed window data was stored correctly"); >+ >+ // enter private browsing mode >+ pb.privateBrowsingEnabled = true; >+ ok(pb.privateBrowsingEnabled, "private browsing enabled"); >+ >+ // ensure that we have 0 undo windows when entering PB >+ is(ss.getClosedWindowCount(), 0, >+ "Recently Closed Windows are removed when entering Private Browsing"); >+ is(ss.getClosedWindowData(), "[]", >+ "Recently Closed Windows data is cleared when entering Private Browsing"); >+ >+ // open another window in PB >+ let pbWin = openDialog(location, "_blank", "chrome,all,dialog=no", testURL_B); >+ pbWin.addEventListener("load", function(aEvent) { >+ pbWin.gBrowser.addEventListener("load", function(aEvent) { >+ executeSoon(function() { >+ gBrowser.removeEventListener("load", arguments.callee, true); >+ >+ // Add another tab, though it's not strictly needed >+ gBrowser.addTab(); >+ >+ // mark the window with some unique data to be restored later on >+ ss.setWindowValue(pbWin, uniqueKey_B, uniqueValue_B); >+ >+ pbWin.close(); >+ >+ // ensure we added window to undo list >+ let data = JSON.parse(ss.getClosedWindowData())[0]; >+ ok(data.toSource().indexOf(uniqueValue_B) > -1, >+ "The closed window data was stored correctly in PB mode"); >+ >+ // exit private browsing mode >+ pb.privateBrowsingEnabled = false; >+ ok(!pb.privateBrowsingEnabled, "private browsing disabled"); >+ >+ // ensure that we still have the closed windows from before >+ is(ss.getClosedWindowCount(), closedWindowCount + 1, >+ "Recently Closed Windows are restored when exiting Private Browsing"); this test comment doesn't really seem right >+ >+ let data = JSON.parse(ss.getClosedWindowData())[0]; >+ ok(data.toSource().indexOf(uniqueValue_A) > -1, >+ "Recently Closed Windows data is restored when exiting Private Browsing"); this test comment is also not really right mostly just nits, r=me.

it's been a long ride, but here she is.

Comment on attachment 374232 [details] [diff] [review] Patch v1.0 (mozilla-central) a191=beltzner for the 191 version of this patch

refreshed to cleanly apply to 1.9.1

Pushed http://hg.mozilla.org/mozilla-central/rev/e167d6ca2023

Pushed to 191: http://hg.mozilla.org/releases/mozilla-1.9.1/rev/43a0291470f3 Kudos for grabbing this and getting it into 3.5!

Commit message says "Bug 394759 - Add undo close window feature. r=zeniko,dietrich,ehsan, a191=beltzner", but zeniko actually gave r- (on v0.5).

This broke several browser chrome tests, I'm hoping this will fix it: http://hg.mozilla.org/mozilla-central/rev/f9e5c69f3a93

And this: http://hg.mozilla.org/mozilla-central/rev/37ab6b890637

So was this patch intended to change the nsISessionStore interface on branch? Is that interface not meant for use by binary extensions?

(In reply to comment #98) > >+ Cu.import("resource://gre/modules/PluralForm.jsm"); > > is this included elsewhere in browser.js scope? do you need to re-import here? > if there are multiple consumers in browser.js scope, maybe should just make a > memoized getter for it. That getter will break any Cu.import("resource://gre/modules/PluralForm.jsm"); in the browser.js scope, which might be a bad idea, considering extensions. At least for 3.5b4, I think you need to undo this... For now bz pushed this, which should fix the remaining unit test orange: http://hg.mozilla.org/mozilla-central/rev/577393262a7c

Pushed http://hg.mozilla.org/mozilla-central/rev/577393262a7c and http://hg.mozilla.org/releases/mozilla-1.9.1/rev/5762ef4fa681 to fix the test orange. That said, the reason the tests were orange is because of this: Error: resource://gre/modules/PluralForm.jsm - Could not set symbol 'PluralForm' on target object. Source File: chrome://browser/content/browser.js Line: 6223 That line over here was: Cu.import("resource://gre/modules/PluralForm.jsm"); coming from browser-places.js. So this patch breaks import of PluralForm.jsm at least in some cases. Would it also break in overlays into the browser window? And if so, is that desirable on branch at this point?

adding a PluralForm setter might work: __defineSetter__("PluralForm", function (val) { delete this.PluralForm; return this.PluralForm = val; });

(In reply to comment #104) > Commit message says "Bug 394759 - Add undo close window feature. > r=zeniko,dietrich,ehsan, a191=beltzner", but zeniko actually gave r- (on v0.5). see comment #88 (In reply to comment #110) > adding a PluralForm setter might work: > > __defineSetter__("PluralForm", function (val) { > delete this.PluralForm; > return this.PluralForm = val; > }); Couldn't that lead to issues if it's set to something that's not the plural form resource (In reply to comment #107) > So was this patch intended to change the nsISessionStore interface on branch? > Is that interface not meant for use by binary extensions? It was intended, but I guess it doesn't need to be? I'm not so familiar with the binary stuff, but if that's the case can we just change the IDL so the new methods are at the bottom and revert the uuid)? (I just learned of that workaround the other day)

(In reply to comment #111) > > adding a PluralForm setter might work: > > > > __defineSetter__("PluralForm", function (val) { > > delete this.PluralForm; > > return this.PluralForm = val; > > }); > > Couldn't that lead to issues if it's set to something that's not the plural > form resource Right, but that would be the extension's fault. Same as with the getter/setter for gBrowser and the likes.

(In reply to comment #112) > (In reply to comment #111) > > > adding a PluralForm setter might work: > > > > > > __defineSetter__("PluralForm", function (val) { > > > delete this.PluralForm; > > > return this.PluralForm = val; > > > }); > > > > Couldn't that lead to issues if it's set to something that's not the plural > > form resource > > Right, but that would be the extension's fault. Same as with the getter/setter > for gBrowser and the likes. So best fix: get rid of the getter and just import when it's used, or use the setter? (I'm not so familiar with all this, so I'm deferring)

unrev the uuid, move the new methods down in the IDL for binary extension compat, made a setter for PluralForm

just defined a setter for PluralForm

Comment on attachment 374297 [details] [diff] [review] Fix Patch (mozilla-central; checked in) nit for whoever checks this in: missing ; after "delete this.PluralForm" in context.

Comment on attachment 374296 [details] [diff] [review] Fix Patch (mozilla-1.9.1; checked in) a191=beltzner

Comment on attachment 374297 [details] [diff] [review] Fix Patch (mozilla-central; checked in) http://hg.mozilla.org/mozilla-central/rev/2157633ea874

Comment on attachment 374296 [details] [diff] [review] Fix Patch (mozilla-1.9.1; checked in) http://hg.mozilla.org/releases/mozilla-1.9.1/rev/b057539eee94

unrevving the uuid means that binary extensions that might want to use the new methods can't. The real right way to add things like that on branches, typically, is to create a new interface...

Oh, and even adding to the end doesn't help if an extension implements an interface derived from this one. Not sure how likely that is in this case.

bz - critical for b4? I agree that creating a new interface is probably righter, but maybe we can do that for RC? we did the right thing on trunk, though, right?

Not being able to use the new functionality on branch isn't that big of a deal. I think it's highly unlikely that this interface is used at all outside of script (and even less likely that it's implemented/extended), so adding to the end and not revving the IID is mostly just paranoia.

verified with Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1b4) Gecko/20090423 Firefox/3.5b4

user-doc-complete <https://support.mozilla.com/kb/Menu+Reference?bl=n>

I get Ventanas cerradas recientemente (Recently closed windows) grayed and inactive even after having opened and closed several windows. Restore form last session is not localized. It should be Restaurar la sesión previa/anterior.

(In reply to comment #126) > I get Ventanas cerradas recientemente (Recently closed windows) grayed and > inactive even after having opened and closed several windows. If you have clear STR then please file a new bug and mention it here. > Restore form last session is not localized. It should be Restaurar la sesión > previa/anterior. That's part of the Nightly Tester Tools.

(In reply to comment #127) > (In reply to comment #126) > > I get Ventanas cerradas recientemente (Recently closed windows) grayed and > > inactive even after having opened and closed several windows. > > If you have clear STR then please file a new bug and mention it here. > > > Restore form last session is not localized. It should be Restaurar la sesión > > previa/anterior. > > That's part of the Nightly Tester Tools. I get Ventanas cerradas recientemente (Recently closed windows) grayed and > > inactive even after having opened and closed several windows. > > If you have clear STR then please file a new bug and mention it here. I've recently created a fresh profile and Recently closed windows is active as it should be. Restore form last session is not localized. It should be Restaurar la sesión > > previa/anterior. There's no Restore form last session option in the fresh profile but there is a Recently closed tabs one. Is this correct?

(In reply to comment #128) > > > Restore form last session is not localized. It should be Restaurar la sesión > > > previa/anterior. > > > > That's part of the Nightly Tester Tools. > > There's no Restore form last session option in the fresh profile but there is a > Recently closed tabs one. > Is this correct? Yes, and the answer is above. The restore entry is part of the Nightly Tester tools. So we are totally fine here.

It's good to know my FF 3.5 is working in an absolutely correct way!

I'm porting this bug to SeaMonkey and during than i got some nits from neil@parkwaycc.co.uk , i think they are useful. If you agree with them I'll be glad to file bug and provide a patch: >+ // 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.

(In reply to comment #131) > If you agree with them I'll be glad to file bug and provide a patch: That would be much appreciated.

Those new functions haven't been documented yet: https://developer.mozilla.org/en/nsISessionStore

The documentation has been updated.