Last Comment Bug 708488 - decouple tab state save code from the session restore service
: decouple tab state save code from the session restore service
Status: RESOLVED WONTFIX
:
Product: Firefox
Classification: Client Software
Component: Session Restore (show other bugs)
: unspecified
: All All
: -- normal with 3 votes (vote)
: ---
Assigned To: Andres Hernandez [:andreshm]
:
:
Mentors:
Depends on: 768648 742047 759782
Blocks: sessionRestoreJank 827852
  Show dependency treegraph
 
Reported: 2011-12-07 17:31 PST by Dietrich Ayala (:dietrich)
Modified: 2013-08-29 01:45 PDT (History)
19 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
wip (54.12 KB, patch)
2012-01-18 21:30 PST, Dietrich Ayala (:dietrich)
paul: feedback+
Details | Diff | Splinter Review
v1 (47.54 KB, patch)
2012-02-13 16:25 PST, Dietrich Ayala (:dietrich)
paul: feedback+
Details | Diff | Splinter Review
decouple tab state save code from the session restore service ( (47.86 KB, patch)
2012-02-29 19:21 PST, Dietrich Ayala (:dietrich)
no flags Details | Diff | Splinter Review
decouple tab state save code from the session restore service ( (26.26 KB, patch)
2012-03-01 15:37 PST, Dietrich Ayala (:dietrich)
no flags Details | Diff | Splinter Review
decouple tab state save code from the session restore service ( (47.89 KB, patch)
2012-03-01 15:40 PST, Dietrich Ayala (:dietrich)
no flags Details | Diff | Splinter Review
Updated patch (45.53 KB, patch)
2012-04-09 15:02 PDT, Andres Hernandez [:andreshm]
no flags Details | Diff | Splinter Review
Patch v2 (41.82 KB, patch)
2012-04-25 17:03 PDT, Andres Hernandez [:andreshm]
no flags Details | Diff | Splinter Review
Patch v3 (46.50 KB, patch)
2012-05-09 11:33 PDT, Andres Hernandez [:andreshm]
ttaubert: review-
Details | Diff | Splinter Review
patch v4 (70.26 KB, patch)
2012-05-15 16:35 PDT, Andres Hernandez [:andreshm]
no flags Details | Diff | Splinter Review
Updated patch (36.89 KB, patch)
2012-06-20 13:46 PDT, Andres Hernandez [:andreshm]
no flags Details | Diff | Splinter Review
Patch v6 (30.78 KB, patch)
2012-06-28 15:56 PDT, Andres Hernandez [:andreshm]
no flags Details | Diff | Splinter Review

Description Dietrich Ayala (:dietrich) 2011-12-07 17:31:42 PST
as a first step towards SSv2, i've started decoupling tab and window code into classes that expose methods for saving/restoring their data such that it's independent from the core SS service and exposes both sync and async apis for both actions.

mostly this consists of copying and pasting code from SessionStoreService into separate classes, and updating the service to use the external pieces. i've got the tab part working, with all tests passing.

subsequent phases look hand-wavily like:

- convert to async or event-based, but still residing in nsSessionStore.js
- experiment with patterns for decoupling
- move the standalone pieces over to their true homes in tab/window code itself
Comment 1 Dietrich Ayala (:dietrich) 2012-01-18 21:30:51 PST
Created attachment 589777 [details] [diff] [review]
wip

Got this working, and passing all tests. Is far enough to get some feedback on approach. The async conversion is pretty tacky, but works well enough. The idea is to have those methods support both async and sync callers for now, to address the immediate problem of blocking the UI. Eventually, once the Tab/Window stuff is completely migrated to the separate classes, we can clean up and separate the public API code out too, removing the need to have it mixed up with the periodic saving code.

* move tab saving into separate class (and file). restore moving there later.
* make periodic saves async, mostly.
* move non-service code into separate files. currently #include, but will convert to js modules where it makes sense.

TODO:
* move window saving code into separate class and file.
* move tab/window restore code into their new classes.
* convert a bunch of other internal sync calls to async.
Comment 2 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2012-01-20 11:30:09 PST
Comment on attachment 589777 [details] [diff] [review]
wip

We talked on IRC, but for the record, I like the way this is headed.
Comment 3 Dietrich Ayala (:dietrich) 2012-02-13 16:13:35 PST
Narrowing the scope of this for manageability. The costly bit is saving, so focusing on that first. Also, there will be no async-ness here, just compartmentalizing the code in a more sane way.
Comment 4 Dietrich Ayala (:dietrich) 2012-02-13 16:25:59 PST
Created attachment 596846 [details] [diff] [review]
v1

no code change. just moved tab data saving functionality to a separate js module.

open questions and things that still need to happen:

* stop passing the service in. copy shared bits, or move into SessionUtils module?

* move to clearer OO usage where tab is passed into ctor, instead of instance methods.
Comment 5 Dietrich Ayala (:dietrich) 2012-02-13 18:19:20 PST
(In reply to Dietrich Ayala (:dietrich) from comment #4)
> * move to clearer OO usage where tab is passed into ctor, instead of
> instance methods.

Then we allow for the possibility of keeping TabStateManagers around... maybe better off just making this a singleton utils module like DocumentUtils in bug 697903.
Comment 6 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2012-02-14 11:41:21 PST
(In reply to Dietrich Ayala (:dietrich) from comment #4)
> Created attachment 596846 [details] [diff] [review]
> v1
> 
> no code change. just moved tab data saving functionality to a separate js
> module.
> 
> open questions and things that still need to happen:
> 
> * stop passing the service in. copy shared bits, or move into SessionUtils
> module?

I was going to say "why not just use getService", but it's the private stuff you need.

> * move to clearer OO usage where tab is passed into ctor, instead of
> instance methods.

> Then we allow for the possibility of keeping TabStateManagers around...
> maybe better off just making this a singleton utils module like
> DocumentUtils in bug 697903.

Yea, that sounds like a good route.

We could go back a step further even and start this breakout all in nsSessionStore.js. We could move things out to global scope / singletons as we transition to modules, add things like TabStateManager, and make the service a dumb wrapper. Or pulling into modules now works too (definitely cleaner, just need to think more about the separation)
Comment 7 Dietrich Ayala (:dietrich) 2012-02-29 19:21:05 PST
Created attachment 601850 [details] [diff] [review]
decouple tab state save code from the session restore service (
Comment 8 Dietrich Ayala (:dietrich) 2012-03-01 15:37:45 PST
Created attachment 602171 [details] [diff] [review]
decouple tab state save code from the session restore service (
Comment 9 Dietrich Ayala (:dietrich) 2012-03-01 15:40:23 PST
Created attachment 602173 [details] [diff] [review]
decouple tab state save code from the session restore service (
Comment 10 Tim Taubert [:ttaubert] 2012-04-03 15:51:41 PDT
Please apply the patch from bug 742047 when working on this one so you don't need to take care of the session storage functions.
Comment 11 Andres Hernandez [:andreshm] 2012-04-09 15:02:38 PDT
Created attachment 613407 [details] [diff] [review]
Updated patch

Updated the previous patch. 
Also fixes a minor typo on bug 742047 patch.
Comment 12 Tim Taubert [:ttaubert] 2012-04-10 07:16:07 PDT
Comment on attachment 613407 [details] [diff] [review]
Updated patch

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

We probably shouldn't call it *Manager, this says nothing about the responsibility of this object. I think 'TabState' would be sufficient or some more meaningful appendix.

The whole module should be a singleton as noted in comment #5 so there's no need to create new instances and keep them around or cache them. The session store service doesn't need to be passed we can easily access it (it's a service :).

By and large the general direction is good, these points above aren't really major concerns. Did you make sure the test suite completes successfully? Thanks!u

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

Thanks for noticing! I'm gonna fix this in my patch.
Comment 13 Andres Hernandez [:andreshm] 2012-04-12 17:17:39 PDT
I discussed this with Tim and Paul on IRC, but for the record. 

The issue is that in order to have the TabState as singleton, it need to access some information from the SSS. But, currently, it's not possible, since not all methods and data are public in the idl file. 

Some are just preferences or other information that can be copied, except for: _checkPrivacyLevel and saveStateDelayed. 

So, I'll create a new bug to solve this issue first as dependency of this one.
Comment 14 Andres Hernandez [:andreshm] 2012-04-25 17:03:32 PDT
Created attachment 618482 [details] [diff] [review]
Patch v2

Updated patch.
Comment 15 Andres Hernandez [:andreshm] 2012-05-09 11:33:44 PDT
Created attachment 622435 [details] [diff] [review]
Patch v3
Comment 16 Tim Taubert [:ttaubert] 2012-05-11 15:30:44 PDT
Comment on attachment 622435 [details] [diff] [review]
Patch v3

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

Looks good!

::: browser/components/sessionstore/src/SessionStore.jsm
@@ +224,5 @@
> +   *        don't do normal check for deferred
> +   * @returns bool
> +   */
> +  checkPrivacyLevel: function SessionStore_checkPrivacyLevel(aIsHTTPS, aUseDefaultPref) {
> +    return SessionStoreInternal._checkPrivacyLevel(aIsHTTPS, aUseDefaultPref);

Nit: we should remove the underscore from "_checkPrivacyLevel" as it's not really "private" to SessionStoreInternal anymore.
Comment 17 Tim Taubert [:ttaubert] 2012-05-12 04:20:33 PDT
Comment on attachment 622435 [details] [diff] [review]
Patch v3

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

Sorry, changing to r- because I oversaw some important points in this patch.

_deserializeHistoryEntry() and _deserializeSessionStorage() need to be in TabState.jsm as well. This is nothing that SessionStore should care about. TabState actually has two tasks: collecting and restoring tab states. Therefore it should have two "public" methods: .retrieveTabState() and .restoreTabState(). restoreTabState() will then be called from SessionStore.restoreHistory().

Also, we don't need a separate updateTextAndScrollData() function anymore. This data should just be collected when .retrieveTabState() is called. The only place that currently calls it separately from collectTabData() is:

http://mxr.mozilla.org/mozilla-central/source/browser/components/sessionstore/src/nsSessionStore.js#2711

and we event don't need this anymore because _saveWindowHistory() from the line before calls collectTabData() already.

Oh, and can you please divide the TabState.jsm into a public "TabState" object and an internal "TabStateInternal" object like you did with SessionStore.jsm? Thanks!
Comment 18 Andres Hernandez [:andreshm] 2012-05-15 16:35:07 PDT
Created attachment 624235 [details] [diff] [review]
patch v4
Comment 19 Tim Taubert [:ttaubert] 2012-05-31 02:36:14 PDT
Comment on attachment 624235 [details] [diff] [review]
patch v4

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

We have some new bugs that this depends on. They contain some bigger refactoring useful for this bug. We should wait until they're landed or have received mostly positive feedback.
Comment 20 Andres Hernandez [:andreshm] 2012-06-20 13:46:39 PDT
Created attachment 635049 [details] [diff] [review]
Updated patch

The patch is updated based on the almost ready patch from bug 759782.
Comment 21 Tim Taubert [:ttaubert] 2012-06-23 10:01:59 PDT
Comment on attachment 635049 [details] [diff] [review]
Updated patch

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

This looks good but before continuing with this bug I think we should further modularize and move all 'text and scroll data' code to its own JSM in a new bug blocking this one. When doing this we should keep the 'selectedPageStyle' rather in the TabState.jsm - it's unrelated to 'text and scroll data', not sure why it's in there now.

Can you please file a new bug for this? Thanks!
Comment 22 Andres Hernandez [:andreshm] 2012-06-28 15:56:13 PDT
Created attachment 637716 [details] [diff] [review]
Patch v6

Please apply depending bugs patches first.
Comment 23 David Teller [:Yoric] (please use "needinfo") 2013-01-08 07:45:48 PST
Andres, your patch has bitrotten. Could you please update it?
Comment 24 Andres Hernandez [:andreshm] 2013-01-08 08:01:13 PST
I think we should wait first for bug 759782 to land and feedback on bug 768648, do you agree?
Comment 25 David Teller [:Yoric] (please use "needinfo") 2013-01-08 08:04:40 PST
Waiting for bug 759782 makes sense. I don't really know why bug 768648 is useful, but if you think it is, I am willing to take your word on it.

All of this is starting to be blocking for my ongoing work on splitting sessionstore collection into an asynchronous loop, though, so I will start marking them as blockers.
Comment 26 David Teller [:Yoric] (please use "needinfo") 2013-01-11 10:13:43 PST
Removing blocker status, I have found another angle for my current work that doesn't seem blocked by this bug.
Comment 27 Tim Taubert [:ttaubert] 2013-03-22 03:30:58 PDT
Comment on attachment 637716 [details] [diff] [review]
Patch v6

Let's put this on hold for now. We first need to evaluate the basic direction we're going with sessionstore in the future.
Comment 28 Tim Taubert [:ttaubert] 2013-08-29 01:45:55 PDT
Lots of changes have landed recently and e10s forces us to go slightly different directions that we will evaluate as we progress.

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