Bug 1434800 - Don't treat children moved out of a deleted synced folder as orphans. r?markh
MozReview-Commit-ID: GAQMYNCvyDj
--- a/toolkit/components/places/SyncedBookmarksMirror.jsm
+++ b/toolkit/components/places/SyncedBookmarksMirror.jsm
@@ -3295,19 +3295,19 @@ class BookmarkMerger {
// Make sure the remote child isn't locally deleted. If it is, we need
// to move all descendants that aren't also remotely deleted to the
// merged node. This handles the case where a user deletes a folder
// on this device, and adds a bookmark to the same folder on another
// device. We want to keep the folder deleted, but we also don't want
// to lose the new bookmark, so we move the bookmark to the deleted
// folder's parent.
- let locallyDeleted = this.checkForLocalDeletionOfRemoteNode(mergedNode,
- remoteChildNode);
- if (locallyDeleted) {
+ let locallyMovedOrDeleted = this.checkForLocalMoveOrDeletionOfRemoteNode(
+ mergedNode, remoteChildNode);
+ if (locallyMovedOrDeleted) {
return true;
}
// The remote child isn't locally deleted. Does it exist in the local tree?
let localChildNode = this.localTree.nodeForGuid(remoteChildNode.guid);
if (!localChildNode) {
// Remote child doesn't exist locally, either. Try to find a content
// match in the containing folder, and dedupe the local item if we can.
@@ -3421,19 +3421,19 @@ class BookmarkMerger {
MirrorLog.trace("Merging local child ${localChildNode} of " +
"${localParentNode} into ${mergedNode}",
{ localChildNode, localParentNode, mergedNode });
// Now, we know we haven't seen the local child before, and it's not in
// this folder on the server. Check if the child is remotely deleted.
// If so, we need to move any new local descendants to the merged node,
// just as we did for new remote descendants of locally deleted parents.
- let remotelyDeleted = this.checkForRemoteDeletionOfLocalNode(mergedNode,
- localChildNode);
- if (remotelyDeleted) {
+ let remotelyMovedOrDeleted = this.checkForRemoteMoveOrDeletionOfLocalNode(
+ mergedNode, localChildNode);
+ if (remotelyMovedOrDeleted) {
return true;
}
// At this point, we know the local child isn't deleted. See if it
// exists in the remote tree.
let remoteChildNode = this.remoteTree.nodeForGuid(localChildNode.guid);
if (!remoteChildNode) {
// The local child doesn't exist remotely, but we still need to walk
@@ -3572,24 +3572,29 @@ class BookmarkMerger {
value: "structure",
extra: { type: "new" },
});
mergedNode.mergeState = newMergeState;
}
}
/**
- * Walks a locally deleted remote node's children, reparenting any children
- * that aren't also deleted remotely to the merged node. Returns `true` if
- * `remoteNode` is deleted locally; `false` if `remoteNode` is not deleted or
- * doesn't exist locally.
+ * Checks if a remote node is locally moved or deleted, and reparents any
+ * descendants that aren't also remotely deleted to the merged node. Returns
+ * `true` if `remoteNode` is locally moved or deleted; `false` if `remoteNode`
+ * is not deleted or doesn't exist locally.
*
- * This is the inverse of `checkForRemoteDeletionOfLocalNode`.
+ * This is the inverse of `checkForRemoteMoveOrDeletionOfLocalNode`.
*/
- checkForLocalDeletionOfRemoteNode(mergedNode, remoteNode) {
+ checkForLocalMoveOrDeletionOfRemoteNode(mergedNode, remoteNode) {
+ if (this.mergedGuids.has(remoteNode.guid)) {
+ // Already merged, so must be locally moved.
+ return true;
+ }
+
if (!this.localTree.isDeleted(remoteNode.guid)) {
return false;
}
if (remoteNode.needsMerge) {
if (!remoteNode.isFolder()) {
// If a non-folder child is deleted locally and changed remotely, we
// ignore the local deletion and take the remote child.
@@ -3626,24 +3631,29 @@ class BookmarkMerger {
this.relocateOrphansTo(mergedNode, mergedOrphanNodes);
MirrorLog.trace("Relocating remote orphans ${mergedOrphanNodes} to " +
"${mergedNode}", { mergedOrphanNodes, mergedNode });
return true;
}
/**
- * Walks a remotely deleted local node's children, reparenting any children
- * that aren't also deleted locally to the merged node. Returns `true` if
- * `localNode` is deleted remotely; `false` if `localNode` is not deleted or
- * doesn't exist locally.
+ * Checks if a local node is remotely moved or deleted, and reparents any
+ * descendants that aren't also locally deleted to the merged node. Returns
+ * `true` if `localNode` is moved or deleted remotely; `false` if `localNode`
+ * is not deleted or doesn't exist locally.
*
- * This is the inverse of `checkForLocalDeletionOfRemoteNode`.
+ * This is the inverse of `checkForLocalMoveOrDeletionOfRemoteNode`.
*/
- checkForRemoteDeletionOfLocalNode(mergedNode, localNode) {
+ checkForRemoteMoveOrDeletionOfLocalNode(mergedNode, localNode) {
+ if (this.mergedGuids.has(localNode.guid)) {
+ // Already merged, so must be remotely moved.
+ return true;
+ }
+
if (!this.remoteTree.isDeleted(localNode.guid)) {
return false;
}
if (localNode.needsMerge) {
if (!localNode.isFolder()) {
MirrorLog.trace("Local non-folder ${localNode} deleted remotely and " +
"changed locally; taking local change", { localNode });
@@ -3683,21 +3693,21 @@ class BookmarkMerger {
* haven't also been deleted remotely. This can happen if the user adds a
* bookmark to a folder on another device, and deletes that folder locally.
* This is the inverse of `processLocalOrphansForNode`.
*/
processRemoteOrphansForNode(mergedNode, remoteNode) {
let remoteOrphanNodes = [];
for (let remoteChildNode of remoteNode.children) {
- let locallyDeleted = this.checkForLocalDeletionOfRemoteNode(mergedNode,
- remoteChildNode);
- if (locallyDeleted) {
- // The remote child doesn't exist locally, or is also deleted locally,
- // so we can safely delete its parent.
+ let locallyMovedOrDeleted = this.checkForLocalMoveOrDeletionOfRemoteNode(
+ mergedNode, remoteChildNode);
+ if (locallyMovedOrDeleted) {
+ // The remote child doesn't exist locally, or is already moved or
+ // deleted locally, so we can safely ignore it.
continue;
}
remoteOrphanNodes.push(remoteChildNode);
}
let mergedOrphanNodes = [];
for (let remoteOrphanNode of remoteOrphanNodes) {
let localOrphanNode = this.localTree.nodeForGuid(remoteOrphanNode.guid);
@@ -3717,21 +3727,21 @@ class BookmarkMerger {
processLocalOrphansForNode(mergedNode, localNode) {
if (!localNode.isFolder()) {
// The local node isn't a folder, so it won't have orphans.
return [];
}
let localOrphanNodes = [];
for (let localChildNode of localNode.children) {
- let remotelyDeleted = this.checkForRemoteDeletionOfLocalNode(mergedNode,
- localChildNode);
- if (remotelyDeleted) {
- // The local child doesn't exist or is also deleted on the server, so we
- // can safely delete its parent without orphaning any local children.
+ let remotelyMovedOrDeleted = this.checkForRemoteMoveOrDeletionOfLocalNode(
+ mergedNode, localChildNode);
+ if (remotelyMovedOrDeleted) {
+ // The local child doesn't exist remotely, or is already moved or
+ // deleted remotely, so we can safely ignore it.
continue;
}
localOrphanNodes.push(localChildNode);
}
let mergedOrphanNodes = [];
for (let localOrphanNode of localOrphanNodes) {
let remoteOrphanNode = this.remoteTree.nodeForGuid(localOrphanNode.guid);
--- a/toolkit/components/places/tests/sync/test_bookmark_deletion.js
+++ b/toolkit/components/places/tests/sync/test_bookmark_deletion.js
@@ -644,8 +644,116 @@ add_task(async function test_nonexistent
},
},
});
await buf.finalize();
await PlacesUtils.bookmarks.eraseEverything();
await PlacesSyncUtils.bookmarks.reset();
});
+
+add_task(async function test_clear_folder_then_delete() {
+ let buf = await openMirror("clear_folder_then_delete");
+
+ info("Set up mirror");
+ await PlacesUtils.bookmarks.insertTree({
+ guid: PlacesUtils.bookmarks.menuGuid,
+ children: [{
+ guid: "folderAAAAAA",
+ type: PlacesUtils.bookmarks.TYPE_FOLDER,
+ title: "A",
+ children: [{
+ guid: "bookmarkBBBB",
+ url: "http://example.com/b",
+ title: "B",
+ }],
+ }, {
+ guid: "folderCCCCCC",
+ type: PlacesUtils.bookmarks.TYPE_FOLDER,
+ title: "C",
+ children: [{
+ guid: "bookmarkDDDD",
+ url: "http://example.com/d",
+ title: "D",
+ }],
+ }],
+ });
+ await buf.store([{
+ id: "menu",
+ type: "folder",
+ title: "Bookmarks Menu",
+ children: ["folderAAAAAA", "folderCCCCCC"],
+ }, {
+ id: "folderAAAAAA",
+ type: "folder",
+ title: "A",
+ children: ["bookmarkBBBB"],
+ }, {
+ id: "bookmarkBBBB",
+ type: "bookmark",
+ title: "A",
+ bmkUri: "http://example.com/b",
+ }, {
+ id: "folderCCCCCC",
+ type: "folder",
+ title: "C",
+ children: ["bookmarkDDDD"],
+ }, {
+ id: "bookmarkDDDD",
+ type: "bookmark",
+ title: "D",
+ bmkUri: "http://example.com/d",
+ }], { needsMerge: false });
+ await PlacesTestUtils.markBookmarksAsSynced();
+
+ info("Make local changes: Menu > D, delete C");
+ await PlacesUtils.bookmarks.update({
+ guid: "bookmarkDDDD",
+ parentGuid: PlacesUtils.bookmarks.menuGuid,
+ index: 0,
+ });
+ await PlacesUtils.bookmarks.remove("folderCCCCCC");
+
+ info("Make remote changes: Menu > B, delete A");
+ await buf.store([{
+ id: "menu",
+ type: "folder",
+ title: "Bookmarks Menu",
+ children: ["bookmarkBBBB"],
+ }, {
+ id: "folderAAAAAA",
+ deleted: true,
+ }]);
+
+ info("Apply remote");
+ let changesToUpload = await buf.apply();
+ deepEqual(await buf.fetchUnmergedGuids(), [], "Should merge all items");
+
+ let idsToUpload = inspectChangeRecords(changesToUpload);
+ deepEqual(idsToUpload, {
+ updated: ["bookmarkDDDD", "menu"],
+ deleted: ["folderCCCCCC"],
+ }, "Should upload locally moved and deleted items");
+
+ await assertLocalTree(PlacesUtils.bookmarks.menuGuid, {
+ guid: PlacesUtils.bookmarks.menuGuid,
+ type: PlacesUtils.bookmarks.TYPE_FOLDER,
+ index: 0,
+ title: "Bookmarks Menu",
+ children: [{
+ guid: "bookmarkBBBB",
+ type: PlacesUtils.bookmarks.TYPE_BOOKMARK,
+ index: 0,
+ title: "B",
+ url: "http://example.com/b",
+ }, {
+ guid: "bookmarkDDDD",
+ type: PlacesUtils.bookmarks.TYPE_BOOKMARK,
+ index: 1,
+ title: "D",
+ url: "http://example.com/d",
+ }],
+ }, "Should not treat moved children of a deleted folder as orphans");
+
+ await buf.finalize();
+ await PlacesUtils.bookmarks.eraseEverything();
+ await PlacesSyncUtils.bookmarks.reset();
+});