Add "sync tracker" support to ProfileStorage

RESOLVED FIXED in Firefox 56

Status

()

RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: markh, Assigned: tcsc)

Tracking

unspecified
mozilla56
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox56 fixed)

Details

(Whiteboard: [form autofill:M4])

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

2 years ago
Sync needs to track various changes to profilestorage items so it can work out what to sync and when. We've prototyped this, but it should land as its own bug with its own tests. It will wait for Luke's refactor.
Comment hidden (mozreview-request)
(Reporter)

Comment 2

2 years ago
Comment on attachment 8878406 [details]
Bug 1363999 - Add sync metadata to formautofill records.

The sync metadata we need - hopefully the commit message and tests are self-explanatory. For additional context, see https://github.com/mhammond/gecko/tree/formautofill

I think this is actually ready for review, but I fully expect some things you'd all like changing, so only requesting feedback for now.

(This is mostly Thom's work, but he's on PTO, so I thought I'd get it up anyway)
Attachment #8878406 - Flags: feedback?(selee)
Attachment #8878406 - Flags: feedback?(lchang)
(Reporter)

Comment 3

2 years ago
mozreview-review
Comment on attachment 8878406 [details]
Bug 1363999 - Add sync metadata to formautofill records.

https://reviewboard.mozilla.org/r/149748/#review154436

::: browser/extensions/formautofill/ProfileStorage.jsm:219
(Diff revision 1)
> +        } else {
> +          throw new Error(`Record ${record.guid} already exists`);
> +        }
> +      }
> +      let recordToSave = Object.assign({
> +        // `timeLastUsed` and `timesUsed` are always local.

I think values from `record` will win here, and I think we carry them in sync (which we maybe shouldn't!)

::: browser/extensions/formautofill/ProfileStorage.jsm:223
(Diff revision 1)
> +      let recordToSave = Object.assign({
> +        // `timeLastUsed` and `timesUsed` are always local.
> +        timeLastUsed: 0,
> +        timesUsed: 0,
> +      }, record);
> +      return this._saveRecord(recordToSave, {sourceSync});

should we call `_recordWriteProcessor()` here? A normal "update" does (via normalizeRecord - but that won't allow us to specify a GUID)

::: browser/extensions/formautofill/ProfileStorage.jsm:482
(Diff revision 1)
> +      let tombstone = {
> +        guid,
> +        timeLastModified: Date.now(),
> +        deleted: true,
> +      };
> +      this._store.data[this._collectionName].push(tombstone);

the 2 lines below are still manipulating tombstone, so this should move below them for "cleanliness" sake :)

::: browser/extensions/formautofill/test/unit/test_storage_syncfields.js:64
(Diff revision 1)
> +  address = profileStorage.addresses.getAll({includePrivateFields: true})[0];
> +  equal(address._sync.changeCounter, 1);
> +});
> +
> +add_task(async function test_changeCounter_update() {
> +  // This test should probably also check .status, and need splitting so

also, there's no .status anymore. Probably just kill this test.
Whiteboard: [form autofill:MVP]

Comment 4

2 years ago
mozreview-review
Comment on attachment 8878406 [details]
Bug 1363999 - Add sync metadata to formautofill records.

https://reviewboard.mozilla.org/r/149748/#review157634

The patch looks good to me and needs a rebase for ProfileStorage. The rebase task should be controlled in a reasonable and expected scope, so we need a discussion to figure out the order of Sync and ProfileStorage patches during all-hands.

::: browser/extensions/formautofill/ProfileStorage.jsm:246
(Diff revision 1)
> +    recordToSave.timesUsed = 0;
> +
> +    return this._saveRecord(recordToSave);
> +  }
> +
> +  _saveRecord(record, {sourceSync = false} = {}) {

`_saveRecord` seems for adding operation only. Do we need a more specific name for `add` only?

After saving a record, `_computeFields` (see bug 1368008, it was _recordReadProcessor) should be invoked to make sure the following `get` or `getAll` operation will provide the computed values.

::: browser/extensions/formautofill/ProfileStorage.jsm:402
(Diff revision 1)
> +   * @param   {boolean} [options.includePrivateFields = false]
> +   *          Should fields starting with underscore be included in the result?
>     * @returns {Object}
>     *          A clone of the record.
>     */
> -  get(guid) {
> +  get(guid, {includePrivateFields = false} = {}) {

Just curious. Is there any use-case of `includePrivateFields = true`? IIUC, it's only for test. BTW, I am fine with this option.
Attachment #8878406 - Flags: feedback?(selee) → feedback+

Comment 5

2 years ago
mozreview-review
Comment on attachment 8878406 [details]
Bug 1363999 - Add sync metadata to formautofill records.

https://reviewboard.mozilla.org/r/149748/#review158704

Overall it looks good. Sorry for the late reply.

::: browser/extensions/formautofill/ProfileStorage.jsm:430
(Diff revision 1)
> +   * @param   {boolean} [config.includePrivateFields = false]
> +   *          Should fields starting with underscore be included in the result?
>     * @returns {Array.<Object>}
>     *          An array containing clones of all records.
>     */
> -  getAll({noComputedFields = false, includeDeleted = false} = {}) {
> +  getAll({noComputedFields = false, includeDeleted = false, includePrivateFields = false} = {}) {

I'm going to introduce an option `rawData` in bug 1368008 to replace `noComputedFields`. It indicates that the returned data is untouched, without the modifications by `_recordReadProcessor` and the computed fields. I think this option will be used by sync function only so I suspect that `rawData` can also cover `includePrivateFields`. What do you think?

::: browser/extensions/formautofill/ProfileStorage.jsm:680
(Diff revision 1)
>        }
>      }
>    }
>  
> +  // A test-only helper.
> +  _nukeAllRecords() {

It's not used in this patch. Will it be used in the follow-up patches?
Attachment #8878406 - Flags: feedback?(lchang) → feedback+
(Assignee)

Comment 6

2 years ago
mozreview-review
Comment on attachment 8878406 [details]
Bug 1363999 - Add sync metadata to formautofill records.

https://reviewboard.mozilla.org/r/149748/#review158986

I don't think I can close these issues so I'll just respond to them. Rebased and updated patch incoming.

::: browser/extensions/formautofill/ProfileStorage.jsm:223
(Diff revision 1)
> +      let recordToSave = Object.assign({
> +        // `timeLastUsed` and `timesUsed` are always local.
> +        timeLastUsed: 0,
> +        timesUsed: 0,
> +      }, record);
> +      return this._saveRecord(recordToSave, {sourceSync});

That (well, the renamed version) is now called in saveRecord.

::: browser/extensions/formautofill/ProfileStorage.jsm:402
(Diff revision 1)
> +   * @param   {boolean} [options.includePrivateFields = false]
> +   *          Should fields starting with underscore be included in the result?
>     * @returns {Object}
>     *          A clone of the record.
>     */
> -  get(guid) {
> +  get(guid, {includePrivateFields = false} = {}) {

I believe it's used in later patches as well as tests, but it's also helpful for tests.

::: browser/extensions/formautofill/ProfileStorage.jsm:430
(Diff revision 1)
> +   * @param   {boolean} [config.includePrivateFields = false]
> +   *          Should fields starting with underscore be included in the result?
>     * @returns {Array.<Object>}
>     *          An array containing clones of all records.
>     */
> -  getAll({noComputedFields = false, includeDeleted = false} = {}) {
> +  getAll({noComputedFields = false, includeDeleted = false, includePrivateFields = false} = {}) {

I've made rawData cover includePrivateFields to meet this request (this will require a slightly tricky rebase for the other sync patches but worse things have happened).

::: browser/extensions/formautofill/ProfileStorage.jsm:482
(Diff revision 1)
> +      let tombstone = {
> +        guid,
> +        timeLastModified: Date.now(),
> +        deleted: true,
> +      };
> +      this._store.data[this._collectionName].push(tombstone);

Got it.

::: browser/extensions/formautofill/ProfileStorage.jsm:680
(Diff revision 1)
>        }
>      }
>    }
>  
> +  // A test-only helper.
> +  _nukeAllRecords() {

Removed.

::: browser/extensions/formautofill/test/unit/test_storage_syncfields.js:64
(Diff revision 1)
> +  address = profileStorage.addresses.getAll({includePrivateFields: true})[0];
> +  equal(address._sync.changeCounter, 1);
> +});
> +
> +add_task(async function test_changeCounter_update() {
> +  // This test should probably also check .status, and need splitting so

Removed the test.
Comment hidden (mozreview-request)
(Assignee)

Comment 8

2 years ago
Comment on attachment 8882728 [details]
Bug 1363999 - Add sync metadata to formautofill records.

Unsure about who should review this, let me know if I should change who I flagged.
Attachment #8882728 - Flags: review?(markh)
Attachment #8882728 - Flags: review?(lchang)
(Assignee)

Updated

2 years ago
Attachment #8878406 - Attachment is obsolete: true
(Reporter)

Comment 9

2 years ago
mozreview-review
Comment on attachment 8882728 [details]
Bug 1363999 - Add sync metadata to formautofill records.

https://reviewboard.mozilla.org/r/153820/#review159396

Awesome, thanks!
Attachment #8882728 - Flags: review?(markh) → review+
I think this bitrotted again. :-(
Sorry, it didn't; I forgot to cherry-pick bug 1368008 first.
Depends on: 1368008

Comment 12

a year ago
mozreview-review
Comment on attachment 8882728 [details]
Bug 1363999 - Add sync metadata to formautofill records.

https://reviewboard.mozilla.org/r/153820/#review161884

Looks great! Thanks.

::: browser/extensions/formautofill/ProfileStorage.jsm:45
(Diff revision 1)
>   *       // metadata
>   *       timeCreated,          // in ms
>   *       timeLastUsed,         // in ms
>   *       timeLastModified,     // in ms
>   *       timesUsed
> + *       _sync: { ... sync metadata },

nit: "optional sync metadata" to align with the other one.

::: browser/extensions/formautofill/test/unit/test_storage_syncfields.js:212
(Diff revision 1)
> +
> +  let tombstone = findGUID(profileStorage.addresses, guid, {
> +    rawData: true,
> +    includeDeleted: true,
> +  });
> +  ok(tombstone.deleted, 0);

`ok(tombstone.deleted);`

::: browser/extensions/formautofill/test/unit/test_storage_syncfields.js:236
(Diff revision 1)
> +
> +  let tombstone = findGUID(profileStorage.addresses, guid, {
> +    rawData: true,
> +    includeDeleted: true,
> +  });
> +  ok(tombstone.deleted, 0);

`ok(tombstone.deleted);`

::: browser/extensions/formautofill/test/unit/test_storage_syncfields.js:253
(Diff revision 1)
> +  let testAddr = Object.assign({guid: addedDirectGUID},
> +                               TEST_ADDRESS_1, TEST_ADDRESS_3);

Curious why we need `TEST_ADDRESS_1` here?
Attachment #8882728 - Flags: review?(lchang) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 15

a year ago
mozreview-review-reply
Comment on attachment 8882728 [details]
Bug 1363999 - Add sync metadata to formautofill records.

https://reviewboard.mozilla.org/r/153820/#review161884

> Curious why we need `TEST_ADDRESS_1` here?

For whatever reason, I remember there being an issue due to `TEST_ADDRESS_3` being a very small record. It doesn't seem to cause any problems now though, so I've removed the use of `TEST_ADDRESS_1` from that expression.
(Reporter)

Comment 16

a year ago
Comment on attachment 8882728 [details]
Bug 1363999 - Add sync metadata to formautofill records.

Luke's r+ somehow got dropped here.
Attachment #8882728 - Flags: review+
Comment hidden (mozreview-request)
(Reporter)

Updated

a year ago
Attachment #8882728 - Attachment is obsolete: true
(Reporter)

Comment 18

a year ago
Comment on attachment 8878406 [details]
Bug 1363999 - Add sync metadata to formautofill records.

stooopid mozreview - this version of the patch just has the eslint failure fixed - not sure why mozreview thinks it is something different.
Attachment #8878406 - Flags: review?(markh)
Attachment #8878406 - Flags: review?(lchang)
Attachment #8878406 - Flags: review+
(Reporter)

Comment 19

a year ago
mozreview-review
Comment on attachment 8878406 [details]
Bug 1363999 - Add sync metadata to formautofill records.

https://reviewboard.mozilla.org/r/149748/#review163908

Comment 20

a year ago
mozreview-review
Comment on attachment 8878406 [details]
Bug 1363999 - Add sync metadata to formautofill records.

https://reviewboard.mozilla.org/r/149748/#review163918
Attachment #8878406 - Flags: review+

Comment 21

a year ago
Pushed by mhammond@skippinet.com.au:
https://hg.mozilla.org/integration/autoland/rev/0f38f4687b6f
Add sync metadata to formautofill records. r=lchang
Whiteboard: [form autofill:MVP] → [form autofill:M4]

Comment 22

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/0f38f4687b6f
Status: ASSIGNED → RESOLVED
Last Resolved: a year 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.