Port Bug 448976 (turn the Session Restore prompt into an error page) to SeaMonkey

RESOLVED FIXED in seamonkey2.0a3

Status

SeaMonkey
Session Restore
RESOLVED FIXED
9 years ago
8 years ago

People

(Reporter: Misak Khachatryan, Assigned: Misak Khachatryan)

Tracking

({fixed-seamonkey2.0})

Trunk
seamonkey2.0a3
fixed-seamonkey2.0
Dependency tree / graph
Bug Flags:
wanted-seamonkey2.0 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 13 obsolete attachments)

962 bytes, patch
Robert Kaiser
: review+
Details | Diff | Splinter Review
74.05 KB, patch
neil@parkwaycc.co.uk
: review+
neil@parkwaycc.co.uk
: superreview+
Details | Diff | Splinter Review
609 bytes, patch
Details | Diff | Splinter Review
(Assignee)

Description

9 years ago
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.
Flags: wanted-seamonkey2?
(Assignee)

Updated

9 years ago
Depends on: 36810
(Assignee)

Comment 1

9 years ago
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).
Attachment #342810 - Flags: review?(ajschult)
(Assignee)

Comment 2

9 years ago
Created attachment 342856 [details] [diff] [review]
patch v2

css tweaks, almost final.
Attachment #342810 - Attachment is obsolete: true
Attachment #342856 - Flags: ui-review?(neil)
Attachment #342856 - Flags: review?(ajschult)
Attachment #342810 - Flags: review?(ajschult)
(Assignee)

Updated

9 years ago
Depends on: 448976

Updated

9 years ago
Attachment #342856 - Flags: ui-review?(neil) → ui-review-

Comment 3

9 years ago
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.
(Assignee)

Comment 4

9 years ago
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.
Attachment #342856 - Attachment is obsolete: true
Attachment #343235 - Flags: ui-review?(neil)
Attachment #343235 - Flags: review?(ajschult)
Attachment #342856 - Flags: review?(ajschult)
(Assignee)

Comment 5

9 years ago
>>+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

9 years ago
(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 ...;
}
(Assignee)

Comment 7

9 years ago
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.
Attachment #343235 - Attachment is obsolete: true
Attachment #349630 - Flags: review?(ajschult)
Attachment #343235 - Flags: ui-review?(neil)
Attachment #343235 - Flags: review?(ajschult)
(Assignee)

Comment 8

9 years ago
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
Attachment #349630 - Attachment is obsolete: true
Attachment #354150 - Flags: review?(ajschult)
Attachment #349630 - Flags: review?(ajschult)
(Assignee)

Comment 9

8 years ago
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.
Attachment #354150 - Flags: ui-review?(neil)
(Assignee)

Comment 10

8 years ago
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 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.
(Assignee)

Comment 12

8 years ago
Created attachment 360083 [details] [diff] [review]
patch v 4.1

Some fixes in Sessionstore API, patch against latest patch from bug 36810
Attachment #354150 - Attachment is obsolete: true
Attachment #360083 - Flags: ui-review?(neil)
Attachment #360083 - Flags: superreview?(neil)
Attachment #360083 - Flags: review?(ajschult)
Attachment #354150 - Flags: ui-review?(neil)
Attachment #354150 - Flags: review?(ajschult)
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 ;-)
(Assignee)

Comment 14

8 years ago
Created attachment 360113 [details] [diff] [review]
v 4.1 with missing file included

Sorry, missed file now in the patch.
Attachment #360083 - Attachment is obsolete: true
Attachment #360113 - Flags: ui-review?(neil)
Attachment #360113 - Flags: superreview?(neil)
Attachment #360113 - Flags: review?(ajschult)
Attachment #360083 - Flags: ui-review?(neil)
Attachment #360083 - Flags: superreview?(neil)
Attachment #360083 - Flags: review?(ajschult)
(Assignee)

Comment 15

8 years ago
(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.
(Assignee)

Comment 16

8 years ago
Created attachment 360264 [details] [diff] [review]
v 4.2

Fixed most nits, added missed file, also contains fixes for bug 447951, and bug 476161.
Attachment #360113 - Attachment is obsolete: true
Attachment #360264 - Flags: ui-review?(neil)
Attachment #360264 - Flags: superreview?(neil)
Attachment #360264 - Flags: review?(ajschult)
Attachment #360113 - Flags: ui-review?(neil)
Attachment #360113 - Flags: superreview?(neil)
Attachment #360113 - Flags: review?(ajschult)

Comment 17

8 years ago
(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);
}
(Assignee)

Comment 21

8 years ago
Created attachment 360327 [details] [diff] [review]
v 4.3

All comments done, only thing left is modern css. Taking another review respin.
Attachment #360264 - Attachment is obsolete: true
Attachment #360327 - Flags: ui-review?(neil)
Attachment #360327 - Flags: superreview?(neil)
Attachment #360327 - Flags: review?(ajschult)
Attachment #360264 - Flags: ui-review?(neil)
Attachment #360264 - Flags: superreview?(neil)
Attachment #360264 - Flags: review?(ajschult)
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.
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.
(Assignee)

Comment 25

8 years ago
(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.
(Assignee)

Comment 26

8 years ago
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 ;) .
Attachment #360327 - Attachment is obsolete: true
Attachment #360486 - Flags: ui-review?(neil)
Attachment #360486 - Flags: superreview?(neil)
Attachment #360486 - Flags: review?(ajschult)
Attachment #360327 - Flags: ui-review?(neil)
Attachment #360327 - Flags: superreview?(neil)
Attachment #360327 - Flags: review?(ajschult)

Updated

8 years ago
Attachment #360367 - Flags: review?(iann_bugzilla)
Comment on attachment 360486 [details] [diff] [review]
v 4.4, comments fixed

OK, this works well with attachment 360367 [details] [diff] [review].
Attachment #360486 - Flags: ui-review?(neil)
Attachment #360486 - Flags: ui-review+
Attachment #360486 - Flags: superreview?(neil)
Attachment #360486 - Flags: superreview+
(Assignee)

Comment 28

8 years ago
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.
Attachment #360001 - Flags: superreview?(neil)
Attachment #360001 - Flags: review?(ajschult)

Updated

8 years ago
Attachment #360367 - Flags: review?(iann_bugzilla) → review+

Comment 29

8 years ago
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.

Updated

8 years ago
Flags: wanted-seamonkey2? → wanted-seamonkey2+

Comment 31

8 years ago
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.
Attachment #360486 - Flags: review?(iann_bugzilla)

Updated

8 years ago
Assignee: nobody → misak
Target Milestone: seamonkey2.0 → seamonkey2.0a3
(Assignee)

Comment 32

8 years ago
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.
Attachment #360486 - Attachment is obsolete: true
Attachment #361749 - Flags: superreview?(neil)
Attachment #361749 - Flags: review?
Attachment #360486 - Flags: review?(iann_bugzilla)
Attachment #360486 - Flags: review?(ajschult)
(Assignee)

Updated

8 years ago
Attachment #361749 - Flags: review? → review?(iann_bugzilla)
(Assignee)

Updated

8 years ago
Attachment #360001 - Flags: superreview?(neil)
Attachment #360001 - Flags: review?(iann_bugzilla)
Attachment #360001 - Flags: review?(ajschult)
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.
Attachment #361749 - Flags: superreview?(neil) → superreview-
(Assignee)

Comment 34

8 years ago
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.
Attachment #361749 - Attachment is obsolete: true
Attachment #361771 - Flags: superreview?(neil)
Attachment #361771 - Flags: review?(iann_bugzilla)
Attachment #361749 - Flags: review?(iann_bugzilla)

Updated

8 years ago
Attachment #361771 - Flags: superreview?(neil) → superreview+

Comment 35

8 years ago
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.
Attachment #361771 - Flags: review?(neil)

Updated

8 years ago
Attachment #361771 - Flags: review?(neil) → review+

Comment 36

8 years ago
http://hg.mozilla.org/comm-central/rev/1cba9a3b59cb
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED

Comment 37

8 years ago
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.
(Assignee)

Comment 38

8 years ago
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.
(Assignee)

Comment 39

8 years ago
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
Attachment #360001 - Attachment is obsolete: true
Attachment #362289 - Flags: review?(neil)
Attachment #360001 - Flags: review?(iann_bugzilla)

Comment 40

8 years ago
Comment on attachment 362286 [details] [diff] [review]
fix accidentally added line (pushed)

I landed this as a bustage fix (changeset ee8599c65f12)
Attachment #362286 - Attachment description: fix accidentally added line → fix accidentally added line (pushed)

Comment 41

8 years ago
Comment on attachment 360367 [details] [diff] [review]
Fix for netError.css (pushed)

Landed this as 9930c42f3ccf (OK from Neil)
Attachment #360367 - Attachment description: Fix for netError.css → Fix for netError.css (pushed)

Updated

8 years ago
Attachment #361771 - Attachment description: v 4.6 → v 4.6 (pushed)
Attachment #361771 - Flags: review?(iann_bugzilla)
(Assignee)

Comment 42

8 years ago
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.

Updated

8 years ago
Attachment #362289 - Attachment is patch: true
Attachment #362289 - Attachment mime type: application/octet-stream → text/plain
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. :-)
Depends on: 478506

Comment 44

8 years ago
(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>.
Depends on: 478470
(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 ;-)
Attachment #362289 - Flags: review?(neil) → review?(ajschult)
(Assignee)

Updated

8 years ago
Blocks: 480109
(Assignee)

Updated

8 years ago
Blocks: 480110
(Assignee)

Comment 48

8 years ago
Comment on attachment 362289 [details] [diff] [review]
updated tests

Tests moved to bug 480109.
Attachment #362289 - Attachment is obsolete: true
Attachment #362289 - Flags: review?(ajschult)

Updated

8 years ago
Blocks: 479992

Updated

8 years ago
Component: UI Design → Session Restore
QA Contact: ui-design → session.restore

Comment 49

8 years ago
Adding fixed-seamonkey2.0 to make it go away from "unfixed wanted" query
Keywords: fixed-seamonkey2.0
You need to log in before you can comment on or make changes to this bug.