decouple tab state save code from the session restore service

RESOLVED WONTFIX

Status

()

Firefox
Session Restore
RESOLVED WONTFIX
6 years ago
4 years ago

People

(Reporter: dietrich, Assigned: andreshm)

Tracking

(Depends on: 1 bug, Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 10 obsolete attachments)

(Reporter)

Description

6 years ago
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
(Reporter)

Comment 1

5 years ago
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.
Assignee: nobody → dietrich
Attachment #589777 - Flags: feedback?(paul)
Attachment #589777 - Attachment is patch: true
Comment on attachment 589777 [details] [diff] [review]
wip

We talked on IRC, but for the record, I like the way this is headed.
Attachment #589777 - Flags: feedback?(paul) → feedback+
(Reporter)

Updated

5 years ago
Blocks: 669034
(Reporter)

Comment 3

5 years ago
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.
Summary: decouple tab state save/restore code from the session restore service → decouple tab state save code from the session restore service
(Reporter)

Comment 4

5 years ago
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.
Attachment #589777 - Attachment is obsolete: true
Attachment #596846 - Flags: feedback?(paul)
(Reporter)

Comment 5

5 years ago
(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)
Attachment #596846 - Flags: feedback?(paul) → feedback+
(Reporter)

Comment 7

5 years ago
Created attachment 601850 [details] [diff] [review]
decouple tab state save code from the session restore service (
(Reporter)

Updated

5 years ago
Attachment #596846 - Attachment is obsolete: true
(Reporter)

Comment 8

5 years ago
Created attachment 602171 [details] [diff] [review]
decouple tab state save code from the session restore service (
(Reporter)

Updated

5 years ago
Attachment #601850 - Attachment is obsolete: true
(Reporter)

Comment 9

5 years ago
Created attachment 602173 [details] [diff] [review]
decouple tab state save code from the session restore service (
(Reporter)

Updated

5 years ago
Attachment #602171 - Attachment is obsolete: true
(Reporter)

Updated

5 years ago
Assignee: dietrich → nobody
(Reporter)

Updated

5 years ago
OS: Mac OS X → All
Hardware: x86 → All
(Assignee)

Updated

5 years ago
Assignee: nobody → andres
(Assignee)

Updated

5 years ago
Status: NEW → ASSIGNED
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.
(Assignee)

Comment 11

5 years ago
Created attachment 613407 [details] [diff] [review]
Updated patch

Updated the previous patch. 
Also fixes a minor typo on bug 742047 patch.
Attachment #602173 - Attachment is obsolete: true
Attachment #613407 - Flags: review?(ttaubert)
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.
Attachment #613407 - Flags: review?(ttaubert)
(Assignee)

Comment 13

5 years ago
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.
(Assignee)

Updated

5 years ago
Depends on: 745040
(Assignee)

Comment 14

5 years ago
Created attachment 618482 [details] [diff] [review]
Patch v2

Updated patch.
Attachment #613407 - Attachment is obsolete: true
Attachment #618482 - Flags: review?(ttaubert)
(Assignee)

Updated

5 years ago
Attachment #618482 - Flags: review?(paul)
(Assignee)

Updated

5 years ago
Blocks: 742047
(Assignee)

Comment 15

5 years ago
Created attachment 622435 [details] [diff] [review]
Patch v3
Attachment #618482 - Attachment is obsolete: true
Attachment #618482 - Flags: review?(ttaubert)
Attachment #618482 - Flags: review?(paul)
Attachment #622435 - Flags: review?(ttaubert)
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.
Attachment #622435 - Flags: review?(ttaubert) → review+
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!
Attachment #622435 - Flags: review+ → review-
(Assignee)

Comment 18

5 years ago
Created attachment 624235 [details] [diff] [review]
patch v4
Attachment #622435 - Attachment is obsolete: true
Attachment #624235 - Flags: review?(ttaubert)
No longer blocks: 742047
Depends on: 742047
No longer depends on: 745040
Depends on: 759782
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.
Attachment #624235 - Flags: review?(ttaubert)
(Assignee)

Comment 20

5 years ago
Created attachment 635049 [details] [diff] [review]
Updated patch

The patch is updated based on the almost ready patch from bug 759782.
Attachment #624235 - Attachment is obsolete: true
Attachment #635049 - Flags: review?(ttaubert)
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!
Attachment #635049 - Flags: review?(ttaubert)
(Assignee)

Updated

5 years ago
Depends on: 768648
(Assignee)

Comment 22

5 years ago
Created attachment 637716 [details] [diff] [review]
Patch v6

Please apply depending bugs patches first.
Attachment #635049 - Attachment is obsolete: true
Attachment #637716 - Flags: review?(ttaubert)
Andres, your patch has bitrotten. Could you please update it?
Flags: needinfo?(andres)
(Assignee)

Comment 24

4 years ago
I think we should wait first for bug 759782 to land and feedback on bug 768648, do you agree?
Flags: needinfo?(andres)
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.
Severity: normal → blocker
Blocks: 827852
Removing blocker status, I have found another angle for my current work that doesn't seem blocked by this bug.
Severity: blocker → normal
(Assignee)

Updated

4 years ago
Status: ASSIGNED → NEW
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.
Attachment #637716 - Flags: review?(ttaubert)
Lots of changes have landed recently and e10s forces us to go slightly different directions that we will evaluate as we progress.
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.