Bug 1273234 - Reduce the number of false positives reported by sync's bookmark_validator. r?markh
MozReview-Commit-ID: 6VulTTDcfs3
--- a/services/sync/modules/bookmark_validator.js
+++ b/services/sync/modules/bookmark_validator.js
@@ -17,38 +17,56 @@ class BookmarkValidator {
createClientRecordsFromTree(clientTree) {
// Iterate over the treeNode, converting it to something more similar to what
// the server stores.
let records = [];
function traverse(treeNode) {
let guid = BookmarkSpecialIds.specialGUIDForId(treeNode.id) || treeNode.guid;
let itemType = 'item';
+ treeNode.ignored = PlacesUtils.annotations.itemHasAnnotation(treeNode.id, BookmarkAnnos.EXCLUDEBACKUP_ANNO);
treeNode.id = guid;
switch (treeNode.type) {
case PlacesUtils.TYPE_X_MOZ_PLACE:
let query = null;
if (treeNode.annos && treeNode.uri.startsWith("place:")) {
query = treeNode.annos.find(({name}) =>
name === BookmarkAnnos.SMART_BOOKMARKS_ANNO);
}
if (query && query.value) {
itemType = 'query';
} else {
itemType = 'bookmark';
}
break;
case PlacesUtils.TYPE_X_MOZ_PLACE_CONTAINER:
- itemType = 'folder';
+ let isLivemark = false;
+ if (treeNode.annos) {
+ for (let anno of treeNode.annos) {
+ if (anno.name === PlacesUtils.LMANNO_FEEDURI) {
+ isLivemark = true;
+ treeNode.feedUri = anno.value;
+ } else if (anno.name === PlacesUtils.LMANNO_SITEURI) {
+ isLivemark = true;
+ treeNode.siteUri = anno.value;
+ }
+ }
+ }
+ itemType = isLivemark ? "livemark" : "folder";
break;
case PlacesUtils.TYPE_X_MOZ_PLACE_SEPARATOR:
itemType = 'separator';
break;
}
+ if (treeNode.tags) {
+ treeNode.tags = treeNode.tags.split(",");
+ } else {
+ treeNode.tags = [];
+ }
treeNode.type = itemType;
treeNode.pos = treeNode.index;
treeNode.bmkUri = treeNode.uri;
records.push(treeNode);
if (treeNode.type === 'folder') {
treeNode.childGUIDs = [];
if (!treeNode.children) {
treeNode.children = [];
@@ -148,17 +166,17 @@ class BookmarkValidator {
deletedItemIds.add(record.id);
} else {
if (idToRecord.has(record.id)) {
problemData.duplicates.push(record.id);
continue;
}
idToRecord.set(record.id, record);
}
- if (record.children) {
+ if (record.children && record.type !== "livemark") {
if (record.type !== 'folder') {
problemData.childrenOnNonFolder.push(record.id);
}
folders.push(record);
if (new Set(record.children).size !== record.children.length) {
problemData.duplicateChildren.push(record.id)
}
@@ -340,40 +358,50 @@ class BookmarkValidator {
}
return cycles;
}
/**
* Compare the list of server records with the client tree.
*
- * Returns the problemData described in the inspectServerRecords comment,
+ * Returns the same data as described in the inspectServerRecords comment,
* with the following additional fields.
- * - clientMissing: Array of ids on the server missing from the client
- * - serverMissing: Array of ids on the client missing from the server
- * - differences: Array of {id: string, differences: string array} recording
- * the properties that are differente between the client and server
+ * - clientRecords: an array of client records in a similar format to
+ * the .records (ie, server records) entry.
+ * - problemData is the same as for inspectServerRecords, but with the
+ * following additional entries.
+ * - clientMissing: Array of ids on the server missing from the client
+ * - serverMissing: Array of ids on the client missing from the server
+ * - serverUnexpected: Array of ids that appear on the server but shouldn't
+ * because the client attempts to never upload them.
+ * - differences: Array of {id: string, differences: string array} recording
+ * the properties that are differente between the client and server
*/
compareServerWithClient(serverRecords, clientTree) {
let clientRecords = this.createClientRecordsFromTree(clientTree);
let inspectionInfo = this.inspectServerRecords(serverRecords);
+ inspectionInfo.clientRecords = clientRecords;
// Mainly do this to remove deleted items and normalize child guids.
serverRecords = inspectionInfo.records;
let problemData = inspectionInfo.problemData;
problemData.clientMissing = [];
problemData.serverMissing = [];
+ problemData.serverDeleted = [];
+ problemData.serverUnexpected = [];
problemData.differences = [];
problemData.good = [];
let matches = [];
let allRecords = new Map();
+ let serverDeletedLookup = new Set(inspectionInfo.deletedRecords.map(r => r.id));
for (let sr of serverRecords) {
allRecords.set(sr.id, {client: null, server: sr});
}
for (let cr of clientRecords) {
let unified = allRecords.get(cr.id);
if (!unified) {
@@ -385,21 +413,29 @@ class BookmarkValidator {
for (let [id, {client, server}] of allRecords) {
if (!client && server) {
problemData.clientMissing.push(id);
continue;
}
if (!server && client) {
- problemData.serverMissing.push(id);
+ if (serverDeletedLookup.has(id)) {
+ problemData.serverDeleted.push(id);
+ } else if (!client.ignored && client.id != "places") {
+ problemData.serverMissing.push(id);
+ }
continue;
}
+ if (server && client && client.ignored) {
+ problemData.serverUnexpected.push(id);
+ }
let differences = [];
- if (client.title !== server.title) {
+ // We want to treat undefined, null and an empty string as identical
+ if ((client.title || "") !== (server.title || "")) {
differences.push('title');
}
if (client.parentid || server.parentid) {
if (client.parentid !== server.parentid) {
differences.push('parentid');
}
// Need to special case 'unfiled' due to it's recent name change
@@ -413,31 +449,47 @@ class BookmarkValidator {
if (client.tags || server.tags) {
let cl = client.tags || [];
let sl = server.tags || [];
if (cl.length !== sl.length || !cl.every((tag, i) => sl.indexOf(tag) >= 0)) {
differences.push('tags');
}
}
- if (client.type !== server.type) {
+ let sameType = client.type === server.type;
+ if (!sameType) {
+ if (server.type === "query" && client.type === "bookmark" && client.bmkUri.startsWith("place:")) {
+ sameType = true;
+ }
+ }
+
+
+ if (!sameType) {
differences.push('type');
} else {
switch (server.type) {
case 'bookmark':
case 'query':
if (server.bmkUri !== client.bmkUri) {
differences.push('bmkUri');
}
break;
case 'separator':
if (server.pos !== client.pos) {
differences.push('pos');
}
break;
+ case "livemark":
+ if (server.feedUri != client.feedUri) {
+ differences.push("feedUri");
+ }
+ if (server.siteUri != client.siteUri) {
+ differences.push("siteUri");
+ }
+ break;
case 'folder':
if (server.id === 'places' && !problemData.rootOnServer) {
// It's the fabricated places root. It won't have the GUIDs, but
// it doesn't matter.
break;
}
if (client.childGUIDs || server.childGUIDs) {
let cl = client.childGUIDs || [];
@@ -449,14 +501,14 @@ class BookmarkValidator {
break;
}
}
if (differences.length) {
problemData.differences.push({id, differences});
}
}
- return problemData;
+ return inspectionInfo;
}
};
--- a/services/sync/tests/unit/test_bookmark_validator.js
+++ b/services/sync/tests/unit/test_bookmark_validator.js
@@ -180,61 +180,61 @@ function getDummyServerAndClient() {
};
return {server, client};
}
add_test(function test_cswc_valid() {
let {server, client} = getDummyServerAndClient();
- let c = new BookmarkValidator().compareServerWithClient(server, client);
+ let c = new BookmarkValidator().compareServerWithClient(server, client).problemData;
equal(c.clientMissing.length, 0);
equal(c.serverMissing.length, 0);
equal(c.differences.length, 0);
run_next_test();
});
add_test(function test_cswc_serverMissing() {
let {server, client} = getDummyServerAndClient();
// remove c
server.pop();
server[0].children.pop();
- let c = new BookmarkValidator().compareServerWithClient(server, client);
+ let c = new BookmarkValidator().compareServerWithClient(server, client).problemData;
deepEqual(c.serverMissing, ['cccccccccccc']);
equal(c.clientMissing.length, 0);
deepEqual(c.differences, [{id: 'aaaaaaaaaaaa', differences: ['childGUIDs']}]);
run_next_test();
});
add_test(function test_cswc_clientMissing() {
let {server, client} = getDummyServerAndClient();
client.children[0].children.pop();
- let c = new BookmarkValidator().compareServerWithClient(server, client);
+ let c = new BookmarkValidator().compareServerWithClient(server, client).problemData;
deepEqual(c.clientMissing, ['cccccccccccc']);
equal(c.serverMissing.length, 0);
deepEqual(c.differences, [{id: 'aaaaaaaaaaaa', differences: ['childGUIDs']}]);
run_next_test();
});
add_test(function test_cswc_differences() {
{
let {server, client} = getDummyServerAndClient();
client.children[0].children[0].title = 'asdf';
- let c = new BookmarkValidator().compareServerWithClient(server, client);
+ let c = new BookmarkValidator().compareServerWithClient(server, client).problemData;
equal(c.clientMissing.length, 0);
equal(c.serverMissing.length, 0);
deepEqual(c.differences, [{id: 'bbbbbbbbbbbb', differences: ['title']}]);
}
{
let {server, client} = getDummyServerAndClient();
server[2].type = 'bookmark';
- let c = new BookmarkValidator().compareServerWithClient(server, client);
+ let c = new BookmarkValidator().compareServerWithClient(server, client).problemData;
equal(c.clientMissing.length, 0);
equal(c.serverMissing.length, 0);
deepEqual(c.differences, [{id: 'cccccccccccc', differences: ['type']}]);
}
run_next_test();
});
function run_test() {