Closed Bug 517998 Opened 11 years ago Closed 11 years ago

There should be way to tell navigator.js that sessionstore is restoring window to avoid triggering "browser.windows.loadOnNewWindow" or "browser.startup.page"

Categories

(SeaMonkey :: Session Restore, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
seamonkey2.0

People

(Reporter: misak.bugzilla, Assigned: misak.bugzilla)

References

Details

(Keywords: fixed-seamonkey2.0)

Attachments

(1 file, 4 obsolete files)

After bug 515006 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.
There should be way to tell navigator.js that sessionstore is restoring window to avoid triggering "browser.windows.loadOnNewWindow" or "browser.startup.page". More details can be found on bug 515006 comment #11
browser.startup.page is actually handled in nsBrowserContentHandler.js; the easiest way to communicate between that and Session Storage seems to be to add a new method to nsISessionStore.idl to see if it is about to restore a window. Then getURLToLoad() can call that method and return "about:blank" (like it does when restoring from session startup).
Ok, then it session restore bug. Taking.
Status: NEW → ASSIGNED
Component: Tabbed Browser → Session Restore
QA Contact: tabbed-browser → session.restore
Flags: wanted-seamonkey2.0?
Attached patch use doRestore (obsolete) — Splinter Review
There is method in nsSessionStartup called doRestore. It used in GetURLtoLoad. Should do the job here too.
Assignee: nobody → misak
Attachment #404779 - Flags: superreview?(neil)
Attachment #404779 - Flags: review?(neil)
Comment on attachment 404779 [details] [diff] [review]
use doRestore

nsBrowserContentHandler already calls doRestore, and that doesn't work anyway because doRestore doesn't know about _restoreLastWindow.
Attachment #404779 - Flags: superreview?(neil)
Attachment #404779 - Flags: superreview-
Attachment #404779 - Flags: review?(neil)
Attached patch add new method and use it (obsolete) — Splinter Review
Attachment #404779 - Attachment is obsolete: true
Attachment #404796 - Flags: superreview?(neil)
Attachment #404796 - Flags: review?(neil)
Comment on attachment 404796 [details] [diff] [review]
add new method and use it

>diff --git a/suite/browser/navigator.js b/suite/browser/navigator.js
You don't need this change because BrowserOpenWindow() calls nsBrowserContentHandler anyway.

>+    var st = Components.classes["@mozilla.org/suite/sessionstore;1"]
>+             .getService(Components.interfaces.nsISessionStore);
Bah, I just noticed that the nsISessionStartup that you copied this from doesn't have the .getService lined up with .classes either :-(

>+  boolean getRestoreLastWindow();
I think that this might be better named doRestoreLastWindow.

>+  getRestoreLastWindow: function sss_getRestoreLastWindow() {
>+    return this._restoreLastWindow;
This isn't quite right; you need to check the resume prefs and check that you have a non-popup window that you can actually restore. (See sss_onLoad. About the only thing you don't have to check for is the toolbar, since we know we're not opening a popup window.)
Attached patch add new method and use it v2 (obsolete) — Splinter Review
Fixed all your comments, ran chrome tests, all passing.
Attachment #404796 - Attachment is obsolete: true
Attachment #404801 - Flags: superreview?(neil)
Attachment #404801 - Flags: review?(neil)
Attachment #404796 - Flags: superreview?(neil)
Attachment #404796 - Flags: review?(neil)
(In reply to comment #7)
> Fixed all your comments
Not quite, you didn't check for a window that wasn't a popup.
Attached patch use same check as in sss_onLoad (obsolete) — Splinter Review
OK, i've used same check as in sss_onLoad. That will be enough if my English doesn't playing with me again.
Attachment #404801 - Attachment is obsolete: true
Attachment #404812 - Flags: superreview?(neil)
Attachment #404812 - Flags: review?(neil)
Attachment #404801 - Flags: superreview?(neil)
Attachment #404801 - Flags: review?(neil)
Attachment #404812 - Flags: superreview?(neil)
Attachment #404812 - Flags: review?(neil)
Attachment #404812 - Flags: review-
Comment on attachment 404812 [details] [diff] [review]
use same check as in sss_onLoad

>+    return (this._restoreLastWindow && aWindow.toolbar.visible &&
>+            this._closedWindows.length && this._doResumeSession());
Sorry, that's the wrong check - it's the closed window that you need to check.
Flags: wanted-seamonkey2.0? → wanted-seamonkey2.0+
Attachment #404812 - Attachment is obsolete: true
Attachment #405836 - Flags: superreview?(neil)
Attachment #405836 - Flags: review?(neil)
Attachment #405836 - Flags: superreview?(neil)
Attachment #405836 - Flags: superreview+
Attachment #405836 - Flags: review?(neil)
Attachment #405836 - Flags: review+
Keywords: checkin-needed
Attachment #405836 - Flags: approval-seamonkey2.0?
Attachment #405836 - Flags: approval-seamonkey2.0? → approval-seamonkey2.0+
Comment on attachment 405836 [details] [diff] [review]
fixed patch [Checkin: Comment 12]

http://hg.mozilla.org/comm-central/rev/3291966677c0
Attachment #405836 - Attachment description: fixed patch → fixed patch [Checkin: Comment 12]
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.0
You need to log in before you can comment on or make changes to this bug.