745040 - Move the Session Store Service to a module

Status: RESOLVED FIXED

(Firefox :: Session Restore, defect)

Description

[Andres Hernandez [:andreshm]]

The idea is to remove the restriction to access the methods and data for other modules, because of the idl file. 

Paul suggest to make the service dumb and move everything to a module. The service could just call into the module. 
Or, just export to a module when needed, like creating a Privacy module, to handle the method '_checkPrivacyLevel' needed in other modules.

Please, correct and confirm what is the best way to approach this.

Comment 1

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Andres, I didn't get back to you by Monday as promised, but here's my answer:

We'll move the whole service out and the module would get lazy loaded by the service when needed. The module should only expose what's needed and we can very carefully expose more as needed.

Comment 2

[Andres Hernandez [:andreshm]]

Attachment: Patch v1

Please apply first the patches from bugs 697903, 742051 and 742047.

Comment 3

[Andres Hernandez [:andreshm]]

Attachment: Patch v2

Patch v2 is the same as patch v1 but just applying bug 697903 and bug 742051, without patch on bug 742047.

Comment 4

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Comment on attachment 617085 [details] [diff] [review]
Patch v2

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

r- at a glance for one huge reason. This is exposing the world from the module, but we only want to expose what's needed. To start, that's only what's in the service.

Comment 5

[Andres Hernandez [:andreshm]]

Attachment: Patch v3

Exposing only the checkPrivacyLevel method to start.

Comment 6

[Andres Hernandez [:andreshm]]

Attachment: patch v4

Now there is SessionStore that expose what is currently public in the service and a private SessionStoreInternal with all methods.

Comment 7

[Andres Hernandez [:andreshm]]

Attachment: Updated patch

Comment 8

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 622428 [details] [diff] [review]
Updated patch

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

This looks really good to me. Some thoughts:

1) Can we switch nsSessionStore.js to the new license header? Paul, what do you think? Not really important, just came to my mind.

2) Instead of having all this boilerplate code that just redirects to the SessionStore module, how about doing the following:

> SessionStoreService.prototype = SessionStore;
> SessionStoreService.prototype.classID = Components.ID("{5280606b-2510-4fe0-97ef-9b5a22eafe6b}");
> SessionStoreService.prototype.QueryInterface = XPCOMUtils.generateQI([Ci.nsISessionStore]);

Doesn't seem necessary to me to keep it as flexible as it is now because we still have the SessionStore object in the JSM.

Finally, I'd like to have Paul doing the final review for restructuring his module :)

Comment 9

[Andres Hernandez [:andreshm]]

Attachment: Updated patch

Applied Tim suggestions.

Comment 10

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Comment on attachment 623839 [details] [diff] [review]
Updated patch

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

I can't tell if you did this or not (not downloaded), but now is the perfect time to clean up any trailing whitespace. There was a bunch. If you're not sure and don't know an easy way to check, then no problem - we can get this landed and then do bug 664324 immediately after.

Assuming it doesn't explode on try server and you fix the nits, then this is mostly good to go. Freezing is my main concern at this point and so I'm going to r- until that's figured out.

::: browser/components/sessionstore/src/Makefile.in
@@ +52,5 @@
>  	$(NSINSTALL) $(srcdir)/*.jsm $(FINAL_TARGET)/modules/sessionstore
>  
> +SS_EXTRA_PP_JS_MODULES = \
> +  SessionStore.jsm \
> +  $(NULL)

Nit: everything else in this file uses tabs, so let's stick with that and match what is used (though I would love to stop that pattern).

::: browser/components/sessionstore/src/SessionStore.jsm
@@ +85,5 @@
> +XPCOMUtils.defineLazyModuleGetter(this, "ScratchpadManager",
> +  "resource:///modules/devtools/scratchpad-manager.jsm");
> +XPCOMUtils.defineLazyModuleGetter(this, "DocumentUtils",
> +  "resource:///modules/sessionstore/DocumentUtils.jsm");
> +#ifdef MOZ_CRASHREPORTER

Nit: can you put an empty line above here to give the #ifdef some breathing room.

@@ +207,5 @@
> +   * to merge data into the current session. If a window was opened at startup
> +   * with pinned tab(s), then the remaining data from the previous session for
> +   * that window will be opened into that winddow. Otherwise new windows will
> +   * be opened.
> +   */

I think we should either comment all of these or none of them (and in that case, leave the comments with the Internal method). Your call.

@@ +211,5 @@
> +   */
> +  restoreLastSession: function SessionStore_restoreLastSession() {
> +    SessionStoreInternal.restoreLastSession();
> +  }
> +};

I think we want to freeze this object since the service is tied tightly to this. We wouldn't want somebody to import this module and then change what a method points to. I think that's a possibility with modules (we've used freeze in similar situations). (https://developer.mozilla.org/en/JavaScript/Reference/Global_Objects/Object/freeze)

@@ +484,5 @@
> +   * Start tracking a window.
> +   * This function also initializes the component if it's not already
> +   * initialized.
> +   */
> +  init: function SessionStore_init(aWindow) {

Can you change all the function prefixes to SessionStoreInternal (or even just go ss_ and ssi_). The Internal ones have the same names as the public object, so looking at stacks won't be super clear (there are line numbers to help, but we should differentiate names as well).

@@ +4507,5 @@
> +// This is used to help meter the number of restoring tabs. This is the control
> +// point for telling the next tab to restore. It gets attached to each gBrowser
> +// via gBrowser.addTabsProgressListener
> +let gRestoreTabsProgressListener = {
> +  ss: null,

You may be able to remove this now too (but watch out for leaks with uninit).

@@ +4529,5 @@
> +// restored. We need to catch reloads that occur before the tab is restored
> +// because otherwise, docShell will reload an old URI (usually about:blank).
> +function SessionStoreSHistoryListener(ss, aTab) {
> +  this.tab = aTab;
> +  this.ss = ss;

Can you file a cleanup bug for after: we don't need to pass ss in, we can just use SessionStoreInternal.

::: browser/components/sessionstore/src/nsSessionStore.js
@@ +1,3 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */

We already have MPL2 in there now :)

@@ +27,5 @@
> +SessionStoreService.prototype = SessionStore;
> +SessionStoreService.prototype.classID =
> +  Components.ID("{5280606b-2510-4fe0-97ef-9b5a22eafe6b}");
> +SessionStoreService.prototype.QueryInterface =
> +  XPCOMUtils.generateQI([Ci.nsISessionStore]);

If you freeze the object in the module, then this isn't going to work (since the prototype is the module's object). If you freeze and want to do something like this, then you'll need to copy the methods from the frozen object into sss.prototype.

I made a little script to show the ways this works: http://jsfiddle.net/PhjHA/ (see your web console)

Comment 11

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch v5

[Andres is out for a couple of weeks so I'm jumping in.]

(In reply to Paul O'Shannessy [:zpao] from comment #10)
> I can't tell if you did this or not (not downloaded), but now is the perfect
> time to clean up any trailing whitespace.

Andres removed all trailing white spaces already. We should keep bug 664324 for nsSessionStartup.js - don't think we should include it here.

> ::: browser/components/sessionstore/src/Makefile.in
> @@ +52,5 @@
> >  	$(NSINSTALL) $(srcdir)/*.jsm $(FINAL_TARGET)/modules/sessionstore
> >  
> > +SS_EXTRA_PP_JS_MODULES = \
> > +  SessionStore.jsm \
> > +  $(NULL)
> 
> Nit: everything else in this file uses tabs, so let's stick with that and
> match what is used (though I would love to stop that pattern).

Done.

> ::: browser/components/sessionstore/src/SessionStore.jsm
> @@ +85,5 @@
> > +XPCOMUtils.defineLazyModuleGetter(this, "ScratchpadManager",
> > +  "resource:///modules/devtools/scratchpad-manager.jsm");
> > +XPCOMUtils.defineLazyModuleGetter(this, "DocumentUtils",
> > +  "resource:///modules/sessionstore/DocumentUtils.jsm");
> > +#ifdef MOZ_CRASHREPORTER
> 
> Nit: can you put an empty line above here to give the #ifdef some breathing
> room.

Done.

> @@ +207,5 @@
> > +   * to merge data into the current session. If a window was opened at startup
> > +   * with pinned tab(s), then the remaining data from the previous session for
> > +   * that window will be opened into that winddow. Otherwise new windows will
> > +   * be opened.
> > +   */
> 
> I think we should either comment all of these or none of them (and in that
> case, leave the comments with the Internal method). Your call.

Removed the comments and we should probably just leave them with the internal object.

> @@ +211,5 @@
> > +   */
> > +  restoreLastSession: function SessionStore_restoreLastSession() {
> > +    SessionStoreInternal.restoreLastSession();
> > +  }
> > +};
> 
> I think we want to freeze this object since the service is tied tightly to
> this. We wouldn't want somebody to import this module and then change what a
> method points to. I think that's a possibility with modules (we've used
> freeze in similar situations).

Added.

> @@ +484,5 @@
> > +   * Start tracking a window.
> > +   * This function also initializes the component if it's not already
> > +   * initialized.
> > +   */
> > +  init: function SessionStore_init(aWindow) {
> 
> Can you change all the function prefixes to SessionStoreInternal (or even
> just go ss_ and ssi_). The Internal ones have the same names as the public
> object, so looking at stacks won't be super clear (there are line numbers to
> help, but we should differentiate names as well).

Changed function prefixes to ss_ and ssi_.

> @@ +4507,5 @@
> > +// This is used to help meter the number of restoring tabs. This is the control
> > +// point for telling the next tab to restore. It gets attached to each gBrowser
> > +// via gBrowser.addTabsProgressListener
> > +let gRestoreTabsProgressListener = {
> > +  ss: null,
> 
> You may be able to remove this now too (but watch out for leaks with uninit).

Will include this in the follow-up for the SHistoryListener.

> @@ +4529,5 @@
> > +// restored. We need to catch reloads that occur before the tab is restored
> > +// because otherwise, docShell will reload an old URI (usually about:blank).
> > +function SessionStoreSHistoryListener(ss, aTab) {
> > +  this.tab = aTab;
> > +  this.ss = ss;
> 
> Can you file a cleanup bug for after: we don't need to pass ss in, we can
> just use SessionStoreInternal.

Will do.

> @@ +27,5 @@
> > +SessionStoreService.prototype = SessionStore;
> > +SessionStoreService.prototype.classID =
> > +  Components.ID("{5280606b-2510-4fe0-97ef-9b5a22eafe6b}");
> > +SessionStoreService.prototype.QueryInterface =
> > +  XPCOMUtils.generateQI([Ci.nsISessionStore]);
> 
> If you freeze the object in the module, then this isn't going to work (since
> the prototype is the module's object). If you freeze and want to do
> something like this, then you'll need to copy the methods from the frozen
> object into sss.prototype.

I implemented this using Object.defineProperty() because there's a getter in there and it's more flexible for supporting future additions.

Comment 12

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Comment on attachment 627165 [details] [diff] [review]
patch v5

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

Thanks for finishing it up Tim! And thanks Andres for doing most of the work :)

Comment 13

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/integration/fx-team/rev/5113edeacec3

Comment 14

[Rob Campbell [:rc] (:robcee)]

https://hg.mozilla.org/mozilla-central/rev/5113edeacec3