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

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 on attachment 589777 [details] [diff] [review] wip We talked on IRC, but for the record, I like the way this is headed.

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.

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.

(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.

(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)

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.

Updated the previous patch. Also fixes a minor typo on bug 742047 patch.

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.

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.

Updated patch.

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 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 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.

The patch is updated based on the almost ready patch from bug 759782.

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!

Please apply depending bugs patches first.

Andres, your patch has bitrotten. Could you please update it?

I think we should wait first for bug 759782 to land and feedback on bug 768648, do you agree?

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.

Removing blocker status, I have found another angle for my current work that doesn't seem blocked by this bug.

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.

Lots of changes have landed recently and e10s forces us to go slightly different directions that we will evaluate as we progress.