From parent bug: 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? Also i changed some logic in nsSuiteGlue.js, and in windowsisclosing function in navigator.js

I'm a bit worried about what to do with mailnews (in the longer term), but this sounds like nice polish to current behavior at least, so wanted+ (I'll cancel the approval request as it can go in with just the wanted+ and I don't feel like approving a patch that doesn't have any review yet).

Fixes some wrong string usage, also contains fix for bug 514388

+#ifdef XP_MACOSX + // OS X doesn't quit the application when the last window is closed, but keeps + // the session alive. Hence don't prompt users to save tabs, but warn about + // closing multiple tabs. + return getBrowser().warnAboutClosingTabs(true); +#else Mostly Suite code tries to avoid preprocessing files by using code such as: if (/Mac/.test(navigator.platform)) of course nsSessionStore.js etc are already preprocessed.

Comment on attachment 399261 [details] [diff] [review] better fix >- content/navigator/navigator.js >+* content/navigator/navigator.js What Ratty said. >@@ -2357,34 +2355,11 @@ function WindowIsClosing() > var browser = getBrowser(); > var cn = browser.tabContainer.childNodes; > var numtabs = cn.length; >- var reallyClose = true; >+ var reallyClose = closeWindow(false, warnAboutClosingWindow); I don't think closeWindow makes sense here, because it triggers a quit request. I also don't see the need to move the prompt to tabbrowser.xml. The code here should look something like this: if (!/Mac/.test(navigator.platform) && isClosingLastBrowser()) { // trigger the last browser close request in nsSuiteGlue } else if (numtabs > 1) { // existing warn on close tabs code And since you won't be triggering the last browser close request on the Mac, you don't need to #ifdef the code in the other files either. >+ !this._inPrivateBrowsing) { No such variable.

If i understand correctly what you said ...

> + !this._inPrivateBrowsing) { Ahem! We *still* don't have Private Browsing.

I know, saw it after submitting, just waiting for Neil comments to update patch :(

previous patch misses some vital observer logic, plus i added test. Test passes with two exceptions: TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/suite/common/tests/browser/browser_bug515006.js | Restored window in-session with otherpopup windows around - Got 4, expected 3 TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/suite/common/tests/browser/browser_bug515006.js | Restored window in-session with otherpopup windows around - Got 4, expected 3 IMHO there is something other than wrong patch, i need suggestion here. I removed private browsing part from test, so i adjusted const NOTIFICATIONS_EXPECTED to 4 from 6, maybe something similar should be done with these errors too.

Comment on attachment 399426 [details] [diff] [review] v2, with mac detectiong suggestions >+ if (!/Mac/.test(navigator.platform) && isClosingLastBrowser()) { The browser-lastwindow-close-requested/granted code belongs here. >+ return reallyClose; >+ } else if (numtabs > 1) { This is my fault; you always return here, so no else needed. >+ let foundOtherBrowserWindow = false; You don't need this, see below. >+ let wm = Components.classes["@mozilla.org/appshell/window-mediator;1"].getService(Components.interfaces.nsIWindowMediator); Nit: wrap long line Components.classes .getService >+ foundOtherBrowserWindow = true; Just return false; immediately. >+ os.notifyObservers(null, "browser-lastwindow-close-granted", null); As above, this doesn't belong here. >+ var sessionWillBeSaved = prefBranch.getIntPref("browser.startup.page") == 3 || >+ prefBranch.getBoolPref("browser.sessionstore.resume_session_once"); >+ if (sessionWillBeSaved || !prefBranch.getBoolPref("browser.warnOnQuit")) Not sure why you have a separate variable for this, you only use it once.

Well, we are a bit out of sync, here is patch with nits fixed. Previous comment about test still apply.

Comment on attachment 399474 [details] [diff] [review] v 2.1 with test, nits fixed OK, so I tried this out, and it mostly works, but there is a problem when you have a home page, and that is when you close the browser window and then reopen it you get the home page as well as the restored tabs. I think this is also the problem that is causing your tests to fail, but for a different reason; in that case, you're triggering the new window preference, which is also to open the home page. >+ let os = Components.classes["@mozilla.org/observer-service;1"].getService(Components.interfaces.nsIObserverService); Nit: needs wrapping >+ return true; >+ } else if (numtabs > 1) { Nit: no else. In fact, please leave a blank line, i.e. return true; } if (numtabs > 1) { >+ * Checks if this is the last full *browser* window around. Nit: trailing space. Also, grammar nit: Checks whether this >+ if (win != window && win.toolbar.visible) >+ foundOtherBrowserWindow = true; As mentioned, just return false immediately. > } > >+ else if (this._restoreLastWindow && aWindow.toolbar.visible && >+ this._closedWindows.length && this._doResumeSession()) { >+ >+ // default to the most-recently closed window Nit: remove these blank lines. >+ var sessionWillBeSaved = prefBranch.getIntPref("browser.startup.page") == 3 || >+ prefBranch.getBoolPref("browser.sessionstore.resume_session_once"); >+ if (sessionWillBeSaved || !prefBranch.getBoolPref("browser.warnOnQuit")) Nit: as mentioned, eliminate this variable.

(In reply to comment #11) > (From update of attachment 399474 [details] [diff] [review]) > OK, so I tried this out, and it mostly works, but there is a problem when you > have a home page, and that is when you close the browser window and then reopen > it you get the home page as well as the restored tabs. I think this is also the > problem that is causing your tests to fail, but for a different reason; in that > case, you're triggering the new window preference, which is also to open the > home page. > Well, when i modify line this.restoreWindow(aWindow, state, true); which means to overwrite tabs, tests passing. But then another issue arrives. when you call seamonkey someURL, session restores and last tab in restored session loads someURL.

I modified code by stealing some logic from navigator.js, not sure You will like it. Nothing else not coming to my head. Test is passing.

Comment on attachment 401825 [details] [diff] [review] v 2.5, fix new window pref behavior >+ let foundOtherBrowserWindow = false; Nit: variable no longer used. >+ // open a new window the previously closed window should be restored to >+ newWin = openDialog(location, "_blank", CHROME_FEATURES); Unfortunately this isn't actually how the UI opens a browser window, so although your workaround passes for the test, it might not work for real.

Comment on attachment 401825 [details] [diff] [review] v 2.5, fix new window pref behavior >+ //workaround "browser.windows.loadOnNewWindow" behavior In fact this is a dead giveaway, because if there are no browser windows open, then we actually trigger the "browser.startup.page" behaviour.

Comment on attachment 401825 [details] [diff] [review] v 2.5, fix new window pref behavior So, can you try to a) restore this version of the code >+ if (state) { >+ delete state.hidden; >+ state = { windows: [state] }; >+ this._restoreCount = 1; >+ this.restoreWindow(aWindow, state, this._isCmdLineEmpty(aWindow)); >+ } b) change the tests to always pass about:blank as the argument. >+ let newWin = openDialog(location, "_blank", CHROME_FEATURES, "about:blank"); c) leave the problem in comment 11 for a followup bug.

OK, done as You suggest, filed Bug 517998, but no idea how to properly fix it :( .

Comment on attachment 401957 [details] [diff] [review] nits fixed, Neil suggestions done [Checkin: Comment 19] wanted+ so wouldn't even need a+ ;-)

http://hg.mozilla.org/comm-central/rev/b9f6a5af7814 http://hg.mozilla.org/comm-central/rev/631b2247f293 (forgot to "hg add" the test file)

(In reply to comment #19) > http://hg.mozilla.org/comm-central/rev/631b2247f293 (forgot to "hg add" the > test file) http://mxr.mozilla.org/comm-central/source/suite/common/tests/browser/browser_bug515006.js fails on MacOSX since it was pushed: { TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/suite/common/tests/browser/browser_bug515006.js | First close attempt denied TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/suite/common/tests/browser/browser_bug515006.js | browser-lastwindow-close-requested notifications observed - Got 0, expected 4 TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/suite/common/tests/browser/browser_bug515006.js | Notification count for -request and -grant matches - Got 0, expected 1 }

I havent looked at the failures and the code, but can this be related to the fact that on mac you're not quitting the application when you close all windows?

(In reply to comment #21) That's the idea I had too. Misak, please at least disable the test (on MacOSX) ftb to workaround the permanent orange.

(In reply to comment #21) > I havent looked at the failures and the code, but can this be related to the > fact that on mac you're not quitting the application when you close all > windows? I don't thinks so, because this is a port from Firefox bug and same test don't fail there. (In reply to comment #22) > (In reply to comment #21) > > That's the idea I had too. > > Misak, please at least disable the test (on MacOSX) ftb to workaround the > permanent orange. Sorry, i don't know how to do that. Help needed.

Workaround the permanent orange until someone with a Mac can investigate...

Comment on attachment 404735 [details] [diff] [review] (Bv1) Disable test on MacOSX until it passes [Checkin: See comment 25] http://hg.mozilla.org/comm-central/rev/0de962bec599 Bv1, with new bug number.

I filed bug 520787 to re-enable the test on MacOSX.