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.

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

css tweaks, almost final.

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.

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

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

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

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.

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

all sessionstore tests ported, some of them timing out because of different implementation of undo close tab functionality.

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.

Some fixes in Sessionstore API, patch against latest patch from bug 36810

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

Sorry, missed file now in the patch.

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

Fixed most nits, added missed file, also contains fixes for bug 447951, and bug 476161.

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

All comments done, only thing left is modern css. Taking another review respin.

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.

One of the problems with Modern is really in netError.css, I'll look into it.

With this patch, you can remove all the styles from aboutSessionStore.css except for the #tabList and the various treechildren styles.

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

Taking into account that u said Unix is the wrong way i've removed that part ;) .

Comment on attachment 360486 [details] [diff] [review] v 4.4, comments fixed OK, this works well with attachment 360367 [details] [diff] [review].

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

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

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

Removed tabs.undoclose.depth and replaced by browser.sessionstore.max_tabs_undo, cleaned up relevant code.

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

i've accidentally added one line in the v 4.6 patch, it should be removed.

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 on attachment 362286 [details] [diff] [review] fix accidentally added line (pushed) I landed this as a bustage fix (changeset ee8599c65f12)

Comment on attachment 360367 [details] [diff] [review] Fix for netError.css (pushed) Landed this as 9930c42f3ccf (OK from Neil)

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.

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

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

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

(In reply to comment #37) Filed as bug 478470.

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 on attachment 362289 [details] [diff] [review] updated tests Tests moved to bug 480109.

Adding fixed-seamonkey2.0 to make it go away from "unfixed wanted" query