1016733 - Implement form auto-fill profile storage

Status: RESOLVED FIXED

(Toolkit :: Form Manager, defect, P3)

Description

[Matthew N. [:MattN]]

Bug 939351 will implement in-memory storage so this will be about implementing persistent storage.

It should be kept in mind that credit card data may be stored in profiles in the future but that doesn't necessarily need to block an initial implementation.

Comment 1

[Luke Chang [:lchang] (inactive)]

Attachment: Bug 1016733 - Implement form auto-fill profile storage;

Review commit: https://reviewboard.mozilla.org/r/83538/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/83538/

Comment 2

[Luke Chang [:lchang] (inactive)]

Comment on attachment 8797948 [details]
Bug 1016733 - Implement form auto-fill profile storage;

Hi Matt,

This patch implements an interface of the storage with basic CRUD methods. Note that it depends on JSONStore.jsm implemented in bug 1304322.

It would be great to hear your thoughts and feedback before moving on. Thanks.

Comment 3

[Matthew N. [:MattN]]

Comment on attachment 8797948 [details]
Bug 1016733 - Implement form auto-fill profile storage;

https://reviewboard.mozilla.org/r/83538/#review83304

This is a good start.

::: browser/extensions/formautofill/content/ProfileStorage.jsm:41
(Diff revision 1)
> + *   payments: [
> + *     // out of scope
> + *   ]

Nit: I wouldn't mention this until we implement it

::: browser/extensions/formautofill/content/ProfileStorage.jsm:104
(Diff revision 1)
> +XPCOMUtils.defineLazyModuleGetter(this, "JSONStore",
> +                                  "resource://gre/modules/JSONStore.jsm");

Nit: I guess you didn't rebase upon the rename yet.

::: browser/extensions/formautofill/content/ProfileStorage.jsm:115
(Diff revision 1)
> +                                   "@mozilla.org/uuid-generator;1",
> +                                   "nsIUUIDGenerator");
> +
> +const DB_VERSION = 1;
> +const VALID_FIELD = [
> +  "givenName", "additionalName", "familyName", "organization", "streetAddress",

As we discussed before, I think we should not implement names for M1 to reduce scope.

::: browser/extensions/formautofill/content/ProfileStorage.jsm:119
(Diff revision 1)
> +function ProfileStorage() {
> +}
> +
> +ProfileStorage.prototype = {
> +  /**
> +   * Loads the profile data from file to memory.
> +   *
> +   * @returns {Promise}
> +   * @resolves When the operation finished successfully.
> +   * @rejects  JavaScript exception.
> +   */
> +  initialize() {
> +    let jsonPath = OS.Path.join(OS.Constants.Path.profileDir,
> +                                "autofill-profile.json");

It probably makes sense to have the file name as an argument to the constructor if you're not making this a Singleton. Then in automated tests it's easy to test against testing JSON files and check they were read properly.

::: browser/extensions/formautofill/content/ProfileStorage.jsm:141
(Diff revision 1)
> +  /**
> +   * Adds a new profile to storage according to {Profile}.
> +   *
> +   * @param {Profile} profile
> +   *        The new profile for saving.
> +   */
> +  addWithProfile(profile) {

I think `add` is sufficient

::: browser/extensions/formautofill/content/ProfileStorage.jsm:152
(Diff revision 1)
> +    // TODO: needs to change to 12-chars GUID
> +    profileToSave.guid = gUUIDGenerator.generateUUID().toString();

gUUIDGenerator.generateUUID().toString().replace(/[{}-]/g, "").substring(0, 12);

::: browser/extensions/formautofill/content/ProfileStorage.jsm:166
(Diff revision 1)
> +  /**
> +   * Adds a new profile to storage according to {Fields}
> +   *
> +   * @param {Fields} fields
> +   *        The {Fields} whose all "value" are set for saving.
> +   */
> +  addWithFields(fields) {
> +    this.addWithProfile(this._fieldsToProfile(fields));
> +  },

I think the storage module shouldn't know anything about form/fields and we will have a glue module between content and storage (similar to LoginManagerParent)

::: browser/extensions/formautofill/content/ProfileStorage.jsm:253
(Diff revision 1)
> +  /**
> +   * Returns all profiles and fills in "fields" if specified.

Add getAll for management UI which doesn't know about fields.

::: browser/extensions/formautofill/content/ProfileStorage.jsm:346
(Diff revision 1)
> +  _fillInFields(fields, profile) {
> +    if (!Array.isArray(fields)) {
> +      // Returns the raw profile data if "fields" is omitted. Data is cloned, so
> +      // it won't be accidentally modified outside.
> +      return Object.assign({}, profile);
> +    }
> +
> +    let meta = this._getMeta(profile);
> +    fields = fields.map(field => {
> +      let value = this._getDataByFieldName(profile, field.fieldName);
> +      if (value !== undefined) {
> +        field.value = value;
> +      }
> +      return field;
> +    });
> +
> +    return {meta, fields};
> +  },

This and related helpers can move to bug 1300990 for the glue module in the parent that receives messages from the child and connects with storage.

Comment 4

[Luke Chang [:lchang] (inactive)]

Comment on attachment 8797948 [details]
Bug 1016733 - Implement form auto-fill profile storage;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/83538/diff/1-2/

Comment 5

[Luke Chang [:lchang] (inactive)]

Matt,

Thanks for your feedback. I've updated my patch according to the comments. I'll add some tests for it soon.

Comment 6

[Luke Chang [:lchang] (inactive)]

I've added tests in my WIP patch [1]. Since it's based on several config files that introduced in Bug 1300988, I'll send the review request once that bug lands.

[1] https://github.com/luke-chang/gecko/commit/e1da83866a0ec0bec0b440b1b44166331f33a3a9

Comment 7

[Luke Chang [:lchang] (inactive)]

Comment on attachment 8797948 [details]
Bug 1016733 - Implement form auto-fill profile storage;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/83538/diff/2-3/

Comment 8

[Luke Chang [:lchang] (inactive)]

Hi Matt,

Bug 1300988 has landed, so this patch is ready for review. Could you please take a look? Thanks a lot.

Comment 9

[Matthew N. [:MattN]]

Comment on attachment 8797948 [details]
Bug 1016733 - Implement form auto-fill profile storage;

https://reviewboard.mozilla.org/r/83538/#review86878

My main concern is the cast to a String

::: browser/extensions/formautofill/content/ProfileStorage.jsm:18
(Diff revision 3)
> + *       givenName,
> + *       additionalName,   // Middle Name(s)
> + *       familyName,

Please remove these for now

::: browser/extensions/formautofill/content/ProfileStorage.jsm:31
(Diff revision 3)
> + *       country,          // ISO 3166
> + *       tel,
> + *       email,
> + *
> + *       // meta
> + *       timeCreated,      // in ms (Align with nslLoginMetaInfo)

Nit: remove " (Align with nslLoginMetaInfo)"

::: browser/extensions/formautofill/content/ProfileStorage.jsm:58
(Diff revision 3)
> +
> +XPCOMUtils.defineLazyServiceGetter(this, "gUUIDGenerator",
> +                                   "@mozilla.org/uuid-generator;1",
> +                                   "nsIUUIDGenerator");
> +
> +const DB_VERSION = 1;

Nit: rename to SCHEMA_VERSION since there's no real DB

::: browser/extensions/formautofill/content/ProfileStorage.jsm:61
(Diff revision 3)
> +                                   "nsIUUIDGenerator");
> +
> +const DB_VERSION = 1;
> +
> +// Name-related fields will be handled in follow-up bugs due to the complexity.
> +const VALID_FIELD = [

Nit: VALID_FIELDS?

::: browser/extensions/formautofill/content/ProfileStorage.jsm:106
(Diff revision 3)
> +    let profileToSave = this._normalizeProfile(profile);
> +
> +    profileToSave.guid = gUUIDGenerator.generateUUID().toString()
> +                                       .replace(/[{}-]/g, "").substring(0, 12);
> +
> +    // Meta data

Nit: metadata is one word (in other places of this file too)

::: browser/extensions/formautofill/content/ProfileStorage.jsm:108
(Diff revision 3)
> +    profileToSave.timeLastUsed = 0;
> +    profileToSave.timeLastModified = 0;
> +    profileToSave.timesUsed = 0;

When we create from a form I think we should probably record that as a usage too (like we do for password manager)

::: browser/extensions/formautofill/content/ProfileStorage.jsm:109
(Diff revision 3)
> +                                       .replace(/[{}-]/g, "").substring(0, 12);
> +
> +    // Meta data
> +    profileToSave.timeCreated = Date.now();
> +    profileToSave.timeLastUsed = 0;
> +    profileToSave.timeLastModified = 0;

In pwmgr we set the timeLastModified = timeCreated so we don't have to special-case 0 when display dates. It would probably be useful here too.

i.e. an never modified profile has `timeCreated === timeLastModified` instead of `timeLastModified == 0`.

::: browser/extensions/formautofill/content/ProfileStorage.jsm:169
(Diff revision 3)
> +   * Removes the specified profile.
> +   *
> +   * @param  {string} guid
> +   *         Indicates which profile to remove.

Perhaps we should note that no error occurs if a profile with the specified guid doesn't exist?

::: browser/extensions/formautofill/content/ProfileStorage.jsm:187
(Diff revision 3)
> +   * @returns {Profile}
> +   *          The profile.

Note that it's a clone

::: browser/extensions/formautofill/content/ProfileStorage.jsm:193
(Diff revision 3)
> +    let profileFound = this._findByGUID(guid);
> +    if (profileFound) {
> +      // Profile is cloned to avoid accidental modifications from outside.
> +      return this._clone(profileFound);
> +    }

I think the normal path should be to return the clone but we should throw if the guid doesn't match any records.

::: browser/extensions/formautofill/content/ProfileStorage.jsm:203
(Diff revision 3)
> +   * @returns {Array.<Profile>}
> +   *          An array containing all {Profile}s.
> +   */

Mention that it's a clone here too

::: browser/extensions/formautofill/content/ProfileStorage.jsm:223
(Diff revision 3)
> +    for (let key in profile) {
> +      if (VALID_FIELD.includes(key) && (typeof profile[key] === "string" ||
> +                                        typeof profile[key] === "number")) {
> +        result[key] = String(profile[key]);
> +      }
> +    }

Did you consider throwing so we can catch typos? I think it's better to be more restrictive from the beginning and loosen checks as needed as it catches errors in the meantime.

::: browser/extensions/formautofill/content/ProfileStorage.jsm:226
(Diff revision 3)
> +  _normalizeProfile(profile) {
> +    let result = {};
> +    for (let key in profile) {
> +      if (VALID_FIELD.includes(key) && (typeof profile[key] === "string" ||
> +                                        typeof profile[key] === "number")) {
> +        result[key] = String(profile[key]);

Do we really want to represent numbers as strings? It seems like that's what will happen with numeric metadata fields when called by `update`

::: browser/extensions/formautofill/test/unit/test_profileStorage.js:1
(Diff revision 3)
> +/* global ProfileStorage */

I may be wrong but isn't it preferred to put this on or near the line that defines the global e.g. the import line?

::: browser/extensions/formautofill/test/unit/test_profileStorage.js:87
(Diff revision 3)
> +  do_check_eq(profiles[0].timesUsed, 0);
> +
> +  do_check_eq(profiles[1].invalidField, undefined);
> +});
> +
> +add_task(function* test_update() {

Can you test some exception cases with `Assert.throws` in this file too?

Comment 10

[Luke Chang [:lchang] (inactive)]

Comment on attachment 8797948 [details]
Bug 1016733 - Implement form auto-fill profile storage;

https://reviewboard.mozilla.org/r/83538/#review86878

> When we create from a form I think we should probably record that as a usage too (like we do for password manager)

Makes sense! Thanks! In addition, since the "add" action is treated as the first use, should I also record the timestamp to "timeLastUsed"?

> Did you consider throwing so we can catch typos? I think it's better to be more restrictive from the beginning and loosen checks as needed as it catches errors in the meantime.

You're right. Thanks for pointing it out. BTW, should we also throw errors if the profile contains metadata fields (since it shouldn't participate in the further actions)? We won't be able to simply treat the output profile as another input profile though.

> Do we really want to represent numbers as strings? It seems like that's what will happen with numeric metadata fields when called by `update`

Was thinking the fields in a profile are all supposed to be strings (metadata are excluded). However, I don't have strong opinions on this especially when containing numbers in profile won't cause any problems. I'll leave it as-is.

> I may be wrong but isn't it preferred to put this on or near the line that defines the global e.g. the import line?

It looks like that it's put in the beginning in most files (some are put before "use strict" and the others are after that). Since we're a new add-on, which convention do you think we prefer?

> Can you test some exception cases with `Assert.throws` in this file too?

Sure.

Comment 11

[Luke Chang [:lchang] (inactive)]

Comment on attachment 8797948 [details]
Bug 1016733 - Implement form auto-fill profile storage;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/83538/diff/3-4/

Comment 12

[Luke Chang [:lchang] (inactive)]

Hi Matt,

Thanks for your review. I've updated my patch accordingly and have some questions in comment 10. Also, the test suite has been changed a bit. Would you mind taking a look again? Thanks.

Comment 13

[Luke Chang [:lchang] (inactive)]

Comment on attachment 8797948 [details]
Bug 1016733 - Implement form auto-fill profile storage;

https://reviewboard.mozilla.org/r/83538/#review86878

> Makes sense! Thanks! In addition, since the "add" action is treated as the first use, should I also record the timestamp to "timeLastUsed"?

Another question just occurred to me. Should we set "timesUsed" to 1 as well if "add" is called from the creating dialog in Preferences?

Comment 14

[Matthew N. [:MattN]]

Comment on attachment 8797948 [details]
Bug 1016733 - Implement form auto-fill profile storage;

https://reviewboard.mozilla.org/r/83538/#review89954

Comment 15

[Matthew N. [:MattN]]

Comment on attachment 8797948 [details]
Bug 1016733 - Implement form auto-fill profile storage;

https://reviewboard.mozilla.org/r/83538/#review86878

> Another question just occurred to me. Should we set "timesUsed" to 1 as well if "add" is called from the creating dialog in Preferences?

*shrugs* I guess if timeLastUsed is set to the time its added then the timesUsed should be >0 to align? I don't have strong feelings about this especially if we don't show it to users (we do show it optionally in the pwmgr). It's probably easiest to have non-zero dates at all times so we can do that if you want.

> You're right. Thanks for pointing it out. BTW, should we also throw errors if the profile contains metadata fields (since it shouldn't participate in the further actions)? We won't be able to simply treat the output profile as another input profile though.

Do you mean fields like timesUsed or a custom property that autofill doesn't use? I'm not exactly sure what you're thinking though. I don't think this decision needs to block landing so feel free to autoland and we can change this later if we want.

> Was thinking the fields in a profile are all supposed to be strings (metadata are excluded). However, I don't have strong opinions on this especially when containing numbers in profile won't cause any problems. I'll leave it as-is.

Well I'm thinking that metadata fields like timesUsed and the date fields should be Numbers. I agree that things like the phone number should be strings.

> It looks like that it's put in the beginning in most files (some are put before "use strict" and the others are after that). Since we're a new add-on, which convention do you think we prefer?

I would rather we didn't need it at all and eslint would know that the global exists :) I don't have strong feelings so it's up to you.

Comment 16

[Luke Chang [:lchang] (inactive)]

Comment on attachment 8797948 [details]
Bug 1016733 - Implement form auto-fill profile storage;

https://reviewboard.mozilla.org/r/83538/#review86878

> Do you mean fields like timesUsed or a custom property that autofill doesn't use? I'm not exactly sure what you're thinking though. I don't think this decision needs to block landing so feel free to autoland and we can change this later if we want.

The scenario is:

```js
let profile = profileStorage.get("some-guid");
profile.tel = "some new tel";
profileStorage.update("some-guid", profile); // this will cause the exception because the retrieved "profile" contains metadata such as "guid" that isn't in VALID_FIELDS defined above 
```

My thought is to make valid metadata field not cause throwing. That is, there are three types of properties in "profile" when calling update:

1. valid profile field such as "tel", "email" - will be updated accordingly.
2. valid metadata field such as "guid", "timeCreated" - will be ignored but won't throw any error
3. any other field - will throw error to prevent typo

However, I'm afraid it misleads users that metadata is updatable. Anyway, agree with you that it shouldn't be a blocker. I'll keep throwing on any invalid field including metadata for now.

Comment 17

[Luke Chang [:lchang] (inactive)]

Comment on attachment 8797948 [details]
Bug 1016733 - Implement form auto-fill profile storage;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/83538/diff/4-5/

Comment 18

[Luke Chang [:lchang] (inactive)]

Comment on attachment 8797948 [details]
Bug 1016733 - Implement form auto-fill profile storage;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/83538/diff/5-6/

Comment 19

[Luke Chang [:lchang] (inactive)]

Comment on attachment 8797948 [details]
Bug 1016733 - Implement form auto-fill profile storage;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/83538/diff/6-7/

Comment 20

[Luke Chang [:lchang] (inactive)]

Patch is updated. Matt, thanks for the review.

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d22ae5961c62
Implement form auto-fill profile storage; r=MattN

Comment 22

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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