The default bug view has changed. See this FAQ.

Implement form auto-fill profile storage

RESOLVED FIXED in Firefox 52

Status

()

Toolkit
Form Manager
P3
normal
RESOLVED FIXED
3 years ago
5 months ago

People

(Reporter: MattN, Assigned: lchang)

Tracking

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

unspecified
mozilla52
Points:
5
Dependency tree / graph
Bug Flags:
firefox-backlog +
qe-verify -

Firefox Tracking Flags

(firefox52 fixed)

Details

(Whiteboard: [form autofill:M1][ETA=10/21])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

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.
Flags: firefox-backlog+
QA Whiteboard: [qa-]
Blocks: 1018304
No longer blocks: 939351

Updated

3 years ago
Whiteboard: p=5 → [form autofill] p=5

Updated

2 years ago
Points: --- → 5
QA Whiteboard: [qa-]
Flags: qe-verify-
Whiteboard: [form autofill] p=5 → [form autofill]

Updated

7 months ago
Blocks: 1299819
Whiteboard: [form autofill] → [form autofill:M1]
Depends on: 1301544
(Assignee)

Updated

6 months ago
Depends on: 1304322
Priority: -- → P3
(Assignee)

Updated

6 months ago
Assignee: nobody → lchang
Status: NEW → ASSIGNED
(Assignee)

Updated

6 months ago
Whiteboard: [form autofill:M1] → [form autofill:M1][ETA=10/21]
Iteration: --- → 52.3 - Nov 7
Comment hidden (mozreview-request)
(Assignee)

Comment 2

6 months ago
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.
Attachment #8797948 - Flags: feedback?(MattN+bmo)
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.
Attachment #8797948 - Flags: feedback?(MattN+bmo) → feedback+
Comment hidden (mozreview-request)
(Assignee)

Comment 5

5 months ago
Matt,

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

Comment 6

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

Updated

5 months ago
Depends on: 1309481
Comment hidden (mozreview-request)
(Assignee)

Comment 8

5 months ago
Hi Matt,

Bug 1300988 has landed, so this patch is ready for review. Could you please take a look? Thanks a lot.
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?
Attachment #8797948 - Flags: review?(MattN+bmo) → review+
(Assignee)

Comment 10

5 months ago
mozreview-review-reply
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 hidden (mozreview-request)
(Assignee)

Comment 12

5 months ago
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.
Flags: needinfo?(MattN+bmo)
(Assignee)

Comment 13

5 months ago
mozreview-review-reply
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 on attachment 8797948 [details]
Bug 1016733 - Implement form auto-fill profile storage;

https://reviewboard.mozilla.org/r/83538/#review89954
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.
Flags: needinfo?(MattN+bmo)
(Assignee)

Comment 16

5 months ago
mozreview-review-reply
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 20

5 months ago
Patch is updated. Matt, thanks for the review.
(Assignee)

Updated

5 months ago
Keywords: checkin-needed

Comment 21

5 months ago
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d22ae5961c62
Implement form auto-fill profile storage; r=MattN
Keywords: checkin-needed

Comment 22

5 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/d22ae5961c62
Status: ASSIGNED → RESOLVED
Last Resolved: 5 months ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in before you can comment on or make changes to this bug.