938248 - [Session Restore] Move GlobalState from SessionStore.jsm to its own JSM

Status: RESOLVED FIXED

(Firefox :: Session Restore, defect)

Description

[Steven MacLeod [:smacleod]]

Bug 899213 introduces an API to store global session data. The GlobalState object in SessionStore.jsm takes care of storing this global data, and providing the interface for SesssionStore to retrieve the data to be saved.

It would be nice to move GlobalState out of SessionStore.jsm and into its own JSM to make things more maintainable.

Comment 1

[Peiyong Lin[:lpy](UTC+8)]

Hi Steven. If I move the |GlobalState| object from http://mxr.mozilla.org/mozilla-central/source/browser/components/sessionstore/src/SessionStore.jsm#4105 to another JSM file, how could I expose itself to SessionStore.jsm? Any document I should read before I start to work on it?

Comment 2

[Steven MacLeod [:smacleod]]

(In reply to Peiyong Lin[:lpy] from comment #1)
> Hi Steven. If I move the |GlobalState| object from
> http://mxr.mozilla.org/mozilla-central/source/browser/components/
> sessionstore/src/SessionStore.jsm#4105 to another JSM file, how could I
> expose itself to SessionStore.jsm? Any document I should read before I start
> to work on it?

SessionStore.jsm can load the new module you'll be creating. If you look here http://mxr.mozilla.org/mozilla-central/source/browser/components/sessionstore/src/SessionStore.jsm#88 you can see a number of modules that SessionStore.jsm already imports.

You can read about JSMs here: https://developer.mozilla.org/en-US/docs/Mozilla/JavaScript_code_modules

Comment 3

[Steven MacLeod [:smacleod]]

Hi Peiyong. Are you still interested in working on this? Do you need anymore guidance getting started, or do you have any more questions?

Comment 4

[Peiyong Lin[:lpy](UTC+8)]

Attachment: bug938248.patch

Hi Steven, I am so sorry for delay.

Comment 5

[Steven MacLeod [:smacleod]]

Comment on attachment 8347588 [details] [diff] [review]
bug938248.patch

Review of attachment 8347588 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good but I think since we're moving this out to a JSM we need to protect things a bit more (With this code, something could grab a reference to GlobalState.state and mess with it).

How about in GlobalState.jsm we only expose a constructor which returns a new instance of the GlobalState object. So, inside SessionStore.jsm we'd have something like |let GlobalState = new GlobalState()|. This would allow us to still expose |GlobalState.state| from the returned object so SessionStore doesn't have to copy it, but would protect the data inside SessionStore from outside modification.

We should also call |Object.freeze()| on the constructor we'll expose in |GlobalState.jsm| so it can't be modified.

Let me know if you have any questions!

Comment 6

[Peiyong Lin[:lpy](UTC+8)]

Attachment: bug938248-V2.patch

Thank you :)

Comment 7

[Steven MacLeod [:smacleod]]

Comment on attachment 8349358 [details] [diff] [review]
bug938248-V2.patch

Review of attachment 8349358 [details] [diff] [review]:
-----------------------------------------------------------------

Looking good, just a few more things to change :D

::: browser/components/sessionstore/src/GlobalState.jsm
@@ +8,5 @@
> +
> +/**
> + * Module that contains global session data.
> + */
> +this.GlobalState = function() {

Instead of defining all of the methods inside the constructor, they should be defined in the prototype.

Here is a great example of a JSM which provides a constructor as the interface:
http://mxr.mozilla.org/mozilla-central/source/browser/components/sessionstore/src/FrameTree.jsm

::: browser/components/sessionstore/src/SessionStore.jsm
@@ +142,5 @@
>   * |true| if we are in debug mode, |false| otherwise.
>   * Debug mode is controlled by preference browser.sessionstore.debug
>   */
>  let gDebuggingEnabled = false;
> +let gGlobalState = new GlobalState();

Instead of making this a global variable, lets make it an attribute of SessionStoreInternal. Put something like |_globalState: new GlobalState(),| in the object literal definition of SessionStoreInternal.

Comment 8

[Peiyong Lin[:lpy](UTC+8)]

Attachment: bug938248-V3.patch

patch updated  :)

Comment 9

[Steven MacLeod [:smacleod]]

Comment on attachment 8350668 [details] [diff] [review]
bug938248-V3.patch

Review of attachment 8350668 [details] [diff] [review]:
-----------------------------------------------------------------

Looking great, just one last thing. Also, after updating could you push this to try and post a link here?

::: browser/components/sessionstore/src/GlobalState.jsm
@@ +15,5 @@
> +  this.state = {};
> +  let internal = new GlobalStateInternal();
> +  let external = {};
> +  for (let method of EXPORTED_METHODS) {
> +    external[method] = internal[method].bind(this);

It's a little strange that we're binding the internal methods to the external object. Lets change this and make the |state| property part of GlobalStateInternal, then allow access to it from a new simple method.

Comment 10

[Peiyong Lin[:lpy](UTC+8)]

Attachment: bug938248-V4.patch

push to try, https://tbpl.mozilla.org/?tree=Try&rev=b9026e4d905e

Comment 11

[Steven MacLeod [:smacleod]]

Comment on attachment 8358813 [details] [diff] [review]
bug938248-V4.patch

Review of attachment 8358813 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the patch! :D

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/23f54599d93a

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/23f54599d93a