Closed Bug 1413110 Opened 7 years ago Closed 7 years ago

[Form Autofill] No need to compute fields on tombstone while data migration

Categories

(Toolkit :: Form Manager, enhancement, P5)

enhancement

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox58 --- fixed

People

(Reporter: lchang, Assigned: lchang)

References

(Blocks 1 open bug)

Details

(Whiteboard: [form autofill])

Attachments

(1 file)

We should skip updating the computed fields when doing migration.
It causes every tombstones in the json file to contain redundant fields.
Assignee: nobody → lchang
Status: NEW → ASSIGNED
Comment on attachment 8923718 [details]
Bug 1413110 - [Form Autofill] Skip tombstones when migrating records and computing fields.

https://reviewboard.mozilla.org/r/194868/#review200860

::: browser/extensions/formautofill/ProfileStorage.jsm:1205
(Diff revision 2)
>      //       computing algorithm changes. (No need to bump when just adding new
>      //       computed fields)
>  
>      let hasNewComputedFields = false;
>  
> +    if (address.deleted) {

I'm not sure if it's the right place to early retun in the `_computeFields`. It might make sences to skip tombstones for migration/update(although it's unlikely to update a tombstones anyway), but I'm not sure if it still valid to skip the `_computeFields` for other internal API for Sync like `_replaceRecordAt` or `_forkLocalRecord`
Comment on attachment 8923718 [details]
Bug 1413110 - [Form Autofill] Skip tombstones when migrating records and computing fields.

https://reviewboard.mozilla.org/r/194868/#review200914
Comment on attachment 8923718 [details]
Bug 1413110 - [Form Autofill] Skip tombstones when migrating records and computing fields.

https://reviewboard.mozilla.org/r/194868/#review201254

Looks good to me, but it might be a good idea to add a test for tombstone migration since we already have a unit test about migrateRecord(test_migrateRecords.js).
I didn't realize we have that already. Tests added. Thanks.
Comment on attachment 8923718 [details]
Bug 1413110 - [Form Autofill] Skip tombstones when migrating records and computing fields.

https://reviewboard.mozilla.org/r/194868/#review201308
Attachment #8923718 - Flags: review?(schung) → review+
Pushed by lchang@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4feff840625b
[Form Autofill] Skip tombstones when migrating records and computing fields. r=steveck
https://hg.mozilla.org/mozilla-central/rev/4feff840625b
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: