744809 - Move session store from harvester to provider model

Status: RESOLVED WONTFIX

(Firefox :: Session Restore, defect)

Description

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch v1 (WIP)

Session store should ideally know very little about the data it is storing and restoring. Therefore we need to stop collecting and start asking providers for their data.

That's a very early work-in-progress patch I'd like to get some feedback on. The RestoreDataProviders.jsm is actually just a bad hack to register all data providers *once* on startup before session store starts restoring things. I'm sure there's a better way to do it but I wanted to make sessionstore tests run :)

For a start, I integrated SessionStorage (per-tab) and Scratchpads (per-browser). I couldn't find anything per-window for now, maybe we don't even need this.

Comment 1

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

Comment on attachment 614411 [details] [diff] [review]
patch v1 (WIP)

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

First off, awesome :) Ok a couple quick comments...

1. This is much closer to what I was thinking for SS2 (https://wiki.mozilla.org/User:Zpao/SessionStore2). I know what you have is a transitional state, but I was envisioning a cleaner break. This still pigeon holes us into a browser > window > tab structure and forces providers to think like that. I guess we're not going away from that setup anytime soon, though so it's probably fine (I hadn't gotten to the details like this, so these are just first glance thoughts).

2. Let's do away with passing stringified JS objects around and constantly needing to parse & stringify. Use jsvals (we already started this with nsISessionStartup). We can make that change starting with providers and then make it in the core service too.

3. Does this open us up to getting rid of extData?

4. I'd envisioned something like registerProvider(key, provider) but something like this would work too. I think we want to provide a way to check if there's already a provider for a key pre-registration

So f+ for the general work happening, but I think we should hammer out the end goal before putting too much more work in.

Comment 2

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 614411 [details] [diff] [review]
patch v1 (WIP)

Cancelling feedback request. Let's spec out the details first.

Comment 3

[Tim Taubert [:ttaubert] (inactive)]

Closing, we have different plans now.