558637 - Port Bug 528451 [sessionstore-browser-state-restored is not reliable regarding closed windows] to Seamonkey

Status: RESOLVED INVALID

(SeaMonkey :: Session Restore, defect)

Description

[Misak Khachatryan]

Attachment: patch

From parent bug:

sessionstore-browser-state-set fires after setBrowserState finishes, but at
that point windows are still closing, so the browser state is only partially
set.
PrivateBrowsing will wait for this notification before starting, and some test
do the same, so i think it should not lie, and fire only when the state has
been really set (all windows that should be open are open, all windows that
should be closed are closed).

I ran test on my Fedora 12, no failed test seen.

Comment 1

[[e-mail]]

Comment on attachment 438333 [details] [diff] [review]
patch

>-    // close all other browser windows
Was there a reason that this block was moved down?

>+    var self = this;
I'd prefer if you used var _closingWindows = this._closingWindows; and use _closingWindows.push(aWindow); in the inner function.

>+    if (this._restoreCount && !aOnWindowClose)
>       this._restoreCount--;
I don't like the _restoreCount check here, I think you should always decrement _restoreCount if aOnWindowClose isn't true.

Comment 2

[Misak Khachatryan]

Attachment: patch v2

Didn't found any reason moving that block too. Updated patch to address all comments.

Comment 3

[Misak Khachatryan]

Attachment: patch v3

Comment 4

[Misak Khachatryan]

Well, i don't know the magic behind moving that block, but one test fails if block stays on its previous position.

TEST-UNEXPECTED-FAIL |
chrome://mochikit/content/browser/browser/components/sessionstore/test/browser/browser_526613.js
| number of open browser windows according to getBrowserState - Got 3, expected
2

I CCed Marko to this bug, the author of original patch, maybe he can explain.
Moved block again.

Comment 5

[[e-mail]]

Comment on attachment 438367 [details] [diff] [review]
patch v3

>-    // close all other browser windows
So there is a reason to move the block?

Comment 6

[Misak Khachatryan]

Yes, but I can understand it. Maybe some timing thing ...

Comment 7

[Misak Khachatryan]

Sorry, can't understand.

Comment 8

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

the original patch in sessionstore has been removed by dao in a next change, afaik, so i'm not sure you even need to port it

Comment 9

[Misak Khachatryan]

Thanks Marco, my bad, I already ported bug Bug 528776 before and forgotten :(