1364825 - [Form Autofill] Implement merge method in ProfileStorage

Status: RESOLVED FIXED

(Toolkit :: Form Manager, enhancement)

Description

[Steve Chung [:steveck]]

Implement merge API for updating the address/credit card storage and sync.

Comment 1

[Steve Chung [:steveck]]

Since sync merge will need exactly same data between 2 data without these cases in bug 990219 comment 33, close as duo for now.

Comment 2

[Steve Chung [:steveck]]

Reopen the bug since we'll need to discussion for the merge detail(see bug 990219 comment 68).

Comment 3

[Steve Chung [:steveck]]

Attachment: Bug 1364825 - Add merge function in autofill storage.

Review commit: https://reviewboard.mozilla.org/r/141434/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/141434/

Comment 4

[Steve Chung [:steveck]]

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?

Comment 5

[Steve Chung [:steveck]]

Comment on attachment 8869893 [details]
Bug 1364825 - Add merge function in autofill storage.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/141434/diff/1-2/

Comment 6

[Steve Chung [:steveck]]

Comment on attachment 8869893 [details]
Bug 1364825 - Add merge function in autofill storage.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/141434/diff/2-3/

Comment 7

[Matthew N. [:MattN]]

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.

Comment 8

[Steve Chung [:steveck]]

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 9

[Matthew N. [:MattN]]

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.

Comment 10

[Matthew N. [:MattN]]

(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.

Comment 11

[Steve Chung [:steveck]]

Comment on attachment 8869893 [details]
Bug 1364825 - Add merge function in autofill storage.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/141434/diff/3-4/

Comment 12

[Matthew N. [:MattN]]

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

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/59dcf0550abc
Add merge function in form autofill storage. r=MattN

Comment 14

[Steve Chung [:steveck]]

Comment on attachment 8869893 [details]
Bug 1364825 - Add merge function in autofill storage.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/141434/diff/4-5/

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/59dcf0550abc