Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8.1) Gecko/20060929 BonEcho/2.0 ID:2006092903 From mirp @ http://forums.mozillazine.org/viewtopic.php?p=2515163#2515163 1. New profile, start firefox 2. Tools > Options > Main > Startup, set 'When Bon Echo Starts' to 'Show my tabs and windows from last time' 3. Open some tabs. 4. Open downloads window from Tools > Downloads 5. Close the browser window (so only the download window remains open) 6. Whilst the download window is still open, relaunch firefox Expected: Firefox window starts up with previous session's tabs present Actual: Firefox starts up, but not with the session-you-just-closed's tabs I guess this is because when you shut down the last browser window but with the downloads window still active, firefox doesn't consider this to be closing firefox down, so no session data is saved. Perhaps instead of firefox saving session date only when the firefox.exe process is closing, it should do it when the last browser window is closing?

*** Bug 357895 has been marked as a duplicate of this bug. ***

It's not clear to me that the problem is that the session isn't being saved -- I think it's much more likely that the session isn't being restored in the case where we have a pre-existing process with no open windows.

(In reply to comment #2) Should rather have duped this bug to bug 357895 which describes the issue more accurately: a "session" starts when Firefox is launched and won't end before it being completely terminated (as in: no more process). Most users would however expect a session to go from first browser window opened to last browser window closed (the odd Mac user being the exception). Making SessionStore behave as expected might even prevent accidental dataloss...

(In reply to comment #3) > (In reply to comment #2) > Should rather have duped this bug to bug 357895 which describes the issue more > accurately: a "session" starts when Firefox is launched and won't end before it > being completely terminated (as in: no more process). I resummarized to the summary from bug 357895 in hopes that that would make this clearer.

how can startup actions take place unless the application process is totally gone? isn't this at best an enhancement request?

(In reply to comment #5) > how can startup actions take place unless the application process is totally > gone? > isn't this at best an enhancement request? I don't know what the best categorization of this is technically, but it certainly _appears_ to be a bug to a user. In fact, I'm adding the dataloss keyword based on my encounter of this tonight: (1) Begin watching a quicktime video in a separate pop-up window (2) Close main browser window containing many tabs I wanted to restore next time; continue watching video (3) Half an hour passes (4) Re-launch browser as video ends, but before I've closed pop-up window (mentally, I no longer was thinking that this video was a continuation of my long-finished browsing session) (5) Browser pulls up a single blank tab, rather than my saved tabs (6) The only way to finish my session now is to close this window -- which makes its useless set of a single blank tab into the set of tabs to be restored on next startup. (7) Close windows and restart browsing session. Peer hopefully into recently closed tabs menu, in vain; my previous tabs aren't there. In other words, if I forget that my session hasn't finished, I can, without warning, irrevocably blow away all my prior data simply by asking the browser to open a window. This is pretty disastrous. Now, a careful reader will note that the circumstance I described in the above steps wouldn't actually be fixed by the fix proposed on this bug, because my remaining open window was in fact showing page content, not, say, my list of downloads or some other non-content. "Ah," you will say, "this was really a browser window, so what would you have us do?" Well, in this case, I would note that this was a chromeless popup, and I would propose that chromeless windows like this should perhaps be treated identically to non-content windows like the downloads window in terms of deciding whether there are already open browser windows. Since the proposed solution here is to restore the previously-open set of tabs when the "first" browser window is open, this will solve my most recent case.

Peter, here's a scenario similar to yours: 1. Open 2 windows, each with some tabs 2. Open a chromeless content window with a movie or whatever 3. Close both tabbed windows in quick succession, to focus on the movie Given the user model being described in this bug (if i understand it correctly), the user would expect both windows to be restored after a restart. In the proposed solution however, only the last-closed tabbed window would be restored after a restart. Even given the solution proposed, it's difficult to always behave in accordance with the user model WRT to what constitutes a session. Maybe we should have an explicit "restore me?" checkbox on the window-close prompt.

(In reply to comment #7) > Peter, here's a scenario similar to yours: > > 1. Open 2 windows, each with some tabs > 2. Open a chromeless content window with a movie or whatever > 3. Close both tabbed windows in quick succession, to focus on the movie > > Given the user model being described in this bug (if i understand it > correctly), the user would expect both windows to be restored after a restart. > In the proposed solution however, only the last-closed tabbed window would be > restored after a restart. Actually, I would only expect the most recently closed window to be restored. If I quit my app by closing multiple windows one-by-one, then on restore I get the last-closed window. This wouldn't change that. The user model has little to do with how quickly the windows are closed, and much to do with whether they are "browser windows" or not. The difficulty is in defining what a "browser window" is, in a way that matches user expectations.

I think the best way of solving this would be to make Firefox aware of when there is no browser window open, so that when only Download Manager is open and a browser window is opened, everything proceeds just like Firefox would have been just started. If your setting was "When Firefox starts: Show my windows and tabs from last time", the tabs from the previously closed browser window are now reopened. This way no data is lost. I can't think of a downside to this. And as Peter pointed out, normally on startup only the most recently closed window is closed. That would happen in this case too.

There's a very-much-related OSX issue in bug 365324 where users can easily enter a case where they close a window without closing an application. Also, I *think* a slightly clearer way to express the dataloss problem is: "Someone closes his top-level window, then sees the DL window, loses session, takes a shot." In STR: 1. Open a bunch of tabs. 2. Open the download window. 3. Click the window close button at the upper right of the browser window. 4. "Yeah, yeah, close N tabs, I got session restore, baby!" 5. Notice there's still a download manager window, and now you've closed all your tabs. 6. Take shot.

(In reply to comment #11) > "Someone closes his top-level window, then sees the DL window, loses session, > takes a shot." Not quite. Opposed to the original STR there's no necessary dataloss in this scenario: if you close the Download Manager (resp. the app on OS X) and _then_ reopen Firefox, you'll have your last browser window restored as expected. BTW: When trying to be *clearer*, please do this by also using simpler language. ;-) ("Taking a shot" doesn't really make sense to me in this context.)

This patch moves SessionStartup initialization back (which should also fix bug 365581) so that it can be reused on demand. When the last browser window is closed, SessionStore shuts down (saving state to disk if needed) and lets SessionStartup take over again, (almost) as if no browser window had been opened yet. Note that browser popups are currently considered browser windows as well (that fact can be changed independently in bug 368677).

this generally looks good, seems to address the STRs in the comments. however, i saw the error below on mac after applying the patch and following these steps: 1. close last window 2. open a new window (restores session) 3. close the app ************************************************************ * Call to xpconnect wrapped JSObject produced this error: * [Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIObserverService.removeObserver]" nsresult: "0x80004005 (NS_ERROR_FAILURE)" location: "JS frame :: file:///Users/dayala/moz/newplaces/build-dbg/dist/MinefieldDebug.app/Contents/MacOS/components/nsSessionStore.js :: sss_uninit :: line 256" data: no] ************************************************************

It could happen that the domwindowopened observer was removed twice, once when changing back to "stopped" state and once at application-quit. Making sure that we only remove it in the first case should get rid of your error.

BTW: The behavior introduce here might/should get further tweaked in bug 382006 to adapt to the Mac-way, where window-less sessions are not only possible but AFAICT even expected.

see: https://bugzilla.mozilla.org/show_bug.cgi?id=391229 An enhanced warning may help, but enhanced saving and reloading would be better. as is: "You are about to close 32 tabs. Are you sure you want to continue?" without enhanced session saving, suggest something like: "You are about to close 32 tabs. There are other existing windows, popups and or processes. Closing this window now will cause loss of data for these 37 tabs and they WILL NOT RESTORE on restart. Are you sure you want to continue?" enhancing what is saved on [x] may negate the need for such a warning.

Sorry to get into your discussion all of the sudden but I don't believe making warning messages more complex would make it better. People will just learn to click on OK each time they get that message or (older people) will be afraid and not know what they should do if they see such a long text, with so many numbers.

I agree that an enhanced saving/recall method is preferable. I am aware of the problem and yet it catches me by surprise. I had a window with 40+ tabs open for projects i was researching. I closed it after seeing the existing caution. I then discovered that a stray window had been left open and so the single [and unneeded] view on it was all that was preserved for reload on startup. solving that scenario is the real solution. I wish those with talent to do such things much success.

Comment on attachment 275483 [details] [diff] [review] take 2, unbitrotted Canceling review request as it's still not clear this is really the way to go and I might not have the time to implement it in a different way.

moving out bugs that don't need to block b1

hmm, for this to work one needs a) to use only single browser window with sessionsaver (as I do) + fix the corner cases b) - a checkbox under "show the windows from last time" that reads (and implements) "Save all closed windows with multiple tabs in the session" - an option "File" -> "close all tabs" and /or in tab popup "close other tabs", in tab bar popup "close all tabs" - "History" -> "show closed windows" or similar to restore the saved windows without restarting firefox c) an UI for the multiple session backups that are stored in the profile (looks like there is ~10 of them which should be good enough) - a new backup should be done after major changes

this is gonna sting a little, but not going to block on this, not a regression from Fx2. We really want to get this solved, but its no worse than the current stable release, and there's a lot of other fixes we want to get in the hands of users.

Don't know if this is a dupe or related, but when I close the download window and close the window before the 'All downloads finished" slider appears, I get the wrong "Quit Minefield" dialog, where the "Save and Quit" option is missing.

I am seeing something similar if you open two tabs, one to google, and one to a myspace page and have it playing music, and then close the browser and choose to save and quit. The browser closes, but there is a delay where the music process ist still playing in the background, and I think when that happens, on starting back up Firefox, the session isn't restored. I am using Google and the following myspace page as the test tabs, and making sure that the music is playing in the music box when I close the browser: http://profile.myspace.com/index.cfm?fuseaction=user.viewprofile&friendID=34549275 It doesn't always happen, so you may have to try it a couple times to reproduce it.

This would help for automatic testing, as well.

The Session Restore part of the patch would really be quite simple, unfortunately there are also the browser glue bits that would have to be taught to prompt for "Save and Quit" instead of the Confirm close warning...

I think we should reconsider Session Restore based on our Recently closed tabs/Undo close tab implementation. This is how I think it should be done: -When a Firefox window is closed, save the list of tabs that were open in that window. Possibly exclude popup windows with only one tab from this. (However, this brings us to the question: what exactly counts as a popup window? In my opinion, a window should be considered a popup if it's a toolbarless window that was launched via JavaScript.) We should, however, definitely save popup windows that had multiple tabs open. XXX: Should we save the type of the window, or always restore it as a full-blown window with a menu? -When saving a window, also save its Recently closed tabs list. -Empty windows should be saved if they possess a Recently closed tabs list. However, mark these as EMPTY in the session store. -Add a new "Recently closed windows" or "Undo close window" UI to Firefox. I suggest that the UI should be a window with 2 panes (that can be opened from the History menu), one containing the recently closed windows, and one listing the tabs in the selected window. Order the windows with the most recently closed one on the top. A menu, similar to Recently closed tabs, is not enough, as it can't list the tabs that were present in the window, only maybe the title of the active tab. -If the user initiates the "restore window" process from an empty window, restore into that window, otherwise restore in a new window. -When a window is reopened/"undo closed", also restore its Recently closed tabs list, allowing the user to restore a closed window, then restore a tab that was closed in that window before the window itself was closed. -When the user quits Firefox while multiple windows are open, save all the windows (like now), and set a bit (let's call it RestoreMultiWindow) in the session store, and save all windows, like we do now. Also set this bit when Firefox is being restarted after an update/extension change. -When the user reopens Firefox after a process shutdown with "Show my tabs and windows from last time" enabled, check the RestoreMultiWindow bit. a) If the bit is set (i.e. after a "Quit Firefox"/Cmd+Q/restart), restore all the windows that were present at the time Firefox was shut down. b) If the bit isn't set (i.e. if Firefox was closed window-by-window), restore the window that was closed last (excluding single-tab popups and empty windows). -When a new window is opened from a no-windows-open-but-process-still-running state, check browser.sessionstore.allow_empty_session (default 1 on Mac, 0 otherwise). a) If the pref is 1, open an empty window. b) If it's 0, reopen the last closed window. -Clearing History using Clear private data should empty the Recently closed windows list. This way we account for almost all possible restore cases, including extreme ones like the following (yes, this once happened to me): -The user has 2 windows open, and one of them has a tab. Let's call this tab "A". -Tab A is in window 2. -The download manager is open in the background, minimized. -The user closes tab A. -The user "shuts down" the browser by first closing window 2, then window 1. He forgets about the download manager. -The user finds out that he needs to access tab A again. If we implement this, the user only needs to do the following: -Open a Firefox window. (This automatically restores the tabs of window 1 - let's call it Window 1'.) -From window 1', the user initiates a Restore window operation, and reopens window 2, creating Window 2'. -In Window 2', the user recovers tab A. --Gábor

Updating title to make it more discoverable.

i see that this bug is there for a while. Please could you do something about it? I'm loosing my tabs regularly 4-5 times a week due to this bug.. and i always hate it.. Thx alot!

As a workaround, if you always want to see your previous tabs, just set your homepage (tools->options->main) to "Show My Windows and Tabs From Last Time".

Requesting blocking, this might be already to late for this "old" bug but this seems to be a very frequent issue now if you count the number of dupes.

Nope, still not blocking, though agreed that it's unfortunate. Hopefully bug 394759 can help in the interim.

So I'm not sure if I'm adding any value here, but I'll throw in my 2 cents anyhow... (In reply to comment #37) Some of what you suggested has been added. Like Beltzner said, bug 394759 landed which adds undo close window functionality, so at the very least you can recover that window you closed right before you realized the downloads window was still open. This also persists across sessions, so you can recover that window upon next run of Firefox (starting with 3.5 beta 4). It wasn't implemented exactly as you suggested, but it's there. So your "extreme" case is recoverable, though with an additional step. I know that while I was on Windows this would always kick my ass, so it would be good to see a fix. But is the suspended state the path to take? I like that it would be controllable via a pref (albeit a pretty undiscoverable pref for a major shift in behavior). But it is a pretty major shift in the behavior of the browser. Add in the details about which dialog gets shown ("are you sure you want to close this window" vs "you're quitting. save session?") and things get a little hairier, especially since those dialogs aren't actually controller by session store. I would have to look again to see the logic surrounding those dialogs & prefs, but that's a pretty dicey area. I think the dialogs issue is the most critical. It needs to be clear to the user that clicking new window *might* not actually open a new blank window.

(In reply to comment #11) > There's a very-much-related OSX issue in bug 365324 where users can easily > enter a case where they close a window without closing an application. > > Also, I *think* a slightly clearer way to express the dataloss problem is: > > "Someone closes his top-level window, then sees the DL window, loses session, > takes a shot." > > In STR: > > 1. Open a bunch of tabs. > 2. Open the download window. > 3. Click the window close button at the upper right of the browser window. > 4. "Yeah, yeah, close N tabs, I got session restore, baby!" > 5. Notice there's still a download manager window, and now you've closed all > your tabs. > 6. Take shot. Just throwing my two cents in. I thought this pretty clearly explained the bug in question. I can't tell you how many times I have been affected by this exact situation. I read comment #49 for the workaround but as far as a suggestion for fixing this bug goes - would it be possible to have the Downloads (or any open chromeless window, content or non-content) take focus when the close command is executed (the main browser window is X'd out of)? For example, you have multiple tabs open. You have the downloads window open but does not have focus. You close the browser by clicking the X in the corner. Normally Firefox prompts you to save your session. Instead, the "pop-up" window takes focus and prompts you to close it before your session can be saved. Either that or maybe even have any windows other than the main browsing window close when Firefox is exited. Just my two cents. I'm not a programmer, just a frustrated user who loses a dozen tabs frequently because I forget the download window is open.

I've been bitten by this too. I'd suggest that the download window (or any other window that has nothing to do with tabs) not be taken into consideration by the session manager mechanism.

This patch goes down another road. UI part: * let the normal canCloseApplication happen if required and only kick in otherwise (this is handled by the existing closeWindow()) * otherwise check if it is the last real browser window (not just a popup) * if it is post browser-quit-lastwindow-requested notification. * if that is successful go ahead and notify anybody by posting browser-quit-lastwindow-granted * if it is vetoed abort closing the window BrowserGlue: * process browser-quit-lastwindow-requested like quit-application-requested i.e. prompt or use the prefs to check if it should be allowed or denied * process browser-quit-lastwindow-granted like quit-application-granted i.e. mark the session as restore-once nsSessionStore: * process browser-quit-lastwindow-granted set some flag (_restoreLastWindow) if granted * in onLoad check _restoreLastWindow, and if set, _doResume() and not in private browsing mode then restore the last window using data from undo window close. Patch characteristics: * sessionrestore works for browser windows even if the last browser window is closed while other windows are still around. * works across sessions * works in the same session (i.e. the app is still around and a browser window is opened again) * the last window going down is now observable and cancel-able, not only by the application but addons as well. * not suspending the session; in fact not fiddling with _loadState at all. * Addon-Compatiblity: Extensions completely replacing (patching) any of the functions should cause no more harm than disabling the new functionality again. The notable new thing from an addon perspective is the new "lastwindow" aQuitType in nsBrowserGlue._onQuitRequest. However, this vakze could be changed to the already known "quit" if the reviewer preferred so. * No UI or l10n changes (just re-using existing functionality) * No new preferences. Additionally to running mochikit plain, chrome and browser-chrome tests, the patch was manually tested on XP 64bit/Vista 32bit/Ubuntu Jaunty 32bit (no mac available) using the following procedure: Set up * Open two tabs (google or whatever) if not already there. Tests 1 (Old functionality) * Try to close browser twice, first select "Cancel" and then select "Save and Quit". -> Save session prompts still work, and only prompted once * Open again -> Restore over sessions still works * Open new browser window * Try to close old browser window. Cancel prompt to close 2 tabs. -> Tab close warnings still work * Enter and leave private browsing mode -> Restore after PBM still works * Install some addon that allows to open new browser windows from it's own window. For example DownThemAll! (manager window processes Ctrl+N). Need to disable addon compatiblity checking first. Restart Firefox from the addons window. -> Restart works * Close the browser window (Save and Quit) and open an URL from another application -> Restore over sessions still works, new URL gets a opened in a new tab Tests 2 (Bugfix related, across sessions) * Open Downloads window. * Try to close browser twice (which would trigger the bug), first select "Cancel" and then select "Save and Quit". -> New logic kicks in * Close Downloads window and restart Firefox -> New logic works across sessions Tests 3 (Bugfix related, in the same session) * Open extension window you've installed and close the browser window (Cancel/Save and Quit) -> New logic kicks in * From that extension window open a new browser window -> New logic works in the same session Tests 4 (Bugfix related, PBM) * Enter private browsing mode * Open Downloads window * Close browser window (Save and Quit) then close Downloads and open browser again -> Restore after PBM works * Open an the extension window * Enter private browsing mode and close the window -> Not being prompted * Open browser window from extension window -> Still in private browsing * Leave private browsing -> Restore works Test 5 (Bugfix related, opening external url) * Open Downloads window * Close browser window (Save and Quit) * Open URL from an external application -> Restore in the same session works, new URL gets a opened in a new tab I guess this should cover it?! I have no idea how to produce automated tests for this as it seems mochikit doesn't allow to repeatedly open and close the browser window (with no other windows around). Any ideas? The new notifications also cannot be tested appropriately it seems.

Comment on attachment 379540 [details] [diff] [review] patch, v1 Looking good. A few drive-by comments: >+ // this function is only executed when the application is still "alive" >+ // first lets get the word out that the last browser window was requested to be closed >+ switch (warnAboutLastWindow()) { Wouldn't it be easier to let warnAboutLastWindow call warnAboutClosingTabs if needed and here just return warnAboutLastWindow's result? Otherwise you'll have to rename the function and/or its constants as they don't currently do what they claim to. >+ * The user has then a chance to about it, save tabs. Nit: s/about/abort/? ... _or_ save tabs. >+ var wm = Components.classes["@mozilla.org/appshell/window-mediator;1"] >+ .getService(Components.interfaces.nsIWindowMediator); Nit: Use Cc and Ci instead of Components.classes/interfaces. Also, in this file, getService should be aligned with the opening bracket (and the dot trail the previous line. >+ if (domWindow.location.href == getBrowserURL() && !domWindow.isPopup) { We don't have an isPopup attribute on nsIDOMWindows. Use e.g. domWindow.toolbar.visible instead. >+ // something fishy going on here. Are you catching anything specific? If so, please state this, otherwise drop the try-catch bits. >+ if (windowCount > 1) { >+ // UNKNOWN so that the normal browse prompt may kick in. If you revert this condition, you can end the function with |return gBrowser.warnAboutClosingTabs(...);|, cleaning up the logic (there's one return for every possible value) and allowing you to simplify the switch I commented about above. >+ let os = Components.classes["@mozilla.org/observer-service;1"] >+ .getService(Components.interfaces.nsIObserverService); >+ if (!os) { Can this actually happen? In case you don't know, please assume that it can't. >+ os.notifyObservers(closingCanceled, "browser-close-lastwindow-requested", null); Nit: "browser-lastwindow-close-requested" sounds more logical (reading backwards: request closing last browser). >+ if (aQuitType != "restart") >+ aQuitType = "quit"; >+ >+ Any reason for relocating these two lines? Also: There should only be one blank line following - and it should be properly indented. >+ case "browser-close-lastwindow-granted": >+#ifdef XP_MACOSX >+ // don't restore last window on mac. >+#else Nit: You can slightly simplify this by enclosing the whole case with #ifndef XP_MACOSX (i.e. don't ship any code at all for OS X). >+ else if (this._restoreLastWindow && this._doResumeSession() && aWindow.document.documentElement.getAttribute("windowtype") == "navigator:browser") { Nit: At this point your window is guaranteed to be a "navigator:browser" one (cf. condition at the top of the function). >+ if (this._closedWindows && this._closedWindows.length && !this._inPrivateBrowsing) { Nit: _closedWindows is always defined - no need to test for it. >+ let state = { windows: [this._closedWindows.shift()] }; >+ if (state.windows[0]) { Nit: state.windows[0] is always defined at this point. >+ // we actually restored the session jsut now. Nit: s/jstu/just/. >+ // mark ourselves as running >+ this.saveState(true); Nit: We _are_ already running.

(In reply to comment #60) > (From update of attachment 379540 [details] [diff] [review]) > Looking good. A few drive-by comments: > > >+ // this function is only executed when the application is still "alive" > >+ // first lets get the word out that the last browser window was requested to be closed > >+ switch (warnAboutLastWindow()) { > > Wouldn't it be easier to let warnAboutLastWindow call warnAboutClosingTabs if > needed and here just return warnAboutLastWindow's result? Otherwise you'll have > to rename the function and/or its constants as they don't currently do what > they claim to. > You're right. Will remove the switch altogether and the anonymous function. > >+ if (domWindow.location.href == getBrowserURL() && !domWindow.isPopup) { > Seems my memory played a trick here. > > >+ // something fishy going on here. > > Are you catching anything specific? If so, please state this, otherwise drop > the try-catch bits. Just being over-protective I guess. Here and in a few other places. > > >+ if (windowCount > 1) { > >+ // UNKNOWN so that the normal browse prompt may kick in. > > If you revert this condition, you can end the function with |return > gBrowser.warnAboutClosingTabs(...);|, cleaning up the logic (there's one return > for every possible value) and allowing you to simplify the switch I commented > about above. Will do. > > >+ let os = Components.classes["@mozilla.org/observer-service;1"] > >+ .getService(Components.interfaces.nsIObserverService); > >+ if (!os) { > > Can this actually happen? In case you don't know, please assume that it can't. I saw this check in globalOverlay.js and just carried it over, but I guess it can't actually happen under normal circumstances. It would be caught by the surrounding try-catch, so there is no need for this, anyway. > > >+ os.notifyObservers(closingCanceled, "browser-close-lastwindow-requested", null); > > Nit: "browser-lastwindow-close-requested" sounds more logical (reading > backwards: request closing last browser). Will be changed accordingly. > > >+ if (aQuitType != "restart") > >+ aQuitType = "quit"; > >+ > >+ > > Any reason for relocating these two lines? Also: There should only be one blank > line following - and it should be properly indented. There was a reason in an intermediate iteration of the patch. Just forget to put it back where it was. > >+ // mark ourselves as running > >+ this.saveState(true); > > Nit: We _are_ already running. That bit is actually copied from the few lines above. Makes no sense in both places.

Comment on attachment 379556 [details] [diff] [review] patch, v2 - nits addressed Nitpicking, round 2: >+ let os = Components.classes["@mozilla.org/observer-service;1"] >+ .getService(Components.interfaces.nsIObserverService); Nit: Components.classes/interfaces instead of Cc/Ci and bad alignment (see my previous comment and please also correct further down). >+ catch (innerex) { >+ // if some observer throws it is their deal >+ } Nit: Here you catch, for browser-lastwindow-close-requested you don't. Please be consistent and drop the try-catch here as well (it's really not required). >+ catch (ex) { >+ Components.utils.reportError(ex); >+ } Please drop the (selective) over-protectiveness, unless you've got a reason. > break; >+ Nit: Wrongly indented (easiest is to kill the whitespace altogether). >+ // The application is not actually going down, Nit: s/going down/quitting/ (alternatives are "shutting down" and "exiting"). Please fix this everywhere it appears. >+ case "browser-lastwindow-close-granted": >+ if (this._saveSession) { >+ this._setPrefToSaveSession(); >+ } >+ break; >+ Nits: Both break and the following line are wrongly indented. Also, you don't need braces around a single statement, if both the condition and the statement fit on a single line (the same applies in several places). >+ osvr.removeObserver(this, "browser-close-lastwindow-requested"); >+ osvr.removeObserver(this, "browser-close-lastwindow-granted"); Missed these while renaming... >+ case "browser-lastwindow-close-granted": >+#ifndef XP_MACOSX I meant it the other way round (i.e. #ifndef before |case| and |break| before #endif). >- // mark ourselves as running Please leave the comment in, it does make sense here - opposed to both the comment _and_ the statement where I asked you to remove it. >+ else if (this._restoreLastWindow && this._doResumeSession()) { Shouldn't you reset _restoreLastWindow here, even if !this._doResumeSession() ? IOW: Shouldn't the second half of the condition move to the inner conditional? >+ this.saveState(true); BTW: That's the line that isn't needed. As for testing: You should be able to write a browser-test to test this. Just hide the toolbars of the window the test actually runs in (so that it's considered a popup) and then simulate your window dance (you can prevent the prompts by setting/resetting the involved preferences).

(In reply to comment #63) > (From update of attachment 379556 [details] [diff] [review]) > Nitpicking, round 2: > > >+ catch (innerex) { > >+ // if some observer throws it is their deal > >+ } > > Nit: Here you catch, for browser-lastwindow-close-requested you don't. Please > be consistent and drop the try-catch here as well (it's really not required). > > >+ catch (ex) { > >+ Components.utils.reportError(ex); > >+ } > > Please drop the (selective) over-protectiveness, unless you've got a reason. Hmmm, rechecked and it turns out that nsObserverService::NotifyObservers doesn't actually propagate errors back. So the checks are indeed void at the moment. > > >+ case "browser-lastwindow-close-granted": > >+ if (this._saveSession) { > >+ this._setPrefToSaveSession(); > >+ } > >+ break; > >+ > > Nits: Both break and the following line are wrongly indented. Also, you don't > need braces around a single statement, if both the condition and the statement > fit on a single line (the same applies in several places). I consider it a "best practice" to use braces around single-statement blocks for increased readability and as it is easier to append to the block or use some editor block highlighting. Unless it's considered a no-go I'd like to keep the statement enclosed by braces. > > >+ osvr.removeObserver(this, "browser-close-lastwindow-requested"); > >+ osvr.removeObserver(this, "browser-close-lastwindow-granted"); > > Missed these while renaming... Damn, I remember to have renamed these (not just the addObserver ones). > >- // mark ourselves as running > > Please leave the comment in, it does make sense here - opposed to both the > comment _and_ the statement where I asked you to remove it. It still doesn't make sense to me?! > > Shouldn't you reset _restoreLastWindow here, even if !this._doResumeSession() ? > IOW: Shouldn't the second half of the condition move to the inner conditional? > Both will yield the same result at the moment, but what you proposed would actually be the correct thing to do. Will change this. > >+ this.saveState(true); > > BTW: That's the line that isn't needed. I think it is needed as we do want to save the session again as we just restored a window, don't we? > > As for testing: You should be able to write a browser-test to test this. Just > hide the toolbars of the window the test actually runs in (so that it's > considered a popup) and then simulate your window dance (you can prevent the > prompts by setting/resetting the involved preferences). I can test the restore-in-session bits, but I cannot test restore-over-sessions (meaning the old bits still working) as this would require an actual browser restart, right? Do you know any existing test off-hand that I may derive from?

(In reply to comment #64) > It still doesn't make sense to me?! Just leave the already existing |this.saveState(true);| and its comment where it is and don't add another one a few lines down. > I think it is needed as we do want to save the session again as we just > restored a window, don't we? It'll be saved anyway within a few seconds. The only reason we have to force a save operation above is so that the session's state changes from "stopped" to "running" so that we correctly count crashes happening during startup. You don't introduce a new state, so saving isn't that urgent and the line isn't needed. > I can test the restore-in-session bits, but I cannot test restore-over-sessions > (meaning the old bits still working) as this would require an actual browser > restart, right? Indeed, for the old bits we need a Litmus test (if we haven't already got one). > Do you know any existing test off-hand that I may derive from? Try any test from /browser/components/sessionstore/test/browser/ which opens new windows (e.g. browser_394759.js).

Added tests covering * New notifications actually posted * lastwindow-close actually cancelable * normal in session restore * not restoring in private browsing mode * restoring after leaving private browsing mode again * functionality when there are popup windows around That should cover the testable-part, I guess. Mac will only test notifications, as that platform has different rules for the last window and hence the windows won't be restored. I cannot test mac myself, just "simulate" it: /me not a proud mac owner. There seem to be some litmus tests for the old behavior. I adjusted the comment I wanted to remove accordingly to zeniko's explanation of it.

Comment on attachment 379660 [details] [diff] [review] patch v3, nits addressed, tests added Thanks for the nice tests! This patch is starting to look great. Nonetheless a last round of comments and nits, before you'll get the r+=me on the SessionStore bits and I'll let a browser peer have another pass at the rest: >+ // we're good, there are still some windows around >+ if (windowCount > 1) { >+ // the user may want to abort anyway to keep the tabs open >+ return gBrowser.warnAboutClosingTabs(true); >+ } Will this preserve the correct behavior on OS X (i.e. don't offer to save the session when the last browser window is closed)? AFAICT it won't, so you'll have to unconditionally return gBrowser.warnAboutClosingTabs(true) on that platform and never dispatch the browser-lastwindow-close-notifications. >+ else if (this._restoreLastWindow) { Maybe we should also make sure at this point that the newly opened window isn't a popup window (into which we wouldn't want to restore what's supposed to be a non-popup window). Also, what about the following: 1) Open a browser window and two browser popups from it. 2) Close the non-popup browser window and tell Firefox to save the session. 3) Close one of the two popups. 4) Launch Firefox (which opens a new browser window). ATM this will reopen the popup as a non-popup window. I guess we could live with that for Firefox 3.5, but you'd at least have to file a follow-up bug to revisit this decision. >+/** >+ * Checking that restoring the last browser window in session is actually >+ * working. >+ * Check the new notifications are correctly pushed and processed. Could you please be more specific as to what you're going to test? Do you mean that the last non-popup browser window was closed and should be restored when a new browser window is opened? Also: in three months, nobody will know what the "new" notifications are... ;-) (and grammar nit: it's "check that"). >+ * @note It is implicitly tested that restore of the last window functions when >+ * non-browser windows are around, as the run test window and the test browser >+ * window are around Not that my English is that much better than yours, but what about something like "@note The window running the test will be morphed into a non-browser window, so it is implicitly tested that the last browser window is correctly restored after it's been closed with a non-browser windows or a popup window around." >+ // we need to wait for a lot of events Nit: Unneeded comment. >+ // observe these >+ const kObserving = { >+ 'browser-lastwindow-close-requested': 0, >+ 'browser-lastwindow-close-granted': 0 >+ }; Nits: In JavaScript code, constants are written in UPPERCASE. Also, we never make object references constant (as it could lead you to think that the object itself is constant, which it isn't). Finally: Is there a method behind when you use single and when double quotes? >+ // then throw >+ case 2: >+ throw new Error("just testing"); At one point, we'll likely require our tests not to throw uncought exceptions. Since this just seems to test that observers can throw without stopping the notification, it doesn't belong into this test - so please drop it. >+ const kObserverService = Cc["@mozilla.org/observer-service;1"]. >+ getService(Ci.nsIObserverService); Nit: XPCOM objects aren't usually constants, either. >+ function setPrefs(prefs) { Considering how few prefs you're actually setting, this looks like overkill. Just set (and later reset) the two prefs explicitly. Also a nit: Function _a_rguments start with an "a" prefix (e.g. here it'd be "aPrefs"). >+ executeSoon(function() testFn(newWin)); This won't ensure that the tabs have actually loaded your URLs. If you rely on that, please wait for the "load" events. >+ // Test if newWin.BrowserTryToCloseWindow is set instead of newWin.opened, >+ // because the latter doesn't work as advertised >+ ok(!!newWin.BrowserTryToCloseWindow, "First close attempt denied"); There is no "newWin.opened". Are you looking for "newWin.closed"? Also: Why don't we get a "close multiple tabs" prompt at this point? >+ // open a new window the previously closed window should be restored to Grammar nits: Interpunctuation between sentences, please (also it's ", too" instead of "to"). >+ newWin = openDialog(location, "_blank", kChromeFeatures); >+ newWin.addEventListener('load', function() { >+ executeSoon(function() { Why the executeSoon? >+ is(newWin.gBrowser.browsers.length, kTestUrls.length + 1, >+ "Correctly in session with popup windows around"); Grammar nit: verb? >+ pb.privateBrowsingEnabled = true; >+ >+ // open a new window the previously closed window should be restored to Shouldn't the previously closed window _not_ be reopened at this point? (It doesn't seem to be). >+ function testNotificationCount(nextFn) { >+ >+ executeSoon(nextFn); Nits: _check_NotificationCount and there's no need for an executeSoon at this point. >+ function() cleanupTestsuite() + finish() I guess, it'd read even nicer with an "&" instead of the "+". However, why not just append "finish()" to cleanupTestsuite?

(In reply to comment #67) > (From update of attachment 379660 [details] [diff] [review]) > Thanks for the nice tests! This patch is starting to look great. Nonetheless a > last round of comments and nits, before you'll get the r+=me on the > SessionStore bits and I'll let a browser peer have another pass at the rest: > > >+ // we're good, there are still some windows around > >+ if (windowCount > 1) { > >+ // the user may want to abort anyway to keep the tabs open > >+ return gBrowser.warnAboutClosingTabs(true); > >+ } > > Will this preserve the correct behavior on OS X (i.e. don't offer to save the > session when the last browser window is closed)? AFAICT it won't, so you'll > have to unconditionally return gBrowser.warnAboutClosingTabs(true) on that > platform and never dispatch the browser-lastwindow-close-notifications. > You're right. However in my opinion the the notifications should be posted anyway. There might be addons wanting to listen for them, so it would be better to keep this consistent across platforms. Hence I'll #ifdef the last return true statement to return warnAboutClosingTabs on mac. > >+ else if (this._restoreLastWindow) { > > Maybe we should also make sure at this point that the newly opened window isn't > a popup window (into which we wouldn't want to restore what's supposed to be a > non-popup window). > > Also, what about the following: 1) Open a browser window and two browser popups > from it. 2) Close the non-popup browser window and tell Firefox to save the > session. 3) Close one of the two popups. 4) Launch Firefox (which opens a new > browser window). > > ATM this will reopen the popup as a non-popup window. I guess we could live > with that for Firefox 3.5, but you'd at least have to file a follow-up bug to > revisit this decision. It shouldn't be too hard to write a test and fix the behavior to open the last real browser. Should be a matter of not just simply restoring the top of _closedWindows but instead the first one which is not a popup. I'll see if that will be possible in a matter of minutes, and if it's not I'll file a follow-up bug. > > >+/** > >+ * Checking that restoring the last browser window in session is actually > >+ * working. > >+ * Check the new notifications are correctly pushed and processed. > > Could you please be more specific as to what you're going to test? Do you mean > that the last non-popup browser window was closed and should be restored when a > new browser window is opened? Also: in three months, nobody will know what the > "new" notifications are... ;-) (and grammar nit: it's "check that"). > I thought I was pretty specific considering that most other tests contain a comment like "Test for Bug XXXXXX" and that all. :p > >+ * @note It is implicitly tested that restore of the last window functions when > >+ * non-browser windows are around, as the run test window and the test browser > >+ * window are around > > Not that my English is that much better than yours, but what about something > like "@note The window running the test will be morphed into a non-browser > window, so it is implicitly tested that the last browser window is correctly > restored after it's been closed with a non-browser windows or a popup window > around." > Wow, did I really write that. Guess I'll be receiving my official letter of nomination for "best use of gibberish in a code comment" soon? :p > >+ // observe these > >+ const kObserving = { > >+ 'browser-lastwindow-close-requested': 0, > >+ 'browser-lastwindow-close-granted': 0 > >+ }; > > Nits: In JavaScript code, constants are written in UPPERCASE. Also, we never > make object references constant (as it could lead you to think that the object > itself is constant, which it isn't). Finally: Is there a method behind when you > use single and when double quotes? > Yeah, I read to much code using the kConst-style in the recent past. However I prefer const'ing such things so that nobody (accidentally) assigns a new value to "my" object-references. Single or double quotes is a matter of randomness as produced by the PRNG in my head. ;) Actually I prefer single quoted (because I wrote a fair chunk of php). Will make this consistent. > >+ executeSoon(function() testFn(newWin)); > > This won't ensure that the tabs have actually loaded your URLs. If you rely on > that, please wait for the "load" events. I don't use the content actually. I test only if the correct number of tabs is persisted and the restored. There should be already other tests around (like your undoWindow tests) that check whether tabs are correctly restored. executeSoon is there to give the DOM of the browser/tabbrowser a change to do what's needed. > > >+ // Test if newWin.BrowserTryToCloseWindow is set instead of newWin.opened, > >+ // because the latter doesn't work as advertised > >+ ok(!!newWin.BrowserTryToCloseWindow, "First close attempt denied"); > > There is no "newWin.opened". Are you looking for "newWin.closed"? Tested for .closed, wrote wrote .opened. > > Also: Why don't we get a "close multiple tabs" prompt at this point? > There is no way that this can happen at this point (other than the test itself is broken). There is only one browser window around, so either the user is prompted to "Save and Quit", "Quit", "Cancel" or has some preference set because of "Never ask again" or has configured "Show my windows from last time". This is consistent with the old behavior. Actually it is the same code path located in nsBrowserGlue._onQuitRequest. > >+ // open a new window the previously closed window should be restored to > > Grammar nits: Interpunctuation between sentences, please (also it's ", too" > instead of "to"). That's in fact just one sentence and the correct "to". A pretty bad sentence, that is, admittedly. ;) > > >+ newWin = openDialog(location, "_blank", kChromeFeatures); > >+ newWin.addEventListener('load', function() { > >+ executeSoon(function() { > > Why the executeSoon? Just reimplementing the good old "when dealing with DOM stuff during load use setTimeout if in doubt" pattern. That pattern seems to be a sibling of kungFuDeathGrip. > >+ pb.privateBrowsingEnabled = true; > >+ > >+ // open a new window the previously closed window should be restored to > > Shouldn't the previously closed window _not_ be reopened at this point? (It > doesn't seem to be). Yep, the comments lacks the _not_ > > >+ function testNotificationCount(nextFn) { > >+ > >+ executeSoon(nextFn); > > Nits: _check_NotificationCount and there's no need for an executeSoon at this > point. > > >+ function() cleanupTestsuite() + finish() > > I guess, it'd read even nicer with an "&" instead of the "+". Not a big fan of &s as people tend to "correct" them to && and then wondering why stuff goes to hell. ;) Especially when used in a context like this one. > However, why not > just append "finish()" to cleanupTestsuite? finish() is located together with the code that executes the tests so people don't have to search to random functions to find out where it is called.

Ok, another round: * Mac will still post the notifications, which can still be used to cancel. However if not canceled then warnAboutClosingTabs() is called always. * nsBrowserGlue and nsSessionStore got more #ifndefs XP_MACOSX to completely remove the code from mac builds. This should also make it more clear to people reading the sources that mac in fact doesn't implement restoring tabs in the same session. * Popup windows are NOT restored. If there are only popup windows no window at all will be restored. A regular new window is then opened. (See changes in nsSessionManager._onLoad) * The popup test from the last patch was modified to close one popup after the real browser window, and then checks it is not the popup window but the browser window that gets restored. * An additional popup test was created that will not open a browser window at all. Instead just a popup is opened and closed again, and afterwards the a new browser window is opened to verify that indeed nothing at all gets restored. I also put in some more comments and addressed the nits.

Comment on attachment 379749 [details] [diff] [review] patch v4, issues and nits addressed AFAICT there's one issue left with the new popup handling bits (third paragraph). With that and a few further nits fixed, this would be r+=me. Thanks a lot for your swift work! Please ask a browser UI peer (such as Gavin or Dão) for the next review round. >+#ifndef XP_MACOSX >+ // whether the last window was closed and should be restored >+ _restoreLastWindow: false, >+#endif I guess, it wouldn't harm leaving this in, so we don't get too many #ifdefs. Not sure it matters either way, though. >+ this._closedWindows = this._closedWindows.filter(function(aWinState) { This just removes all popups prior to the first non-popup window, which is dataloss and thus unwanted. >+ if (!!state) { Nit: There's no need for !! in JavaScript. >+ * 4.1) Open a popup >+ * 4.2) Add another tab to the popup (so that it gets stored) and close it again >+ * 4.3) Open a window >+ * --> Nothing at all should be restored Unless there's another closed browser window already stored? >+ // Window features of popup windows >+ const POPUP_FEATURES = 'toolbar=no,resizable=no,status=no'; >+ >+ // Window features of browser windows >+ const CHROME_FEATURES = "chrome,all,dialog=no"; Nit: Please just s/'/"/g outside of comments. >+ let observing = { For clarification, maybe notificationCounts ? >+ let observerService = Cc["@mozilla.org/observer-service;1"]. >+ getService(Ci.nsIObserverService); Nit: Indentation. >+ executeSoon(function() testFn(newWin)); Nit: Spacing. >+ // Test if newWin.BrowserTryToCloseWindow is set instead of newWin.closed, >+ // because the latter doesn't work as advertised. Bug number? The following still doesn't feel right except as a hack-to-be-fixed: >+ ok(!!newWin.BrowserTryToCloseWindow, "First close request was denied"); >+ if (newWin.BrowserTryToCloseWindow) { BTW: You don't need to check this, as if this condition doesn't hold, the test fails right above anyway. >+ // close the popup window >+ // it should be saved, on top of the closed windows stack of the >+ // session manager, as it contains more than one tab >+ // The test is successful when not this popup window is restored >+ // but instead newWin >+ popup2.close(); Can you also close this popup through BrowserTryToCloseWindow for consistency's sake (also further below)? Also: SessionStore saves all popups, not only those containing more than one tab. (And there's a "not" looking for a better place.) >+ // don't use open here, as we won't receive the load event. For consistency, just use openDialog anywhere and drop these comments.

(In reply to comment #70) > (From update of attachment 379749 [details] [diff] [review]) > AFAICT there's one issue left with the new popup handling bits (third > paragraph). With that and a few further nits fixed, this would be r+=me. Thanks > a lot for your swift work! Please ask a browser UI peer (such as Gavin or Dão) > for the next review round. > > >+#ifndef XP_MACOSX > >+ // whether the last window was closed and should be restored > >+ _restoreLastWindow: false, > >+#endif > > I guess, it wouldn't harm leaving this in, so we don't get too many #ifdefs. > Not sure it matters either way, though. More #ifndefs shouldn't really matter in js code IIRC. The processing overhead should be minimal there. But those #ifndefs send a clear message to the occasional reader that mac doesn't support this. > > >+ this._closedWindows = this._closedWindows.filter(function(aWinState) { > > This just removes all popups prior to the first non-popup window, which is > dataloss and thus unwanted. No, it does not. Array.filter will only filter the items you return false for. The only time this might happen is if state == null and !aWinData.isPopup, which may happen exactly one, namely for the closed state we'd actually like to get out of that array and restore. > > >+ if (!!state) { > > Nit: There's no need for !! in JavaScript. Cannot harm. And sometimes there is a need: this is actually a nice cast to bool that won't keep references on the object in question. > > >+ * 4.1) Open a popup > >+ * 4.2) Add another tab to the popup (so that it gets stored) and close it again > >+ * 4.3) Open a window > >+ * --> Nothing at all should be restored > > Unless there's another closed browser window already stored? hmmm... I just realized that there isn't yet a check to not use popup windows to restore to. May happen that some code (this test for example, or add-ons) opens a popup when there is no browser window around anymore. Will add toolbar.visible checks. I'll add another test for this. As to your question: using close() instead of BrowserTryClose (you mentioned as a consistency nit) will bypass the whole logic as close() doesn't generate a 'close' event. So _restoreLastWindow is never set in the end of each test as the last action there is always to open a window to check what is restored and just .close() it. This allows the later tests to start in a clean state. I really should document this. Thinking about it, it seems the "popup only" test is invalid as it doesn't seem to test what it says. Giving myself review- additionally to yours. > > >+ let observing = { > > For clarification, maybe notificationCounts ? > Will amend the comment simply. > >+ let observerService = Cc["@mozilla.org/observer-service;1"]. > >+ getService(Ci.nsIObserverService); > > Nit: Indentation. > >+ // Test if newWin.BrowserTryToCloseWindow is set instead of newWin.closed, > >+ // because the latter doesn't work as advertised. > > Bug number? The following still doesn't feel right except as a > hack-to-be-fixed: > > >+ ok(!!newWin.BrowserTryToCloseWindow, "First close request was denied"); > >+ if (newWin.BrowserTryToCloseWindow) { > > BTW: You don't need to check this, as if this condition doesn't hold, the test > fails right above anyway. Will check for the bug (or why this didn't work). The testSuite will still continue. not doing some check will either call a now undefined function (i.e. throw) or will leave a window behind. Either is bad. > > >+ // close the popup window > >+ // it should be saved, on top of the closed windows stack of the > >+ // session manager, as it contains more than one tab > >+ // The test is successful when not this popup window is restored > >+ // but instead newWin > >+ popup2.close(); > > Can you also close this popup through BrowserTryToCloseWindow for consistency's > sake (also further below)? See remarks above > > Also: SessionStore saves all popups, not only those containing more than one > tab. (And there's a "not" looking for a better place.) > I now realized that, too. ;) I will tackle the issues tomorrow, as it is getting late and I'm tired, and request another review from you, as there these issues noted above weren't apparent when you spoke that almost-r+. Open issues: * restoring the last window to popup windows shouldn't be possible. * "popup only" test is likely invalid * other nits

(In reply to comment #72) > No, it does not. Array.filter will only filter the items you return false for. Indeed. I haven't seen a filter used this way, yet. I guess iterating over the windows and then splicing out the first non-popup one would be slightly more obvious... > > Nit: There's no need for !! in JavaScript. > > Cannot harm. It harms readability. Please only use it when actually needed (not here). > As to your question: using close() instead of BrowserTryClose (you mentioned as > a consistency nit) will bypass the whole logic as close() doesn't generate a > 'close' event. Note: I wasn't asking you to generally replace close with BrowserTryToCloseWindow, but just in two specific places before the actual cleaning up starts, as that's when it'd be called if a user closed the popups. > Open issues: Another thing I've stumbled upon: 1. Open a popup window. 2. Close all non-popup browser windows. 3. Focus the popup and hit Ctrl+Shift+N (reopen recently closed window). 4. Additionally open a new Firefox window. AFAICT at step 4 you try to restore another recently closed window. This means that you'll have to always reset _restoreLastWindow in sss_onLoad when _restoreLastWindow is set and the new window isn't a popup. Adding another test for this should be fairly easy (just call undoCloseWindow(0); instead of synthesizing Ctrl+Shift+N).

* Not restoring to popup windows anymore * popup only Test fixed * Not restoring directly after undoCloseWindow + test * .closed does indeed work. Must have messed something up the first time I tried that property

Comment on attachment 379892 [details] [diff] [review] patch v5, popup stuff fixed, nits addressed >+#ifndef XP_MACOSX >+ else if (this._restoreLastWindow && aWindow.toolbar.visible >+ && this._closedWindows.length && this._doResumeSession() >+ && !this._inPrivateBrowsing) { Nit: This should read as follows (note the indentation and the operator placement): >+ else if (this._restoreLastWindow && aWindow.toolbar.visible && >+ this._closedWindows.length && this._doResumeSession() && >+ !this._inPrivateBrowsing) { >+ if (aWindow.toolbar.visible) { Please only test for aWindow.toolbar.visible when this._restoreLastWindow is true. >+ // we don't want to restore a window directly after, for example, >+ // undoLastWindow was executed. Nit: It's undoCloseWindow (s/Last/Close/). >+ * @note There is a difference when closing the last browser window with >+ * BrowserTryClose() as opposed to close(). The former will make nsSessionManager Nit: We don't have a session _manager_. The file is nsSessionStore.js. (Also please adjust all other comments referring to a session manager.) >+ // Close the popup window >+ // The test is successful when not this popup window is restored >+ // but instead newWin >+ popup2.close(); Considering the note you've added at the top of the test, this really should be BrowserTryToCloseWindow() instead of close(), as with close() the popup won't be considered for restoration at all - and you want to test that it isn't considered even with BrowserTryToCloseWindow(). IOW: This isn't cleanup... Please also fix this in the next test. >+ is(popup.gBrowser.browsers.length, 1, >+ "Not restored the popup window (1)"); Nit: Please align function arguments with each other (the string should be aligned to |popup|, not the preceding parenthesis (applies at least a dozen times throughout the test). Also a grammar nit: "Didn't restore ..." or "The popup window wasn't restored" (applies again further down). >+ // close() the first to avoid prompts >+ newWin.close(); Nit: The correct way to avoid the prompt is to set and later reset the appropriate pref (browser.tabs.warnOnClose). r+=me with these final nits fixed - and thanks again!

(In reply to comment #75) > >+ // Close the popup window > >+ // The test is successful when not this popup window is restored > >+ // but instead newWin > >+ popup2.close(); > > Considering the note you've added at the top of the test, this really should be > BrowserTryToCloseWindow() instead of close(), as with close() the popup won't > be considered for restoration at all - and you want to test that it isn't > considered even with BrowserTryToCloseWindow(). IOW: This isn't cleanup... > Please also fix this in the next test. Nope, the window is still still stored as a recently closed window. It just won't be considered for resurrecting via _restoreLastWindow as this flag is never set (as we never call BrowserTryToCloseWindow). Leaving this as is, or else risking warnAboutClosingTabs prompts to pop up. > >+ // close() the first to avoid prompts > >+ newWin.close(); > > Nit: The correct way to avoid the prompt is to set and later reset the > appropriate pref (browser.tabs.warnOnClose). I'm not very fond of setting prefs. Reason is that the test-runner profile doesn't set modified prefs as default (which can be reverted by a simple clearUserPref()) but instead uses pref.js (i.e. user set prefs). Will modify as to your comment anyway. > > r+=me with these final nits fixed - and thanks again! Next is to request superreview? ui-review? Can you do this step, as you know what to do and who to ask, and I obviously don't? ;) I'll post the patch with nits addressed in a bit.

Nits from comment 75 addressed.

Comment on attachment 379959 [details] [diff] [review] patch v5.1, minor nits addressed Next step is to have a browser UI peer review at least the non-SessionStore related bits. After that, it's either more nit-fixing or checkin-needed...

Wanted to open a ticket for this problem and found this one per search funktion, hopefuly this can be solved as it has been here since tabs have been added as a feature in firefox. greetings Mischa

Dão: ping? Or would this rather be a task for Gavin?

It's gonna take at least one more week until I come to this. Nils knows. In the meantime, please remove the spare curly brackets around single code lines...

Comment on attachment 379959 [details] [diff] [review] patch v5.1, minor nits addressed Still need to get a better sense of the overall concept, just a few initial comments: >+ let domWindow = e.getNext().QueryInterface(Ci.nsIDOMWindow); >+ if (domWindow.location.href == getBrowserURL() && domWindow.toolbar.visible) { >+ ++windowCount; >+ } Nit: remove the curly brackets Is QueryInterface really needed? And the domWindow.location.href check, given that you're enumerating navigator:browser windows? >+ // we're good, there are still some windows around >+ // or we're a popup window >+ if (windowCount > 1 || !toolbar.visible) { >+ // the user may want to abort anyway to keep the tabs open >+ return gBrowser.warnAboutClosingTabs(true); >+ } Wouldn't it make sense to check !toolbar.visible first and avoid the window enumeration? >+ case "browser-lastwindow-close-requested": >+ this._onQuitRequest(subject, 'lastwindow'); use double quotes >+ case "browser-lastwindow-close-granted": >+ if (this._saveSession) { >+ this._setPrefToSaveSession(); >+ } remove the curly brackets >+ if (!mostRecentBrowserWindow) { >+ mostRecentBrowserWindow = browser; >+ } remove the curly brackets

(In reply to comment #87) > (From update of attachment 379959 [details] [diff] [review]) > Still need to get a better sense of the overall concept, just a few initial > comments: > > >+ let domWindow = e.getNext().QueryInterface(Ci.nsIDOMWindow); > >+ if (domWindow.location.href == getBrowserURL() && domWindow.toolbar.visible) { > >+ ++windowCount; > >+ } > > Nit: remove the curly brackets I outline in comment #64 why i use curly brackets. I you really feel that they cannot be there then please remove them yourself. > > Is QueryInterface really needed? And the domWindow.location.href check, given > that you're enumerating navigator:browser windows? > From my experience with nsISimpleEnumerator it is (and even if not it won't harm; this isn't the application inner loop). I saw a lot of extension windows by now declaring they are 'navigator:browser', hence the check. > >+ // we're good, there are still some windows around > >+ // or we're a popup window > >+ if (windowCount > 1 || !toolbar.visible) { > >+ // the user may want to abort anyway to keep the tabs open > >+ return gBrowser.warnAboutClosingTabs(true); > >+ } > > Wouldn't it make sense to check !toolbar.visible first and avoid the window > enumeration? Yep, the enumeration first is not necessary. There will be a little more code, but that's fine I guess. Awaiting your full review before I post an updated patch.

(In reply to comment #88) > I outline in comment #64 why i use curly brackets. Personally, I don't think these brackets are inherently more or less readable. However, they aren't considered "best practice" in browser code, and the inconsistency certainly doesn't help overall readability. > I saw a lot of extension windows by now declaring they are 'navigator:browser', Seems like that would break other code enumerating navigator:browser windows. We shouldn't workaround it just here, but expect those extension windows to be fixed. Regarding the QueryInterface, neither location nor toolbar are defined in nsIDOMWindow.

(In reply to comment #89) > (In reply to comment #88) > > I saw a lot of extension windows by now declaring they are 'navigator:browser', > > Seems like that would break other code enumerating navigator:browser windows. > We shouldn't workaround it just here, but expect those extension windows to be > fixed. In an ideal world extension windows would get fixed, yes. But in reality I as a user prefer that my session restore/browser even works when an extension misbehaves ;) > Regarding the QueryInterface, neither location nor toolbar are defined in > nsIDOMWindow. Well, they are for nsIDOMWindowInternal (which is derived from nsIDOMWindow2 which is derived from nsIDOMWindow). I guess this can be clarified by using nsIDOMWindowInternal directly.

(In reply to comment #90) > In an ideal world extension windows would get fixed, yes. But in reality I as a > user prefer that my session restore/browser even works when an extension > misbehaves ;) If you have concrete knowledge about affected extensions, it might be appropriate to inform the authors or even AMO, where they could look for similarly broken extensions automatically. Working around the misbehavior in one place isn't beneficial in the long run, since other code relies on navigator:browser being used correctly. > I guess this can be clarified by using nsIDOMWindowInternal directly. No, please don't use nsIDOMWindowInternal instead. Just leave it out, as it's not of any use.

Comment on attachment 379959 [details] [diff] [review] patch v5.1, minor nits addressed >diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js >+ var e = wm.getEnumerator('navigator:browser'); Use double quotes here as well. >+ if (closingCanceled.data) { >+ // closing was canceled >+ return false; >+ } The comment just repeats the variable name, so remove it. >diff --git a/browser/components/nsBrowserGlue.js b/browser/components/nsBrowserGlue.js >+#ifndef XP_MACOSX >+ // The application is not actually quitting, >+ // however the last real browser window is about to be closed. >+ case "browser-lastwindow-close-requested": >+ this._onQuitRequest(subject, 'lastwindow'); >+ break; >+ case "browser-lastwindow-close-granted": >+ if (this._saveSession) { >+ this._setPrefToSaveSession(); >+ } >+ break; >+#endif "The application is not actually quitting, however the last real browser window is about to be closed" would be equally valid for OS X, wouldn't it? So why is OS X excluded? >+ // store the most recent browser window so that we may feed it >+ // to promptService later. >+ var mostRecentBrowserWindow = null; > while (browserEnum.hasMoreElements()) { > windowcount++; > > var browser = browserEnum.getNext(); >+ if (!mostRecentBrowserWindow) { >+ mostRecentBrowserWindow = browser; >+ } > var tabbrowser = browser.document.getElementById("content"); > if (tabbrowser) > pagecount += tabbrowser.browsers.length; > } You're not necessarily getting the most recent window this way. Why do you want to pass it to the promptService anyway?

There are also some trailing spaces, at least in the warnAboutClosingWindow function.

Comment on attachment 379959 [details] [diff] [review] patch v5.1, minor nits addressed Probably should have denied the review explicitly two days ago, but I hoped you or Simon would respond with a brief explanation of why the way you're handling OS X is correct. Lacking that and since it still doesn't seem to make sense to me, r- for now.

(In reply to comment #92) > "The application is not actually quitting, however the last real browser window > is about to be closed" would be equally valid for OS X, wouldn't it? So why is > OS X excluded? For the same reason that if you first close the last real browser window and then hit Cmd+Q, we won't restore that browser window (because we had a 0-window session and thus nothing to restore), while when you close the last real browser window and then quit by closing all other windows (under Windows and Linux), we actually restore that last browser window (because the concept of 0-window sessions isn't native on these platforms). IOW: Closing the last browser window on OS X doesn't have the implicit connotation of quitting, so doesn't have to be handled specially, either.

OK, makes sense. Waiting for a patch that addresses the other stuff.

Nits addressed as I see fit. Remarks: * Added some comments to point out Mac 0-window sessions * warnAboutClosingWindow returns early now skipping the window enumeration if the window is a popup window * warnAboutClosingWindow: Use let exclusively (let is the new var, right) * moved mostRecentBrowserWindow right above .confirmEx. It is now populated by an actual call to .getMostRecentWindow. It is required so that an actually browser window is the parent of the prompt. Before it was possible that it did show up in another window (such as Addons) which was plain confusing. This is because the .confirmEx will use the active window if the window parameter is null.

Looks quite good. As suggested, I'm addressing the remaining nits myself. This also uses gWindowMediator and gObserverService from bug 506111. The new observer topics probably need sr.

(In reply to comment #99) > This also uses gWindowMediator and gObserverService from bug 506111. Unfortunately, bug 506111 got backed out. If it doesn't reland in time, that change needs to be reverted.

Comment on attachment 391287 [details] [diff] [review] patch v5.3, nits addressed Looks fine, except for this: >+# OS X has the concept of zero-window sessions and therefore ignores the >+# browser-lastwindow-close-* topics. >+# >+#ifndef XP_MACOSX >+#define OBSERVE_LASTWINDOW_CLOSE_TOPICS 1 >+#endif Don't use # for comments in these files; use normal /**/ or //, but inside whatever the relevant if block is. This is done in a few places, all MACOSX related.

I removed the dependency on bug 506111.

Comment on attachment 391395 [details] [diff] [review] not for check-in The test is broken on OS X. TEST-INFO | chrome://mochikit/content/browser/browser/components/sessionstore/test/browser/browser_354894.js | Console message: [JavaScript Error: "testMacNotificationCount is not defined" {file: "chrome://mochikit/content/browser/browser/components/sessionstore/test/browser/browser_354894.js" line: 481}] NEXT ERROR TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/browser/components/sessionstore/test/browser/browser_354894.js | Timed out

(In reply to comment #103) > (From update of attachment 391395 [details] [diff] [review]) > The test is broken on OS X. > > TEST-INFO | > chrome://mochikit/content/browser/browser/components/sessionstore/test/browser/browser_354894.js > | Console message: [JavaScript Error: "testMacNotificationCount is not defined" > {file: > "chrome://mochikit/content/browser/browser/components/sessionstore/test/browser/browser_354894.js" > line: 481}] > NEXT ERROR TEST-UNEXPECTED-FAIL | > chrome://mochikit/content/browser/browser/components/sessionstore/test/browser/browser_354894.js > | Timed out Whoops, that function got renamed between v5 and v5.1. s/testMacNotificationCount/testMacNotifications/g ;) Should work, at least it does on my windows machine (using the mac tests, of course).

Thanks for working on this, Nils. http://hg.mozilla.org/mozilla-central/rev/cd25ab8c2f30

Same as the central check-in version. Applied cleanly to moz1.9.1 as well (with some minor --fuzz) Checked it was correctly applied and run mochitest-browser-chrome suite which then passed. Judging from the number bug reports and wanted-firefox3 I suggest landing this on moz1.9.1. Please advice on what steps to take to make this happen (or carry out the necessary steps yourself).

Comment on attachment 391869 [details] [diff] [review] patch 5.2.1, moz1.9.1 version No need for another review if this is the same patch and applies cleanly. Feel free to request approval. I'm not sure this should land on 1.9.1 though, as it might affect extensions in an unforeseen way, and 3.6 will be out in the near future.

Comment on attachment 391869 [details] [diff] [review] patch 5.2.1, moz1.9.1 version (In reply to comment #107) > (From update of attachment 391869 [details] [diff] [review]) > No need for another review if this is the same patch and applies cleanly. Feel > free to request approval. > > I'm not sure this should land on 1.9.1 though, as it might affect extensions in > an unforeseen way, and 3.6 will be out in the near future. This bug seems to be a bug a lot of people indeed "suffer" from. Coming from the add-ons community myself I of course considered add-on compatibility. There is of course a smallish possibility that this will affect some add-ons, but I really think that it's most likely not the case. The method signatures and return values didn't change. Opening/closing browser windows didn't change, at least from the add-on perspective, as add-ons always have to expect that the window in question will have tabs restored to. Closing a browser window might or might not prompt the user but that's already the case when warning about tabs of if the window in question is the last window. Using notifications across the different components should be transparent (enough) to add-ons. If in doubt there is always the addons blog and newsletter where add-on authors could be notified of this change (that most likely won't affect them at all). I think that this is a good and important fix that would by far outweight any (In reply to comment #107) > (From update of attachment 391869 [details] [diff] [review]) > No need for another review if this is the same patch and applies cleanly. Feel > free to request approval. > > I'm not sure this should land on 1.9.1 though, as it might affect extensions in > an unforeseen way, and 3.6 will be out in the near future. This bug seems to be a bug a lot of people indeed "suffer" from. Coming from the add-ons community myself I of course considered add-on compatibility. There is of course a smallish possibility that this will affect some add-ons, as there always is, but I really think that it's most likely not the case. The method signatures and return values didn't change. Opening/closing browser windows didn't change, at least from the add-on perspective, as add-ons always have to expect that the window in question will have tabs restored to. Closing a browser window might or might not prompt the user but that's already the case when warning about tabs or if the window in question is the last window. Using notifications across the different components should be transparent (enough) to add-ons. If in doubt there is always the addons blog and newsletter where add-on authors could be notified of this change (that most likely won't affect them at all). So in conclusion I'm requesting approval.

This has probably caused/triggered bug 507605.

(In reply to comment #109) > This has probably caused/triggered bug 507605. Or maybe not. See also bug 507784.

Comment on attachment 391869 [details] [diff] [review] patch 5.2.1, moz1.9.1 version 1.9.1.2 is almost shipped. Pushing approval request out to 1.9.1.3.

Comment on attachment 391869 [details] [diff] [review] patch 5.2.1, moz1.9.1 version This is too much change for a non-blocking bug to risk on the stable branches. 1.9.1 approval denied.

Which patch applies to 1.9.2? One says "not for check-in" and the other says it's for 1.9.1.

"not for check-in" + a minor fix for the test is what landed.

Doesn't look like there are any developer-facing changes here. What needs documenting, actually?

(In reply to comment #118) > Doesn't look like there are any developer-facing changes here. What needs > documenting, actually? Well, the new observer topics might be of interest to developers: browser-lastwindow-close-requested browser-lastwindow-close-granted I don't know if it would be a good idea to note the changed behavior as well? Session restore might now happen without actually restarting the application, which might be important for developers interacting with the sessionstore service.

Added: https://developer.mozilla.org/en/Session_store_API#Restoring_without_restarting Also added the new topics here: https://developer.mozilla.org/en/Observer_Notifications#Browser

It seems this bug is back in Firefox 4.0 and 4.0.1!

(In reply to comment #122) > It seems this bug is back in Firefox 4.0 and 4.0.1! Please file a new bug.

(In reply to comment #122) > It seems this bug is back in Firefox 4.0 and 4.0.1! Firefox has closed the Downloads window when closing the last window for a while now - which results in Firefox actually quitting. Do you mean that this bug is back under different circumstances?