593681 - Small fixes to keep sessionstore in sync with FF

Status: RESOLVED FIXED

(SeaMonkey :: Session Restore, defect)

Description

[Misak Khachatryan]

Attachment: fix

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

Comment 1

[Misak Khachatryan]

Comment on attachment 472260 [details] [diff] [review]
fix

Sorry for syntax error.

Comment 2

[[e-mail]]

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.

Comment 3

[Misak Khachatryan]

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.

Comment 4

[[e-mail]]

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

Comment 5

[[e-mail]]

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

Comment 6

[Misak Khachatryan]

Attachment: fix v1.1

I renamed savedBrowsers to _savedBrowsers, but didn't touched others yet, and include all keys except browser and history to blacklist.

Comment 7

[Misak Khachatryan]

Attachment: v 1.2

added browser and history to blacklist.

Comment 8

[[e-mail]]

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?]

Comment 9

[Misak Khachatryan]

Attachment: v 1.3

renames tabb, browser and history to _tab, _browser and history.

As always, my English understanding failed again. Hope this is right one.

Comment 10

[[e-mail]]

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?

Comment 11

[Misak Khachatryan]

Attachment: v 1.3a

with added deletion of aTabData._tab

Comment 12

[[e-mail]]

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

Comment 13

[Misak Khachatryan]

Pushed with nit fixed:
http://hg.mozilla.org/comm-central/rev/6f8326fad3a4