742051 - Remove the backwards compatibility for the old formdata format.

Status: RESOLVED FIXED

(Firefox :: Session Restore, defect)

Description

[Andres Hernandez [:andreshm]]

As requested per Tim in bug 697903, see: https://bugzilla.mozilla.org/show_bug.cgi?id=697903#c12 

We should use the new formdata format 'id/xpath' instead of the old format.

Comment 1

[Andres Hernandez [:andreshm]]

Attachment: Patch

Please apply first the patch in bug 697903.

Comment 2

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 612350 [details] [diff] [review]
Patch

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

This looks good to me but let's wait what Paul says in bug 697903. If we're ever going to change the default format I'm not sure when to do that. Should we wait for one release? Two? Three?

::: browser/components/sessionstore/src/nsSessionStore.js
@@ +2186,5 @@
>          if (Object.keys(formData.id).length ||
>              Object.keys(formData.xpath).length) {
>            // The object returned from getFormData() is not shared, so we reuse
> +          // the object for our storage.
> +          aData.formdata = { xpath: formData.xpath, id: formData.id };

|aData.formdata = formData| should be sufficient.

@@ +3363,5 @@
>  
> +        // for about:sessionrestore we saved the field as JSON to avoid
> +        // nested instances causing humongous sessionstore.js files.
> +        // cf. bug 467409
> +        if (aData.url == "about:sessionrestore" &&

Please add |"sessionData" in formdata.id &&| to the condition before checking the type.

Comment 3

[Andres Hernandez [:andreshm]]

Attachment: Patch v2

Updated patch.

Comment 4

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

Comment on attachment 615780 [details] [diff] [review]
Patch v2

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

Couple things:
1. The existing tests can't possibly be passing. We depend on the current format in several of them. Please be sure to at least run the session restore tests.
2. I'd like a test that explicitly checks we're returning the right format. I think you could tack that onto the test in bug 697903 - just getTabState and check that formdata is using {id, xpath} instead of [#id, /xpath]. As I said there are tests that depend on the format already but I'd like to have more explicit unit tests as opposed to our smattering of tests that sort of depend on things.

Comment 5

[Andres Hernandez [:andreshm]]

Attachment: Patch v3

Improved the formdata format test to check that is using id and xpath and fixed the formdata related tests.

Comment 6

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

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

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

Sorry for the delay, I thought I'd posted this review already!

Can you this batch of changes to try server so we can get a run before we check it in?

::: browser/components/sessionstore/test/browser_formdata_format.js
@@ +85,4 @@
>  
> +      // test format
> +      ok("id" in restoredFormData && "xpath" in restoredFormData,
> +        "FormData format is valid");

Can you also ensure that there are no other keys (since that's how the old format would be in there). This test combined with
> is(Object.keys(restoredFormData).length, 2)
should cover it.

Comment 7

[Andres Hernandez [:andreshm]]

Attachment: Patch v4

Updated tests to ensure there are no old keys. 

I appreciate if you can explain further this comment:
"Can you this batch of changes to try server so we can get a run before we check it in?"

Comment 8

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Andres Hernandez [:andreshm] from comment #7)
> I appreciate if you can explain further this comment:
> "Can you this batch of changes to try server so we can get a run before we
> check it in?"

He accidentally a word. He wanted you to push the patches to the try server to see if tests are failing before we turn the tree red. I just did this for you :)

https://tbpl.mozilla.org/?tree=Try&rev=a1ce76c33852

Comment 9

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 619246 [details] [diff] [review]
Patch v4

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

::: browser/components/sessionstore/src/nsSessionStore.js
@@ +3420,5 @@
>      function restoreTextDataAndScrolling(aContent, aData, aPrefix) {
>        if (aData.formdata && hasExpectedURL(aContent.document, aData.url)) {
>          let formdata = aData.formdata;
>  
>          // handle backwards compatibility

That's not exactly backwards compatibility but rather a migration to the new format now. We should file a bug to remove this as well in the (distant) future.

@@ +3439,5 @@
> +          // add the missing xpath key
> +          formdata = { xpath: {}, id: aData.formdata.id };
> +        } else if (Object.keys(formdata).length != 2) {
> +          // remove the old format keys
> +          formdata = { xpath: aData.formdata.xpath, id: aData.formdata.id };

All these branches seem to deal with situations that should never happen if the user didn't manipulate his saved form data. In this case I'd say we should just not touch it if we have they "id" and "xpath" keys.

Comment 10

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Tim Taubert [:ttaubert] from comment #8)
> He accidentally a word. He wanted you to push the patches to the try server
> to see if tests are failing before we turn the tree red. I just did this for
> you :)
> 
> https://tbpl.mozilla.org/?tree=Try&rev=a1ce76c33852

Try is all green.

Comment 11

[Andres Hernandez [:andreshm]]

(In reply to Tim Taubert [:ttaubert] from comment #9)
> @@ +3439,5 @@
> > +          // add the missing xpath key
> > +          formdata = { xpath: {}, id: aData.formdata.id };
> > +        } else if (Object.keys(formdata).length != 2) {
> > +          // remove the old format keys
> > +          formdata = { xpath: aData.formdata.xpath, id: aData.formdata.id };
> 
> All these branches seem to deal with situations that should never happen if
> the user didn't manipulate his saved form data. In this case I'd say we
> should just not touch it if we have they "id" and "xpath" keys.

I agree, that most of these situations should never happen under normal circumstances. But, Paul previously asked (comment 6) to ensure that there no other keys:
"Can you also ensure that there are no other keys (since that's how the old format would be in there)."

That's why I added all possible test cases with combinations of both formats and we are checking that only the new format will be used. 
Let me know then if it's desired to keep the old keys if they are present.

Comment 12

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Andres Hernandez [:andreshm] from comment #11)
> I agree, that most of these situations should never happen under normal
> circumstances. But, Paul previously asked (comment 6) to ensure that there
> no other keys:
> "Can you also ensure that there are no other keys (since that's how the old
> format would be in there)."

I thought he was referring to the test, only?

Comment 13

[Andres Hernandez [:andreshm]]

(In reply to Tim Taubert [:ttaubert] from comment #12)
> I thought he was referring to the test, only?

With |is(Object.keys(restoredFormData).length, 2)| a lot of combination tests would fail, because the restored formData would keep the old keys.

Comment 14

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Andres Hernandez [:andreshm] from comment #13)
> (In reply to Tim Taubert [:ttaubert] from comment #12)
> > I thought he was referring to the test, only?
> 
> With |is(Object.keys(restoredFormData).length, 2)| a lot of combination
> tests would fail, because the restored formData would keep the old keys.

Right, but you're testing invalid combinations here. They shouldn't appear in the wild without manipulating sessionstore.js. Anyway, I'd be fine with keeping those lines but they feel unnecessary.

What's your take on that, Paul?

Comment 15

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

(In reply to Tim Taubert [:ttaubert] from comment #12)
> (In reply to Andres Hernandez [:andreshm] from comment #11)
> > I agree, that most of these situations should never happen under normal
> > circumstances. But, Paul previously asked (comment 6) to ensure that there
> > no other keys:
> > "Can you also ensure that there are no other keys (since that's how the old
> > format would be in there)."
> 
> I thought he was referring to the test, only?

I was.

(In reply to Andres Hernandez [:andreshm] from comment #13)
> (In reply to Tim Taubert [:ttaubert] from comment #12)
> > I thought he was referring to the test, only?
> 
> With |is(Object.keys(restoredFormData).length, 2)| a lot of combination
> tests would fail, because the restored formData would keep the old keys.

Only because collection isn't kicked off again so we have the cached form data saved for the tab. Firing a "change" event on an input should make us clear that cache & recollect. Then the test should pass for the combination cases (add a comment to that effect too).

For the mixed states cached data, I'm fine returning what we got passed in. I care more about the case where we collect the data & ensuring that doesn't have anything else in it.

Comment 16

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

Comment on attachment 619246 [details] [diff] [review]
Patch v4

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

r- just for some cleanup. Next review will be super fast (or if Tim beats me to it, that works too)

::: browser/components/sessionstore/src/nsSessionStore.js
@@ +3420,5 @@
>      function restoreTextDataAndScrolling(aContent, aData, aPrefix) {
>        if (aData.formdata && hasExpectedURL(aContent.document, aData.url)) {
>          let formdata = aData.formdata;
>  
>          // handle backwards compatibility

At the very least, let's add a comment that this is a migration from pre-firefox 15 and reference this conversion bug. (can just do that here, no need to do it in the other bug).

@@ +3439,5 @@
> +          // add the missing xpath key
> +          formdata = { xpath: {}, id: aData.formdata.id };
> +        } else if (Object.keys(formdata).length != 2) {
> +          // remove the old format keys
> +          formdata = { xpath: aData.formdata.xpath, id: aData.formdata.id };

I agree with Tim here. If there's extra data or a missing id/xpath key, then that's ok. So long as there's one or the other. So let's kill these extra checks. (see my previous comment for more info)

Comment 17

[Andres Hernandez [:andreshm]]

Attachment: Updated patch

Removed the extra checks and improved the test.

Comment 18

[Andres Hernandez [:andreshm]]

Attachment: Updated patch

Minor fix to previous patch.

Comment 19

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

Comment on attachment 623843 [details] [diff] [review]
Updated patch

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

Thanks for sticking it out Andres!

Comment 20

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/integration/fx-team/rev/aa466d084fe3

Comment 21

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/aa466d084fe3