From f2e65e077bcc06907e00f6e3fd06568dadf48688 Mon Sep 17 00:00:00 2001
From: Jared Wein <jwein@mozilla.com>
Date: Thu, 27 Aug 2020 20:45:23 +0000
Subject: [PATCH] Bug 1659736 - Move tracking of bookmark roots during IE
 migration later so the source is clearer. r=Gijs

Differential Revision: https://phabricator.services.mozilla.com/D88059
---
 .../components/migration/MSMigrationUtils.jsm | 13 +++++++++----
 .../migration/tests/unit/test_IE_bookmarks.js | 19 +++++++++++++++++++
 .../tests/unit/test_Safari_bookmarks.js       |  6 ++----
 3 files changed, 30 insertions(+), 8 deletions(-)

diff --git a/browser/components/migration/MSMigrationUtils.jsm b/browser/components/migration/MSMigrationUtils.jsm
index 87b936d662ad..1aa460e29e21 100644
--- a/browser/components/migration/MSMigrationUtils.jsm
+++ b/browser/components/migration/MSMigrationUtils.jsm
@@ -415,8 +415,6 @@ Bookmarks.prototype = {
   migrate: function B_migrate(aCallback) {
     return (async () => {
       // Import to the bookmarks menu.
-      this._histogramBookmarkRoots |=
-        MigrationUtils.SOURCE_BOOKMARK_ROOTS_BOOKMARKS_MENU;
       let folderGuid = PlacesUtils.bookmarks.menuGuid;
       await this._migrateFolder(this._favoritesFolder, folderGuid);
       Services.telemetry
@@ -441,6 +439,15 @@ Bookmarks.prototype = {
     if (!bookmarks.length) {
       return;
     }
+
+    if (aDestFolderGuid == PlacesUtils.bookmarks.menuGuid) {
+      this._histogramBookmarkRoots |=
+        MigrationUtils.SOURCE_BOOKMARK_ROOTS_BOOKMARKS_MENU;
+    } else if (aDestFolderGuid == PlacesUtils.bookmarks.toolbarGuid) {
+      this._histogramBookmarkRoots |=
+        MigrationUtils.SOURCE_BOOKMARK_ROOTS_BOOKMARKS_TOOLBAR;
+    }
+
     if (
       !MigrationUtils.isStartupMigration &&
       PlacesUtils.getChildCountForFolder(aDestFolderGuid) >
@@ -474,8 +481,6 @@ Bookmarks.prototype = {
             entry.parent.equals(this._favoritesFolder);
           if (isBookmarksFolder && entry.isReadable()) {
             // Import to the bookmarks toolbar.
-            this._histogramBookmarkRoots |=
-              MigrationUtils.SOURCE_BOOKMARK_ROOTS_BOOKMARKS_TOOLBAR;
             let folderGuid = PlacesUtils.bookmarks.toolbarGuid;
             await this._migrateFolder(entry, folderGuid);
             PlacesUIUtils.maybeToggleBookmarkToolbarVisibilityAfterMigration();
diff --git a/browser/components/migration/tests/unit/test_IE_bookmarks.js b/browser/components/migration/tests/unit/test_IE_bookmarks.js
index 57e508c6e524..4055797b1c55 100644
--- a/browser/components/migration/tests/unit/test_IE_bookmarks.js
+++ b/browser/components/migration/tests/unit/test_IE_bookmarks.js
@@ -13,10 +13,18 @@ add_task(async function() {
   // on the actual favorites stored on the local machine's IE favorites database.
   // As such, we can't assert that bookmarks were migrated to both the bookmarks
   // menu and the bookmarks toolbar.
+  let bookmarkRoots = 0;
   let itemCount = 0;
   let listener = events => {
     for (let event of events) {
       if (event.itemType == PlacesUtils.bookmarks.TYPE_BOOKMARK) {
+        if (event.parentGuid == PlacesUtils.bookmarks.toolbarGuid) {
+          bookmarkRoots |=
+            MigrationUtils.SOURCE_BOOKMARK_ROOTS_BOOKMARKS_TOOLBAR;
+        } else if (event.parentGuid == PlacesUtils.bookmarks.menuGuid) {
+          bookmarkRoots |= MigrationUtils.SOURCE_BOOKMARK_ROOTS_BOOKMARKS_MENU;
+        }
+        info("bookmark added: " + event.parentGuid);
         itemCount++;
       }
     }
@@ -45,6 +53,17 @@ add_task(async function() {
     itemCount,
     "Ensure telemetry matches actual number of imported items."
   );
+  await TestUtils.waitForCondition(() => {
+    let snapshot = Services.telemetry.getSnapshotForKeyedHistograms(
+      "main",
+      false
+    ).parent.FX_MIGRATION_BOOKMARKS_ROOTS;
+    if (!snapshot || !snapshot.ie) {
+      return false;
+    }
+    info(`Expected ${bookmarkRoots}, got ${snapshot.ie.sum}`);
+    return snapshot.ie.sum == bookmarkRoots;
+  }, "Wait until telemetry is updated");
 
   // Check the bookmarks have been imported to all the expected parents.
   Assert.ok(observerNotified, "The observer should be notified upon migration");
diff --git a/browser/components/migration/tests/unit/test_Safari_bookmarks.js b/browser/components/migration/tests/unit/test_Safari_bookmarks.js
index 79cbcf9203c0..0caeeb040e65 100644
--- a/browser/components/migration/tests/unit/test_Safari_bookmarks.js
+++ b/browser/components/migration/tests/unit/test_Safari_bookmarks.js
@@ -90,10 +90,8 @@ add_task(async function() {
       if (!snapshot || !snapshot.safari) {
         return false;
       }
-      let sum = arr => Object.values(arr).reduce((a, b) => a + b, 0);
-      let sumOfValues = sum(snapshot.safari.values);
-      info(`Expected ${bookmarkRoots}, got ${sumOfValues}`);
-      return sumOfValues == bookmarkRoots;
+      info(`Expected ${bookmarkRoots}, got ${snapshot.safari.sum}`);
+      return snapshot.safari.sum == bookmarkRoots;
     },
     "Wait until telemetry is updated"
   );