Closed
Bug 904003
Opened 11 years ago
Closed 11 years ago
[Session Restore] Move session saving code into its own module
Categories
(Firefox :: Session Restore, defect)
Firefox
Session Restore
Tracking
()
RESOLVED
FIXED
Firefox 26
People
(Reporter: ttaubert, Assigned: ttaubert)
References
(Blocks 1 open bug)
Details
Attachments
(1 file)
25.99 KB,
patch
|
Yoric
:
review+
|
Details | Diff | Splinter Review |
We should move saveState(), saveStateDelayed(), saveStateObject() and all their logic into its own module.
Assignee | ||
Comment 1•11 years ago
|
||
Assignee: nobody → ttaubert
Attachment #788861 -
Flags: review?(dteller)
Comment 2•11 years ago
|
||
Comment on attachment 788861 [details] [diff] [review] Move session saving code to SessionSaver.jsm Review of attachment 788861 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/components/sessionstore/src/SessionSaver.jsm @@ +65,5 @@ > +let stopWatchFinish = stopWatch("finish"); > + > +/** > + * The external API implemented by the SessionSaver module. > + */ If it's an external API, I would suggest documenting it. @@ +93,5 @@ > + * The internal API. > + */ > +let SessionSaverInternal = { > + _timeoutID: null, > + _lastSaveTime: 0, Nit: A little documentation for both fields? @@ +107,5 @@ > + * Saves the current session to disk delayed by a given amount of time. Should > + * another delayed run be scheduled already, we will ignore the given delay > + * and state saving may occur a little earlier. > + */ > + runDelayed: function (delay = 2000) { Nit: Could you document that it's a number of milliseconds? Also, since this is a public API, could you either check that |delay| is a number and that it's >= 0 or specify the behavior if it's <0? @@ +144,5 @@ > + clearTimeout(this._timeoutID); > + this._timeoutID = null; > + }, > + > + _saveState: function (updateAll = false) { Could you take the opportunity to rename |updateAll| to something more meaningful? @@ +148,5 @@ > + _saveState: function (updateAll = false) { > + // Cancel any pending timeouts or just clear > + // the timeout if this is why we've been called. > + clearTimeout(this._timeoutID); > + this._timeoutID = null; You might just call this.cancel(). @@ +215,5 @@ > + let data = JSON.stringify(state); > + stopWatchFinish("SERIALIZE_DATA_MS", "SERIALIZE_DATA_LONGEST_OP_MS"); > + > + // Give observers a chance to modify session data. > + data = this._notifyObserversBeforeStateWrite(data); Just give me the word and I'll break that :) ::: browser/components/sessionstore/src/SessionStore.jsm @@ +3602,5 @@ > > /* ........ Disk Access .............. */ > > /** > + * save state delayed Well, since you're changing that title, you might as well make it understandable :)
Attachment #788861 -
Flags: review?(dteller) → review+
Updated•11 years ago
|
Summary: Move session saving code into its own module → [Session Restore] Move session saving code into its own module
Assignee | ||
Comment 3•11 years ago
|
||
(In reply to David Rajchenbach Teller [:Yoric] from comment #2) > > +/** > > + * The external API implemented by the SessionSaver module. > > + */ > > If it's an external API, I would suggest documenting it. I thought I'd rather not duplicate the documentation as it's the same for the internal API but it doesn't really hurt. > > + runDelayed: function (delay = 2000) { > > Nit: Could you document that it's a number of milliseconds? > Also, since this is a public API, could you either check that |delay| is a > number and that it's >= 0 or specify the behavior if it's <0? Will document. I added "0" to the Math.max() argument list to let delay not fall below zero. Also, it's not a public API, the delay argument is only available to callers that have access to SessionSaverInternal so I think we shouldn't have too many checks here. > > + _saveState: function (updateAll = false) { > > Could you take the opportunity to rename |updateAll| to something more > meaningful? Yeah, good idea. > > + clearTimeout(this._timeoutID); > > + this._timeoutID = null; > > You might just call this.cancel(). Right, will do. > > + // Give observers a chance to modify session data. > > + data = this._notifyObserversBeforeStateWrite(data); > > Just give me the word and I'll break that :) I'm looking forward. We'll need to wait for bug 899213, though. > > /** > > + * save state delayed > > Well, since you're changing that title, you might as well make it > understandable :) Indeed.
Assignee | ||
Comment 4•11 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/1cac48ea6d81
Whiteboard: [fixed-in-fx-team]
Comment 5•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/1cac48ea6d81
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 26
You need to log in
before you can comment on or make changes to this bug.
Description
•