344860 - [SessionStore] Minor code cleanup

Status: VERIFIED FIXED

(Firefox :: Session Restore, defect)

Description

[Simon Bünzli]

Some details I've noticed while going through the code...

Comment 1

[Simon Bünzli]

Attachment: clean up

Mainly white space nits, removing some obsolete comments of mine, lower-casing Cookies, removing the unneeded _readFile function and merging FileWriter into _writeFile (even if we want threaded IO back, I'd rather use it from saveFile so that the call to toSource can be moved into the thread as well).

Comment 2

[Dietrich Ayala (:dietrich)]

Comment on attachment 229400 [details] [diff] [review]
clean up

This looks good, thanks very much Simon.

Comment 3

[Dietrich Ayala (:dietrich)]

gavin_: dietrich: are you aware that Components.returnCode currently doesn't work, due to bug 287107?

Removing checkin note. Maybe create a const to use for now, and change it's value later once 287107 is fixed?

Comment 4

[:Gavin Sharp [email: [e-mail]]]

I actually wasn't referring to this bug specifically when I said that - the session store component uses Components.returnCode in a bunch of places already. The fact that it's broken just means that those methods never throw.

Comment 5

[Simon Bünzli]

Please go ahead with the checkin. If you want that we also throw errors as long as bug 287107 is pending, please file a new bug (either for SessionStore or for all callers).

Comment 6

[Dietrich Ayala (:dietrich)]

Checked-in on trunk.

Comment 7

[Simon Bünzli]

Attachment: more clean up

More whitespace nits (there'd be many more: blank lines vs. space-only lines - should I fix these, too?), getting rid of the pointless _getSessionFile (Crash Recovery didn't cache the files, SessionStore does), of the unused _sessionFileBackup in nsSessionStartup and of most _loadState specifics in nsSessionStartup.

BTW: If you don't mind, I'll keep this bug open at least until beta 2 so that I don't have to file a new bug for each detail.

Comment 8

[Simon Bünzli]

Attachment: more clean up

The same as above and additionally some more references to .INI files removed.

Comment 9

[Simon Bünzli]

Comment on attachment 229400 [details] [diff] [review]
clean up

Drivers: These patches don't change any functionality and are thus of low risk.

Comment 10

[Mike Connor [:mconnor]]

Comment on attachment 229400 [details] [diff] [review]
clean up

a=mconnor on behalf of drivers for 1.8.1 checkin

Comment 11

[Dietrich Ayala (:dietrich)]

first patch checked-in on branch.

Comment 12

[Simon Bünzli]

Attachment: more clean up (unbitrotted)

Dietrich: Could you please review these changes in a free minute? Or if you know that you won't get any any time soon, please tell me so (and I'll file two important bits of this clean up apart).

Comment 13

[Dietrich Ayala (:dietrich)]

Comment on attachment 236437 [details] [diff] [review]
more clean up (unbitrotted)

The changes look a-ok. I ran through a series of functional tests as well, and didn't find any problems.

Comment 14

[Simon Bünzli]

Thanks.

Comment 15

[Pablo Greco]

Simon, I've been getting "SessionStore: TypeError: aBrowser.currentURI has no properties" while middle-clicking last tab on a window. Should I file a bug on that (didn't find it), or you can check it here. Happens on branch and trunk versions:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1b2) Gecko/20060902 BonEcho/2.0b2
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060901 Minefield/3.0a1

Comment 16

[Adam Guthrie]

Pablo, yes, please do file a bug for that error.

Comment 17

[Simon Bünzli]

(In reply to comment #15)
I've filed bug 351189 for that issue. Thanks for reporting it.

Comment 18

[Simon Bünzli]

Comment on attachment 236437 [details] [diff] [review]
more clean up (unbitrotted)

Drivers: This low-risk patch (1) removes unneeded code, (2) reduces the privileges of the sandbox used to parse the session state at startup and (3) ensures that the session state backup created after crashes is always removed when the user clears all history data.

Comment 19

[Mike Schroepfer]

Comment on attachment 236437 [details] [diff] [review]
more clean up (unbitrotted)

a=schrep for drivers based on parts 2 and 3 of comment 18 since this addresses a few issues with session restore.