477657 - saner default handling for _closedTabs and sizemode

Status: RESOLVED FIXED

(Firefox :: Session Restore, defect)

Description

[Simon Bünzli]

If a session state containing window data without _closedTabs is passed to setBrowserState or setWindowState (or found in sessionstore.js), that window's _closedTabs list should be cleared.

Omitting a window's sizemode property should preserve the current state (and not restore to a non-maximized, non-minimized state).

Comment 1

[Simon Bünzli]

Attachment: patch (without tests)

We should also clear window values (set with setWindowValue) when overwriting a window's state with setWindowState. Tests follow as soon as I (or anybody else) find time for writing them.

Comment 2

[(no longer active)]

Simon, part of this patch is no longer needed with bug 476463 landed...

Comment 3

[Simon Bünzli]

Attachment: patch and tests

This includes a test for the bits already checked in in bug 476463.

Comment 4

[Dietrich Ayala (:dietrich)]

Comment on attachment 363420 [details] [diff] [review]
patch and tests

looks fine, r=me

Comment 5

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/6fa6667b903f

Comment 6

[Mike Beltzner [:beltzner, not reading bugmail]]

This test is failing on trunk right now:

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1236027288.1236033209.11742.gz

TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/browser/components/sessionstore/test/browser/browser_477657.js | the window was maximized - Got 3, expected 1

There's a chance that nobody noticed because of the recurring focus-related oranges until now.

I suggest we back this out.

Comment 7

[Dão Gottwald [:dao]]

backed out

Comment 8

[Simon Bünzli]

Attachment: fixed test

I was so focused on using executeSoon instead of setTimeout that I managed to miss the fact that we need a setTimeout here after all: to mirror the setTimeout in nsSessionStore.js's sss_restoreWindowFeatures.

Comment 9

[Dão Gottwald [:dao]]

(In reply to comment #8)
> mirror the setTimeout in nsSessionStore.js's sss_restoreWindowFeatures.

Won't that be fragile?

Comment 10

[Simon Bünzli]

(In reply to comment #9)
> Won't that be fragile?

As long as these timeouts are queued on the same thread, they'll always happen in the same order.

Comment 11

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/841e2745a834

Comment 12

[Dão Gottwald [:dao]]

This failed today.

on mac:
the window was explicitly unmaximized - Didn't expect 1, but got it

and windows:
the window was maximized - Got 3, expected 1