Closed Bug 904003 Opened 7 years ago Closed 7 years ago

[Session Restore] Move session saving code into its own module

Categories

(Firefox :: Session Restore, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 26

People

(Reporter: ttaubert, Assigned: ttaubert)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

We should move saveState(), saveStateDelayed(), saveStateObject() and all their logic into its own module.
Assignee: nobody → ttaubert
Attachment #788861 - Flags: review?(dteller)
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+
Summary: Move session saving code into its own module → [Session Restore] Move session saving code into its own module
(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.
https://hg.mozilla.org/mozilla-central/rev/1cac48ea6d81
Status: ASSIGNED → RESOLVED
Closed: 7 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.