--- a/toolkit/components/places/SyncedBookmarksMirror.jsm
+++ b/toolkit/components/places/SyncedBookmarksMirror.jsm
@@ -3529,53 +3529,63 @@ class BookmarkMerger {
return false;
}
/**
* Recursively merges the children of a local folder node and a matching
* remote folder node.
*
* @param {MergedBookmarkNode} mergedNode
- * The merged folder state. This method mutates the merged node to
- * append merged children, and change the node's merge state to new
- * if needed.
+ * The merged folder node. This method mutates the merged node to
+ * append local and remote children, and sets a new merge state
+ * state if needed.
* @param {BookmarkNode?} localNode
* The local folder node. May be `null` if the folder only exists
* remotely.
* @param {BookmarkNode?} remoteNode
* The remote folder node. May be `null` if the folder only exists
* locally.
*/
mergeChildListsIntoMergedNode(mergedNode, localNode, remoteNode) {
let mergeStateChanged = false;
- // Walk and merge remote children first.
- MirrorLog.trace("Merging remote children of ${remoteNode} into " +
- "${mergedNode}", { remoteNode, mergedNode });
- if (remoteNode) {
- for (let remoteChildNode of remoteNode.children) {
- let remoteChildrenChanged = this.mergeRemoteChildIntoMergedNode(
- mergedNode, remoteNode, remoteChildNode);
- if (remoteChildrenChanged) {
+ if (localNode && remoteNode) {
+ if (localNode.newerThan(remoteNode)) {
+ // The folder exists locally and remotely, and the local node is newer.
+ // Walk and merge local children first, followed by remaining unmerged
+ // remote children.
+ if (this.mergeLocalChildrenIntoMergedNode(mergedNode, localNode)) {
+ mergeStateChanged = true;
+ }
+ if (this.mergeRemoteChildrenIntoMergedNode(mergedNode, remoteNode)) {
+ mergeStateChanged = true;
+ }
+ } else {
+ // The folder exists locally and remotely, and the remote node is newer.
+ // Merge remote children first, then remaining local children.
+ if (this.mergeRemoteChildrenIntoMergedNode(mergedNode, remoteNode)) {
+ mergeStateChanged = true;
+ }
+ if (this.mergeLocalChildrenIntoMergedNode(mergedNode, localNode)) {
mergeStateChanged = true;
}
}
- }
-
- // Now walk and merge any local children that we haven't already merged.
- MirrorLog.trace("Merging local children of ${localNode} into " +
- "${mergedNode}", { localNode, mergedNode });
- if (localNode) {
- for (let localChildNode of localNode.children) {
- let remoteChildrenChanged = this.mergeLocalChildIntoMergedNode(
- mergedNode, localNode, localChildNode);
- if (remoteChildrenChanged) {
- mergeStateChanged = true;
- }
+ } else if (localNode) {
+ // The folder only exists locally, so no remote children to merge.
+ if (this.mergeLocalChildrenIntoMergedNode(mergedNode, localNode)) {
+ mergeStateChanged = true;
}
+ } else if (remoteNode) {
+ // The folder only exists remotely, so local children to merge.
+ if (this.mergeRemoteChildrenIntoMergedNode(mergedNode, remoteNode)) {
+ mergeStateChanged = true;
+ }
+ } else {
+ // Should never happen.
+ throw new TypeError("Can't merge children for two nonexistent nodes");
}
// Update the merge state if we moved children orphaned on one side by a
// deletion on the other side, if we kept newer locally moved children,
// or if the child order changed. We already updated the merge state of the
// orphans, but we also need to flag the containing folder so that it's
// reuploaded to the server along with the new children.
if (mergeStateChanged) {
@@ -3588,16 +3598,70 @@ class BookmarkMerger {
value: "structure",
extra: { type: "new" },
});
mergedNode.mergeState = newMergeState;
}
}
/**
+ * Recursively merges the children of a remote folder node.
+ *
+ * @param {MergedBookmarkNode} mergedNode
+ * The merged folder node. This method mutates the merged node to
+ * append remote children.
+ * @param {BookmarkNode} remoteNode
+ * The remote folder node.
+ * @return {Boolean}
+ * `true` if the merge produced a new structure that should be
+ * reuploaded to the server; `false` otherwise.
+ */
+ mergeRemoteChildrenIntoMergedNode(mergedNode, remoteNode) {
+ MirrorLog.trace("Merging remote children of ${remoteNode} into " +
+ "${mergedNode}", { remoteNode, mergedNode });
+
+ let mergeStateChanged = false;
+ for (let remoteChildNode of remoteNode.children) {
+ let remoteChildrenChanged = this.mergeRemoteChildIntoMergedNode(
+ mergedNode, remoteNode, remoteChildNode);
+ if (remoteChildrenChanged) {
+ mergeStateChanged = true;
+ }
+ }
+ return mergeStateChanged;
+ }
+
+ /**
+ * Recursively merges the children of a local folder node.
+ *
+ * @param {MergedBookmarkNode} mergedNode
+ * The merged folder node. This method mutates the merged node to
+ * append local children.
+ * @param {BookmarkNode} localNode
+ * The local folder node.
+ * @return {Boolean}
+ * `true` if the merge produced a new structure that should be
+ * reuploaded to the server; `false` otherwise.
+ */
+ mergeLocalChildrenIntoMergedNode(mergedNode, localNode) {
+ MirrorLog.trace("Merging local children of ${localNode} into " +
+ "${mergedNode}", { localNode, mergedNode });
+
+ let mergeStateChanged = false;
+ for (let localChildNode of localNode.children) {
+ let remoteChildrenChanged = this.mergeLocalChildIntoMergedNode(
+ mergedNode, localNode, localChildNode);
+ if (remoteChildrenChanged) {
+ mergeStateChanged = true;
+ }
+ }
+ return mergeStateChanged;
+ }
+
+ /**
* Checks if a remote node is locally moved or deleted, and reparents any
* descendants that aren't also remotely deleted to the merged node.
*
* This is the inverse of `checkForRemoteStructureChangeOfLocalNode`.
*
* @param {MergedBookmarkNode} mergedNode
* The merged folder node to hold relocated remote orphans.
* @param {BookmarkNode} remoteParentNode
--- a/toolkit/components/places/tests/sync/test_bookmark_structure_changes.js
+++ b/toolkit/components/places/tests/sync/test_bookmark_structure_changes.js
@@ -735,8 +735,247 @@ add_task(async function test_complex_mov
title: "mobile",
}],
}, "Should take remote order and preserve local children");
await buf.finalize();
await PlacesUtils.bookmarks.eraseEverything();
await PlacesSyncUtils.bookmarks.reset();
});
+
+add_task(async function test_reorder_and_insert() {
+ let buf = await openMirror("reorder_and_insert");
+
+ info("Set up mirror");
+ await PlacesUtils.bookmarks.insertTree({
+ guid: PlacesUtils.bookmarks.menuGuid,
+ children: [{
+ guid: "bookmarkAAAA",
+ url: "http://example.com/a",
+ title: "A",
+ }, {
+ guid: "bookmarkBBBB",
+ url: "http://example.com/b",
+ title: "B",
+ }, {
+ guid: "bookmarkCCCC",
+ url: "http://example.com/c",
+ title: "C",
+ }],
+ });
+ await PlacesUtils.bookmarks.insertTree({
+ guid: PlacesUtils.bookmarks.toolbarGuid,
+ children: [{
+ guid: "bookmarkDDDD",
+ url: "http://example.com/d",
+ title: "D",
+ }, {
+ guid: "bookmarkEEEE",
+ url: "http://example.com/e",
+ title: "E",
+ }, {
+ guid: "bookmarkFFFF",
+ url: "http://example.com/f",
+ title: "F",
+ }],
+ });
+ await buf.store(shuffle([{
+ id: "menu",
+ type: "folder",
+ title: "Bookmarks Menu",
+ children: ["bookmarkAAAA", "bookmarkBBBB", "bookmarkCCCC"],
+ }, {
+ id: "bookmarkAAAA",
+ type: "bookmark",
+ title: "A",
+ bmkUri: "http://example.com/a",
+ }, {
+ id: "bookmarkBBBB",
+ type: "bookmark",
+ title: "B",
+ bmkUri: "http://example.com/b",
+ }, {
+ id: "bookmarkCCCC",
+ type: "bookmark",
+ title: "C",
+ bmkUri: "http://example.com/c",
+ }, {
+ id: "toolbar",
+ type: "folder",
+ title: "Bookmarks Toolbar",
+ children: ["bookmarkDDDD", "bookmarkEEEE", "bookmarkFFFF"],
+ }, {
+ id: "bookmarkDDDD",
+ type: "bookmark",
+ title: "D",
+ bmkUri: "http://example.com/d",
+ }, {
+ id: "bookmarkEEEE",
+ type: "bookmark",
+ title: "E",
+ bmkUri: "http://example.com/e",
+ }, {
+ id: "bookmarkFFFF",
+ type: "bookmark",
+ title: "F",
+ bmkUri: "http://example.com/f",
+ }]), { needsMerge: false });
+ await PlacesTestUtils.markBookmarksAsSynced();
+
+ let now = Date.now();
+
+ info("Make local changes: Reorder Menu, Toolbar > (G H)");
+ await PlacesUtils.bookmarks.reorder(PlacesUtils.bookmarks.menuGuid, [
+ "bookmarkCCCC", "bookmarkAAAA", "bookmarkBBBB"]);
+ await PlacesUtils.bookmarks.insertTree({
+ guid: PlacesUtils.bookmarks.toolbarGuid,
+ children: [{
+ guid: "bookmarkGGGG",
+ url: "http://example.com/g",
+ title: "G",
+ dateAdded: new Date(now),
+ lastModified: new Date(now),
+ }, {
+ guid: "bookmarkHHHH",
+ url: "http://example.com/h",
+ title: "H",
+ dateAdded: new Date(now),
+ lastModified: new Date(now),
+ }],
+ });
+
+ info("Make remote changes: Reorder Toolbar, Menu > (I J)");
+ await buf.store(shuffle([{
+ // The server has a newer toolbar, so we should use the remote order (F D E)
+ // as the base, then append (G H).
+ id: "toolbar",
+ type: "folder",
+ title: "Bookmarks Toolbar",
+ children: ["bookmarkFFFF", "bookmarkDDDD", "bookmarkEEEE"],
+ modified: now / 1000 + 5,
+ }, {
+ // The server has an older menu, so we should use the local order (C A B)
+ // as the base, then append (I J).
+ id: "menu",
+ type: "folder",
+ title: "Bookmarks Menu",
+ children: ["bookmarkAAAA", "bookmarkBBBB", "bookmarkCCCC", "bookmarkIIII",
+ "bookmarkJJJJ"],
+ modified: now / 1000 - 5,
+ }, {
+ id: "bookmarkIIII",
+ type: "bookmark",
+ title: "I",
+ bmkUri: "http://example.com/i",
+ }, {
+ id: "bookmarkJJJJ",
+ type: "bookmark",
+ title: "J",
+ bmkUri: "http://example.com/j",
+ }]));
+
+ info("Apply remote");
+ let changesToUpload = await buf.apply({
+ remoteTimeSeconds: now / 1000,
+ localTimeSeconds: now / 1000,
+ });
+ deepEqual(await buf.fetchUnmergedGuids(), [], "Should merge all items");
+
+ let idsToUpload = inspectChangeRecords(changesToUpload);
+ deepEqual(idsToUpload, {
+ updated: ["bookmarkGGGG", "bookmarkHHHH", "menu", "toolbar"],
+ deleted: [],
+ }, "Should upload records for merged and new local items");
+
+ await assertLocalTree(PlacesUtils.bookmarks.rootGuid, {
+ guid: PlacesUtils.bookmarks.rootGuid,
+ type: PlacesUtils.bookmarks.TYPE_FOLDER,
+ index: 0,
+ title: "",
+ children: [{
+ guid: PlacesUtils.bookmarks.menuGuid,
+ type: PlacesUtils.bookmarks.TYPE_FOLDER,
+ index: 0,
+ title: "Bookmarks Menu",
+ children: [{
+ guid: "bookmarkCCCC",
+ type: PlacesUtils.bookmarks.TYPE_BOOKMARK,
+ index: 0,
+ url: "http://example.com/c",
+ title: "C",
+ }, {
+ guid: "bookmarkAAAA",
+ type: PlacesUtils.bookmarks.TYPE_BOOKMARK,
+ index: 1,
+ url: "http://example.com/a",
+ title: "A",
+ }, {
+ guid: "bookmarkBBBB",
+ type: PlacesUtils.bookmarks.TYPE_BOOKMARK,
+ index: 2,
+ url: "http://example.com/b",
+ title: "B",
+ }, {
+ guid: "bookmarkIIII",
+ type: PlacesUtils.bookmarks.TYPE_BOOKMARK,
+ index: 3,
+ url: "http://example.com/i",
+ title: "I",
+ }, {
+ guid: "bookmarkJJJJ",
+ type: PlacesUtils.bookmarks.TYPE_BOOKMARK,
+ index: 4,
+ url: "http://example.com/j",
+ title: "J",
+ }],
+ }, {
+ guid: PlacesUtils.bookmarks.toolbarGuid,
+ type: PlacesUtils.bookmarks.TYPE_FOLDER,
+ index: 1,
+ title: "Bookmarks Toolbar",
+ children: [{
+ guid: "bookmarkFFFF",
+ type: PlacesUtils.bookmarks.TYPE_BOOKMARK,
+ index: 0,
+ url: "http://example.com/f",
+ title: "F",
+ }, {
+ guid: "bookmarkDDDD",
+ type: PlacesUtils.bookmarks.TYPE_BOOKMARK,
+ index: 1,
+ url: "http://example.com/d",
+ title: "D",
+ }, {
+ guid: "bookmarkEEEE",
+ type: PlacesUtils.bookmarks.TYPE_BOOKMARK,
+ index: 2,
+ url: "http://example.com/e",
+ title: "E",
+ }, {
+ guid: "bookmarkGGGG",
+ type: PlacesUtils.bookmarks.TYPE_BOOKMARK,
+ index: 3,
+ url: "http://example.com/g",
+ title: "G",
+ }, {
+ guid: "bookmarkHHHH",
+ type: PlacesUtils.bookmarks.TYPE_BOOKMARK,
+ index: 4,
+ url: "http://example.com/h",
+ title: "H",
+ }],
+ }, {
+ guid: PlacesUtils.bookmarks.unfiledGuid,
+ type: PlacesUtils.bookmarks.TYPE_FOLDER,
+ index: 3,
+ title: "Other Bookmarks",
+ }, {
+ guid: PlacesUtils.bookmarks.mobileGuid,
+ type: PlacesUtils.bookmarks.TYPE_FOLDER,
+ index: 4,
+ title: "mobile",
+ }],
+ }, "Should use timestamps to decide base folder order");
+
+ await buf.finalize();
+ await PlacesUtils.bookmarks.eraseEverything();
+ await PlacesSyncUtils.bookmarks.reset();
+});