From f26a05836126d1f3b34ca2b7b7b4e92c155f0fbc Mon Sep 17 00:00:00 2001 From: Neil Deakin Date: Thu, 16 Sep 2021 19:11:17 +0000 Subject: [PATCH] Bug 1690390, add ability to perform telemetry when a notification bar is shown, a button is pressed, or is dismissed. Use this for the subframe crash notification bar which currently does this a different way, r=mstriemer,chutten Differential Revision: https://phabricator.services.mozilla.com/D107756 --- browser/base/content/tabbrowser.js | 2 + .../content/test/notificationbox/browser.ini | 1 + .../browser_notificationbar_telemetry.js | 150 ++++++++++++++++++ browser/modules/ContentCrashHandlers.jsm | 44 +---- dom/ipc/tests/browser_crash_oopiframe.js | 39 ++--- toolkit/components/telemetry/Scalars.yaml | 68 ++++++-- toolkit/content/widgets/notificationbox.js | 80 ++++++++++ 7 files changed, 308 insertions(+), 76 deletions(-) create mode 100644 browser/base/content/test/notificationbox/browser_notificationbar_telemetry.js diff --git a/browser/base/content/tabbrowser.js b/browser/base/content/tabbrowser.js index 4252c922ba2b..e4d84a5aeb31 100644 --- a/browser/base/content/tabbrowser.js +++ b/browser/base/content/tabbrowser.js @@ -1239,6 +1239,8 @@ this._tabAttrModified(oldTab, ["selected"]); this._tabAttrModified(newTab, ["selected"]); + this.readNotificationBox(newBrowser)?.shown(); + this._startMultiSelectChange(); this._multiSelectChangeSelected = true; this.clearMultiSelectedTabs(); diff --git a/browser/base/content/test/notificationbox/browser.ini b/browser/base/content/test/notificationbox/browser.ini index 7462d639bf0a..2a2c4cced17d 100644 --- a/browser/base/content/test/notificationbox/browser.ini +++ b/browser/base/content/test/notificationbox/browser.ini @@ -1,2 +1,3 @@ +[browser_notificationbar_telemetry.js] [browser_notification_stacking.js] [browser_tabnotificationbox_switch_tabs.js] diff --git a/browser/base/content/test/notificationbox/browser_notificationbar_telemetry.js b/browser/base/content/test/notificationbox/browser_notificationbar_telemetry.js new file mode 100644 index 000000000000..1a24ec7d1b5d --- /dev/null +++ b/browser/base/content/test/notificationbox/browser_notificationbar_telemetry.js @@ -0,0 +1,150 @@ +const TELEMETRY_BASE = "notificationbar."; + +ChromeUtils.defineModuleGetter( + this, + "TelemetryTestUtils", + "resource://testing-common/TelemetryTestUtils.jsm" +); + +add_task(async function showNotification() { + Services.telemetry.clearScalars(); + + let tab1 = await BrowserTestUtils.openNewForegroundTab( + gBrowser, + "https://example.com/" + ); + + ok(!gBrowser.readNotificationBox(), "no notificationbox created yet"); + + let box1 = gBrowser.getNotificationBox(); + + ok(gBrowser.readNotificationBox(), "notificationbox was created"); + + let tab2 = await BrowserTestUtils.openNewForegroundTab( + gBrowser, + "https://example.org/" + ); + + let tab3 = await BrowserTestUtils.openNewForegroundTab( + gBrowser, + "data:text/html,Hello" + ); + let box3 = gBrowser.getNotificationBox(); + + verifyTelemetry("initial", 0, 0, 0, 0, 0, 0); + + let notif3 = box3.appendNotification("infobar-testtwo-value", { + label: "Message for tab 3", + priority: box3.PRIORITY_INFO_HIGH, + telemetry: TELEMETRY_BASE + "testtwo", + }); + + verifyTelemetry("first notification", 0, 0, 0, 0, 0, 1); + + let notif1 = box1.appendNotification( + "infobar-testone-value", + { + label: "Message for tab 1", + priority: box1.PRIORITY_INFO_HIGH, + telemetry: TELEMETRY_BASE + "testone", + }, + [ + { + label: "Button1", + telemetry: "button1-pressed", + }, + { + label: "Button2", + telemetry: "button2-pressed", + }, + { + label: "Button3", + }, + ] + ); + verifyTelemetry("second notification", 0, 0, 0, 0, 0, 1); + + await BrowserTestUtils.switchTab(gBrowser, tab1); + verifyTelemetry("switch to first tab", 1, 0, 0, 0, 0, 1); + + await BrowserTestUtils.switchTab(gBrowser, tab2); + verifyTelemetry("switch to second tab", 1, 0, 0, 0, 0, 1); + + await BrowserTestUtils.switchTab(gBrowser, tab3); + verifyTelemetry("switch to third tab", 1, 0, 0, 0, 0, 1); + + await BrowserTestUtils.switchTab(gBrowser, tab1); + verifyTelemetry("switch to first tab again", 1, 0, 0, 0, 0, 1); + + notif1.buttonContainer.lastElementChild.click(); + verifyTelemetry("press third button", 1, 1, 0, 0, 0, 1); + + notif1.buttonContainer.lastElementChild.previousElementSibling.click(); + verifyTelemetry("press second button", 1, 1, 0, 1, 0, 1); + + notif1.buttonContainer.lastElementChild.previousElementSibling.previousElementSibling.click(); + verifyTelemetry("press first button", 1, 1, 1, 1, 0, 1); + + notif1.dismiss(); + verifyTelemetry("dismiss notification for box 1", 1, 1, 1, 1, 1, 1); + + notif3.dismiss(); + verifyTelemetry("dismiss notification for box 3", 1, 1, 1, 1, 1, 1, 1); + + BrowserTestUtils.removeTab(tab1); + BrowserTestUtils.removeTab(tab2); + BrowserTestUtils.removeTab(tab3); +}); + +function verify(scalars, scalar, key, expected, exists) { + scalar = TELEMETRY_BASE + scalar; + + if (expected > 0) { + TelemetryTestUtils.assertKeyedScalar(scalars, scalar, key, expected); + return; + } + + Assert.equal( + scalar in scalars, + exists, + `expected ${scalar} to be ${exists ? "present" : "unset"}` + ); + + if (exists) { + Assert.ok( + !(key in scalars[scalar]), + "expected key " + key + " to be unset" + ); + } +} + +function verifyTelemetry( + desc, + box1shown, + box1action, + box1button1, + box1button2, + box1dismissed, + box3shown, + box3dismissed = 0 +) { + let scalars = TelemetryTestUtils.getProcessScalars("parent", true, false); + + info(desc); + let n1exists = + box1shown || box1action || box1button1 || box1button2 || box1dismissed; + + verify(scalars, "testone", "shown", box1shown, n1exists); + verify(scalars, "testone", "action", box1action, n1exists); + verify(scalars, "testone", "button1-pressed", box1button1, n1exists); + verify(scalars, "testone", "button2-pressed", box1button2, n1exists); + verify(scalars, "testone", "dismissed", box1dismissed, n1exists); + verify(scalars, "testtwo", "shown", box3shown, box3shown || box3dismissed); + verify( + scalars, + "testtwo", + "dismissed", + box3dismissed, + box3shown || box3dismissed + ); +} diff --git a/browser/modules/ContentCrashHandlers.jsm b/browser/modules/ContentCrashHandlers.jsm index ca2d6dd41908..8fe62b3ba420 100644 --- a/browser/modules/ContentCrashHandlers.jsm +++ b/browser/modules/ContentCrashHandlers.jsm @@ -425,23 +425,15 @@ var TabCrashHandler = { }, ]; - // Add telemetry indicating that the subframe crash UI is shown, but wait until the tab - // is switched to. - let removeTelemetryFn = this.telemetryIfTabSelected( - browser, - "dom.contentprocess.crash_subframe_ui_presented" - ); - notification = notificationBox.appendNotification( value, { label: messageFragment, image: TABCRASHED_ICON_URI, priority: notificationBox.PRIORITY_INFO_MEDIUM, + telemetry: "notificationbar.crash_subframe_ui", eventCallback: eventName => { if (eventName == "disconnected") { - removeTelemetryFn(); - let existingItem = this.notificationsMap.get(childID); if (existingItem) { let idx = existingItem.indexOf(notification); @@ -474,40 +466,6 @@ var TabCrashHandler = { } }, - /** - * If the browser tab is selected, increase the telemetry probe. If the browser tab - * is not selected, wait until the browser tab is selected before increasing the - * telemetry probe. This means that a crash in a background tab won't trigger the - * probe until the tab is switched to. - * - * Returns a function to be called to cancel the telemetry when it no longer applies, - */ - telemetryIfTabSelected(browser, telemetryKey) { - let gBrowser = browser.getTabBrowser(); - let tab = gBrowser.getTabForBrowser(browser); - - let seenNotification = event => { - if (tab == event.target) { - tab.removeEventListener("TabSelect", seenNotification, true); - - Services.telemetry.scalarAdd(telemetryKey, 1); - } - }; - - // Add telemetry indicating that the subframe crash UI is shown, but wait until the tab - // is switched to. - if (gBrowser.selectedTab == tab) { - Services.telemetry.scalarAdd(telemetryKey, 1); - return () => {}; - } - - tab.addEventListener("TabSelect", seenNotification, true); - - return () => { - tab.removeEventListener("TabSelect", seenNotification, true); - }; - }, - /** * This method is exposed for SessionStore to call if the user selects * a tab which will restore on demand. It's possible that the tab diff --git a/dom/ipc/tests/browser_crash_oopiframe.js b/dom/ipc/tests/browser_crash_oopiframe.js index e9f0e5d8264e..e5ac6d20c315 100644 --- a/dom/ipc/tests/browser_crash_oopiframe.js +++ b/dom/ipc/tests/browser_crash_oopiframe.js @@ -6,8 +6,7 @@ ChromeUtils.defineModuleGetter( "resource://testing-common/TelemetryTestUtils.jsm" ); -const SUBFRAME_CRASH_PRESENTED_KEY = - "dom.contentprocess.crash_subframe_ui_presented"; +const SUBFRAME_CRASH_PRESENTED_KEY = "notificationbar.crash_subframe_ui"; /** * Opens a number of tabs containing an out-of-process iframe. @@ -126,30 +125,33 @@ async function testFrameCrash(numTabs) { // Next, check that the crash notification bar has appeared. await notificationPromise; - TelemetryTestUtils.assertScalar( - TelemetryTestUtils.getProcessScalars("parent"), + info("Subframe crashed ui count"); + TelemetryTestUtils.assertKeyedScalar( + TelemetryTestUtils.getProcessScalars("parent", true), SUBFRAME_CRASH_PRESENTED_KEY, - 1, - "Subframe crashed ui count" + "shown", + 1 ); if (numTabs > 1) { // Showing another tab should increase the subframe crash UI telemetry probe as the other // notification will now be visible. await BrowserTestUtils.switchTab(gBrowser, gBrowser.tabs[1]); - TelemetryTestUtils.assertScalar( - TelemetryTestUtils.getProcessScalars("parent"), + info("Subframe crashed ui count after switching tab"); + TelemetryTestUtils.assertKeyedScalar( + TelemetryTestUtils.getProcessScalars("parent", true), SUBFRAME_CRASH_PRESENTED_KEY, - 2, - "Subframe crashed ui count after switching tab" + "shown", + 2 ); await BrowserTestUtils.switchTab(gBrowser, gBrowser.tabs[2]); - TelemetryTestUtils.assertScalar( - TelemetryTestUtils.getProcessScalars("parent"), + info("Subframe crashed ui count after switching tab again"); + TelemetryTestUtils.assertKeyedScalar( + TelemetryTestUtils.getProcessScalars("parent", true), SUBFRAME_CRASH_PRESENTED_KEY, - 3, - "Subframe crashed ui count after switching tab again" + "shown", + 3 ); } @@ -203,11 +205,12 @@ async function testFrameCrash(numTabs) { BrowserTestUtils.removeTab(gBrowser.selectedTab); } - TelemetryTestUtils.assertScalar( - TelemetryTestUtils.getProcessScalars("parent"), + info("Subframe crashed ui count at end of test"); + TelemetryTestUtils.assertKeyedScalar( + TelemetryTestUtils.getProcessScalars("parent", true), SUBFRAME_CRASH_PRESENTED_KEY, - numTabs > 1 ? 3 : 1, - "Subframe crashed ui count at end of test" + "shown", + numTabs > 1 ? 3 : 1 ); } diff --git a/toolkit/components/telemetry/Scalars.yaml b/toolkit/components/telemetry/Scalars.yaml index 1150713283ce..1bd0f5f58ae8 100644 --- a/toolkit/components/telemetry/Scalars.yaml +++ b/toolkit/components/telemetry/Scalars.yaml @@ -2277,21 +2277,6 @@ dom.contentprocess: - 'firefox' record_in_processes: - 'main' - crash_subframe_ui_presented: - bug_numbers: - - 1676943 - description: > - The number of times the subframe crash UI is shown to the user. This - only triggers when a tab containing the subframe is switched to. - expires: "98" - kind: uint - notification_emails: - - nlayzell@mozilla.com - release_channel_collection: opt-out - products: - - 'firefox' - record_in_processes: - - 'main' # Note that the unsubmitted notification is currently only enabled on Nightly builds unsubmitted_ui_presented: bug_numbers: @@ -3884,6 +3869,59 @@ formautofill.creditCards: record_in_processes: - 'content' +# This section is for notification bars +notificationbar: + crash_subframe_ui: + bug_numbers: + - 1676943 + description: > + Notifications for when the subframe crash UI is shown to the user. This + only triggers when a tab containing the subframe is switched to. + expires: "98" + kind: uint + keyed: true + notification_emails: + - nlayzell@mozilla.com + release_channel_collection: opt-out + products: + - 'firefox' + record_in_processes: + - 'main' + + # Used only for testing + testone: + bug_numbers: + - 1676943 + description: > + Used to test notificationbar telemetry + expires: never + kind: uint + keyed: true + notification_emails: + - telemetry-client-dev@mozilla.com + release_channel_collection: opt-out + products: + - 'firefox' + record_in_processes: + - 'main' + + # Used only for testing + testtwo: + bug_numbers: + - 1676943 + description: > + Used to test notificationbar telemetry + expires: never + kind: uint + keyed: true + notification_emails: + - telemetry-client-dev@mozilla.com + release_channel_collection: opt-out + products: + - 'firefox' + record_in_processes: + - 'main' + # The following section contains scalars for Screenshots. screenshots: download: diff --git a/toolkit/content/widgets/notificationbox.js b/toolkit/content/widgets/notificationbox.js index fa7625f9ef02..03963741a5dc 100644 --- a/toolkit/content/widgets/notificationbox.js +++ b/toolkit/content/widgets/notificationbox.js @@ -100,6 +100,11 @@ * notificationIs * Defines a Custom Element name to use as the "is" value on creation. * This allows subclassing the created element. + * telemetry + * Specifies the telemetry key to use that triggers when the notification + * is shown, dismissed and an action taken. This telemetry is a keyed scalar with keys for: + * 'shown', 'dismissed' and 'action'. If a button specifies a separate key, + * then 'action' is replaced by values specific to each button. * aButtons * Array of objects defining action buttons: * { @@ -126,6 +131,11 @@ * popup: * If specified, the button will open the popup element with this * ID, anchored to the button. This is alternative to "callback". + * telemetry: + * Specifies the key to add for the telemetry to trigger when the + * button is pressed. If not specified, then 'action' is used for + * a press on any button. Specify this only if you want to distinguish + * which button has been pressed in telemetry data. * is: * Defines a Custom Element name to use as the "is" value on * button creation. @@ -194,6 +204,10 @@ newitem.setButtons(aButtons); } + if (aNotification.telemetry) { + newitem.telemetry = aNotification.telemetry; + } + newitem.priority = aNotification.priority; if (aNotification.priority == this.PRIORITY_SYSTEM) { newitem.setAttribute("type", "system"); @@ -218,6 +232,13 @@ event.initEvent("AlertActive", true, true); newitem.dispatchEvent(event); + // If the notification is not visible, don't call shown() on the + // new notification until it is visible. This will typically be + // a tabbrowser that does this when a tab is selected. + if (this.isShown) { + newitem.shown(); + } + return newitem; } @@ -284,6 +305,22 @@ } } + shown() { + for (let notification of this.allNotifications) { + notification.shown(); + } + } + + get isShown() { + let stack = this.stack; + let parent = this.stack.parentNode; + if (parent.localName == "named-deck") { + return parent.selectedViewName == stack.getAttribute("name"); + } + + return true; + } + _showNotification(aNotification, aSlideIn, aSkipAnimation) { this._finishAnimation(); @@ -375,6 +412,8 @@ this.persistence = 0; this.priority = 0; this.timeout = 0; + this.telemetry = []; + this._shown = false; } connectedCallback() { @@ -468,6 +507,8 @@ * should call close() instead. */ dismiss() { + this._doTelemetry("dismissed"); + if (this.eventCallback) { this.eventCallback("dismissed"); } @@ -481,12 +522,29 @@ this.control.removeNotification(this); } + // This will be called when the host (such as a tabbrowser) determines that + // the notification is made visible to the user. + shown() { + if (!this._shown) { + this._shown = true; + this._doTelemetry("shown"); + } + } + + _doTelemetry(type) { + if (this.telemetry) { + Services.telemetry.keyedScalarAdd(this.telemetry, type, 1); + } + } + _doButtonCommand(event) { if (!("buttonInfo" in event.target)) { return; } var button = event.target.buttonInfo; + this._doTelemetry(button.telemetry || "action"); + if (button.popup) { document .getElementById(button.popup) @@ -525,6 +583,8 @@ this.persistence = 0; this.priority = 0; this.timeout = 0; + this.telemetry = []; + this._shown = false; } connectedCallback() { @@ -560,6 +620,12 @@ } } + _doTelemetry(type) { + if (this.telemetry) { + Services.telemetry.keyedScalarAdd(this.telemetry, type, 1); + } + } + get control() { return this.closest(".notificationbox-stack")._notificationBox; } @@ -571,6 +637,15 @@ this.control.removeNotification(this); } + // This will be called when the host (such as a tabbrowser) determines that + // the notification is made visible to the user. + shown() { + if (!this._shown) { + this._shown = true; + this._doTelemetry("shown"); + } + } + setAlertRole() { // Wait a little for this to render before setting the role for more // consistent alerts to screen readers. @@ -590,6 +665,9 @@ if ("buttonInfo" in e.target) { let { buttonInfo } = e.target; let { callback, popup } = buttonInfo; + + this._doTelemetry(buttonInfo.telemetry || "action"); + if (popup) { document .getElementById(popup) @@ -669,6 +747,8 @@ } dismiss() { + this._doTelemetry("dismissed"); + if (this.eventCallback) { this.eventCallback("dismissed"); }