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

RESOLVED INVALID

Status

SeaMonkey
Session Restore
RESOLVED INVALID
8 years ago
8 years ago

People

(Reporter: Misak Khachatryan, Assigned: Misak Khachatryan)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

2.89 KB, patch
Details | Diff | Splinter Review
(Assignee)

Description

8 years ago
Created attachment 438333 [details] [diff] [review]
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.
Attachment #438333 - Flags: superreview?(neil)
Attachment #438333 - Flags: review?(neil)

Comment 1

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

Comment 2

8 years ago
Created attachment 438362 [details] [diff] [review]
patch v2

Didn't found any reason moving that block too. Updated patch to address all comments.
Attachment #438333 - Attachment is obsolete: true
Attachment #438362 - Flags: superreview?(neil)
Attachment #438362 - Flags: review?(neil)
Attachment #438333 - Flags: superreview?(neil)
Attachment #438333 - Flags: review?(neil)

Updated

8 years ago
Attachment #438362 - Flags: superreview?(neil)
Attachment #438362 - Flags: superreview+
Attachment #438362 - Flags: review?(neil)
Attachment #438362 - Flags: review+
(Assignee)

Comment 3

8 years ago
Created attachment 438367 [details] [diff] [review]
patch v3
Attachment #438362 - Attachment is obsolete: true
Attachment #438367 - Flags: superreview?(neil)
Attachment #438367 - Flags: review?(neil)
(Assignee)

Comment 4

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

8 years ago
Comment on attachment 438367 [details] [diff] [review]
patch v3

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

Comment 6

8 years ago
Yes, but I can understand it. Maybe some timing thing ...
(Assignee)

Comment 7

8 years ago
Sorry, can't understand.
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
(Assignee)

Updated

8 years ago
Attachment #438367 - Flags: superreview?(neil)
Attachment #438367 - Flags: review?(neil)
(Assignee)

Comment 9

8 years ago
Thanks Marco, my bad, I already ported bug Bug 528776 before and forgotten :(
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → INVALID
You need to log in before you can comment on or make changes to this bug.