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.
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.
Created attachment 438362 [details] [diff] [review] patch v2 Didn't found any reason moving that block too. Updated patch to address all comments.
Created attachment 438367 [details] [diff] [review] patch v3
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 on attachment 438367 [details] [diff] [review] patch v3 >- // close all other browser windows So there is a reason to move the block?
Yes, but I can understand it. Maybe some timing thing ...
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
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.