Last Comment Bug 742047 - Move sessionStorage functionality to JSM
: Move sessionStorage functionality to JSM
Status: RESOLVED FIXED
[fixed-in-fx-team]
:
Product: Firefox
Classification: Client Software
Component: Session Restore (show other bugs)
: Trunk
: All All
: -- normal with 3 votes (vote)
: Firefox 15
Assigned To: Andres Hernandez [:andreshm]
:
Mentors:
Depends on: 745040
Blocks: 669603 708488
  Show dependency treegraph
 
Reported: 2012-04-03 13:48 PDT by Tim Taubert [:ttaubert]
Modified: 2012-06-03 13:45 PDT (History)
12 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch v1 (13.06 KB, patch)
2012-04-03 13:48 PDT, Tim Taubert [:ttaubert]
paul: feedback+
Details | Diff | Review
patch v2 (13.05 KB, patch)
2012-04-10 07:17 PDT, Tim Taubert [:ttaubert]
no flags Details | Diff | Review
patch v3 (12.98 KB, patch)
2012-04-20 10:46 PDT, Andres Hernandez [:andreshm]
no flags Details | Diff | Review
Updated patch (14.27 KB, patch)
2012-05-09 15:58 PDT, Andres Hernandez [:andreshm]
no flags Details | Diff | Review
Updated patch (12.87 KB, patch)
2012-05-15 16:52 PDT, Andres Hernandez [:andreshm]
no flags Details | Diff | Review
Part 1 - create SessionStorage.jsm and migrate existing code (16.74 KB, patch)
2012-05-25 06:11 PDT, Tim Taubert [:ttaubert]
paul: review+
Details | Diff | Review
Part 2 - Refactor SessionStorage.jsm and its API (9.29 KB, patch)
2012-05-30 03:39 PDT, Tim Taubert [:ttaubert]
no flags Details | Diff | Review
Part 2 - Refactor SessionStorage.jsm and its API (10.70 KB, patch)
2012-05-30 08:16 PDT, Tim Taubert [:ttaubert]
paul: review+
Details | Diff | Review

Description Tim Taubert [:ttaubert] 2012-04-03 13:48:50 PDT
Created attachment 611966 [details] [diff] [review]
patch v1

In preparation for bug 669603 I thought it might be beneficial to split this big patch into multiple steps. This patch moves all the sessionStorage functionality out of the session store service into its own JSM. It's still the same synchronous behavior, but cleaned up a bit.
Comment 1 Tim Taubert [:ttaubert] 2012-04-10 07:17:11 PDT
Created attachment 613589 [details] [diff] [review]
patch v2

Fixed a little typo.
Comment 2 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2012-04-10 10:10:21 PDT
Comment on attachment 611966 [details] [diff] [review]
patch v1

(splinter trashed my in progress review when the attachment became obsolete, so hoping un-obsoleting it bring it back)
Comment 3 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2012-04-10 11:45:41 PDT
Comment on attachment 611966 [details] [diff] [review]
patch v1

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

Looking at this patch because I had already started on it...

I think it's a good start but I have some concerns. These aren't "do what I say concerns" so let me know what you think!

::: browser/components/sessionstore/src/SessionStorage.jsm
@@ +7,5 @@
> +let EXPORTED_SYMBOLS = ["SessionStorage"];
> +
> +const Cu = Components.utils;
> +const Cc = Components.classes;
> +const Ci = Components.interfaces;

You don't use Cc or Ci, so no need.

@@ +17,5 @@
> +const PRIVACY_ENCRYPTED = 1;
> +const PRIVACY_FULL = 2;
> +
> +XPCOMUtils.defineLazyServiceGetter(this, "gScriptSecurityManager",
> +  "@mozilla.org/scriptsecuritymanager;1", "nsIScriptSecurityManager");

Can we keep this SecuritySvc to match what's in nsSessionStore?

@@ +32,5 @@
> +   * @param aPrivacyLevel
> +   *        The session store privacy level to use
> +   */
> +  serialize: function SessionStorage_serialize(aTabData, aHistory, aDocShell,
> +                                               aPrivacyLevel) {

We could take this opportunity to clean this up and bring it closer to mirroring deserialize.

serialize shouldn't know anything about aTabData, it should just return an object and then in sessionstore we decide what to do with it. If it always returns an object then we can just set & be done with it.

I think we can get away with not passing in history too - it should be available on docshell.sessionHistory (quick test in error console seems to work). Or we could pass the browser into each method here and get history/docshell from it. A little bit of duplication with what's happening in sessionstore, but as we pull more of that out into modules that's ok. We should try to design the most robust API instead of pigeonholing into what sessionstore is currently doing.

@@ +63,5 @@
> +   */
> +  read: function DomStorage_read(aHistory, aDocShell, aPrivacyLevel) {
> +    let data = {};
> +
> +    History.getEntries(aHistory).forEach(function (aEntry) {

I know structurally this is cleaner, but we're now iterating over every entry twice (for every tab... often). I don't have scientific data to back this up, but I'm guessing that isn't good.

@@ +70,5 @@
> +        return;
> +
> +      // Check if we're allowed to store sessionStorage data.
> +      if (aPrivacyLevel == PRIVACY_NONE ||
> +          (aEntry.uri.schemeIs("https") && aPrivacyLevels == PRIVACY_ENCRYPTED))

I don't really like moving the privacy level checks around like this :/
It works though, but moves the logic around - if we change the rules for _checkPrivacyLevel, we need to make sure that is updated too.
Can you put a nice big XXX comment here and at _checkPrivacyLevel making it clear how it's used outside of there. The other option would be passing SSS in to here so we could duplicate _checkPrivacyLevel, but that's not great either

Also, aPrivacyLevel (no s!) if we do this (I'm guessing this was the typo fix)

::: browser/components/sessionstore/src/nsSessionStore.js
@@ +160,5 @@
>    "@mozilla.org/scriptsecuritymanager;1", "nsIScriptSecurityManager");
>  
> +XPCOMUtils.defineLazyModuleGetter(this, "SessionStorage",
> +  "resource:///modules/sessionstore/SessionStorage.jsm");
> +

Nit: move this up with the other lazy module imports.

@@ +1872,5 @@
> +        if (!this._checkPrivacyLevel(true, aTab.pinned))
> +          privacyLevel = PRIVACY_ENCRYPTED;
> +        else if (!this._checkPrivacyLevel(false, aTab.pinned))
> +          privacyLevel = PRIVACY_NONE;
> +      }

Uhh, so this... privacy level should be determined for each entry, not a once & done thing.

(oh I see...)
Comment 4 Andres Hernandez [:andreshm] 2012-04-20 10:46:25 PDT
Created attachment 617033 [details] [diff] [review]
patch v3

Added Paul suggestions. 

Also, I changed it to use a callback to check privacy. When Bug 745040 is ready, we can update it to call the module directly.
Comment 5 Andres Hernandez [:andreshm] 2012-05-09 15:58:31 PDT
Created attachment 622562 [details] [diff] [review]
Updated patch
Comment 6 Andres Hernandez [:andreshm] 2012-05-15 16:52:26 PDT
Created attachment 624239 [details] [diff] [review]
Updated patch
Comment 7 Tim Taubert [:ttaubert] 2012-05-25 06:11:23 PDT
Created attachment 627209 [details] [diff] [review]
Part 1 - create SessionStorage.jsm and migrate existing code

[Taking over while Andres is out for a couple of weeks.]

We should put bug 708488 on hold for a moment because it's really hard to refactor all the TabState code if it's all over the place.

As a first step we should just create a SessionStorage.jsm, move _serializeSessionStorage() and _deserializeSessionStorage() to their new place and make those internal methods accessible that we need.

The second step, a follow-up bug, should be refactoring the whole code and the API to prepare making all this async.
Comment 8 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2012-05-25 16:46:27 PDT
Comment on attachment 627209 [details] [diff] [review]
Part 1 - create SessionStorage.jsm and migrate existing code

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

(In reply to Tim Taubert [:ttaubert] from comment #7)
> As a first step we should just create a SessionStorage.jsm, move
> _serializeSessionStorage() and _deserializeSessionStorage() to their new
> place and make those internal methods accessible that we need.
> 
> The second step, a follow-up bug, should be refactoring the whole code and
> the API to prepare making all this async.

Ok, so you're thinking we just move wholesale & have an unadvertised SessionStorage API that we'll change later? If possible, I'd prefer to have all of that happen within a single version (much like the format changes) so we don't churn too much. SessionStorage.jsm would hopefully only be used by us, but there aren't any guarantees...

That's fine by me if you'll coordinate to make that happen sooner rather than later (and keep my comments & Andres's changes in mind).
Comment 9 Tim Taubert [:ttaubert] 2012-05-30 03:36:06 PDT
(In reply to Paul O'Shannessy [:zpao] from comment #8)
> Ok, so you're thinking we just move wholesale & have an unadvertised
> SessionStorage API that we'll change later? If possible, I'd prefer to have
> all of that happen within a single version (much like the format changes) so
> we don't churn too much. SessionStorage.jsm would hopefully only be used by
> us, but there aren't any guarantees...

I agree, let's make it all happen in this bug.
Comment 10 Tim Taubert [:ttaubert] 2012-05-30 03:39:59 PDT
Created attachment 628298 [details] [diff] [review]
Part 2 - Refactor SessionStorage.jsm and its API

This patch basically contains all the changes from attachment 624239 [details] [diff] [review]. I simplified the API even more and did a little cleanup.
Comment 11 Tim Taubert [:ttaubert] 2012-05-30 08:16:01 PDT
Created attachment 628346 [details] [diff] [review]
Part 2 - Refactor SessionStorage.jsm and its API

Small cleanup.
Comment 12 Tim Taubert [:ttaubert] 2012-05-30 10:46:02 PDT
Looks good on try, btw.

https://tbpl.mozilla.org/?tree=Try&rev=af5bde28b750
Comment 13 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2012-06-01 15:51:50 PDT
Comment on attachment 628346 [details] [diff] [review]
Part 2 - Refactor SessionStorage.jsm and its API

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

::: browser/components/sessionstore/src/SessionStorage.jsm
@@ +143,5 @@
> +   *        That tab's session history
> +   * @param aIndex
> +   *        The history entry's index
> +   */
> +  getUriForEntry: function History_getUriForEntry(aHistory, aIndex) {

super nit: I like URI all caps in there but I'm fine either way.
Comment 14 Tim Taubert [:ttaubert] 2012-06-02 00:49:22 PDT
(In reply to Paul O'Shannessy [:zpao] from comment #13)
> > +  getUriForEntry: function History_getUriForEntry(aHistory, aIndex) {
> 
> super nit: I like URI all caps in there but I'm fine either way.

I do, too. Will fix.
Comment 16 Rob Campbell [:rc] (:robcee) 2012-06-03 13:45:16 PDT
https://hg.mozilla.org/mozilla-central/rev/ebd52606c461
Comment 17 Rob Campbell [:rc] (:robcee) 2012-06-03 13:45:40 PDT
https://hg.mozilla.org/mozilla-central/rev/329aa567fec9

Note You need to log in before you can comment on or make changes to this bug.