Last Comment Bug 459550 - Port Bug 448976 (turn the Session Restore prompt into an error page) to SeaMonkey
: Port Bug 448976 (turn the Session Restore prompt into an error page) to SeaM...
Status: RESOLVED FIXED
: fixed-seamonkey2.0
Product: SeaMonkey
Classification: Client Software
Component: Session Restore (show other bugs)
: Trunk
: All All
: -- normal (vote)
: seamonkey2.0a3
Assigned To: Misak Khachatryan
:
Mentors:
Depends on: 36810 448976 478470 478506
Blocks: 479992 480109 480110
  Show dependency treegraph
 
Reported: 2008-10-12 02:25 PDT by Misak Khachatryan
Modified: 2009-09-18 06:18 PDT (History)
14 users (show)
kairo: wanted‑seamonkey2.0+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch v1 (25.52 KB, patch)
2008-10-12 14:13 PDT, Misak Khachatryan
no flags Details | Diff | Review
patch v2 (25.34 KB, patch)
2008-10-13 04:32 PDT, Misak Khachatryan
neil: ui‑review-
Details | Diff | Review
patch v3 (31.12 KB, patch)
2008-10-15 07:36 PDT, Misak Khachatryan
no flags Details | Diff | Review
bugfix ports respin (50.73 KB, patch)
2008-11-23 02:19 PST, Misak Khachatryan
no flags Details | Diff | Review
patch v4 (55.07 KB, patch)
2008-12-22 03:48 PST, Misak Khachatryan
no flags Details | Diff | Review
tests (64.83 KB, patch)
2009-02-01 07:49 PST, Misak Khachatryan
no flags Details | Diff | Review
patch v 4.1 (65.56 KB, patch)
2009-02-02 07:04 PST, Misak Khachatryan
no flags Details | Diff | Review
v 4.1 with missing file included (75.03 KB, patch)
2009-02-02 10:27 PST, Misak Khachatryan
no flags Details | Diff | Review
v 4.2 (72.19 KB, patch)
2009-02-03 00:42 PST, Misak Khachatryan
no flags Details | Diff | Review
v 4.3 (72.41 KB, patch)
2009-02-03 08:58 PST, Misak Khachatryan
no flags Details | Diff | Review
Fix for netError.css (pushed) (962 bytes, patch)
2009-02-03 13:44 PST, neil@parkwaycc.co.uk
kairo: review+
Details | Diff | Review
v 4.4, comments fixed (71.26 KB, patch)
2009-02-04 04:05 PST, Misak Khachatryan
neil: superreview+
neil: ui‑review+
Details | Diff | Review
v 4.5 (72.37 KB, patch)
2009-02-11 05:13 PST, Misak Khachatryan
neil: superreview-
Details | Diff | Review
v 4.6 (pushed) (74.05 KB, patch)
2009-02-11 07:32 PST, Misak Khachatryan
neil: review+
neil: superreview+
Details | Diff | Review
fix accidentally added line (pushed) (609 bytes, patch)
2009-02-13 12:20 PST, Misak Khachatryan
no flags Details | Diff | Review
updated tests (64.86 KB, patch)
2009-02-13 12:23 PST, Misak Khachatryan
no flags Details | Diff | Review

Description Misak Khachatryan 2008-10-12 02:25:09 PDT
This bug is big enough to keep away from main  Bug 36810. Patch should be applied over main Bug 36810 patch, it will come shortly.
Comment 1 Misak Khachatryan 2008-10-12 14:13:02 PDT
Created attachment 342810 [details] [diff] [review]
patch v1

This patch does what it should do. It also needs ui review. I've used css from ff's gnomestripe theme in both modern and classic. Patch should be applied on top of main sessionstore patch (bug 36810).
Comment 2 Misak Khachatryan 2008-10-13 04:32:12 PDT
Created attachment 342856 [details] [diff] [review]
patch v2

css tweaks, almost final.
Comment 3 neil@parkwaycc.co.uk 2008-10-13 09:26:43 PDT
Comment on attachment 342856 [details] [diff] [review]
patch v2

>+   content/communicator/aboutSessionRestore.xhtml
Doesn't seem to have been included in the patch...

>+const Cc = Components.classes;
>+const Ci = Components.interfaces;
Don't like these.

>+function NSGetModule(compMgr, fileSpec)
>+  XPCOMUtils.generateModule([AboutSessionRestore]);
{}s please.

>-  locale/@AB_CD@/communicator/sessionstore.properties                       (%chrome/common/sessionstore.properties)
Presumably this will never actually get checked in?

>+#errorPageContainer {
>+  background-image: url("moz-icon://stock/gtk-dialog-question?size=dialog");
>+}
No stock icons, they only work in gtk anyway.

>+#tabList {
>+  width: 100%;
Without seeing the xhtml it's hard to tell but width is 100% by default, no?

>+treechildren::-moz-tree-image(checked) {
>+  list-style-image: url("chrome://global/skin/checkbox/cbox-check.gif");
>+}
>+treechildren::-moz-tree-image(partial) {
>+  list-style-image: url("chrome://global/skin/checkbox/cbox-check-dis.gif");
>+}
Looks like this doesn't work on the Mac... not sure what the fix is here.

>+treechildren::-moz-tree-row(alternate) {
>+  background-color: -moz-oddtreerow;
>+}
>+treechildren::-moz-tree-row(alternate, selected) {
>+  background-color: Highlight;
>+}
What's "alternate" as distinct from odd/even?

>+#buttons {
>+  width: 100%;
>+  -moz-box-direction: reverse;
>+}
>+#buttons > button {
>+  margin-top: 2em;
>+  -moz-margin-start: 5px;
>+}
Again, no xhtml to guide me here, but these look wrong.

>+treechildren::-moz-tree-row(alternate) {
>+  background-color: -moz-oddtreerow;
>+}
>+treechildren::-moz-tree-row(alternate, selected) {
>+  background-color: Highlight;
>+}
These colours are wrong in Modern.
Comment 4 Misak Khachatryan 2008-10-15 07:36:13 PDT
Created attachment 343235 [details] [diff] [review]
patch v3

- ported bug 459651
- included missing file.
- fixed Cc, Ci stuff

>>-  locale/@AB_CD@/communicator/sessionstore.properties                       (%chrome/common/sessionstore.properties)
>Presumably this will never actually get checked in?

Actually yes, if it's sounds reasonable, i can prepare consolidated patch for both bug 36810 and this one.

About other comments: I'll try to fix all of them, but someone's other help for css and theme stuff is definitely wanted, because i don't know these well, and it takes longer.

Requesting ui-review again to clarify all comment #3 issues where aboutSessionRestore.xhtml presence is required.
Comment 5 Misak Khachatryan 2008-10-15 07:39:23 PDT
>>+function NSGetModule(compMgr, fileSpec)
>>+  XPCOMUtils.generateModule([AboutSessionRestore]);
>{}s please.

Oh, forgot to mention, when i put {} like this:

function NSGetModule(compMgr, fileSpec) {
  XPCOMUtils.generateModule([AboutSessionRestore]);
}

my seamonkey crashes before displaying profile selection dialog. So i leaved this function as is.
Comment 6 neil@parkwaycc.co.uk 2008-10-15 09:30:09 PDT
(In reply to comment #5)
> Oh, forgot to mention, when i put {} like this:
> 
> function NSGetModule(compMgr, fileSpec) {
>   XPCOMUtils.generateModule([AboutSessionRestore]);
> }
> 
> my seamonkey crashes before displaying profile selection dialog.
Just goes to show how confusing the new syntax is. It needs {
  return ...;
}
Comment 7 Misak Khachatryan 2008-11-23 02:19:31 PST
Created attachment 349630 [details] [diff] [review]
bugfix ports respin

This patch incorporates bugfixes since previous patch, also addressed Neil syntax comments.

Ported  Bug 459593, Bug 459567, Bug 459950, Bug 395488, Bug 462973 and  Bug 395488.
Comment 8 Misak Khachatryan 2008-12-22 03:48:26 PST
Created attachment 354150 [details] [diff] [review]
patch v4

Some code rearrangement and another bunch of bug ports.

Ported:
Bug 465215,  Bug 453831,  Bug 462863, Bug 464155, Bug 468168,  Bug 463964, Bug 465223, Bug 466937
Comment 9 Misak Khachatryan 2009-01-27 08:06:23 PST
Comment on attachment 354150 [details] [diff] [review]
patch v4

Slightly modified KaiRo comment from main sessionstore bug. As soon as ui things wilbe ok, i'll request sr too.

Requesting ui-review from Neil in parallel to speed up the process of finally getting
this into the tree.
In
https://wiki.mozilla.org/SeaMonkey:StatusMeetings:2009-01-13#Longer-Term_SeaMonkey_2_Planning
we decided we can possibly go with a rs+ from ajschult (and post-facto-review)
on the sessionstore backend copied from Firefox, given those already were
reviewed once, the parts special to SeaMonkey were already reviewed by
ajschult, and the whole patch should get sr from Neil as well.
Comment 10 Misak Khachatryan 2009-02-01 07:49:44 PST
Created attachment 360001 [details] [diff] [review]
tests

all sessionstore tests ported, some of them timing out because of different implementation of undo close tab functionality.
Comment 11 neil@parkwaycc.co.uk 2009-02-02 06:34:09 PST
Comment on attachment 354150 [details] [diff] [review]
patch v4

>+#ifdef XP_UNIX
Not sure what the point of this is. (It's not really an OK/Cancel type decision like a dialog would be.)

>+      <!-- holds the session data for when the tab is closed -->
>+      <input type="text" id="sessionData" style="display: none;"/>
Does <input type="hidden" id="sessionData"/> not work?

>+*  content/communicator/aboutSessionRestore.xhtml
In alphabetical order please.

>+   content/communicator/aboutSessionRestore.js
This should be called nsAboutSessionRestore.js

>+#errorPageContainer {
>+  background-image: url("moz-icon://stock/gtk-dialog-question?size=dialog");
>+}
We can't use this, but maybe we can @import the global netError.css?

>+#errorPageContainer {
>+  background-image: url("moz-icon://stock/gtk-dialog-question?size=dialog");
>+}
And this is obviously wrong for Modern but you can probably copy netError.css

>+treechildren::-moz-tree-row(alternate) {
>+  background-color: -moz-oddtreerow;
>+}
>+treechildren::-moz-tree-row(alternate, selected) {
>+  background-color: Highlight;
>+}
This is also wrong for Modern. (I don't think it supports alternating rows.)

>+#buttons > button {
>+  margin-top: 2em;
>+  -moz-margin-start: 5px;
Changing the margin on a button in the Modern theme is a bad move, but you probably don't need to set the side margin, and you could try moving the top margin to the #buttons style.
Comment 12 Misak Khachatryan 2009-02-02 07:04:19 PST
Created attachment 360083 [details] [diff] [review]
patch v 4.1

Some fixes in Sessionstore API, patch against latest patch from bug 36810
Comment 13 neil@parkwaycc.co.uk 2009-02-02 07:19:15 PST
Comment on attachment 360083 [details] [diff] [review]
patch v 4.1

>+   content/communicator/aboutSessionRestore.js
This is wrong, or the file is missing from the patch. Probably the latter, since the buttons don't work ;-)
Comment 14 Misak Khachatryan 2009-02-02 10:27:53 PST
Created attachment 360113 [details] [diff] [review]
v 4.1 with missing file included

Sorry, missed file now in the patch.
Comment 15 Misak Khachatryan 2009-02-03 00:39:45 PST
(In reply to comment #11)
> (From update of attachment 354150 [details] [diff] [review])
> >+#ifdef XP_UNIX
> Not sure what the point of this is. (It's not really an OK/Cancel type decision
> like a dialog would be.)
> 
It's for Mac OS, where OK/CANCEL buttons swapped.

> Does <input type="hidden" id="sessionData"/> not work?
> 
Seems to work. Changed

> >+*  content/communicator/aboutSessionRestore.xhtml
> In alphabetical order please.
> 
Done.
> >+   content/communicator/aboutSessionRestore.js
> This should be called nsAboutSessionRestore.js
> 
I think you talking about suite/common/src/aboutSessionRestore.js Changed his name to suite/common/src/nsAboutSessionRestore.js

> >+#errorPageContainer {
> >+  background-image: url("moz-icon://stock/gtk-dialog-question?size=dialog");
> >+}
> We can't use this, but maybe we can @import the global netError.css?
> 
Done.
> >+#errorPageContainer {
> >+  background-image: url("moz-icon://stock/gtk-dialog-question?size=dialog");
> >+}
> And this is obviously wrong for Modern but you can probably copy netError.css
> 
Done.
> >+treechildren::-moz-tree-row(alternate) {
> >+  background-color: -moz-oddtreerow;
> >+}
> >+treechildren::-moz-tree-row(alternate, selected) {
> >+  background-color: Highlight;
> >+}
> This is also wrong for Modern. (I don't think it supports alternating rows.)
> 
> >+#buttons > button {
> >+  margin-top: 2em;
> >+  -moz-margin-start: 5px;
> Changing the margin on a button in the Modern theme is a bad move, but you
> probably don't need to set the side margin, and you could try moving the top
> margin to the #buttons style.

Lacking CSS knowledge, will appreciate if someone's helps me.
Comment 16 Misak Khachatryan 2009-02-03 00:42:24 PST
Created attachment 360264 [details] [diff] [review]
v 4.2

Fixed most nits, added missed file, also contains fixes for bug 447951, and bug 476161.
Comment 17 Simon Bünzli 2009-02-03 02:28:38 PST
(In reply to comment #15)
> > Does <input type="hidden" id="sessionData"/> not work?
> Seems to work. Changed

Doesn't work for repeated crashes. StR:
1. Crash SeaMonkey twice (so that you get about:sessionrestore).
2. Surf around (i.e. open a new tab and wait 10 seconds)
3. Crash SeaMonkey again

Actual result:
Restoring the session will show all the tabs opened during step 2 and an about:sessionrestore identical to the one you've already got: a recursive one, which will amuse computer scientists and confuse everybody else. Reason is that we don't restore <input type="hidden"> for various reasons (mostly because they are rarely changed by the user, being hidden and all).

See bug 459561 for what you could do instead.
Comment 18 neil@parkwaycc.co.uk 2009-02-03 04:11:00 PST
Comment on attachment 360264 [details] [diff] [review]
v 4.2

>+++ b/suite/themes/classic/communicator/aboutSessionRestore.css

>+treechildren::-moz-tree-image(noicon) {
>+  list-style-image: url("chrome://communicator/skin/bookmarks/bookmark-item.png");
>+}
>+treechildren::-moz-tree-image(container, noicon) {
>+  list-style-image: url("moz-icon://stock/gtk-directory?size=menu");
>+}
>+treechildren::-moz-tree-image(checked) {
>+  list-style-image: url("chrome://global/skin/checkbox/cbox-check.gif");
>+}
>+treechildren::-moz-tree-image(partial) {
>+  list-style-image: url("chrome://global/skin/checkbox/cbox-check-dis.gif");
>+}
Blank lines between the style rules, please. Also I think the last two should be -moz-tree-checkbox rather than -moz-tree-image.

>+++ b/suite/themes/modern/communicator/aboutSessionRestore.css

>+}
>+treechildren::-moz-tree-image(container, noicon) {
Nit: blank line between rules

>+  list-style-image: url("moz-icon://stock/gtk-directory?size=menu");
Can't use this image in Modern! I think the most suitable image would be chrome://communicator/skin/bookmarks/bookmark-folder-closed.gif but then add another rule
treechildren::-moz-tree-image(container, noicon, open)
for the -open version.

>+treechildren::-moz-tree-image(checked) {
>+  list-style-image: url("chrome://global/skin/checkbox/cbox-check.gif");
>+}
>+treechildren::-moz-tree-image(partial) {
>+  list-style-image: url("chrome://global/skin/checkbox/cbox-check-dis.gif");
>+}
Modern already has tree checkbox styles. So, to make partial work, we have to override them:
treechildren::-moz-tree-checkbox {
  list-style-image: url("chrome://global/skin/checkbox/cbox.gif");
}

treechildren::-moz-tree-checkbox(checked) {
  list-style-image: url("chrome://global/skin/checkbox/cbox-check.gif");
}

treechildren::-moz-tree-checkbox(partial) {
  list-style-image: url("chrome://global/skin/checkbox/cbox-check-dis.gif");
}

>+treechildren::-moz-tree-row(alternate) {
>+  background-color: -moz-oddtreerow;
>+}
>+treechildren::-moz-tree-row(alternate, selected) {
>+  background-color: Highlight;
>+}
Just get rid of these. Modern doesn't do alternate colours as far as I know.

>+#buttons {
>+  -moz-margin-start: 80px;
>+}
>+#buttons > button {
>+  margin-top: 2em;
>+  -moz-margin-start: 5px;
>+}
Move the margin-top to #buttons and remove the -moz-margin-start.
Comment 19 neil@parkwaycc.co.uk 2009-02-03 04:23:43 PST
Comment on attachment 360264 [details] [diff] [review]
v 4.2

>+  hasNextSibling: function(idx, after) {
>+    var thisLevel = this.getLevel(idx);
>+    for (var t = idx + 1; t < gTreeData.length && this.getLevel(t) > thisLevel; t++);
>+    return thisLevel == this.getLevel(t);
There is an error in this calculation; it always returns true for the last window and its last tab. I think this code should work:
for (var t = idx + 1; t < gTreeData.length; t++)
  if (this.getLevel(t) <= thisLevel)
    return this.getLevel(t) == thisLevel;
return false;
Comment 20 neil@parkwaycc.co.uk 2009-02-03 05:01:48 PST
Comment on attachment 360264 [details] [diff] [review]
v 4.2

>+  // restore the session into a new window and close the current tab
>+  var newWindow = top.openDialog(top.location, "_blank", "chrome,dialog=no,all");
This needs an extra , "about:blank" as per bug 36810.

>+  newWindow.addEventListener("load", function() {
>+    newWindow.removeEventListener("load", arguments.callee, true);
>+    ss.setWindowState(newWindow, stateString, true);
>+    
>+    var tabbrowser = top.gBrowser;
>+    var tabIndex = tabbrowser.getBrowserIndexForDocument(document);
>+    tabbrowser.removeTab(tabbrowser.tabContainer.childNodes[tabIndex]);
So, what we need to do here, is
var tab = top.gBrowser.selectedTab;
newWindow.addEventListener("load", function() {
  newWindow.removeEventListener("load", arguments.callee, true);
  ss.setWindowState(newWindow, stateString, true);

  top.gBrowser.removeTab(tab);
}
Comment 21 Misak Khachatryan 2009-02-03 08:58:29 PST
Created attachment 360327 [details] [diff] [review]
v 4.3

All comments done, only thing left is modern css. Taking another review respin.
Comment 22 neil@parkwaycc.co.uk 2009-02-03 09:42:17 PST
Comment on attachment 360327 [details] [diff] [review]
v 4.3

>+    item.open = !item.open;
This needs an extra this.treeBox.invalidateRow(idx); to work properly.

>+    <link rel="stylesheet" href="chrome://global/skin/netError.css" type="text/css" media="all"/>
I've just noticed this line here... see below.

>+#ifdef XP_UNIX
Still not sure why unix would want the buttons the wrong way around ;-)

>       // misak: this shouldn't be needed, will remove in final patch
Still unneeded?

>+@import url("chrome://global/skin/netError.css");
Given the above, we don't need this here.

>+  list-style-image: url("chrome://communicator/skin/bookmarks/bookmark-folder-closed.gif");
I think these are .png in Classic, sorry for the confusion.

>+++ b/suite/themes/modern/communicator/aboutSessionRestore.css
Some of these styles are duplicated in netError.css and so can be removed here.

>+#buttons > button {
>+}
This shouldn't be here at all.
Comment 23 neil@parkwaycc.co.uk 2009-02-03 09:42:46 PST
One of the problems with Modern is really in netError.css, I'll look into it.
Comment 24 neil@parkwaycc.co.uk 2009-02-03 13:44:51 PST
Created attachment 360367 [details] [diff] [review]
Fix for netError.css (pushed)

With this patch, you can remove all the styles from aboutSessionStore.css except for the #tabList and the various treechildren styles.
Comment 25 Misak Khachatryan 2009-02-04 01:44:26 PST
(In reply to comment #22)
> >+#ifdef XP_UNIX
> Still not sure why unix would want the buttons the wrong way around ;-)
> 

OK, pick one you want ;)

> >       // misak: this shouldn't be needed, will remove in final patch
> Still unneeded?
> 
I need to suggestion here. FF Sessionstore uses sessionstore.max_tabs_undo, but Seamonkey has tabs.undoclose.depth for same purpose. Now i'm using tabs.undoclose.depth and didn't introduced sessionstore.max_tabs_undo, but maybe it's not a good idea. Maybe we should have both and use some sort of synchronization to make extension developers life easier.

> Given the above, we don't need this here.
> 
Fixed.

> I think these are .png in Classic, sorry for the confusion.
> 
Fixed.

> >+++ b/suite/themes/modern/communicator/aboutSessionRestore.css
> Some of these styles are duplicated in netError.css and so can be removed here.
Fixed.

> >+#buttons > button {
> >+}
> This shouldn't be here at all.
Fixed.
Comment 26 Misak Khachatryan 2009-02-04 04:05:07 PST
Created attachment 360486 [details] [diff] [review]
v 4.4, comments fixed

Taking into account that u said Unix is the wrong way i've removed that part ;) .
Comment 27 neil@parkwaycc.co.uk 2009-02-04 04:59:18 PST
Comment on attachment 360486 [details] [diff] [review]
v 4.4, comments fixed

OK, this works well with attachment 360367 [details] [diff] [review].
Comment 28 Misak Khachatryan 2009-02-04 08:05:13 PST
Comment on attachment 360001 [details] [diff] [review]
tests

requesting reviews for tests too. Currently one test for bug 456342, and tests related with our undo close tab implementation failing.
Comment 29 Robert Kaiser (not working on stability any more) 2009-02-04 09:10:31 PST
Comment on attachment 360367 [details] [diff] [review]
Fix for netError.css (pushed)

After the bug we had was shown to me, I can verify that his nicely fixes the issue, both on neterror pages and the session restore page (with the new patch from Misak).
Negative background offsets are ugly but it seems necessary in this case, so let's go with it.
Comment 30 :Gavin Sharp [email: gavin@gavinsharp.com] 2009-02-04 21:42:14 PST
I filed bug 476994 on a couple of the issues Neil pointed out.

Neil, if you could file bugs when you find substantive non-style-related issues in ports of patches from Firefox, that would be much appreciated.
Comment 31 Robert Kaiser (not working on stability any more) 2009-02-10 12:15:31 PST
Comment on attachment 360486 [details] [diff] [review]
v 4.4, comments fixed

Ian, could you review this instead of ajschult? He told me on IRC he won't be able to do such a large review atm, and proposed you as one of of the people who could review instead.
Comment 32 Misak Khachatryan 2009-02-11 05:13:30 PST
Created attachment 361749 [details] [diff] [review]
v 4.5

One file i've forgot to delete, and added only one piece of SM specific code - i'm introduced browser.sessionstore.max_tabs_undo and synchronizing it with tabs.undoclose.depth to make sessionstore absolutely the same as in FF.

I don't see any Seamonkey specific code in this patch, except above prefs synchronization so it should be easy to review, regardless of size. It's already reviewed 99.9% in bug 448976. Requesting sr again for prefs sync.
Comment 33 neil@parkwaycc.co.uk 2009-02-11 06:19:56 PST
Comment on attachment 361749 [details] [diff] [review]
v 4.5

tabs.undoclose.depth is a new pref with only three hits in MXR, so IMHO you should just switch the pref over if you want to be more consistent.
Comment 34 Misak Khachatryan 2009-02-11 07:32:34 PST
Created attachment 361771 [details] [diff] [review]
v 4.6 (pushed)

Removed tabs.undoclose.depth and replaced by browser.sessionstore.max_tabs_undo, cleaned up relevant code.
Comment 35 Robert Kaiser (not working on stability any more) 2009-02-13 07:59:32 PST
Comment on attachment 361771 [details] [diff] [review]
v 4.6 (pushed)

As we're already in a freeze for a3 and want to start builds for that release next week, but really would like to have this in for testing there, I think we should go for whoever of Neil or IanN has time for doing the review for this - esp. as this is said to be mostly session restore backend, which should hopefully get consolidated ion toolkit after 1.9.1.
Comment 36 Stefan [:stefanh] 2009-02-13 10:52:31 PST
http://hg.mozilla.org/comm-central/rev/1cba9a3b59cb
Comment 37 Simon Bünzli 2009-02-13 11:55:25 PST
Comment on attachment 361771 [details] [diff] [review]
v 4.6 (pushed)

Comparing what you've checked into comm-central with what we've got in mozilla-central, I have the following suggestions for a follow-up bug:

>+    var maxTabsUndo = this._prefBranch.getIntPref("browser.sessionstore.max_tabs_undo");

s/browser\.// (your pref branch already includes the "browser." prefix.

>+    let closedTabs = aWindow.getBrowser().savedBrowsers.map(function(e) { return e.tabData; });

This line appears a dozen time throughout your nsSessionStore.js. Worth its own function?

>+    // parentheses are for backwards compatibility with older sessionstore files
>+    stateString.data = "(" + this._toJSONString(aStateObj) + ")";

Do you expect users to actually exchange SeaMonkey and Firefox sessionstore.js files? If not, please do yourself a favor and drop the parens here, replace every occurrence of evalInSandbox with JSON.parse and rename sessionstore.js to sessionstore.json.
Comment 38 Misak Khachatryan 2009-02-13 12:20:30 PST
Created attachment 362286 [details] [diff] [review]
fix accidentally added line (pushed)

i've accidentally added one line in the v 4.6 patch, it should be removed.
Comment 39 Misak Khachatryan 2009-02-13 12:23:34 PST
Created attachment 362289 [details] [diff] [review]
updated tests

also i've updated test, mostly changed usage from browser.tabs.undoclose.depth to sessionstore.max_tabs_undo. 5 tests failing all related to our undo close tab implementation
Comment 40 Stefan [:stefanh] 2009-02-13 12:37:36 PST
Comment on attachment 362286 [details] [diff] [review]
fix accidentally added line (pushed)

I landed this as a bustage fix (changeset ee8599c65f12)
Comment 41 Stefan [:stefanh] 2009-02-13 12:38:30 PST
Comment on attachment 360367 [details] [diff] [review]
Fix for netError.css (pushed)

Landed this as 9930c42f3ccf (OK from Neil)
Comment 42 Misak Khachatryan 2009-02-13 12:45:11 PST
Thanks Simon, i'll be very glad for more comments from you. You know this code better than anyone here. I'm sure that i have more things to fix.

Also, i want to make clear, how we can synchronize future bugfixes for sessionstore component, should i file SM bug "Port bug #NN to Seamonkey", or add SM patch to existing sessionstore component bug? Seamonkey Council - please advise.
Comment 43 Jens Hatlak (:InvisibleSmiley) 2009-02-13 15:49:05 PST
about:sessionrestore is not working for me with a current win32 tinderbox build. Could it be that some files are missing from the packages files (win/unix)? Probably nsSessionStartup.js and nsSessionStore.js (it works for stefanh and he has those but he's got a Mac which needs no packages file). Unfortunately I currently cannot build and verify that myself but I could come up with a patch if anyone confirms that it's indeed the above two files that are missing. OTOH I wouldn't feel offended if whoever confirms it comes up with a patch him/herself.

Take-away: don't copy dist/bin files, use the installer. :-)
Comment 44 Robert Kaiser (not working on stability any more) 2009-02-13 17:18:22 PST
(In reply to comment #42)
> Thanks Simon, i'll be very glad for more comments from you. You know this code
> better than anyone here. I'm sure that i have more things to fix.
> 
> Also, i want to make clear, how we can synchronize future bugfixes for
> sessionstore component, should i file SM bug "Port bug #NN to Seamonkey", or
> add SM patch to existing sessionstore component bug? Seamonkey Council - please
> advise.

For both cases, what Simon talks about as additional fixes on that code as well as further ports or improvements to our code, please file new bugs and try to do one bug/patch per feature so things are small enough that they can easily be reviewed. I actually even wonder if we should move the tests patch to a new bug as doing reviews on a fixed bug is not a too good idea.

We also should really look into bug 478506 soon, somehow we seem to be leaking something with session restore now.
Comment 45 Jens Hatlak (:InvisibleSmiley) 2009-02-14 03:01:46 PST
(In reply to comment #43)
> about:sessionrestore is not working for me with a current win32 tinderbox
> build. Could it be that some files are missing from the packages files
> (win/unix)? Probably nsSessionStartup.js and nsSessionStore.js (it works for
> stefanh and he has those but he's got a Mac which needs no packages file).

Obviously Neil fixed this in <http://hg.mozilla.org/comm-central/rev/dc10ce1e0345>.
Comment 46 Serge Gautherie (:sgautherie) 2009-02-14 09:43:39 PST
(In reply to comment #37)

Filed as bug 478470.
Comment 47 neil@parkwaycc.co.uk 2009-02-16 03:35:00 PST
Comment on attachment 362289 [details] [diff] [review]
updated tests

I think our QA lead should review our tests, in a bug of his choosing ;-)
Comment 48 Misak Khachatryan 2009-02-25 05:55:47 PST
Comment on attachment 362289 [details] [diff] [review]
updated tests

Tests moved to bug 480109.
Comment 49 Robert Kaiser (not working on stability any more) 2009-09-18 06:18:29 PDT
Adding fixed-seamonkey2.0 to make it go away from "unfixed wanted" query

Note You need to log in before you can comment on or make changes to this bug.