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

RESOLVED FIXED in Firefox 29

Status

()

Firefox
Session Restore
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: smacleod, Assigned: lpy)

Tracking

Trunk
Firefox 29
x86
All
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [good first bug][mentor=smacleod][lang=js][qa-])

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

4 years ago
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.
(Assignee)

Comment 1

4 years ago
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?
(Reporter)

Comment 2

4 years ago
(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
(Reporter)

Comment 3

4 years ago
Hi Peiyong. Are you still interested in working on this? Do you need anymore guidance getting started, or do you have any more questions?
Flags: needinfo?(pylaurent1314)
(Assignee)

Comment 4

4 years ago
Created attachment 8347588 [details] [diff] [review]
bug938248.patch

Hi Steven, I am so sorry for delay.
Assignee: nobody → pylaurent1314
Attachment #8347588 - Flags: review?(smacleod)
Flags: needinfo?(pylaurent1314)
(Reporter)

Comment 5

4 years ago
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!
Attachment #8347588 - Flags: review?(smacleod) → feedback+
(Assignee)

Comment 6

4 years ago
Created attachment 8349358 [details] [diff] [review]
bug938248-V2.patch

Thank you :)
Attachment #8347588 - Attachment is obsolete: true
Attachment #8349358 - Flags: review?(smacleod)
(Reporter)

Comment 7

4 years ago
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.
Attachment #8349358 - Flags: review?(smacleod) → feedback+
(Assignee)

Comment 8

4 years ago
Created attachment 8350668 [details] [diff] [review]
bug938248-V3.patch

patch updated  :)
Attachment #8349358 - Attachment is obsolete: true
Attachment #8350668 - Flags: review?(smacleod)
(Reporter)

Comment 9

4 years ago
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.
Attachment #8350668 - Flags: review?(smacleod) → feedback+
(Assignee)

Comment 10

4 years ago
Created attachment 8358813 [details] [diff] [review]
bug938248-V4.patch

push to try, https://tbpl.mozilla.org/?tree=Try&rev=b9026e4d905e
Attachment #8350668 - Attachment is obsolete: true
Attachment #8358813 - Flags: review?(smacleod)
(Reporter)

Comment 11

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

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

Thanks for the patch! :D
Attachment #8358813 - Flags: review?(smacleod) → review+
(Reporter)

Updated

4 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/23f54599d93a
Keywords: checkin-needed
Whiteboard: [good first bug][mentor=smacleod][lang=js] → [good first bug][mentor=smacleod][lang=js][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/23f54599d93a
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Whiteboard: [good first bug][mentor=smacleod][lang=js][fixed-in-fx-team] → [good first bug][mentor=smacleod][lang=js]
Target Milestone: --- → Firefox 29

Updated

4 years ago
Whiteboard: [good first bug][mentor=smacleod][lang=js] → [good first bug][mentor=smacleod][lang=js][qa-]
You need to log in before you can comment on or make changes to this bug.