515006 - Port Bug 354894 [Session restore doesn't work if process hasn't exited (Downloads window open)] to SeaMonkey. (browser_bug515006.js)

Status: RESOLVED FIXED

(SeaMonkey :: Session Restore, defect)

Description

[Misak Khachatryan]

Attachment: patch

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

Comment 1

[Robert Kaiser]

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).

Comment 2

[Misak Khachatryan]

Attachment: better fix

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

Comment 3

[Philip Chee]

+#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 4

[[e-mail]]

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.

Comment 5

[Misak Khachatryan]

Attachment: v2, with mac detectiong suggestions

If i understand correctly what you said ...

Comment 6

[Philip Chee]

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

Comment 7

[Misak Khachatryan]

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

Comment 8

[Misak Khachatryan]

Attachment: v 2.1 with test

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 9

[[e-mail]]

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.

Comment 10

[Misak Khachatryan]

Attachment: v 2.1 with test, nits fixed

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

Comment 11

[[e-mail]]

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.

Comment 12

[Misak Khachatryan]

(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.

Comment 13

[Misak Khachatryan]

Attachment: v 2.5, fix new window pref behavior

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 14

[[e-mail]]

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 15

[[e-mail]]

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 16

[[e-mail]]

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.

Comment 17

[Misak Khachatryan]

Attachment: nits fixed, Neil suggestions done [Checkin: Comment 19]

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

Comment 18

[Robert Kaiser]

Comment on attachment 401957 [details] [diff] [review]
nits fixed, Neil suggestions done
[Checkin: Comment 19]

wanted+ so wouldn't even need a+ ;-)

Comment 19

[Stefan [:stefanh]]

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

Comment 20

[Serge Gautherie (:sgautherie)]

(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
}

Comment 21

[Stefan [:stefanh]]

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?

Comment 22

[Serge Gautherie (:sgautherie)]

(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.

Comment 23

[Misak Khachatryan]

(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.

Comment 24

[Serge Gautherie (:sgautherie)]

Attachment: (Bv1) Disable test on MacOSX until it passes [Checkin: See comment 25]

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

Comment 25

[Serge Gautherie (:sgautherie)]

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.

Comment 26

[Serge Gautherie (:sgautherie)]

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