Closed Bug 1368008 Opened 7 years ago Closed 7 years ago

[Form Autofill] Cache the computed fields in profileStorage

Categories

(Toolkit :: Form Manager, enhancement, P3)

Product:
Toolkit
Component:
Form Manager
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla56
Tracking Flags:
Tracking Status
firefox56 --- fixed

People

(Reporter: lchang, Assigned: lchang)

References

(Blocks 1 open bug)

Details

(Whiteboard: [form autofill:M3])

Attachments

(1 file)

Bug 1368008 - [Form Autofill] Cache the computed fields in profileStorage.
59 bytes, text/x-review-board-request
MattN
: review+
steveck
: review+
selee
: review+
Details 
I'd like to cache the computed fields of each record in profileStorage so we don't need to compute the same data every time.
Comment on attachment 8871665 [details]
Bug 1368008 - [Form Autofill] Cache the computed fields in profileStorage.

https://reviewboard.mozilla.org/r/143180/#review150760

::: browser/extensions/formautofill/ProfileStorage.jsm:164
(Diff revision 2)
>  
>      this._store = store;
>      this._collectionName = collectionName;
>      this._schemaVersion = schemaVersion;
> +
> +    this._cachedRecords = {};

You should use a Map for this nowadays
(In reply to Matthew N. [:MattN] (PM if requests are blocking you) from comment #3)
> You should use a Map for this nowadays

Good point! Thanks.

I also rebased my patch on the latest central.
Hi Matt,

As we discussed last week, I updated my patch to cache the computed fields in the disk. In this patch, every computed fields are present in a record object even they're empty strings. Thus, it won't break any existing profile in the disk because we'll recompute them upon loading. I also removed the config "noComputedFields" because the computed fields are always available now and it won't benefit the performance anymore.
Comment on attachment 8871665 [details]
Bug 1368008 - [Form Autofill] Cache the computed fields in profileStorage.

https://reviewboard.mozilla.org/r/143180/#review152788

I only hava a concern in constructure that I'm not quite sure about the correct timing for saving the profile, otherwise the patch looks fine for me

::: browser/extensions/formautofill/ProfileStorage.jsm:419
(Diff revision 5)
>  class Addresses extends AutofillRecords {
>    constructor(store) {
> -    super(store, "addresses", VALID_PROFILE_FIELDS, ADDRESS_SCHEMA_VERSION);
> +    super(store, "addresses", VALID_ADDRESS_FIELDS, VALID_ADDRESS_COMPUTED_FIELDS, ADDRESS_SCHEMA_VERSION);
>    }
>  
> -  _recordReadProcessor(profile, {noComputedFields} = {}) {
> +  _computeFields(address) {

About the return value `hasNewComputedFields`: I'm not sure if it's sufficient to save only when new fields are added. Shouldn't we compare the old computed fields and new computed fields to determine whether the computed fields are updated and save as well?

::: browser/extensions/formautofill/ProfileStorage.jsm:464
(Diff revision 5)
>        }
> -      delete profile.name;
> +      delete address.name;
>      }
>  
>      // Normalize address
> -    if (profile["address-line1"] || profile["address-line2"] ||
> +    if (address["address-line1"] || address["address-line2"] ||

Not sure if `this.VALID_COMPUTED_FIELDS.slice(1).some(key => address[key])` would be simpler, maybe it's confusing since it implies the first field is not address related. Or we can declare ["address-line1", "address-line2", "address-line3"] at first because we can reuse it later.

::: browser/extensions/formautofill/ProfileStorage.jsm:475
(Diff revision 5)
> -        profile["address-line1"] = profile["street-address"];
> -        delete profile["street-address"];
> +        address["address-line1"] = address["street-address"];
> +        delete address["street-address"];
>        }
>  
>        // Remove "address-line*" but keep the values.
>        let addressLines = [1, 2, 3].map(i => {

And you can reuse `this.VALID_COMPUTED_FIELDS.slice(1)` here
Attachment #8871665 - Flags: review?(schung)
Comment on attachment 8871665 [details]
Bug 1368008 - [Form Autofill] Cache the computed fields in profileStorage.

https://reviewboard.mozilla.org/r/143180/#review152788

> About the return value `hasNewComputedFields`: I'm not sure if it's sufficient to save only when new fields are added. Shouldn't we compare the old computed fields and new computed fields to determine whether the computed fields are updated and save as well?

We intend to bump the schema version number once the field-computing algorithm changes. When it happens, we'll strip all computed fields and recompute them so we don't need to compute fields every time. You do reminded me that I should write down this conversion in `_computeFields` in case we forget to bump the version number.

> Not sure if `this.VALID_COMPUTED_FIELDS.slice(1).some(key => address[key])` would be simpler, maybe it's confusing since it implies the first field is not address related. Or we can declare ["address-line1", "address-line2", "address-line3"] at first because we can reuse it later.

`VALID_COMPUTED_FIELDS` will have more fields in the future so it's not enough to just strip the first one. Making them an array sounds good.
Comment on attachment 8871665 [details]
Bug 1368008 - [Form Autofill] Cache the computed fields in profileStorage.

https://reviewboard.mozilla.org/r/143180/#review155552

LGTM!

::: browser/extensions/formautofill/ProfileStorage.jsm:494
(Diff revision 6)
> -        let value = profile["address-line" + i];
> -        delete profile["address-line" + i];
> -        return value;
> -      });
> -
>        // Concatenate "address-line*" if "street-address" is omitted.

nit: s/"address-line*"/STREET_ADDRESS_COMPONENTS ? but maybe using address-line* is more straightforward
Attachment #8871665 - Flags: review?(schung) → review+
Comment on attachment 8871665 [details]
Bug 1368008 - [Form Autofill] Cache the computed fields in profileStorage.

https://reviewboard.mozilla.org/r/143180/#review157592

::: browser/extensions/formautofill/ProfileStorage.jsm:137
(Diff revision 6)
>    "cc-number-masked",
>    "cc-exp-month",
>    "cc-exp-year",
>  ];
>  
> +const VALID_CREDIt_CARD_COMPUTED_FIELDS = [

You have a lowercase "t" in CREDIT
Comment on attachment 8871665 [details]
Bug 1368008 - [Form Autofill] Cache the computed fields in profileStorage.

https://reviewboard.mozilla.org/r/143180/#review157632

::: browser/extensions/formautofill/FormAutofillNameUtils.jsm:228
(Diff revision 6)
>  
> +    if (!name) {
> +      return nameParts;
> +    }
> +
> +    let nameTokens = name.trim().split(/[ ,\u3000\u30FB\u00B7]+/);

Could we assume there is no abnormal name here? e.g. ",,,". If not, it looks like `nameTokens` should be checked if there is any empty token. It would be better to add the test for verifying a string with `/[ ,\u3000\u30FB\u00B7]+/` characters only. e.g. ",,,"

::: browser/extensions/formautofill/ProfileStorage.jsm:266
(Diff revision 6)
>        }
>      }
>  
>      recordFound.timeLastModified = Date.now();
>  
> -    this._store.saveSoon();
> +    this._stripComputedFields(recordFound);

Do we want to handle the case of updating a computed field e.g. `name` or `address-line1`? If `stripComputedFields` is always invoked, does this mean users can not change the computed field? BTW, users never know what the computed fields are.

::: browser/extensions/formautofill/ProfileStorage.jsm:389
(Diff revision 6)
>      return this._store.data[this._collectionName].findIndex(record => record.guid == guid);
>    }
>  
> +  _migrateRecord(record) {
> +    let hasChanges = false;
> +    if (record.version < this.version) {

There should be no version "0", so I suppose we need a check that the schema version is less than 1 or not with a corresponding error message. That would be great if adding a test for `_migrateRecord`.

::: browser/extensions/formautofill/ProfileStorage.jsm:600
(Diff revision 6)
> -    }
> +    //       computed fields)
> +
> +    let hasNewComputedFields = false;
>  
>      // Compute split names
> -    if (creditCard["cc-name"]) {
> +    if (!("cc-given-name" in creditCard)) {

This seems to check if there is any computed field already. If there is a record like this:
```JS
{
  "cc-given-name": "Foo",
  "cc-family-name": "",
}
```
This means that this record does not have to do `FormAutofillNameUtils.splitName`. Why don't we consider the same logic for `cc-family-name` here? However, I can understand an empty `cc-additional-name` is quite common.
Attachment #8871665 - Flags: review?(selee)
Comment on attachment 8871665 [details]
Bug 1368008 - [Form Autofill] Cache the computed fields in profileStorage.

https://reviewboard.mozilla.org/r/143180/#review157632

> Could we assume there is no abnormal name here? e.g. ",,,". If not, it looks like `nameTokens` should be checked if there is any empty token. It would be better to add the test for verifying a string with `/[ ,\u3000\u30FB\u00B7]+/` characters only. e.g. ",,,"

It won't cause any problem with empty tokens according to the current algorithm.

> Do we want to handle the case of updating a computed field e.g. `name` or `address-line1`? If `stripComputedFields` is always invoked, does this mean users can not change the computed field? BTW, users never know what the computed fields are.

We update the computed fields in `_normalizeRecord`. Since the computed fields are always bound to their source fields, they can not be updated separately. In theory, the computed fields should not exist in the storage. What we're going to save in the storage are just cache data so it should be able to be stripped and recomputed anytime.

> There should be no version "0", so I suppose we need a check that the schema version is less than 1 or not with a corresponding error message. That would be great if adding a test for `_migrateRecord`.

Yes, there shouldn't be version "0". However, the "version" schema was added after feature enabled on Nightly. I chose not to check if it's greater then 0 for backward compatibility. What do you think?

> This seems to check if there is any computed field already. If there is a record like this:
> ```JS
> {
>   "cc-given-name": "Foo",
>   "cc-family-name": "",
> }
> ```
> This means that this record does not have to do `FormAutofillNameUtils.splitName`. Why don't we consider the same logic for `cc-family-name` here? However, I can understand an empty `cc-additional-name` is quite common.

Unlike normal fields, there is a precondition that all computed fields should be present in the storage even it's empty. In this case, `cc-given-name`, `cc-additional-name` and `cc-family-name` will be computed at once, so I think it's sufficient to choose one of them to check if `cc-name` field has been computed.
Comment on attachment 8871665 [details]
Bug 1368008 - [Form Autofill] Cache the computed fields in profileStorage.

https://reviewboard.mozilla.org/r/143180/#review157688

LGTM! please fix the error message for `_migrateRecord` as we discussed offline.

::: browser/extensions/formautofill/ProfileStorage.jsm:389
(Diff revision 6)
>      return this._store.data[this._collectionName].findIndex(record => record.guid == guid);
>    }
>  
> +  _migrateRecord(record) {
> +    let hasChanges = false;
> +    if (record.version < this.version) {

We should print the error message for any invalid schema version number, e.g. "0". For any invalid version number, `_migrateRecord` should try to correct the record with a valid version.
Attachment #8871665 - Flags: review+
Comment on attachment 8871665 [details]
Bug 1368008 - [Form Autofill] Cache the computed fields in profileStorage.

https://reviewboard.mozilla.org/r/143180/#review157632

> It won't cause any problem with empty tokens according to the current algorithm.

Agree. Please add a test for it if it's not that difficult.
Flags: qe-verify-
Comment on attachment 8871665 [details]
Bug 1368008 - [Form Autofill] Cache the computed fields in profileStorage.

https://reviewboard.mozilla.org/r/143180/#review157632

> Agree. Please add a test for it if it's not that difficult.

Since it lacks more than this test case, I would prefer addressing them in another bug and keep this bug focus on caching stuff.

> Yes, there shouldn't be version "0". However, the "version" schema was added after feature enabled on Nightly. I chose not to check if it's greater then 0 for backward compatibility. What do you think?

I've added unit tests for `_migrateRecord`. Thanks.
Blocks: 1374500
Blocks: 1363999
No longer blocks: 1374500
Comment on attachment 8871665 [details]
Bug 1368008 - [Form Autofill] Cache the computed fields in profileStorage.

https://reviewboard.mozilla.org/r/143180/#review162314

rs=me
Attachment #8871665 - Flags: review?(MattN+bmo) → review+
Pushed by lchang@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5a881c510fa7
[Form Autofill] Cache the computed fields in profileStorage. r=MattN,seanlee,steveck
https://hg.mozilla.org/mozilla-central/rev/5a881c510fa7
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox56: --- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: