[Form Autofill] Implement merge method in ProfileStorage

RESOLVED FIXED in Firefox 55

Status

()

enhancement
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: steveck, Assigned: steveck)

Tracking

unspecified
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: [form autofill] ETA:612)

Attachments

(1 attachment)

Assignee

Description

2 years ago
Implement merge API for updating the address/credit card storage and sync.
Assignee

Comment 1

2 years ago
Since sync merge will need exactly same data between 2 data without these cases in bug 990219 comment 33, close as duo for now.
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 990219
Assignee

Comment 2

2 years ago
Reopen the bug since we'll need to discussion for the merge detail(see bug 990219 comment 68).
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
See Also: → 990219
Comment hidden (mozreview-request)
Assignee

Comment 4

2 years ago
mozreview-review
Comment on attachment 8869893 [details]
Bug 1364825 - Add merge function in autofill storage.

https://reviewboard.mozilla.org/r/141434/#review145472

::: browser/extensions/formautofill/ProfileStorage.jsm:429
(Diff revision 1)
> +   * @returns {boolean}
> +   *          Return true if the target record is mergeable or false if not.
> +   */
> +  _mergeToStorage(targetRecord) {
> +    for (let record of this._store.data[this._collectionName]) {
> +      if (this.mergeIfPossible(record.guid, targetRecord)) {

Juwie suggested that maybe we should simply merge the target into the all records that is enble to be merged. I think it's fine since it's difficult to assume which one is best candidate(lastUsedTime/timeUsed/matching fields/... all of these topic could be the reason), and users might be ok with huge profiles. wdyt?
Assignee

Updated

2 years ago
Flags: needinfo?(MattN+bmo)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Updated

2 years ago
Whiteboard: ETA:612
Comment on attachment 8869893 [details]
Bug 1364825 - Add merge function in autofill storage.

https://reviewboard.mozilla.org/r/141434/#review149582

::: browser/extensions/formautofill/ProfileStorage.jsm:508
(Diff revision 3)
> +    addressFound.timeLastModified = Date.now();
> +    addressFound.timeLastUsed = Date.now();
> +    addressFound.timesUsed++;

I'm not sure this should be part of a `merge` function. It seems like this is mostly covered by notifyUsed after a merge in the doorhanger case.

::: browser/extensions/formautofill/test/unit/test_addressRecords.js:316
(Diff revision 3)
> +  do_check_eq(profileStorage.addresses.getAll()[1].tel, "123456");
> +
> +  // Merge a subset
> +  let subset = profileStorage.addresses._clone(TEST_ADDRESS_1);
> +  delete subset.tel;

Can you put each of these test cases in their own task to make it easier to read and have them more isolated? If you make a helper for the following then it should reduce duplication/boilerplate:

> let path = getTempFile(TEST_STORE_FILE_NAME).path;
> await prepareTestRecords(path);
>
> let profileStorage = new ProfileStorage(path);
> await profileStorage.initialize();

I also wonder if merge tests should be in a separate test file.
Attachment #8869893 - Flags: review?(MattN+bmo)
Assignee

Comment 8

2 years ago
mozreview-review-reply
Comment on attachment 8869893 [details]
Bug 1364825 - Add merge function in autofill storage.

https://reviewboard.mozilla.org/r/141434/#review149582

> I'm not sure this should be part of a `merge` function. It seems like this is mostly covered by notifyUsed after a merge in the doorhanger case.

Well they are similar but not exactly the same: Here I also update timeLastModified and fire `formautofill-storage-changed` event with data = merge. I think it worth to split into 2 different cases?

> Can you put each of these test cases in their own task to make it easier to read and have them more isolated? If you make a helper for the following then it should reduce duplication/boilerplate:
> 
> > let path = getTempFile(TEST_STORE_FILE_NAME).path;
> > await prepareTestRecords(path);
> >
> > let profileStorage = new ProfileStorage(path);
> > await profileStorage.initialize();
> 
> I also wonder if merge tests should be in a separate test file.

I can try to refine the test case for readability, not sure if it worth to move merge to another test file.
Comment on attachment 8869893 [details]
Bug 1364825 - Add merge function in autofill storage.

https://reviewboard.mozilla.org/r/141434/#review149582

> Well they are similar but not exactly the same: Here I also update timeLastModified and fire `formautofill-storage-changed` event with data = merge. I think it worth to split into 2 different cases?

Well I don't think we should update timeLastModified or send the changed notification if all fields are the same so I think we should fix that.

I also don't think we should set timeLastUsed to now if this API is used by something like sync which doesn't involve a use. In that case the Math.max of the two values should be used.
(In reply to Steve Chung [:steveck] from comment #4)
> Comment on attachment 8869893 [details]
> Bug 1364825 - Add merge function in storage.
> 
> https://reviewboard.mozilla.org/r/141434/#review145472
> 
> ::: browser/extensions/formautofill/ProfileStorage.jsm:429
> (Diff revision 1)
> > +   * @returns {boolean}
> > +   *          Return true if the target record is mergeable or false if not.
> > +   */
> > +  _mergeToStorage(targetRecord) {
> > +    for (let record of this._store.data[this._collectionName]) {
> > +      if (this.mergeIfPossible(record.guid, targetRecord)) {
> 
> Juwie suggested that maybe we should simply merge the target into the all
> records that is enble to be merged. I think it's fine since it's difficult
> to assume which one is best candidate(lastUsedTime/timeUsed/matching
> fields/... all of these topic could be the reason), and users might be ok
> with huge profiles. wdyt?

I'm not sure I agree but I really depends on the specific merge. I really think the proper merge involves taking the field type into account (e.g. name vs. address) and I'm not sure there is general approach to it.
Flags: needinfo?(MattN+bmo)
Comment hidden (mozreview-request)
Comment on attachment 8869893 [details]
Bug 1364825 - Add merge function in autofill storage.

https://reviewboard.mozilla.org/r/141434/#review150724

Thanks for the nice tests!

::: commit-message-84fa3:1
(Diff revision 4)
> +Bug 1364825 - Add merge function in storage. r?MattN

Nit: You don't mention form autofill anywhere here. Maybe add "autofill" before "storage"

::: browser/extensions/formautofill/ProfileStorage.jsm:488
(Diff revision 4)
> +    this._normalizeRecord(addressToMerge);
> +    let hasMatchingField = false;
> +
> +    for (let field of this.VALID_FIELDS) {
> +      if (addressToMerge[field] !== undefined && addressFound[field] !== undefined) {
> +        if (addressToMerge[field] != addressFound[field]) {

I forget what we decided before but I think a case-insenstive comparison would lead to less duplicate profiles. We can deal with that later though.

::: browser/extensions/formautofill/ProfileStorage.jsm:498
(Diff revision 4)
> +      }
> +    }
> +
> +    // We merge the address only when at least one field has the same value.
> +    if (!hasMatchingField) {
> +      this.log.debug("Unable to merge because there's no field has same value");

…because no field has the same value
Attachment #8869893 - Flags: review?(MattN+bmo) → review+

Comment 13

2 years ago
Pushed by mozilla@noorenberghe.ca:
https://hg.mozilla.org/integration/mozilla-inbound/rev/59dcf0550abc
Add merge function in form autofill storage. r=MattN
Status: REOPENED → ASSIGNED
Whiteboard: ETA:612 → [form autofill] ETA:612
Comment hidden (mozreview-request)
https://hg.mozilla.org/mozilla-central/rev/59dcf0550abc
Status: ASSIGNED → RESOLVED
Closed: 2 years ago2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.