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

Status: RESOLVED FIXED

(Firefox :: Session Restore, defect)

Description

[Tim Taubert [:ttaubert] (inactive)]

We should move saveState(), saveStateDelayed(), saveStateObject() and all their logic into its own module.

Comment 1

[Tim Taubert [:ttaubert] (inactive)]

Attachment: Move session saving code to SessionSaver.jsm

Comment 2

[David Teller [:Yoric] - still alive but not very active]

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 :)

Comment 3

[Tim Taubert [:ttaubert] (inactive)]

(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.

Comment 4

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/integration/fx-team/rev/1cac48ea6d81

Comment 5

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1cac48ea6d81