Bug 1682553 - Move bookmarks listeners to PlacesUIUtils so there will only be one listener instead of one per window. r=Gijs

Differential Revision: https://phabricator.services.mozilla.com/D99923

browser/base/content/browser-places.js

@@ -2314,12 +2314,6 @@ var BookmarkingUI = {
               }
             }
           }
-          if (ev.parentGuid == PlacesUtils.bookmarks.toolbarGuid) {
-            Services.telemetry.scalarAdd(
-              "browser.engagement.bookmarks_toolbar_bookmark_added",
-              1
-            );
-          }
           break;
         case "bookmark-removed":
           // If one of the tracked bookmarks has been removed, unregister it.
@@ -2397,12 +2391,6 @@ var BookmarkingUI = {
     if (hasMovedToToolbar || hasMovedOutOfToolbar) {
       this.updateEmptyToolbarMessage();
     }
-    if (hasMovedToToolbar) {
-      Services.telemetry.scalarAdd(
-        "browser.engagement.bookmarks_toolbar_bookmark_added",
-        1
-      );
-    }
   },
 
   onWidgetUnderflow(aNode, aContainer) {

browser/components/BrowserGlue.jsm

@@ -2644,6 +2644,12 @@ BrowserGlue.prototype = {
         },
       },
 
+      {
+        task: () => {
+          PlacesUIUtils.ensureBookmarkToolbarTelemetryListening();
+        },
+      },
+
       // Marionette needs to be initialized as very last step
       {
         task: () => {

browser/components/places/PlacesUIUtils.jsm

@@ -233,6 +233,7 @@ let InternalFaviconLoader = {
 };
 
 var PlacesUIUtils = {
+  _bookmarkToolbarTelemetryListening: false,
   LAST_USED_FOLDERS_META_KEY: "bookmarks/lastusedfolders",
 
   getFormattedString: function PUIU_getFormattedString(key, params) {
@@ -1160,6 +1161,69 @@ var PlacesUIUtils = {
     }
   },
 
+  ensureBookmarkToolbarTelemetryListening() {
+    if (this._bookmarkToolbarTelemetryListening) {
+      return;
+    }
+
+    // This listener is for counting new bookmarks
+    let placesUtilsObserversListener = events => {
+      for (let event of events) {
+        if (
+          event.type == "bookmark-added" &&
+          event.parentGuid == PlacesUtils.bookmarks.toolbarGuid
+        ) {
+          Services.telemetry.scalarAdd(
+            "browser.engagement.bookmarks_toolbar_bookmark_added",
+            1
+          );
+        }
+      }
+    };
+
+    // This listener is for tracking bookmark moves
+    let placesUtilsBookmarksObserver = {
+      onBeginUpdateBatch() {},
+      onEndUpdateBatch() {},
+      onItemChanged() {},
+      onItemMoved(
+        aItemId,
+        aProperty,
+        aIsAnnotationProperty,
+        aNewValue,
+        aLastModified,
+        aItemType,
+        aGuid,
+        oldParentGuid,
+        newParentGuid
+      ) {
+        let hasMovedToToolbar =
+          newParentGuid == PlacesUtils.bookmarks.toolbarGuid &&
+          oldParentGuid != PlacesUtils.bookmarks.toolbarGuid;
+        if (hasMovedToToolbar) {
+          Services.telemetry.scalarAdd(
+            "browser.engagement.bookmarks_toolbar_bookmark_added",
+            1
+          );
+        }
+      },
+    };
+
+    this._bookmarkToolbarTelemetryListening = true;
+    PlacesUtils.observers.addListener(
+      ["bookmark-added"],
+      placesUtilsObserversListener
+    );
+    PlacesUtils.bookmarks.addObserver(placesUtilsBookmarksObserver);
+    PlacesUtils.registerShutdownFunction(() => {
+      PlacesUtils.observers.removeListener(
+        ["bookmark-added"],
+        placesUtilsObserversListener
+      );
+      PlacesUtils.bookmarks.removeObserver(placesUtilsBookmarksObserver);
+    });
+  },
+
   /**
    * Uncollapses PersonalToolbar if its collapsed status is not
    * persisted, and user customized it or changed default bookmarks.

browser/components/places/tests/browser/browser_bookmarks_toolbar_telemetry.js

@@ -64,6 +64,12 @@ add_task(async function test_bookmarks_toolbar_telemetry() {
     1
   );
 
+  // Extra windows are opened to make sure telemetry numbers aren't
+  // double counted since there will be multiple instances of the
+  // bookmarks toolbar.
+  let extraWindows = [];
+  extraWindows.push(await BrowserTestUtils.openNewBrowserWindow());
+
   Services.telemetry.getSnapshotForScalars("main", true);
   let bookmarks = await PlacesUtils.bookmarks.insertTree({
     guid: PlacesUtils.bookmarks.toolbarGuid,
@@ -79,17 +85,39 @@ add_task(async function test_bookmarks_toolbar_telemetry() {
 
   let newtab = await BrowserTestUtils.openNewForegroundTab(gBrowser);
   registerCleanupFunction(() => BrowserTestUtils.removeTab(newtab));
-  Services.telemetry.getSnapshotForScalars("main", true);
   let bookmarkToolbarButton = document.querySelector(
     "#PlacesToolbarItems > toolbarbutton"
   );
   bookmarkToolbarButton.click();
   TelemetryTestUtils.assertScalar(
-    TelemetryTestUtils.getProcessScalars("parent", false, true),
+    TelemetryTestUtils.getProcessScalars("parent"),
     "browser.engagement.bookmarks_toolbar_bookmark_opened",
     1,
     "Bookmarks opened value should be 1"
   );
+
+  extraWindows.push(await BrowserTestUtils.openNewBrowserWindow());
+  extraWindows.push(await BrowserTestUtils.openNewBrowserWindow());
+
+  // Simulate dragging a bookmark within the toolbar to ensure
+  // that the bookmarks_toolbar_bookmark_added probe doesn't increment
+  let srcElement = document.querySelector("#PlacesToolbar .bookmark-item");
+  let destElement = document.querySelector("#PlacesToolbar");
+  await EventUtils.synthesizePlainDragAndDrop({
+    srcElement,
+    destElement,
+  });
+
+  TelemetryTestUtils.assertScalar(
+    TelemetryTestUtils.getProcessScalars("parent"),
+    "browser.engagement.bookmarks_toolbar_bookmark_added",
+    3,
+    "Bookmarks added value should still be 3"
+  );
+
+  for (let win of extraWindows) {
+    await BrowserTestUtils.closeWindow(win);
+  }
 });
 
 async function changeToolbarVisibilityViaContextMenu(nextState) {