Bug 1412788 - [Form Autofill] The sync metadata should be updated while merging address records. r=steveck
authorLuke Chang <lchang@mozilla.com>
Mon, 30 Oct 2017 18:55:50 +0800
changeset 389252 b5110cc80ef6d86469e1bb62118d596ac293d269
parent 389251 4a51f3ef01e58f1258969b2db76056e4ab1d2e05
child 389253 1abbf079b653304fea50a3736597f289584ac94e
push id54402
push userlchang@mozilla.com
push dateTue, 31 Oct 2017 06:35:40 +0000
treeherderautoland@b5110cc80ef6 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerssteveck
bugs1412788
milestone58.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 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);