Closed Bug 593681 Opened 10 years ago Closed 9 years ago

Small fixes to keep sessionstore in sync with FF

Categories

(SeaMonkey :: Session Restore, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

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

References

Details

Attachments

(1 file, 4 obsolete files)

Attached patch fix (obsolete) — Splinter Review
While investigating one of failing tests I've found some little bugs, that i miss when ported relevant portions of FF code to SM.

This incorporates:

Port of bug 551712 - http://hg.mozilla.org/mozilla-central/rev/9219a34ee26c

Useful parts of Bug 387859 - http://hg.mozilla.org/mozilla-central/rev/e3372af81a76

Missed line from port of Bug 528776 - http://hg.mozilla.org/mozilla-central/rev/d1891791ecd2
Attachment #472260 - Flags: superreview?(neil)
Attachment #472260 - Flags: review?(neil)
Comment on attachment 472260 [details] [diff] [review]
fix

Sorry for syntax error.
Attachment #472260 - Attachment description: composide fix → fix
Comment on attachment 472260 [details] [diff] [review]
fix

>+      return (value instanceof Components.interfaces.nsISupports) || !(INTERNAL_KEYS.indexOf(key) == -1) ? undefined : value;
Nit: don't use !(... == -1), use INTERNAL_KEYS.indexOf(key) != -1

We should add the tabbrowser internal keys to the list of internal keys, this way we can remove the instanceof test.

You may want to rename the tabbrowser internal keys to give them a _ prefix.
I added and renamed _savedBrowsers, but there is more keys that i don't understand to where them belong. I tried to print key and it's values and also are they instance of Components.interfaces.nsISupports:

key is browser value is [object XULElement] instance true
key is _docShell value is [xpconnect wrapped (nsISupports, nsIDocShell, nsIInterfaceRequestor, nsIWebProgress, nsIWebNavigation, nsIDocShellHistory, nsIDocShellTreeItem)] instance true
key is securityUI value is [xpconnect wrapped nsISecureBrowserUI] instance true
key is sessionHistory value is [xpconnect wrapped (nsISupports, nsISHistoryInternal, nsISHistory)] instance true
key is rootTransaction value is [xpconnect wrapped nsISHTransaction] instance true
key is sHEntry value is [xpconnect wrapped nsISHEntry] instance true
key is URI value is [xpconnect wrapped nsIURI] instance true
key is history value is [xpconnect wrapped (nsISupports, nsISHistory, nsISHistoryInternal)] instance true
key is SHistoryEnumerator value is [xpconnect wrapped nsISimpleEnumerator] instance true

I need help to find out all of them.
(In reply to comment #3)
> I added and renamed _savedBrowsers, but there is more keys that i don't
> understand to where them belong. I tried to print key and it's values and also
> are they instance of Components.interfaces.nsISupports:
> 
> key is browser value is [object XULElement] instance true
> key is _docShell value is [xpconnect wrapped (nsISupports, nsIDocShell,
> nsIInterfaceRequestor, nsIWebProgress, nsIWebNavigation, nsIDocShellHistory, nsIDocShellTreeItem)] instance true
> key is securityUI value is [xpconnect wrapped nsISecureBrowserUI] instance true
> key is sessionHistory value is [xpconnect wrapped (nsISupports, nsISHistoryInternal, nsISHistory)] instance true
> key is rootTransaction value is [xpconnect wrapped nsISHTransaction] instance true
> key is sHEntry value is [xpconnect wrapped nsISHEntry] instance true
> key is URI value is [xpconnect wrapped nsIURI] instance true
> key is history value is [xpconnect wrapped (nsISupports, nsISHistory, nsISHistoryInternal)] instance true
> key is SHistoryEnumerator value is [xpconnect wrapped nsISimpleEnumerator] instance true
Out of these the only two I recognise are browser and history. The others (including savedBrowsers) shouldn't be getting enumerated.
(In reply to comment #4)
> Out of these the only two I recognise are browser and history. The others
> (including savedBrowsers) shouldn't be getting enumerated.
Bug 528582 adds tab.
Attached patch fix v1.1 (obsolete) — Splinter Review
I renamed savedBrowsers to _savedBrowsers, but didn't touched others yet, and include all keys except browser and history to blacklist.
Attachment #472260 - Attachment is obsolete: true
Attachment #482203 - Flags: review?(neil)
Attachment #472260 - Flags: superreview?(neil)
Attachment #472260 - Flags: review?(neil)
Attached patch v 1.2 (obsolete) — Splinter Review
added browser and history to blacklist.
Attachment #482203 - Attachment is obsolete: true
Attachment #485696 - Flags: review?(neil)
Attachment #482203 - Flags: review?(neil)
Comment on attachment 485696 [details] [diff] [review]
v 1.2

>-      <field name="savedBrowsers">
>+      <field name="_savedBrowsers">
...
>+// These keys are for internal use only - they shouldn't be part of the JSON
>+// that gets saved to disk nor part of the strings returned by the API.
>+const INTERNAL_KEYS = ["_tabStillLoading", "_hosts", "_formDataSaved",
>+                       "tab", "browser", "history"
Sorry, I didn't mean for you to rename savedBrowsers, only tab/browser/history.

>+];
[Why on a separate line?]
Attached patch v 1.3 (obsolete) — Splinter Review
renames tabb, browser and history to _tab, _browser and history.

As always, my English understanding failed again. Hope this is right one.
Attachment #485696 - Attachment is obsolete: true
Attachment #485731 - Flags: review?(neil)
Attachment #485696 - Flags: review?(neil)
Comment on attachment 485731 [details] [diff] [review]
v 1.3

>           this.savedBrowsers.forEach(function(aTabData) {
>-            delete aTabData.browser;
>-            delete aTabData.history;
>+            delete aTabData._browser;
>+            delete aTabData._history;
>           });
Oops, I forgot to delete aTabData._tab, would you mind adding it in?
Attachment #485731 - Flags: review?(neil) → review+
Attached patch v 1.3aSplinter Review
with added deletion of aTabData._tab
Attachment #485731 - Attachment is obsolete: true
Attachment #485744 - Flags: superreview?(neil)
Attachment #485744 - Flags: review+
Comment on attachment 485744 [details] [diff] [review]
v 1.3a

>+            delete aTabData._browser;
>+            delete aTabData._history;
>+            delete aTabData._tab;
[Nit: everywhere else the order is _tab _browser _history for some reason.]
Attachment #485744 - Flags: superreview?(neil) → superreview+
Pushed with nit fixed:
http://hg.mozilla.org/comm-central/rev/6f8326fad3a4
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.