Cleanup SeaMonkey sessionstore code a bit.

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

Trunk
seamonkey2.0a3
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

9 years ago
Zeniko suggested some things to do for SM sessionstore:

Two nits for a follow-up:

>+    //this is firefox part of code, keeping it for reference only. Will be removed in final patch
>+    //misak if (winData._closedTabs && (root._firstTabs || aOverwriteTabs)) {

Wasn't removed.

>+    // restore text data saved by SeaMonkey

/SeaMonkey/Firefox 2.0\/3.0/ (that's our own legacy format which you might not
even need to support).

(From update of attachment 361771 [details] [diff] [review])
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 1

9 years ago
Created attachment 362393 [details] [diff] [review]
patch

suggested code cleanup.
Attachment #362393 - Flags: review?(neil)
(Assignee)

Comment 2

9 years ago
Created attachment 362394 [details] [diff] [review]
v 1.1

Oops, missed some eval's.
Attachment #362393 - Attachment is obsolete: true
Attachment #362394 - Flags: review?(neil)
Attachment #362393 - Flags: review?(neil)

Comment 3

9 years ago
Comment on attachment 362394 [details] [diff] [review]
v 1.1

>-      // duplicate of quit-application request. Seamonkey doesn't have these yet.

Does SeaMonkey have quit-application-granted after all?

>   _safeEval: function sss_safeEval(aStr) {
>+    return JSON.parse(aStr);
>   },

_safeEval no longer does what it claims to. As it stands, it'd be best to just replace all _safeEval calls with JSON.parse.

>+  _getSavedBrowsers: function sss_getSavedBrowsers(aWindow) {

_getClosedTabData ? You don't return references to <xul:browser>s...
(Assignee)

Comment 4

9 years ago
Created attachment 362407 [details] [diff] [review]
v 1.2

(In reply to comment #3)
> Does SeaMonkey have quit-application-granted after all?
> 

It's can be used, if you observe it, because it's generated by GECKO. But code here is up to Neil to decide.

> _safeEval no longer does what it claims to. As it stands, it'd be best to just
> replace all _safeEval calls with JSON.parse.
> 

Fixed.
> _getClosedTabData ? You don't return references to <xul:browser>s...

Fixed.


This patch also fixes leaks (Bug 478506), i don't know how.
Attachment #362394 - Attachment is obsolete: true
Attachment #362394 - Flags: review?(neil)
(Assignee)

Updated

9 years ago
Attachment #362407 - Flags: superreview?(neil)
Attachment #362407 - Flags: review?(neil)
Blocks: 478506, 36810, 459550
Flags: blocking-seamonkey2.0a3?
I noticed the following error after restarting SeaMonkey, I don't know if this is a cleanup or a separate issue.

Error: this.windowToFocus.content is null
Source File: file:///C:/Program%20Files/SeaMonkey/components/nsSessionStore.js
Line: 2125
I'm also see the following error a lot when non-browser windows are open:

Error: aWindow.getBrowser().savedBrowsers is undefined
Source File: file:///C:/Program%20Files/SeaMonkey/components/nsSessionStore.js
Line: 1505

Comment 7

9 years ago
Comment on attachment 362407 [details] [diff] [review]
v 1.2

>+  _getClosedTabData: function sss_getClosedTabData(aWindow) {
>+    return aWindow.getBrowser().savedBrowsers.map(function(e) { return e.tabData; });
>+  },
>+
You've changed all the callers but not the name of the function itself.

Updated

9 years ago
Assignee: nobody → misak
Component: General → UI Design
QA Contact: general → ui-design

Updated

9 years ago
Status: NEW → ASSIGNED
That could explain why calling the getClosedTabData() function returns a bunch of empty arrays.  I closed 3 tabs and then called getClosedTabData and this is what was returned:

(new String("[{},{},{}]"))
Actually it turns out getClosedTabData() is hopelessly broken because SeaMonkey doesn't store any tabData in gBrowser.saveBrowsers.  This is why they keep coming back as an empty array.  

I filed Bug 478579 to document this.
(Assignee)

Comment 10

9 years ago
Created attachment 362454 [details] [diff] [review]
v 1.3

Fixed confusing naming and all callers.
Attachment #362407 - Attachment is obsolete: true
Attachment #362454 - Flags: review?(neil)
Attachment #362407 - Flags: superreview?(neil)
Attachment #362407 - Flags: review?(neil)
(Assignee)

Comment 11

9 years ago
(In reply to comment #5)
> I noticed the following error after restarting SeaMonkey, I don't know if this
> is a cleanup or a separate issue.
> 
> Error: this.windowToFocus.content is null
> Source File: file:///C:/Program%20Files/SeaMonkey/components/nsSessionStore.js
> Line: 2125

I didn't see that error on my console (i'm on Linux), but v 1.2 patch was completely broken. Can you retest? Sorry for bad patch.

BTW line 2125 is pointing to empty line on my local nsSessionStrore.js

Updated

9 years ago
Duplicate of this bug: 478579

Comment 13

9 years ago
Comment on attachment 362454 [details] [diff] [review]
v 1.3

>+    total._closedTabs = this._getClosedTabs(this._windows[aWindow.__SSi]);

This shouldn't work, as this._windows is a collection of window data, not DOMWindows. Unfortunately, we don't seem to have a test for detecting this, so far...

>+    if (winData._closedTabs && (root._firstTabs || aOverwriteTabs)) {
>+      this._windows[aWindow.__SSi]._closedTabs = winData._closedTabs;
>+    }

Have you already filed a bug about the bits right after this one not working yet (i.e. the list of recently closed tabs not being restored)?
Quick question, will this go in before SeaMonkey 2.0a3?  The reason I ask is that since this patch changes Seamonkey to use JSON.parse() to parse session while the version currently in the branch uses evalInSandbox their parameters are not compatible.  Any session data passed to nsSessionStore while the patch is applied cannot be enclosed in parenthesis, while it must be enclosed in parenthesis without the patch applied.

Basically I'm making some changes to SessionManager to better work with SeaMonkey and it can either with this patch or without it, but not both ways.


It would be kind of nice if the API functions could still take session data enclosed in parenthesis and then strip them off before calling JSON.parse(), but I can understand if that is not desired.

BTW, my development version of Session Manager that works with this patch applied is at http://downloads.mozdev.org/sessionmanager/session_manager_dev.xpi

Comment 15

9 years ago
(In reply to comment #14)
> while it must be enclosed in parenthesis without the patch applied.

The SessionStore API asks for JSON data, so you should never (have to) enclose anything in parentheses for neither SeaMonkey nor Firefox. Firefox happens to be lenient and also accepts uneval'd objects (which are enclosed in parens) whereas SeaMonkey won't.

Can't you just change your own callers to use native JSON where available (and fall back to uneval/.toSource where not - which will be Firefox 2 and 3.0)?

Comment 16

9 years ago
We should try to get this in for 2.0a3, yes, esp. as it fixes the leak and as it removes some eval(InSandbox) ugliness when there's a better way to do it. Also, it fixes a pref problem and changes the store format slightly if I see this correctly, so we surely should get this in before pushing it to the public.
Flags: blocking-seamonkey2.0a3? → blocking-seamonkey2.0a3+
(In reply to comment #15)
> Can't you just change your own callers to use native JSON where available (and
> fall back to uneval/.toSource where not - which will be Firefox 2 and 3.0)?

I had actually already changed my code to check for the availability of JSON and use it, but didn't put two and two together that Firefox 3.1 and up would take pure JSON strings.  That actually makes things really simple on my end as I don't need to check to see whether it's SeaMonkey or Firefox, I can simply check for JSON.  

I'll still need to wait for this patch to drop though before officially enabling SeaMonkey support in Session Manager

Updated

9 years ago
Attachment #362454 - Flags: review?(neil) → review+

Comment 18

9 years ago
Pushed as http://hg.mozilla.org/comm-central/rev/347c778f306e
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
What about the issue mentioned in comment #13?  

Namely that there is no way to modify the closedTabs for a window, mainly because they aren't technically maintained by SessionStore.  For example, the code I have in Session Manager to delete specific closed tabs from the Closed Tab list doesn't work even though no error is thrown.

Show a new bug be filed for that?

Updated

9 years ago
Target Milestone: seamonkey2.0 → seamonkey2.0a3

Comment 20

9 years ago
Michael, one bug per issue, please. And we don't intend to switch to a different solution for undo closed tabs at this time. You may need to implement dealing with those in a different way, if possible.
(Assignee)

Comment 21

9 years ago
(In reply to comment #13)
> This shouldn't work, as this._windows is a collection of window data, not
> DOMWindows. Unfortunately, we don't seem to have a test for detecting this, so
> far...
> 
> 
> Have you already filed a bug about the bits right after this one not working
> yet (i.e. the list of recently closed tabs not being restored)?

I think best i can do to get rid of this is to move restoreTab method to sessionstore and maintain _closedTabs instead of savedBrowsers, but use Seamonkey way of undo close tab. I need some investigation.  Bug 478707 filed.

Thanks Simon for your help, is it ok to put you in CC list of Seamonkey specific sessionstore bugs for while? Your comments are very valuable to me.
Flags: blocking-seamonkey2.0a3+ → blocking-seamonkey2.0a3?
Target Milestone: seamonkey2.0a3 → seamonkey2.0
(Assignee)

Updated

9 years ago
Flags: blocking-seamonkey2.0a3?

Comment 22

9 years ago
(In reply to comment #21)
> > This shouldn't work, as this._windows is a collection of window data, not
> > DOMWindows.

This means that your getWindowState API is still broken, which should start annoying Micheal any time soon... ;-)

> Thanks Simon for your help, is it ok to put you in CC list of Seamonkey
> specific sessionstore bugs for while? Your comments are very valuable to me.

Sure. Some of your bugs might be mine as well, anyway.

Updated

9 years ago
Target Milestone: seamonkey2.0 → seamonkey2.0a3
(In reply to comment #22)

> This means that your getWindowState API is still broken, which should start
> annoying Micheal any time soon... ;-)

And that would be today. :)

Calling getWindowState tends to throw an exception.  I filed bug 478768 documenting this problem
Depends on: 478768

Updated

8 years ago
Component: UI Design → Session Restore
QA Contact: ui-design → session.restore
You need to log in before you can comment on or make changes to this bug.