768648 - Move text and scroll data from sessionstore into a new JSM

Status: RESOLVED WORKSFORME

(Firefox :: Session Restore, defect)

Comment 1

[Andres Hernandez [:andreshm]]

Attachment: Patch v1

Comment 2

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 637646 [details] [diff] [review]
Patch v1

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

Thank you, Andres! That looks good so far. I thought a bit about this and I think we should call it 'DocumentState.jsm', after all it's all the code from restoreDocument() and it fits nicely for scroll, form and html data. With the new wording we can re-include pageStyle data, sorry about the confusion, shouldn't be a big change.

Also, I think the API should rather look like this:

> [SessionStore.jsm]
> tabData.documentState = DocumentState.retrieve(/* browser etc */);

... and ...

> [SessionStore.jsm]
> DocumentState.restore(/* browser etc */, tabData.documentState);

This way the JSM acts more like SessionStorage.jsm and SessionStore doesn't need to know anything about the internal format. The separation is a bit more obvious here.

BTW, this patch doesn't necessarily need the one from bug 759782, does it? Can you please re-base your patch with trunk? I think we might land this one earlier.

::: browser/components/sessionstore/src/SessionStore.jsm
@@ +1268,5 @@
>        return;
>      }
>  
>      // make sure that the tab related data is up-to-date
> +    var tabState = this._collectTabData(aTab, false);

Please don't change all these calls just to let the second argument be false. As undefined is false-ish we can leave it that way or do:

> _collectTabData: function ssi_collectTabData(aTab, aFullData = false) {

(Now that default arguments have landed we should use them, they're nice.)

@@ +2980,5 @@
>      }
>  
>      // always call this before injecting content into a document!
>      function hasExpectedURL(aDocument, aURL)
>        !aURL || aURL.replace(/#.*/, "") == aDocument.location.href.replace(/#.*/, "");

Do we still need this here?

@@ +2987,5 @@
>      // away before the loading completed (except for in-page navigation)
>      if (hasExpectedURL(aEvent.originalTarget, aBrowser.__SS_restore_data.url)) {
>        var content = aEvent.originalTarget.defaultView;
> +      var data = aBrowser.__SS_restore_data;
> +      var selectedPageStyle = aBrowser.__SS_restore_pageStyle;

*let.

@@ +3018,5 @@
> +    Array.forEach(aContent.document.styleSheets, function(aSS) {
> +      aSS.disabled = aSS.title && aSS.title != aSelectedPageStyle;
> +    });
> +
> +    for (var i = 0; i < aContent.frames.length; i++) {

*let.

::: browser/components/sessionstore/src/TextAndScrollData.jsm
@@ +1,3 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> +* License, v. 2.0. If a copy of the MPL was not distributed with this file,
> +* You can obtain one at http://mozilla.org/MPL/2.0/. */

Nit: the asterisks should be aligned.

@@ +47,5 @@
> +    if (aBrowser.__SS_data && aBrowser.__SS_tabStillLoading)
> +      return;
> +
> +    var tabIndex = (aTabData.index || aTabData.entries.length) - 1;
> +    var tabDataEntry = aTabData.entries[tabIndex];

Please use 'let' here.

@@ +77,5 @@
> +   */
> +  _retrieveTextAndScrollDataForFrame: function tasdi_retrieveTextAndScrollDataForFrame(
> +    aWindow, aContent, aData, aUpdateFormData, aFullData, aIsPinned) {
> +
> +    for (var i = 0; i < aContent.frames.length; i++) {

*let.

@@ +85,5 @@
> +          aFullData, aIsPinned);
> +      }
> +    }
> +
> +    var isHTTPS = this._isHTTPS(aContent);

Please use 'let'.

@@ +98,5 @@
> +    this._retrieveScrollData(aContent, aData);
> +  },
> +
> +  /**
> +   * Checks if its https.

Nit: Checks if the given content window's url scheme is https.

@@ +105,5 @@
> +   */
> +  _isHTTPS: function tasdi_isHTTPS(aContent) {
> +    var url = (aContent.parent || aContent).document.location.href;
> +    var uri = Services.io.newURI(url, null, null);
> +    return uri.schemeIs("https");

Seems a bit expensive to create a whole nsIURI object just to check if the scheme is https. I think we could do this easier like:

> let url = (aContent.parent || aContent).document.location.href;
> return url.indexOf("https:") == 0;

@@ +116,5 @@
> +   * @param aUpdateFormData update all form data for this tab
> +   * @param aFullData always return privacy sensitive data (use with care)
> +   * @param aIsAboutSR is about:sessionrestore
> +   */
> +  _retrieveFormData : function tasdi_retrieveFormData(

Nit: _retrieveFormData: (no whitespace).

@@ +145,5 @@
> +   * @param aContent frame reference
> +   * @param aData part of a tabData object to add the information to
> +   * @param aFullData always return privacy sensitive data (use with care)
> +   */
> +  _retrieveHTMLData : function tasdi_retrieveHTMLData(

Nit: _retrieveHTMLData: (no whitespace).

@@ +167,5 @@
> +   * a flush or layout.
> +   * @param aContent frame reference
> +   * @param aData part of a tabData object to add the information to
> +   */
> +  _retrieveScrollData : function tasdi_retrieveScrollData(aContent, aData) {

Nit: _retrieveScrollData: (no whitespace).

Sorry for being so nitpicky about that but it drives you mad if you search for a function definition and we use "$methodname:" everywhere else :)

@@ +173,5 @@
> +      aContent.QueryInterface(Ci.nsIInterfaceRequestor).
> +        getInterface(Ci.nsIDOMWindowUtils);
> +    let scrollX = {}, scrollY = {};
> +    domWindowUtils.getScrollXY(false, scrollX, scrollY);
> +    aData.scroll = scrollX.value + "," + scrollY.value;

We shouldn't save it as "x,y" only to use a regex later to parse it again. Let's just use a simple array [x, y] or object {x: 5, y: 6}. We should keep the regex fallback for a little while, though.

@@ +189,5 @@
> +    this._restoreFormData(aContent, aData);
> +    this._restoreHTMLData(aWindow, aContent, aData);
> +    this._restoreScrollData(aContent, aData);
> +
> +    for (var i = 0; i < aContent.frames.length; i++) {

*let.

@@ +203,5 @@
> +   * Restores the form data.
> +   * @param aContent frame reference
> +   * @param aData part of a tabData object to add the information to
> +   */
> +  _restoreFormData : function tasdi_restoreFormData(aContent, aData) {

Nit: _restoreFormData:

@@ +243,5 @@
> +   * @param aData part of a tabData object to add the information to
> +   */
> +  _restoreHTMLData: function tasdi_restoreHTMLData(aWindow, aContent, aData) {
> +    if (aData.innerHTML) {
> +      var that = this;

Please use function.bind() for the setTimeout() call.

@@ +260,5 @@
> +   * @param aContent frame reference
> +   * @param aData part of a tabData object to add the information to
> +   */
> +  _restoreScrollData: function tasdi_restoreScrollData(aContent, aData) {
> +    var match;

*let.

Comment 3

[Andres Hernandez [:andreshm]]

(In reply to Tim Taubert [:ttaubert] from comment #2)
> Comment on attachment 637646 [details] [diff] [review]
> Patch v1
> 
> Sorry for being so nitpicky about that but it drives you mad if you search
> for a function definition and we use "$methodname:" everywhere else :)

No problem! I really appreciate all the feedback! Thanks.  

> @@ +173,5 @@
> > +      aContent.QueryInterface(Ci.nsIInterfaceRequestor).
> > +        getInterface(Ci.nsIDOMWindowUtils);
> > +    let scrollX = {}, scrollY = {};
> > +    domWindowUtils.getScrollXY(false, scrollX, scrollY);
> > +    aData.scroll = scrollX.value + "," + scrollY.value;
> 
> We shouldn't save it as "x,y" only to use a regex later to parse it again.
> Let's just use a simple array [x, y] or object {x: 5, y: 6}. We should keep
> the regex fallback for a little while, though.

I agree, but that means changing the format in file. I think is better to use {x: 5, y: 6}.

Comment 4

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Andres Hernandez [:andreshm] from comment #3)
> > We shouldn't save it as "x,y" only to use a regex later to parse it again.
> > Let's just use a simple array [x, y] or object {x: 5, y: 6}. We should keep
> > the regex fallback for a little while, though.
> 
> I agree, but that means changing the format in file. I think is better to
> use {x: 5, y: 6}.

Yeah, I think the scroll positions is not the most important information we have. We should just support reading the old format for a while to make migrations to newer versions behave as expected.

Comment 5

[Andres Hernandez [:andreshm]]

Attachment: Patch v2

I still need to apply this comment: 

> Also, I think the API should rather look like this:
> 
> > [SessionStore.jsm]
> > tabData.documentState = DocumentState.retrieve(/* browser etc */);
> 
> ... and ...
> 
> > [SessionStore.jsm]
> > DocumentState.restore(/* browser etc */, tabData.documentState);

> This way the JSM acts more like SessionStorage.jsm and SessionStore doesn't need to
> know anything about the internal format. The separation is a bit more obvious here.

But, currently, all the document state info (scroll, form data, innerHTML) is being added to the tab entries (I'm not sure if its only for the last one). So, if I create a tabState.documentState then all the children structure of each entry is going to be created again in this object. That means probably a lot of compatibility changes to reorganize the file. 
As I understand, then the structure will be: 
  - tabs
    - 0
       - entries
         - 0 
            - data (url, title, etc)
            - children [...] /* each one with data */
         - ...
       - documentState
         - scroll
         - formData
         - pageStyle
         - innerHTML
         - children[...] /* each one with scroll, form data, style, etc */
    - ...

Please some advice on how to continue with this. 

Also, I think we can store the scroll info only if it's different than "0,0" (default). To help reduce the file size. Probably is better to do it in a new bug, right?

Comment 6

[Andres Hernandez [:andreshm]]

Attachment: Patch v3

Rebased patch.

Comment 7

[David Teller [:Yoric] - still alive but not very active]

It is my understanding that this bug blocks a blocker. Marking as blocker.

Comment 8

[David Teller [:Yoric] - still alive but not very active]

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

Comment 9

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 666838 [details] [diff] [review]
Patch v3

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 10

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee didn't login in Bugzilla in the last 7 months.
:dao, could you have a look please?
For more information, please visit auto_nag documentation.