Bug 1412788 - [Form Autofill] The sync metadata should be updated while merging address records. r=steveck draft
authorLuke Chang <lchang@mozilla.com>
Mon, 30 Oct 2017 18:55:50 +0800
changeset 689168 c14d71334dfe30d5b2216e784c008166ae66480f
parent 688337 d3910b7628b8066d3f30d58b17b5824b05768854
child 738249 41fa5b67fb8286674bd5ec207b98274da2deb701
push id86945
push userbmo:lchang@mozilla.com
push dateTue, 31 Oct 2017 06:33:55 +0000
reviewerssteveck
bugs1412788
milestone58.0a1
Bug 1412788 - [Form Autofill] The sync metadata should be updated while merging address records. r=steveck MozReview-Commit-ID: 3arMCcVTPPE
browser/extensions/formautofill/ProfileStorage.jsm
browser/extensions/formautofill/test/mochitest/test_on_address_submission.html
browser/extensions/formautofill/test/unit/test_activeStatus.js
browser/extensions/formautofill/test/unit/test_addressRecords.js
browser/extensions/formautofill/test/unit/test_savedFieldNames.js
--- a/browser/extensions/formautofill/ProfileStorage.jsm
+++ b/browser/extensions/formautofill/ProfileStorage.jsm
@@ -442,17 +442,20 @@ class AutofillRecords {
     if (syncMetadata) {
       syncMetadata.changeCounter += 1;
     }
 
     this._computeFields(recordFound);
     this.data[recordFoundIndex] = recordFound;
 
     this._store.saveSoon();
-    Services.obs.notifyObservers(null, "formautofill-storage-changed", "update");
+
+    let str = Cc["@mozilla.org/supports-string;1"].createInstance(Ci.nsISupportsString);
+    str.data = guid;
+    Services.obs.notifyObservers(str, "formautofill-storage-changed", "update");
   }
 
   /**
    * Notifies the storage of the use of the specified record, so we can update
    * the metadata accordingly. This does not bump the Sync change counter, since
    * we don't sync `timesUsed` or `timeLastUsed`.
    *
    * @param  {string} guid
@@ -1414,32 +1417,17 @@ class Addresses extends AutofillRecords 
     // 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._stripComputedFields(addressFound);
-    this._computeFields(addressFound);
-
-    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");
+    this.update(guid, addressToMerge, true);
     return true;
   }
 
   /**
    * Merge the address if storage has multiple mergeable records.
    * @param {Object} targetAddress
    *        The address for merge.
    * @returns {Array.<string>}
--- a/browser/extensions/formautofill/test/mochitest/test_on_address_submission.html
+++ b/browser/extensions/formautofill/test/mochitest/test_on_address_submission.html
@@ -88,17 +88,17 @@ add_task(async function new_address_subm
   // Add country to first address in storage
   await setInput("#country", "US");
   TEST_ADDRESSES[0].country = "US";
   clickOnElement("input[type=submit]");
 
   let expectedAddresses = TEST_ADDRESSES.slice(0);
   // Check if timesUsed is set correctly
   expectedAddresses[0].timesUsed = 2;
-  await onStorageChanged("merge");
+  await onStorageChanged("update");
   let matching = await checkAddresses(expectedAddresses);
   ok(matching, "Address merged as expected");
   delete expectedAddresses[0].timesUsed;
 });
 
 // Submit an updated autofill address and merge.
 add_task(async function check_storage_after_form_submitted() {
   document.querySelector("form").reset();
@@ -109,17 +109,17 @@ add_task(async function check_storage_af
   await setInput("#organization", "Moz");
   doKey("down");
   await expectPopup();
   doKey("down");
   doKey("return");
   clickOnElement("input[type=submit]");
 
   let expectedAddresses = TEST_ADDRESSES.slice(0);
-  await onStorageChanged("merge");
+  await onStorageChanged("update");
   let matching = await checkAddresses(expectedAddresses);
   ok(matching, "Updated address merged as expected");
 });
 
 </script>
 
 <div>
 
--- a/browser/extensions/formautofill/test/unit/test_activeStatus.js
+++ b/browser/extensions/formautofill/test/unit/test_activeStatus.js
@@ -45,17 +45,17 @@ add_task(async function test_activeStatu
   // _active != _computeStatus() => Need to trigger _onStatusChanged
   formAutofillParent._computeStatus.returns(false);
   formAutofillParent._onStatusChanged.reset();
   formAutofillParent.observe(null, "nsPref:changed", "extensions.formautofill.addresses.enabled");
   formAutofillParent.observe(null, "nsPref:changed", "extensions.formautofill.creditCards.enabled");
   do_check_eq(formAutofillParent._onStatusChanged.called, true);
 
   // profile changed => Need to trigger _onStatusChanged
-  ["add", "update", "remove", "reconcile", "merge"].forEach(event => {
+  ["add", "update", "remove", "reconcile"].forEach(event => {
     formAutofillParent._computeStatus.returns(!formAutofillParent._active);
     formAutofillParent._onStatusChanged.reset();
     formAutofillParent.observe(null, "formautofill-storage-changed", event);
     do_check_eq(formAutofillParent._onStatusChanged.called, true);
   });
 
   // profile metadata updated => No need to trigger _onStatusChanged
   formAutofillParent._computeStatus.returns(!formAutofillParent._active);
--- a/browser/extensions/formautofill/test/unit/test_addressRecords.js
+++ b/browser/extensions/formautofill/test/unit/test_addressRecords.js
@@ -306,18 +306,21 @@ add_task(async function test_add() {
 add_task(async function test_update() {
   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");
+  let onChanged = TestUtils.topicObserved(
+    "formautofill-storage-changed",
+    (subject, data) =>
+      data == "update" && subject.QueryInterface(Ci.nsISupportsString).data == guid
+  );
 
   do_check_neq(addresses[1].country, undefined);
 
   profileStorage.addresses.update(guid, TEST_ADDRESS_3);
   await onChanged;
   await profileStorage._saveImmediately();
 
   profileStorage.addresses.pullSyncChanges(); // force sync metadata, which we check below.
@@ -410,53 +413,80 @@ add_task(async function test_remove() {
 });
 
 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();
+    let guid = addresses[0].guid;
+    let timeLastModified = addresses[0].timeLastModified;
+
     // 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
+        data == "update" && subject.QueryInterface(Ci.nsISupportsString).data == guid
     );
-    let timeLastModified = addresses[0].timeLastModified;
-    Assert.equal(
-      profileStorage.addresses.mergeIfPossible(addresses[0].guid, testcase.addressToMerge),
-      true);
+
+    // Force to create sync metadata.
+    profileStorage.addresses.pullSyncChanges();
+    do_check_eq(getSyncChangeCounter(profileStorage.addresses, guid), 1);
+
+    Assert.ok(profileStorage.addresses.mergeIfPossible(guid, testcase.addressToMerge));
     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);
+
+    // Record merging should bump the change counter.
+    do_check_eq(getSyncChangeCounter(profileStorage.addresses, guid), 2);
   });
 });
 
 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 guid = addresses[0].guid;
   let timeLastModified = addresses[0].timeLastModified;
+
+  // Force to create sync metadata.
+  profileStorage.addresses.pullSyncChanges();
+  do_check_eq(getSyncChangeCounter(profileStorage.addresses, guid), 1);
+
   // 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.ok(profileStorage.addresses.mergeIfPossible(guid, TEST_ADDRESS_1));
   Assert.equal(addresses[0].timeLastModified, timeLastModified);
+
+  // ... and won't bump the change counter, either.
+  do_check_eq(getSyncChangeCounter(profileStorage.addresses, guid), 1);
 });
 
 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();
+  let guid = addresses[1].guid;
+
+  // Force to create sync metadata.
+  profileStorage.addresses.pullSyncChanges();
+  do_check_eq(getSyncChangeCounter(profileStorage.addresses, guid), 1);
+
   // Unable to merge because of conflict
-  do_check_eq(profileStorage.addresses.mergeIfPossible(addresses[1].guid, TEST_ADDRESS_3), false);
+  do_check_eq(profileStorage.addresses.mergeIfPossible(guid, TEST_ADDRESS_3), false);
 
   // Unable to merge because no overlap
-  do_check_eq(profileStorage.addresses.mergeIfPossible(addresses[1].guid, TEST_ADDRESS_4), false);
+  do_check_eq(profileStorage.addresses.mergeIfPossible(guid, TEST_ADDRESS_4), false);
+
+  // Shouldn't bump the change counter
+  do_check_eq(getSyncChangeCounter(profileStorage.addresses, guid), 1);
 });
 
 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);
--- a/browser/extensions/formautofill/test/unit/test_savedFieldNames.js
+++ b/browser/extensions/formautofill/test/unit/test_savedFieldNames.js
@@ -20,17 +20,17 @@ add_task(async function test_profileSave
 
 add_task(async function test_profileSavedFieldNames_observe() {
   let formAutofillParent = new FormAutofillParent();
   sinon.stub(formAutofillParent, "_updateSavedFieldNames");
 
   await formAutofillParent.init();
 
   // profile changed => Need to trigger updateValidFields
-  ["add", "update", "remove", "reconcile", "merge"].forEach(event => {
+  ["add", "update", "remove", "reconcile"].forEach(event => {
     formAutofillParent.observe(null, "formautofill-storage-changed", "add");
     do_check_eq(formAutofillParent._updateSavedFieldNames.called, true);
   });
 
   // profile metadata updated => no need to trigger updateValidFields
   formAutofillParent._updateSavedFieldNames.reset();
   formAutofillParent.observe(null, "formautofill-storage-changed", "notifyUsed");
   do_check_eq(formAutofillParent._updateSavedFieldNames.called, false);