diff --git a/browser/base/content/browser-places.js b/browser/base/content/browser-places.js index 7cd58aed89a1..2991108c2622 100644 --- a/browser/base/content/browser-places.js +++ b/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) { diff --git a/browser/components/BrowserGlue.jsm b/browser/components/BrowserGlue.jsm index 7908d169c5eb..2af604bd3a68 100644 --- a/browser/components/BrowserGlue.jsm +++ b/browser/components/BrowserGlue.jsm @@ -2644,6 +2644,12 @@ BrowserGlue.prototype = { }, }, + { + task: () => { + PlacesUIUtils.ensureBookmarkToolbarTelemetryListening(); + }, + }, + // Marionette needs to be initialized as very last step { task: () => { diff --git a/browser/components/places/PlacesUIUtils.jsm b/browser/components/places/PlacesUIUtils.jsm index 12f0507e2644..c8de3b9c40c9 100644 --- a/browser/components/places/PlacesUIUtils.jsm +++ b/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. diff --git a/browser/components/places/tests/browser/browser_bookmarks_toolbar_telemetry.js b/browser/components/places/tests/browser/browser_bookmarks_toolbar_telemetry.js index e82262df9f2e..23f56d11f6d1 100644 --- a/browser/components/places/tests/browser/browser_bookmarks_toolbar_telemetry.js +++ b/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) {