Bug 1364825 - Add merge function in form autofill storage. r=MattN
authorsteveck-chung <schung@mozilla.com>
Wed, 07 Jun 2017 00:00:00 +0800
changeset 413164 59dcf0550abc4a29c5effd411682d4b3cf7bd693
parent 413163 742a51ab32ce7c35c4e3e156771768c39025c95f
child 413165 4af93deae5309632e4930992279a7b73d5b04610
push id1490
push usermtabara@mozilla.com
push dateMon, 31 Jul 2017 14:08:16 +0000
treeherdermozilla-release@70e32e6bf15e [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersMattN
bugs1364825
milestone55.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1364825 - Add merge function in form autofill storage. r=MattN MozReview-Commit-ID: AWYsnzmVJAY
browser/extensions/formautofill/FormAutofillParent.jsm
browser/extensions/formautofill/ProfileStorage.jsm
browser/extensions/formautofill/test/unit/head.js
browser/extensions/formautofill/test/unit/test_addressRecords.js
--- a/browser/extensions/formautofill/FormAutofillParent.jsm
+++ b/browser/extensions/formautofill/FormAutofillParent.jsm
@@ -265,17 +265,19 @@ FormAutofillParent.prototype = {
                                         Services.ppmm.initialProcessData.autofillSavedFieldNames);
     this._updateStatus();
   },
 
   _onFormSubmit(data, target) {
     let {address} = data;
 
     if (address.guid) {
-      // TODO: Show update doorhanger(bug 1303513) and set probe(bug 990200)
-      // if (!profileStorage.addresses.mergeIfPossible(address.guid, address.record)) {
-      // }
+      if (!this.profileStorage.addresses.mergeIfPossible(address.guid, address.record)) {
+        // TODO: Show update doorhanger(bug 1303513) and set probe(bug 990200)
+        return;
+      }
+      this.profileStorage.addresses.notifyUsed(address.guid);
     } else {
       // TODO: Add first time use probe(bug 990199) and doorhanger(bug 1303510)
       // profileStorage.addresses.add(address.record);
     }
   },
 };
--- a/browser/extensions/formautofill/ProfileStorage.jsm
+++ b/browser/extensions/formautofill/ProfileStorage.jsm
@@ -374,16 +374,22 @@ class AutofillRecords {
     }
   }
 
   // An interface to be inherited.
   _recordReadProcessor(record, config) {}
 
   // An interface to be inherited.
   _recordWriteProcessor(record) {}
+
+  // An interface to be inherited.
+  mergeIfPossible(guid, record) {}
+
+  // An interface to be inherited.
+  mergeToStorage(targetRecord) {}
 }
 
 class Addresses extends AutofillRecords {
   constructor(store) {
     super(store, "addresses", VALID_PROFILE_FIELDS, ADDRESS_SCHEMA_VERSION);
   }
 
   _recordReadProcessor(profile, {noComputedFields} = {}) {
@@ -449,16 +455,95 @@ class Addresses extends AutofillRecords 
       });
 
       // Concatenate "address-line*" if "street-address" is omitted.
       if (!profile["street-address"]) {
         profile["street-address"] = addressLines.join("\n");
       }
     }
   }
+
+  /**
+   * Merge new address into the specified address if mergeable.
+   *
+   * @param  {string} guid
+   *         Indicates which address to merge.
+   * @param  {Object} address
+   *         The new address used to merge into the old one.
+   * @returns {boolean}
+   *          Return true if address is merged into target with specific guid or false if not.
+   */
+  mergeIfPossible(guid, address) {
+    this.log.debug("mergeIfPossible:", guid, address);
+
+    let addressFound = this._findByGUID(guid);
+    if (!addressFound) {
+      throw new Error("No matching address.");
+    }
+
+    let addressToMerge = this._clone(address);
+    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]) {
+          this.log.debug("Conflicts: field", field, "has different value.");
+          return false;
+        }
+        hasMatchingField = true;
+      }
+    }
+
+    // We merge the address only when at least one field has the same value.
+    if (!hasMatchingField) {
+      this.log.debug("Unable to merge because no field has the same value");
+      return false;
+    }
+
+    // Early return if the data is the same.
+    let exactlyMatch = this.VALID_FIELDS.every((field) =>
+      addressFound[field] === addressToMerge[field]
+    );
+    if (exactlyMatch) {
+      return true;
+    }
+
+    for (let field in addressToMerge) {
+      if (this.VALID_FIELDS.includes(field)) {
+        addressFound[field] = addressToMerge[field];
+      }
+    }
+
+    addressFound.timeLastModified = Date.now();
+    this._store.saveSoon();
+    let str = Cc["@mozilla.org/supports-string;1"]
+                 .createInstance(Ci.nsISupportsString);
+    str.data = guid;
+    Services.obs.notifyObservers(str, "formautofill-storage-changed", "merge");
+    return true;
+  }
+
+  /**
+   * Merge the address if storage has multiple mergeable records.
+   * @param {Object} targetAddress
+   *        The address for merge.
+   * @returns {boolean}
+   *          Return true if the target address is mergeable or false if not.
+   */
+  mergeToStorage(targetAddress) {
+    let merged = false;
+    for (let address of this._store.data[this._collectionName]) {
+      if (this.mergeIfPossible(address.guid, targetAddress)) {
+        merged = true;
+      }
+    }
+    this.log.debug("Existing records matching and merging is", merged);
+    return merged;
+  }
 }
 
 class CreditCards extends AutofillRecords {
   constructor(store) {
     super(store, "creditCards", VALID_CREDIT_CARD_FIELDS, CREDIT_CARD_SCHEMA_VERSION);
   }
 
   _recordReadProcessor(creditCard, {noComputedFields} = {}) {
--- a/browser/extensions/formautofill/test/unit/head.js
+++ b/browser/extensions/formautofill/test/unit/head.js
@@ -1,13 +1,15 @@
 /**
  * Provides infrastructure for automated formautofill components tests.
  */
 
-/* exported getTempFile, loadFormAutofillContent, runHeuristicsTest, sinon */
+/* exported getTempFile, loadFormAutofillContent, runHeuristicsTest, sinon,
+ *          initProfileStorage
+ */
 
 "use strict";
 
 const {classes: Cc, interfaces: Ci, utils: Cu, results: Cr} = Components;
 
 Cu.import("resource://gre/modules/XPCOMUtils.jsm");
 Cu.import("resource://gre/modules/Services.jsm");
 Cu.import("resource://gre/modules/NetUtil.jsm");
@@ -74,16 +76,36 @@ function getTempFile(leafName) {
     if (file.exists()) {
       file.remove(false);
     }
   });
 
   return file;
 }
 
+async function initProfileStorage(fileName, records) {
+  let {ProfileStorage} = Cu.import("resource://formautofill/ProfileStorage.jsm", {});
+  let path = getTempFile(fileName).path;
+  let profileStorage = new ProfileStorage(path);
+  await profileStorage.initialize();
+
+  if (!records || !Array.isArray(records)) {
+    return profileStorage;
+  }
+
+  let onChanged = TestUtils.topicObserved("formautofill-storage-changed",
+                                          (subject, data) => data == "add");
+  for (let record of records) {
+    do_check_true(profileStorage.addresses.add(record));
+    await onChanged;
+  }
+  await profileStorage._saveImmediately();
+  return profileStorage;
+}
+
 function runHeuristicsTest(patterns, fixturePathPrefix) {
   Cu.import("resource://gre/modules/FormLikeFactory.jsm");
   Cu.import("resource://formautofill/FormAutofillHeuristics.jsm");
   Cu.import("resource://formautofill/FormAutofillUtils.jsm");
 
   patterns.forEach(testPattern => {
     add_task(function* () {
       do_print("Starting test fixture: " + testPattern.fixturePath);
@@ -110,17 +132,17 @@ function runHeuristicsTest(patterns, fix
           expectedField.elementWeakRef = field.elementWeakRef;
           Assert.deepEqual(field, expectedField);
         });
       });
     });
   });
 }
 
-add_task(function* head_initialize() {
+add_task(async function head_initialize() {
   Services.prefs.setBoolPref("extensions.formautofill.experimental", true);
   Services.prefs.setBoolPref("extensions.formautofill.heuristics.enabled", true);
   Services.prefs.setBoolPref("dom.forms.autocomplete.experimental", true);
 
   // Clean up after every test.
   do_register_cleanup(function head_cleanup() {
     Services.prefs.clearUserPref("extensions.formautofill.experimental");
     Services.prefs.clearUserPref("extensions.formautofill.heuristics.enabled");
--- a/browser/extensions/formautofill/test/unit/test_addressRecords.js
+++ b/browser/extensions/formautofill/test/unit/test_addressRecords.js
@@ -1,16 +1,14 @@
 /**
  * Tests ProfileStorage object with addresses records.
  */
 
 "use strict";
 
-const {ProfileStorage} = Cu.import("resource://formautofill/ProfileStorage.jsm", {});
-
 const TEST_STORE_FILE_NAME = "test-profile.json";
 
 const TEST_ADDRESS_1 = {
   "given-name": "Timothy",
   "additional-name": "John",
   "family-name": "Berners-Lee",
   organization: "World Wide Web Consortium",
   "street-address": "32 Vassar Street\nMIT Room 32-G524",
@@ -27,64 +25,115 @@ const TEST_ADDRESS_2 = {
   country: "US",
 };
 
 const TEST_ADDRESS_3 = {
   "street-address": "Other Address",
   "postal-code": "12345",
 };
 
+const TEST_ADDRESS_4 = {
+  "given-name": "Timothy",
+  "additional-name": "John",
+  "family-name": "Berners-Lee",
+  organization: "World Wide Web Consortium",
+};
+
 const TEST_ADDRESS_WITH_INVALID_FIELD = {
   "street-address": "Another Address",
   invalidField: "INVALID",
 };
 
-let prepareTestRecords = async function(path) {
-  let profileStorage = new ProfileStorage(path);
-  await profileStorage.initialize();
-
-  let onChanged = TestUtils.topicObserved("formautofill-storage-changed",
-                                          (subject, data) => data == "add");
-  do_check_true(profileStorage.addresses.add(TEST_ADDRESS_1));
-  await onChanged;
-  do_check_true(profileStorage.addresses.add(TEST_ADDRESS_2));
-  await profileStorage._saveImmediately();
-};
+const MERGE_TESTCASES = [
+  {
+    description: "Merge a superset",
+    addressInStorage: {
+      "given-name": "Timothy",
+      "street-address": "331 E. Evelyn Avenue",
+      "tel": "1-650-903-0800",
+    },
+    addressToMerge: {
+      "given-name": "Timothy",
+      "street-address": "331 E. Evelyn Avenue",
+      "tel": "1-650-903-0800",
+      country: "US",
+    },
+    expectedAddress: {
+      "given-name": "Timothy",
+      "street-address": "331 E. Evelyn Avenue",
+      "tel": "1-650-903-0800",
+      country: "US",
+    },
+  },
+  {
+    description: "Merge a subset",
+    addressInStorage: {
+      "given-name": "Timothy",
+      "street-address": "331 E. Evelyn Avenue",
+      "tel": "1-650-903-0800",
+      country: "US",
+    },
+    addressToMerge: {
+      "given-name": "Timothy",
+      "street-address": "331 E. Evelyn Avenue",
+      "tel": "1-650-903-0800",
+    },
+    expectedAddress: {
+      "given-name": "Timothy",
+      "street-address": "331 E. Evelyn Avenue",
+      "tel": "1-650-903-0800",
+      country: "US",
+    },
+  },
+  {
+    description: "Merge an address with partial overlaps",
+    addressInStorage: {
+      "given-name": "Timothy",
+      "street-address": "331 E. Evelyn Avenue",
+      "tel": "1-650-903-0800",
+    },
+    addressToMerge: {
+      "street-address": "331 E. Evelyn Avenue",
+      "tel": "1-650-903-0800",
+      country: "US",
+    },
+    expectedAddress: {
+      "given-name": "Timothy",
+      "street-address": "331 E. Evelyn Avenue",
+      "tel": "1-650-903-0800",
+      country: "US",
+    },
+  },
+];
 
 let do_check_record_matches = (recordWithMeta, record) => {
   for (let key in record) {
     do_check_eq(recordWithMeta[key], record[key]);
   }
 };
 
 add_task(async function test_initialize() {
-  let path = getTempFile(TEST_STORE_FILE_NAME).path;
-  let profileStorage = new ProfileStorage(path);
-  await profileStorage.initialize();
+  let profileStorage = await initProfileStorage(TEST_STORE_FILE_NAME);
 
   do_check_eq(profileStorage._store.data.version, 1);
   do_check_eq(profileStorage._store.data.addresses.length, 0);
 
   let data = profileStorage._store.data;
   Assert.deepEqual(data.addresses, []);
 
   await profileStorage._saveImmediately();
 
-  profileStorage = new ProfileStorage(path);
-  await profileStorage.initialize();
+  profileStorage = await initProfileStorage(TEST_STORE_FILE_NAME);
 
   Assert.deepEqual(profileStorage._store.data, data);
 });
 
 add_task(async function test_getAll() {
-  let path = getTempFile(TEST_STORE_FILE_NAME).path;
-  await prepareTestRecords(path);
-
-  let profileStorage = new ProfileStorage(path);
-  await profileStorage.initialize();
+  let profileStorage = await initProfileStorage(TEST_STORE_FILE_NAME,
+                                                [TEST_ADDRESS_1, TEST_ADDRESS_2]);
 
   let addresses = profileStorage.addresses.getAll();
 
   do_check_eq(addresses.length, 2);
   do_check_record_matches(addresses[0], TEST_ADDRESS_1);
   do_check_record_matches(addresses[1], TEST_ADDRESS_2);
 
   // Check computed fields.
@@ -99,41 +148,35 @@ add_task(async function test_getAll() {
   do_check_eq(addresses[0]["address-line2"], undefined);
 
   // Modifying output shouldn't affect the storage.
   addresses[0].organization = "test";
   do_check_record_matches(profileStorage.addresses.getAll()[0], TEST_ADDRESS_1);
 });
 
 add_task(async function test_get() {
-  let path = getTempFile(TEST_STORE_FILE_NAME).path;
-  await prepareTestRecords(path);
-
-  let profileStorage = new ProfileStorage(path);
-  await profileStorage.initialize();
+  let profileStorage = await initProfileStorage(TEST_STORE_FILE_NAME,
+                                                [TEST_ADDRESS_1, TEST_ADDRESS_2]);
 
   let addresses = profileStorage.addresses.getAll();
   let guid = addresses[0].guid;
 
   let address = profileStorage.addresses.get(guid);
   do_check_record_matches(address, TEST_ADDRESS_1);
 
   // Modifying output shouldn't affect the storage.
   address.organization = "test";
   do_check_record_matches(profileStorage.addresses.get(guid), TEST_ADDRESS_1);
 
   do_check_eq(profileStorage.addresses.get("INVALID_GUID"), null);
 });
 
 add_task(async function test_getByFilter() {
-  let path = getTempFile(TEST_STORE_FILE_NAME).path;
-  await prepareTestRecords(path);
-
-  let profileStorage = new ProfileStorage(path);
-  await profileStorage.initialize();
+  let profileStorage = await initProfileStorage(TEST_STORE_FILE_NAME,
+                                                [TEST_ADDRESS_1, TEST_ADDRESS_2]);
 
   let filter = {info: {fieldName: "street-address"}, searchString: "Some"};
   let addresses = profileStorage.addresses.getByFilter(filter);
   do_check_eq(addresses.length, 1);
   do_check_record_matches(addresses[0], TEST_ADDRESS_2);
 
   filter = {info: {fieldName: "country"}, searchString: "u"};
   addresses = profileStorage.addresses.getByFilter(filter);
@@ -156,21 +199,18 @@ add_task(async function test_getByFilter
 
   // Prevent broken while searching the property that does not exist.
   filter = {info: {fieldName: "tel"}, searchString: "1"};
   addresses = profileStorage.addresses.getByFilter(filter);
   do_check_eq(addresses.length, 0);
 });
 
 add_task(async function test_add() {
-  let path = getTempFile(TEST_STORE_FILE_NAME).path;
-  await prepareTestRecords(path);
-
-  let profileStorage = new ProfileStorage(path);
-  await profileStorage.initialize();
+  let profileStorage = await initProfileStorage(TEST_STORE_FILE_NAME,
+                                                [TEST_ADDRESS_1, TEST_ADDRESS_2]);
 
   let addresses = profileStorage.addresses.getAll();
 
   do_check_eq(addresses.length, 2);
 
   do_check_record_matches(addresses[0], TEST_ADDRESS_1);
   do_check_record_matches(addresses[1], TEST_ADDRESS_2);
 
@@ -181,38 +221,32 @@ add_task(async function test_add() {
   do_check_eq(addresses[0].timeLastUsed, 0);
   do_check_eq(addresses[0].timesUsed, 0);
 
   Assert.throws(() => profileStorage.addresses.add(TEST_ADDRESS_WITH_INVALID_FIELD),
     /"invalidField" is not a valid field\./);
 });
 
 add_task(async function test_update() {
-  let path = getTempFile(TEST_STORE_FILE_NAME).path;
-  await prepareTestRecords(path);
-
-  let profileStorage = new ProfileStorage(path);
-  await profileStorage.initialize();
+  let profileStorage = await initProfileStorage(TEST_STORE_FILE_NAME,
+                                                [TEST_ADDRESS_1, TEST_ADDRESS_2]);
 
   let addresses = profileStorage.addresses.getAll();
   let guid = addresses[1].guid;
   let timeLastModified = addresses[1].timeLastModified;
 
   let onChanged = TestUtils.topicObserved("formautofill-storage-changed",
                                           (subject, data) => data == "update");
 
   do_check_neq(addresses[1].country, undefined);
 
   profileStorage.addresses.update(guid, TEST_ADDRESS_3);
   await onChanged;
   await profileStorage._saveImmediately();
 
-  profileStorage = new ProfileStorage(path);
-  await profileStorage.initialize();
-
   let address = profileStorage.addresses.get(guid);
 
   do_check_eq(address.country, undefined);
   do_check_neq(address.timeLastModified, timeLastModified);
   do_check_record_matches(address, TEST_ADDRESS_3);
 
   Assert.throws(
     () => profileStorage.addresses.update("INVALID_GUID", TEST_ADDRESS_3),
@@ -221,66 +255,111 @@ add_task(async function test_update() {
 
   Assert.throws(
     () => profileStorage.addresses.update(guid, TEST_ADDRESS_WITH_INVALID_FIELD),
     /"invalidField" is not a valid field\./
   );
 });
 
 add_task(async function test_notifyUsed() {
-  let path = getTempFile(TEST_STORE_FILE_NAME).path;
-  await prepareTestRecords(path);
-
-  let profileStorage = new ProfileStorage(path);
-  await profileStorage.initialize();
+  let profileStorage = await initProfileStorage(TEST_STORE_FILE_NAME,
+                                                [TEST_ADDRESS_1, TEST_ADDRESS_2]);
 
   let addresses = profileStorage.addresses.getAll();
   let guid = addresses[1].guid;
   let timeLastUsed = addresses[1].timeLastUsed;
   let timesUsed = addresses[1].timesUsed;
 
   let onChanged = TestUtils.topicObserved("formautofill-storage-changed",
                                           (subject, data) => data == "notifyUsed");
 
   profileStorage.addresses.notifyUsed(guid);
   await onChanged;
   await profileStorage._saveImmediately();
 
-  profileStorage = new ProfileStorage(path);
-  await profileStorage.initialize();
-
   let address = profileStorage.addresses.get(guid);
 
   do_check_eq(address.timesUsed, timesUsed + 1);
   do_check_neq(address.timeLastUsed, timeLastUsed);
 
   Assert.throws(() => profileStorage.addresses.notifyUsed("INVALID_GUID"),
     /No matching record\./);
 });
 
 add_task(async function test_remove() {
-  let path = getTempFile(TEST_STORE_FILE_NAME).path;
-  await prepareTestRecords(path);
-
-  let profileStorage = new ProfileStorage(path);
-  await profileStorage.initialize();
+  let profileStorage = await initProfileStorage(TEST_STORE_FILE_NAME,
+                                                [TEST_ADDRESS_1, TEST_ADDRESS_2]);
 
   let addresses = profileStorage.addresses.getAll();
   let guid = addresses[1].guid;
 
   let onChanged = TestUtils.topicObserved("formautofill-storage-changed",
                                           (subject, data) => data == "remove");
 
   do_check_eq(addresses.length, 2);
 
   profileStorage.addresses.remove(guid);
   await onChanged;
   await profileStorage._saveImmediately();
 
-  profileStorage = new ProfileStorage(path);
-  await profileStorage.initialize();
-
   addresses = profileStorage.addresses.getAll();
 
   do_check_eq(addresses.length, 1);
 
   do_check_eq(profileStorage.addresses.get(guid), null);
 });
+
+MERGE_TESTCASES.forEach((testcase) => {
+  add_task(async function test_merge() {
+    do_print("Starting testcase: " + testcase.description);
+    let profileStorage = await initProfileStorage(TEST_STORE_FILE_NAME,
+                                                  [testcase.addressInStorage]);
+    let addresses = profileStorage.addresses.getAll();
+    // Merge address and verify the guid in notifyObservers subject
+    let onMerged = TestUtils.topicObserved(
+      "formautofill-storage-changed",
+      (subject, data) =>
+        data == "merge" && subject.QueryInterface(Ci.nsISupportsString).data == addresses[0].guid
+    );
+    let timeLastModified = addresses[0].timeLastModified;
+    Assert.equal(
+      profileStorage.addresses.mergeIfPossible(addresses[0].guid, testcase.addressToMerge),
+      true);
+    await onMerged;
+    addresses = profileStorage.addresses.getAll();
+    Assert.equal(addresses.length, 1);
+    Assert.notEqual(addresses[0].timeLastModified, timeLastModified);
+    do_check_record_matches(addresses[0], testcase.expectedAddress);
+  });
+});
+
+add_task(async function test_merge_same_address() {
+  let profileStorage = await initProfileStorage(TEST_STORE_FILE_NAME, [TEST_ADDRESS_1]);
+  let addresses = profileStorage.addresses.getAll();
+  let timeLastModified = addresses[0].timeLastModified;
+  // Merge same address will still return true but it won't update timeLastModified.
+  Assert.equal(profileStorage.addresses.mergeIfPossible(addresses[0].guid, TEST_ADDRESS_1), true);
+  Assert.equal(addresses[0].timeLastModified, timeLastModified);
+});
+
+add_task(async function test_merge_unable_merge() {
+  let profileStorage = await initProfileStorage(TEST_STORE_FILE_NAME,
+                                                [TEST_ADDRESS_1, TEST_ADDRESS_2]);
+
+  let addresses = profileStorage.addresses.getAll();
+  // Unable to merge because of conflict
+  do_check_eq(profileStorage.addresses.mergeIfPossible(addresses[1].guid, TEST_ADDRESS_3), false);
+
+  // Unable to merge because no overlap
+  do_check_eq(profileStorage.addresses.mergeIfPossible(addresses[1].guid, TEST_ADDRESS_4), false);
+});
+
+add_task(async function test_mergeToStorage() {
+  let profileStorage = await initProfileStorage(TEST_STORE_FILE_NAME,
+                                                [TEST_ADDRESS_1, TEST_ADDRESS_2]);
+  // Merge an address to storage
+  let anotherAddress = profileStorage.addresses._clone(TEST_ADDRESS_2);
+  profileStorage.addresses.add(anotherAddress);
+  anotherAddress.email = "timbl@w3.org";
+  do_check_eq(profileStorage.addresses.mergeToStorage(anotherAddress), true);
+  do_check_eq(profileStorage.addresses.getAll()[1].email, anotherAddress.email);
+  do_check_eq(profileStorage.addresses.getAll()[2].email, anotherAddress.email);
+});