Bug 1450789 - Use more accurate names for structure conflict event telemetry keys. r?tcsc draft
authorKit Cambridge <kit@yakshaving.ninja>
Mon, 02 Apr 2018 14:41:16 -0700
changeset 776355 3709883a0e674d1c543cc0edac8c57e4e3e46f4b
parent 776308 e1dd5af91068dc36b3715f3bf5f0741fa69fa29a
push id104846
push userbmo:kit@mozilla.com
push dateMon, 02 Apr 2018 21:42:39 +0000
reviewerstcsc
bugs1450789
milestone61.0a1
Bug 1450789 - Use more accurate names for structure conflict event telemetry keys. r?tcsc MozReview-Commit-ID: FCLy34YU3qK
toolkit/components/places/SyncedBookmarksMirror.jsm
toolkit/components/places/tests/sync/test_bookmark_deletion.js
--- a/toolkit/components/places/SyncedBookmarksMirror.jsm
+++ b/toolkit/components/places/SyncedBookmarksMirror.jsm
@@ -3402,20 +3402,20 @@ class BookmarkMerger {
     this.remoteTree = remoteTree;
     this.newRemoteContents = newRemoteContents;
     this.matchingDupesByLocalParentNode = new Map();
     this.mergedGuids = new Set();
     this.deleteLocally = new Set();
     this.deleteRemotely = new Set();
     this.structureCounts = {
       new: 0,
-      remoteItemDel: 0,
-      localFolderDel: 0,
-      localItemDel: 0,
-      remoteFolderDel: 0,
+      remoteRevives: 0, // Remote non-folder change wins over local deletion.
+      localDeletes: 0, // Local folder deletion wins over remote change.
+      localRevives: 0, // Local non-folder change wins over remote deletion.
+      remoteDeletes: 0, // Remote folder deletion wins over local change.
     };
     this.dupeCount = 0;
     this.extraTelemetryEvents = [];
   }
 
   summarizeTelemetryEvents() {
     let events = [...this.extraTelemetryEvents];
     if (this.dupeCount > 0) {
@@ -4103,27 +4103,27 @@ class BookmarkMerger {
 
     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.
         MirrorLog.trace("Remote non-folder ${remoteNode} deleted locally " +
                         "and changed remotely; taking remote change",
                         { remoteNode });
-        this.structureCounts.remoteItemDel++;
+        this.structureCounts.remoteRevives++;
         return BookmarkMerger.STRUCTURE.UNCHANGED;
       }
       // For folders, we always take the local deletion and relocate remotely
       // changed grandchildren to the merged node. We could use the mirror to
       // revive the child folder, but it's easier to relocate orphaned
       // grandchildren than to partially revive the child folder.
       MirrorLog.trace("Remote folder ${remoteNode} deleted locally " +
                       "and changed remotely; taking local deletion",
                       { remoteNode });
-      this.structureCounts.localFolderDel++;
+      this.structureCounts.localDeletes++;
     } else {
       MirrorLog.trace("Remote node ${remoteNode} deleted locally and not " +
                        "changed remotely; taking local deletion",
                        { remoteNode });
     }
 
     this.deleteRemotely.add(remoteNode.guid);
 
@@ -4172,22 +4172,22 @@ class BookmarkMerger {
       }
       return BookmarkMerger.STRUCTURE.UNCHANGED;
     }
 
     if (localNode.needsMerge) {
       if (!localNode.isFolder()) {
         MirrorLog.trace("Local non-folder ${localNode} deleted remotely and " +
                         "changed locally; taking local change", { localNode });
-        this.structureCounts.localItemDel++;
+        this.structureCounts.localRevives++;
         return BookmarkMerger.STRUCTURE.UNCHANGED;
       }
       MirrorLog.trace("Local folder ${localNode} deleted remotely and " +
                       "changed locally; taking remote deletion", { localNode });
-      this.structureCounts.remoteFolderDel++;
+      this.structureCounts.remoteDeletes++;
     } else {
       MirrorLog.trace("Local node ${localNode} deleted remotely and not " +
                       "changed locally; taking remote deletion", { localNode });
     }
 
     MirrorLog.trace("Local node ${localNode} deleted remotely; taking remote " +
                     "deletion", { localNode });
 
--- a/toolkit/components/places/tests/sync/test_bookmark_deletion.js
+++ b/toolkit/components/places/tests/sync/test_bookmark_deletion.js
@@ -109,17 +109,17 @@ add_task(async function test_complex_orp
     bmkUri: "http://example.com/f",
   }]));
 
   info("Apply remote");
   let changesToUpload = await buf.apply();
   deepEqual(await buf.fetchUnmergedGuids(), [], "Should merge all items");
   deepEqual(mergeTelemetryEvents, [{
     value: "structure",
-    extra: { new: "2", localFolderDel: "1", remoteFolderDel: "1" },
+    extra: { new: "2", localDeletes: "1", remoteDeletes: "1" },
   }], "Should record telemetry with structure change counts");
 
   let idsToUpload = inspectChangeRecords(changesToUpload);
   deepEqual(idsToUpload, {
     updated: ["bookmarkEEEE", "bookmarkFFFF", "folderAAAAAA", "folderCCCCCC"],
     deleted: ["folderDDDDDD"],
   }, "Should upload new records for (A > E), (C > F); tombstone for D");
 
@@ -301,17 +301,17 @@ add_task(async function test_locally_mod
     deleted: true,
   }]);
 
   info("Apply remote");
   let changesToUpload = await buf.apply();
   deepEqual(await buf.fetchUnmergedGuids(), [], "Should merge all items");
   deepEqual(mergeTelemetryEvents, [{
     value: "structure",
-    extra: { new: "1", localItemDel: "1", remoteFolderDel: "2" },
+    extra: { new: "1", localRevives: "1", remoteDeletes: "2" },
   }], "Should record telemetry for local item and remote folder deletions");
 
   let idsToUpload = inspectChangeRecords(changesToUpload);
   deepEqual(idsToUpload, {
     updated: ["bookmarkAAAA", "bookmarkFFFF", "bookmarkGGGG", "menu"],
     deleted: [],
   }, "Should upload A, relocated local orphans, and menu");
 
@@ -448,17 +448,17 @@ add_task(async function test_locally_del
     bmkUri: "http://example.com/g-remote",
   }]);
 
   info("Apply remote");
   let changesToUpload = await buf.apply();
   deepEqual(await buf.fetchUnmergedGuids(), [], "Should merge all items");
   deepEqual(mergeTelemetryEvents, [{
     value: "structure",
-    extra: { new: "1", remoteItemDel: "1", localFolderDel: "2" },
+    extra: { new: "1", remoteRevives: "1", localDeletes: "2" },
   }], "Should record telemetry for remote item and local folder deletions");
 
   let idsToUpload = inspectChangeRecords(changesToUpload);
   deepEqual(idsToUpload, {
     updated: ["bookmarkFFFF", "bookmarkGGGG", "menu"],
     deleted: ["bookmarkCCCC", "bookmarkEEEE", "folderBBBBBB", "folderDDDDDD"],
   }, "Should upload relocated remote orphans and menu");