697903 - Move form data functions from sessionstore into JSM

Status: RESOLVED FIXED

(Firefox :: Session Restore, defect)

Description

[Gregory Szorc [:gps]]

Attachment: Move form data APIs into new DocumentUtils JSM, v0

I need to perform form data management in Sync code and found that moving the form data APIs outside of nsSessionStore.js made them easier to consume.

Attached is a first draft at doing this. A couple of the mochitests fail, so it is buggy.

This is the first time I've played with this part of the tree and I want to get feedback as early in the process as possible. I figure I'm missing tests for the new JSM. What else do I need to do? Is the name and location of the new JSM acceptable?

Comment 1

[Gregory Szorc [:gps]]

Adding Gavin for e10s feedback, as I know nothing about that and probably violate it horribly with this patch.

Also, my local version fixes the failing existing unit tests. Now, I just need to write some new tests for the new module and I should hopefully be ready for a real review.

Comment 2

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

Ultimately, I think it's great to have this separated, but...

With my limited e10s knowledge, this is almost certainly incompatible. Bug 516755 was moving desktop sessionstore --> e10s compatible and all of the form stuff is in the content script (because chrome won't be able to touch content directly). Since mobile is already e10s compatible, I don't think that you'll be able to use this there anyway.

Gavin will know better, but I think it's possible to use JSMs from content scripts (or something...). If so, we should press forward in that direction while I push sessionstore to be e10s compat.

Comment 3

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

Comment on attachment 570126 [details] [diff] [review]
Move form data APIs into new DocumentUtils JSM, v0

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

And now e10s isn't a thing we're doing so much anymore... I'm still concerned about mobile uses. In the current xul-based world, this probably won't be usable. And I have no idea what the native world looks like or if this is going to be usable there. If this is just going to be used on desktop, it might make sense to make it a module in /browser.

Otherwise, I think doing this is a good step. I've wanted to compartmentalize parts of sessionstore anyway.

FYI, I bitrotted you with bug 640136. It shouldn't be too bad though.

f+ but I'd like to have Gavin give some feedback about where modules live, e10s, & whatnot.

::: toolkit/mozapps/shared/DocumentUtils.jsm
@@ +18,5 @@
> + * Portions created by the Initial Developer are Copyright (C) 2011
> + * the Initial Developer. All Rights Reserved.
> + *
> + * Contributor(s):
> + *  Gregory Szorc <[e-mail]>

Some license comments:
* The original code is actually sessionstore code, right? For the most part this isn't new code.
* Initial Developer may still be Simon in that case.
* The contributor list may need to reflect the original code.

These just jumped out at me as feeling wrong. I'm not a license expert in any sense so we may want to have somebody (Gerv?) give better feedback.

Comment 4

[Gregory Szorc [:gps]]

I think I told Paul offline, but this is off my radar because push to device is in a waiting pattern. If someone else wants to pick it up, please do.

Comment 5

[Tim Taubert [:ttaubert] (inactive)]

Gavin, Paul: just FTR, what's the current state of this patch? Can we just pick it up and continue? What about e10s and mobile compatibility?

Comment 6

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 570126 [details] [diff] [review]
Move form data APIs into new DocumentUtils JSM, v0

This patch should be updated to use XPathGenerator.jsm now that bug 726235 has landed.

Comment 7

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

(In reply to Tim Taubert [:ttaubert] from comment #5)
> Gavin, Paul: just FTR, what's the current state of this patch? Can we just
> pick it up and continue? What about e10s and mobile compatibility?

Like you commented, using XPathGenerator is needed and perhaps some tweaks around that. Otherwise it should be fine to just pick up and go. I think e10s is a non-issue at this point. If that's something we do again in the future we can handle it then.

Comment 8

[Andres Hernandez [:andreshm]]

I'm going to start working on this bug. I'll comment later my findings or questions.

Comment 9

[Andres Hernandez [:andreshm]]

Hi, I have a working version and I'm fixing the failing tests. But I have a question, in the original patch, when restoring the forms values, the "change" events were removed. There is a note: 
// NB: dispatching "change" events might have unintended side-effects 
So, should I keep it like that and fix the events test or should I add the events back, like in the current version?

Comment 10

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

(In reply to Andres Hernandez from comment #9)
> So, should I keep it like that and fix the events test or should I add the
> events back, like in the current version?

The events should be added back. That code changed in bug 640136, which landed after the patch was added here.

Comment 11

[Andres Hernandez [:andreshm]]

Attachment: Previous patch update

Comment 12

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 610769 [details] [diff] [review]
Previous patch update

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

Thank you, this looks good to me! The things I mentioned below are mostly code style issues but there's also a question and an addition regarding backwards compatibility.

f+ with all the code and style issues fixed. Alas, I can't give you r+ for session store code, please ask :zpao for review on the next iteration.

::: browser/components/sessionstore/src/DocumentUtils.jsm
@@ +32,5 @@
> + * and other provisions required by the GPL or the LGPL. If you do not delete
> + * the provisions above, a recipient may use your version of this file under
> + * the terms of any one of the MPL, the GPL or the LGPL.
> + *
> + * ***** END LICENSE BLOCK ***** */

We should pick one of the newer license headers: http://www.mozilla.org/MPL/headers/

@@ +42,5 @@
> +Components.utils.import("resource:///modules/sessionstore/XPathGenerator.jsm");
> +
> +const Cc = Components.classes;
> +const Ci = Components.interfaces;
> +const Cr = Components.results;

const Cu = Components.utils;

Cu.import("resource://gre/modules/XPCOMUtils.jsm");
Cu.import("resource:///modules/sessionstore/XPathGenerator.jsm");

@@ +59,5 @@
> +   *         DOMDocument instance to obtain form data for.
> +   * @return object
> +   *         Form data encoded in an object.
> +   */
> +  getFormData: function getFormData(document) {

Nit: getFormData: function DocumentUtils_getFormData(aDocument) {

So it's easier to read stack traces when debugging and 'aDocument' matches the desired coding conventions in the browser.

@@ +71,5 @@
> +    let ret = {id: {}, xpath: {}};
> +    let node = formNodes.iterateNext();
> +    if (!node) {
> +      return ret;
> +    }

Instead of this and the do-while loop below, we should just use |while (node = formNodes.iterateNext()) {| and return ret at the end of this method.

@@ +155,5 @@
> +   *         DOMDocument instance to which to restore form data.
> +   * @param  data
> +   *         Object defining form data.
> +   */
> +  mergeFormData: function setFormData(document, data) {

Nit: copy and paste error, also better:

mergeFormData: function DocumentUtils_mergeFormData(aDocument, aData) {

@@ +160,5 @@
> +    for each (let [xpath, value] in Iterator(data.xpath)) {
> +      let node = XPathGenerator.resolve(document, xpath);
> +      if (!node) {
> +        continue;
> +      }

Better:

if (node)
  this.restoreFormValue(node, value, document);

@@ +169,5 @@
> +    for each (let [id, value] in Iterator(data.id)) {
> +      let node = document.getElementById(id);
> +      if (!node) {
> +        continue;
> +      }

Better:

if (node)
  this.restoreFormValue(node, value, document);

@@ +191,5 @@
> +   * @param  document [optional]
> +   *         DOMDocument node belongs to. If not defined, node.ownerDocument
> +   *         is used.
> +   */
> +  restoreFormValue: function restoreFormValue(node, value, document) {

restoreFormValue: function DocumentUtils_restoreFormValue(aNode, aValue, aDocument) {

@@ +207,5 @@
> +      eventType = "input";
> +    } else if (typeof value == "boolean") {
> +      // Don't dispatch a change event for no change.
> +      if (node.checked == value) {
> +        return

Nit: missing semicolon.

::: browser/components/sessionstore/src/nsSessionStore.js
@@ +145,5 @@
>  
>  XPCOMUtils.defineLazyGetter(this, "XPathGenerator", function() {
>    Cu.import("resource:///modules/sessionstore/XPathGenerator.jsm");
>    return XPathGenerator;
>  });

This can be removed as we don't need it anymore in the session store service.

@@ +149,5 @@
>  });
> +XPCOMUtils.defineLazyGetter(this, "DocumentUtils", function() {
> +  Cu.import("resource:///modules/sessionstore/DocumentUtils.jsm");
> +  return DocumentUtils;
> +});

XPCOMUtils.defineLazyModuleGetter(this, "DocumentUtils", "resource:///modules/sessionstore/DocumentUtils.jsm");

@@ +2181,5 @@
> +        }
> +
> +        // For backwards compatibility in SessionStore, the structure of the
> +        // stored object is modified slightly.
> +        if (Object.keys(formData).length) {

Isn't this always true because formData always has the keys 'id' and 'xpath'?

@@ +3340,5 @@
>      let selectedPageStyle = aBrowser.__SS_restore_pageStyle;
>      function restoreTextDataAndScrolling(aContent, aData, aPrefix) {
> +      if (aData.formdata && hasExpectedURL(aContent.document, aData.url)) {
> +        let data = {xpath: {}, id: {}};
> +        for each (let [key, value] in Iterator(aData.formdata)) {

To be able to remove this backwards compatibility some time in the future we should probably also support our new 'id/xpath' format here. You could do something like:

let formdata = aData.formdata;
if (!("xpath" in formdata && "id" in formdata)) {
  formdata = // .. convert from old format
}

DocumentUtils.mergeFormData(aContent.document, formdata);

Also, we should file a follow-up bug about removing the bc for the old formdata format.

Comment 13

[Andres Hernandez [:andreshm]]

Attachment: Updated patch

The patch includes Tim's suggestions.

Comment 14

[Tim Taubert [:ttaubert] (inactive)]

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

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

Looks good to me! After talking to Paul I now know that I *can* give r+ for session store patches. Let's keep the r? for Paul though to get additional feedback from him.

Comment 15

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

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

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

I'm going to be a stick in the mud for a second, but why is the format changing? I didn't pick up on it in the feedback cycle (I didn't look that closely), but I am now...

I can't see any particular reason (maybe _minor_ perf not running charAt(0), but more nested objects for JSON.*). So if it's just change for change's sake I'm not sure we should do it.

Comment 16

[Gregory Szorc [:gps]]

Structured data should be structured. You shouldn't rely on heuristics (string parsing in this case) if you can avoid it. It is more work for parsers. Separate fields for separate semantics.

Comment 17

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

(In reply to Gregory Szorc [:gps] from comment #16)
> Structured data should be structured. You shouldn't rely on heuristics
> (string parsing in this case) if you can avoid it. It is more work for
> parsers. Separate fields for separate semantics.

To me that says change for change's sake.

I don't disagree that it's a better structured format, but that alone doesn't convince me to change it. If that were the case, I think there are a bunch of other changes I would make too. I've done as much as I can to maintain the output for consumers, which as we discussed a while back was a concern you had for Sync.

Comment 18

[Gregory Szorc [:gps]]

Only the JS API differentiating the different types was changed: someone else filed the bug to change the actual JSON representation. Or, at least it was originally my intent to only change the JS object format and keep the JSON format backwards-compatible. I haven't looked at the current patch...

Comment 19

[Gregory Szorc [:gps]]

In other words, I viewed what we were building in this bug as a new, proper API. Session store would be a consumer of the "proper" API and would bastardize the format for backwards-compatibility reasons. Other consumers (which Sync will be) will use the new, proper API.

Comment 20

[Richard Newman [:rnewman]]

In case you're not aware, zpao: we're about a year in to what will end up being a total rewrite of Sync -- both the guts (to be entirely async) and the authentication later (to BrowserID). You should consider Sync to be pretty mutable, particularly right now.

A lot of the brittleness was around the add-on, which of course no longer applies.

Comment 21

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Paul O'Shannessy [:zpao] from comment #15)
> I'm going to be a stick in the mud for a second, but why is the format
> changing? I didn't pick up on it in the feedback cycle (I didn't look that
> closely), but I am now...

So this patch doesn't really change the format we're storing form data in sessionstore.js. We're only prepared to switch to this format one day we decide to do it.

(In reply to Gregory Szorc [:gps] from comment #19)
> In other words, I viewed what we were building in this bug as a new, proper
> API. Session store would be a consumer of the "proper" API and would
> bastardize the format for backwards-compatibility reasons. Other consumers
> (which Sync will be) will use the new, proper API.

Exactly this. DocumentUtils.jsm uses a new format internally which probably makes more sense to future consumers. SessionStore with the current patch transforms its back to the current format to be backwards compatible.

Comment 22

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

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

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

Thanks all for clarifying and bearing with me. Now that I've stepped back, let's do it. And let's change the file format too (while supporting the old format for some unspecified indefinite time period)

I commented on an issue inline, but to expand on that, I think we should probably land the followup at the same time to reduce the time where we secretly support a new format.

f+, but r- for some smallish things but also tests.

::: browser/components/sessionstore/src/DocumentUtils.jsm
@@ +8,5 @@
> +Components.utils.import("resource:///modules/sessionstore/XPathGenerator.jsm");
> +
> +const Cc = Components.classes;
> +const Ci = Components.interfaces;
> +const Cr = Components.results;

You aren't using Cc or Cr, so let's not set them. You could set Cu and use that to import (it's fine either way, so your call)

::: browser/components/sessionstore/src/nsSessionStore.js
@@ +3339,5 @@
> +      if (aData.formdata && hasExpectedURL(aContent.document, aData.url)) {
> +        let formdata = aData.formdata;
> +
> +        // handle backwards compatibility
> +        if (!("xpath" in formdata && "id" in formdata)) {

I don't think we want to have to have both checks here. I think one or the other should be enough. Neither "id" nor "xpath" are valid xpath expressions (well, at least in SS since we always get to specific node) so they shouldn't occur naturally on their own.

@@ +3358,5 @@
> +            }
> +          }
> +        }
> +
> +        DocumentUtils.mergeFormData(aContent.document, formdata);

So since we're going to start supporting the new format here (if the file has it, we will use it even if we never write it), we should test that it in fact works.

There's also the small problem of what we return from our APIs. If the file had the new object (or that was used via set*) then that's what we'll return for unrestored tabs.

Comment 23

[Andres Hernandez [:andreshm]]

Attachment: Updated patch

Added tests and applied fixes.

Comment 24

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

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

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

Just a couple things to fix in the test and then it's good to go. Thanks Andres!

::: browser/components/sessionstore/test/browser_697903.js
@@ +40,5 @@
> +  let tab = gBrowser.addTab(testURL);
> +  
> +  tab.linkedBrowser.__SS_restore_tab = tab;
> +  tab.linkedBrowser.__SS_restore_data = { url: testURL };
> +  tab.linkedBrowser.__SS_restore_data.formdata = aFormData;

I don't like the tests using internal stuff like this. You should go through the API using ss.setTabState() and wait for SSTabRestored (which fires after form fill).

@@ +49,5 @@
> +    let value1 = doc.getElementById("input1").value;
> +    let value2 = doc.querySelector("input[name=input2]").value;
> +
> +    // test id 
> +    is(value1, aExpectedValues[0], "Restored value by 'id' not valid");

the last param should describe the success case, not the fail.

Comment 25

[Andres Hernandez [:andreshm]]

Attachment: Updated patch

Updated the tests.

Comment 26

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

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

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

Excellent. Now lets get bug 742051 wrapped up and land them together.

Comment 27

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

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

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

I'm back again! I had a couple thoughts while looking at bug 742051. Let's do 2 things to the test here:
1. just rename the test files to be browser_formdata_format.js/_sample.html
2. also add test cases for the old format and combinations (to ensure we don't break that & the right things override)

Comment 28

[Andres Hernandez [:andreshm]]

Attachment: Updated patch

Added old format and combination test cases and updated the file names.

Comment 29

[Tim Taubert [:ttaubert] (inactive)]

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

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

::: browser/components/sessionstore/src/nsSessionStore.js
@@ +149,1 @@
>  });

Can you please un-bitrot this patch and changes it to use XPCOMUtils.defineLazyModuleGetter() here? Thanks!

Comment 30

[Tim Taubert [:ttaubert] (inactive)]

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

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

::: browser/components/sessionstore/src/nsSessionStore.js
@@ +144,5 @@
>  });
>  
> +XPCOMUtils.defineLazyGetter(this, "DocumentUtils", function() {
> +  Cu.import("resource:///modules/sessionstore/DocumentUtils.jsm");
> +  return DocumentUtils;

Um, here, I picked the wrong line, sorry.

Comment 31

[Andres Hernandez [:andreshm]]

Attachment: Updated patch

Comment 32

[Tim Taubert [:ttaubert] (inactive)]

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

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

Thanks!

Comment 33

[Tim Taubert [:ttaubert] (inactive)]

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

Comment 34

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/068b9453c498

Comment 35

[Eric Shepherd [:sheppy]]

Document DocumentUtils.jsm and the new JSON format. See http://zpao.com/posts/session-restore-changes-in-firefox-15/ for details.