Bug 1412788 - [Form Autofill] The sync metadata should be updated while merging address records. r=steveck
MozReview-Commit-ID: 3arMCcVTPPE
--- 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);