Bug 1536454 - Part 3 - Make POPUP_NOTIFICATION_STATS probe collect data on notification removal instead of dismissal. r=MattN
authorJohann Hofmann <jhofmann@mozilla.com>
Thu, 18 Apr 2019 13:43:18 +0000
changeset 470079 8c6ec5c528ff0d21865a3d22c743dc21b6061614
parent 470078 c4beb7256d16cf3f18739fd20dad8411f4a99092
child 470080 19b40bd2e67f28b2c8b11dc0671cd7337b408911
push id112843
push useraiakab@mozilla.com
push dateFri, 19 Apr 2019 09:50:22 +0000
treeherdermozilla-inbound@c06f27cbfe40 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersMattN
bugs1536454
milestone68.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1536454 - Part 3 - Make POPUP_NOTIFICATION_STATS probe collect data on notification removal instead of dismissal. r=MattN Almost none of the prompts that we are currently showing still get dismissed, which messes up our measurements in this probe. Most of them are persistent now, which means that we should record when they get removed instead of dismissed to receive meaningful data. This patch does that. Differential Revision: https://phabricator.services.mozilla.com/D26944
toolkit/components/telemetry/Histograms.json
toolkit/modules/PopupNotifications.jsm
--- a/toolkit/components/telemetry/Histograms.json
+++ b/toolkit/components/telemetry/Histograms.json
@@ -8474,23 +8474,23 @@
     "n_values": 8,
     "description": "User actions taken in the plugin notification: 0: allownow 1: allowalways 2: block 3: blockalways",
     "bug_numbers": [902075, 1345894, 1398972],
     "alert_emails": ["flashvideo-2015@mozilla.com"]
   },
   "POPUP_NOTIFICATION_STATS": {
     "record_in_processes": ["main"],
     "releaseChannelCollection": "opt-out",
-    "alert_emails": ["fxprivacyandsecurity@mozilla.com", "MattN+telemetry@mozilla.com"],
-    "bug_numbers": [1207089],
+    "alert_emails": ["fxprivacyandsecurity@mozilla.com", "MattN+telemetry@mozilla.com", "jhofmann@mozilla.com"],
+    "bug_numbers": [1207089, 1536454],
     "expires_in_version": "70",
     "kind": "enumerated",
     "keyed": true,
     "n_values": 40,
-    "description": "(Bug 1207089) Usage of popup notifications, keyed by ID (0 = Offered, 1..4 = Action, 5 = Click outside, 6 = Leave page, 7 = Use 'X', 8 = Not now, 10 = Open submenu, 11 = Learn more. Add 20 if happened after reopen.)"
+    "description": "(Bug 1207089) Usage of popup notifications, keyed by ID (0 = Offered, 1..4 = Action (3 is unused), 5 = Click outside (unused), 6 = Leave page, 7 = Use 'X' (unused), 8 = Not now (unused), 10 = Open submenu, 11 = Learn more. Add 20 if happened after reopen.)"
   },
   "POPUP_NOTIFICATION_MAIN_ACTION_MS": {
     "record_in_processes": ["main"],
     "alert_emails": ["fxprivacyandsecurity@mozilla.com", "MattN+telemetry@mozilla.com"],
     "bug_numbers": [1207089],
     "expires_in_version": "70",
     "kind": "exponential",
     "keyed": true,
--- a/toolkit/modules/PopupNotifications.jsm
+++ b/toolkit/modules/PopupNotifications.jsm
@@ -21,19 +21,19 @@ const ICON_ANCHOR_ATTRIBUTE = "popupnoti
 const PREF_SECURITY_DELAY = "security.notification_enable_delay";
 
 // Enumerated values for the POPUP_NOTIFICATION_STATS telemetry histogram.
 const TELEMETRY_STAT_OFFERED = 0;
 const TELEMETRY_STAT_ACTION_1 = 1;
 const TELEMETRY_STAT_ACTION_2 = 2;
 // const TELEMETRY_STAT_ACTION_3 = 3;
 const TELEMETRY_STAT_ACTION_LAST = 4;
-const TELEMETRY_STAT_DISMISSAL_CLICK_ELSEWHERE = 5;
-const TELEMETRY_STAT_DISMISSAL_LEAVE_PAGE = 6;
-const TELEMETRY_STAT_DISMISSAL_CLOSE_BUTTON = 7;
+// const TELEMETRY_STAT_DISMISSAL_CLICK_ELSEWHERE = 5;
+const TELEMETRY_STAT_REMOVAL_LEAVE_PAGE = 6;
+// const TELEMETRY_STAT_DISMISSAL_CLOSE_BUTTON = 7;
 const TELEMETRY_STAT_OPEN_SUBMENU = 10;
 const TELEMETRY_STAT_LEARN_MORE = 11;
 
 const TELEMETRY_STAT_REOPENED_OFFSET = 20;
 
 var popupNotificationsMap = new WeakMap();
 var gNotificationParents = new WeakMap;
 
@@ -265,18 +265,31 @@ function PopupNotifications(tabbrowser, 
   this.window.addEventListener("MozDOMFullscreen:Entered", () => {
     this.panel.setAttribute("followanchor", "false");
   }, true);
   this.window.addEventListener("MozDOMFullscreen:Exited", () => {
     this.panel.setAttribute("followanchor", !locationBarHidden);
   }, true);
 
   this.window.addEventListener("activate", this, true);
-  if (this.tabbrowser.tabContainer)
+  if (this.tabbrowser.tabContainer) {
     this.tabbrowser.tabContainer.addEventListener("TabSelect", this, true);
+
+    this.tabbrowser.tabContainer.addEventListener("TabClose", aEvent => {
+      // If the tab was just closed and we have notifications associated with it,
+      // then the notifications were closed because of the tab removal. We need to
+      // record this event in telemetry and fire the removal callback.
+      this.nextRemovalReason = TELEMETRY_STAT_REMOVAL_LEAVE_PAGE;
+      let notifications = this._getNotificationsForBrowser(aEvent.target.linkedBrowser);
+      for (let notification of notifications) {
+        this._fireCallback(notification, NOTIFICATION_EVENT_REMOVED, this.nextRemovalReason);
+        notification._recordTelemetryStat(this.nextRemovalReason);
+      }
+    });
+  }
 }
 
 PopupNotifications.prototype = {
 
   window: null,
   panel: null,
   tabbrowser: null,
 
@@ -551,16 +564,18 @@ PopupNotifications.prototype = {
    * so that we can update the active notifications accordingly.
    */
   locationChange: function PopupNotifications_locationChange(aBrowser) {
     if (!aBrowser)
       throw new Error("PopupNotifications_locationChange: invalid browser");
 
     let notifications = this._getNotificationsForBrowser(aBrowser);
 
+    this.nextRemovalReason = TELEMETRY_STAT_REMOVAL_LEAVE_PAGE;
+
     notifications = notifications.filter(function(notification) {
       // The persistWhileVisible option allows an open notification to persist
       // across location changes
       if (notification.options.persistWhileVisible &&
           this.isPanelOpen) {
         if ("persistence" in notification.options &&
             notification.options.persistence)
           notification.options.persistence--;
@@ -576,17 +591,18 @@ PopupNotifications.prototype = {
       }
 
       // The timeout option allows a notification to persist until a certain time
       if ("timeout" in notification.options &&
           Date.now() <= notification.options.timeout) {
         return true;
       }
 
-      this._fireCallback(notification, NOTIFICATION_EVENT_REMOVED);
+      notification._recordTelemetryStat(this.nextRemovalReason);
+      this._fireCallback(notification, NOTIFICATION_EVENT_REMOVED, this.nextRemovalReason);
       return false;
     }, this);
 
     this._setNotificationsForBrowser(aBrowser, notifications);
 
     if (this._isActiveBrowser(aBrowser)) {
       this.anchorVisibilityChange();
     }
@@ -642,22 +658,16 @@ PopupNotifications.prototype = {
         if (this.isPanelOpen) {
           for (let elt of this.panel.children)
             elt.notification.timeShown = this.window.performance.now();
           break;
         }
 
       case "TabSelect":
         let self = this;
-        // This is where we could detect if the panel is dismissed if the page
-        // was switched. Unfortunately, the user usually has clicked elsewhere
-        // at this point so this value only gets recorded for programmatic
-        // reasons, like the "Learn More" link being clicked and resulting in a
-        // tab switch.
-        this.nextDismissReason = TELEMETRY_STAT_DISMISSAL_LEAVE_PAGE;
         // setTimeout(..., 0) needed, otherwise openPopup from "activate" event
         // handler results in the popup being hidden again for some reason...
         this.window.setTimeout(function() {
           self._update();
         }, 0);
         break;
       case "click":
       case "keypress":
@@ -689,30 +699,30 @@ PopupNotifications.prototype = {
     if (index == -1)
       return;
 
     if (this._isActiveBrowser(notification.browser))
       notification.anchorElement.removeAttribute(ICON_ATTRIBUTE_SHOWING);
 
     // remove the notification
     notifications.splice(index, 1);
-    this._fireCallback(notification, NOTIFICATION_EVENT_REMOVED);
+    this._fireCallback(notification, NOTIFICATION_EVENT_REMOVED, this.nextRemovalReason);
   },
 
   /**
    * Dismisses the notification without removing it.
+   *
+   * @param {Event} the event associated with the user interaction that
+   *                caused the dismissal
+   * @param {boolean} whether to disable persistent status. Normally,
+   *                  persistent prompts can not be dismissed. You can
+   *                  use this argument to force dismissal.
    */
-  _dismiss: function PopupNotifications_dismiss(event, telemetryReason) {
-    if (telemetryReason) {
-      this.nextDismissReason = telemetryReason;
-    }
-
-    // An explicitly dismissed persistent notification effectively becomes
-    // non-persistent.
-    if (event && telemetryReason == TELEMETRY_STAT_DISMISSAL_CLOSE_BUTTON) {
+  _dismiss: function PopupNotifications_dismiss(event, disablePersistent = false) {
+    if (disablePersistent) {
       let notificationEl = getNotificationFromElement(event.target);
       if (notificationEl) {
         notificationEl.notification.options.persistent = false;
       }
     }
 
     let browser = this.panel.firstElementChild &&
                   this.panel.firstElementChild.notification.browser;
@@ -821,17 +831,17 @@ PopupNotifications.prototype = {
           ("secondEnd" in desc)) {
         popupnotification.setAttribute("secondname", desc.secondName);
         popupnotification.setAttribute("secondendlabel", desc.secondEnd);
       }
 
       popupnotification.setAttribute("id", popupnotificationID);
       popupnotification.setAttribute("popupid", n.id);
       popupnotification.setAttribute("oncommand", "PopupNotifications._onCommand(event);");
-      popupnotification.setAttribute("closebuttoncommand", `PopupNotifications._dismiss(event, ${TELEMETRY_STAT_DISMISSAL_CLOSE_BUTTON});`);
+      popupnotification.setAttribute("closebuttoncommand", `PopupNotifications._dismiss(event, true);`);
       if (n.mainAction) {
         popupnotification.setAttribute("buttonlabel", n.mainAction.label);
         popupnotification.setAttribute("buttonaccesskey", n.mainAction.accessKey);
         popupnotification.toggleAttribute("buttonhighlight", !n.mainAction.disableHighlight);
         popupnotification.setAttribute("buttoncommand", "PopupNotifications._onButtonEvent(event, 'buttoncommand');");
         popupnotification.setAttribute("dropmarkerpopupshown", "PopupNotifications._onButtonEvent(event, 'dropmarkerpopupshown');");
         popupnotification.setAttribute("learnmoreclick", "PopupNotifications._onButtonEvent(event, 'learnmoreclick');");
         popupnotification.setAttribute("menucommand", "PopupNotifications._onMenuCommand(event);");
@@ -1047,20 +1057,16 @@ PopupNotifications.prototype = {
         // Record that the notification was actually displayed on screen.
         // Notifications that were opened a second time or that were originally
         // shown with "options.dismissed" will be recorded in a separate bucket.
         n._recordTelemetryStat(TELEMETRY_STAT_OFFERED);
         // Remember the time the notification was shown for the security delay.
         n.timeShown = this.window.performance.now();
       }, this);
 
-      // Unless the panel closing is triggered by a specific known code path,
-      // the next reason will be that the user clicked elsewhere.
-      this.nextDismissReason = TELEMETRY_STAT_DISMISSAL_CLICK_ELSEWHERE;
-
       let target = this.panel;
       if (target.parentNode) {
         // NOTIFICATION_EVENT_SHOWN should be fired for the panel before
         // anyone listening for popupshown on the panel gets run. Otherwise,
         // the panel will not be initialized when the popupshown event
         // listeners run.
         // By targeting the panel's parent and using a capturing listener, we
         // can have our listener called before others waiting for the panel to
@@ -1365,27 +1371,27 @@ PopupNotifications.prototype = {
     }
 
     otherNotifications = otherNotifications.filter(n => {
       if (this._fireCallback(n, NOTIFICATION_EVENT_SWAPPING, ourBrowser)) {
         n.browser = ourBrowser;
         n.owner = this;
         return true;
       }
-      other._fireCallback(n, NOTIFICATION_EVENT_REMOVED);
+      other._fireCallback(n, NOTIFICATION_EVENT_REMOVED, this.nextRemovalReason);
       return false;
     });
 
     ourNotifications = ourNotifications.filter(n => {
       if (this._fireCallback(n, NOTIFICATION_EVENT_SWAPPING, otherBrowser)) {
         n.browser = otherBrowser;
         n.owner = other;
         return true;
       }
-      this._fireCallback(n, NOTIFICATION_EVENT_REMOVED);
+      this._fireCallback(n, NOTIFICATION_EVENT_REMOVED, this.nextRemovalReason);
       return false;
     });
 
     this._setNotificationsForBrowser(otherBrowser, ourNotifications);
     other._setNotificationsForBrowser(ourBrowser, otherNotifications);
 
     if (otherNotifications.length > 0)
       this._update(otherNotifications);
@@ -1445,21 +1451,21 @@ PopupNotifications.prototype = {
       // Record the time of the first notification dismissal if the main action
       // was not triggered in the meantime.
       let timeSinceShown = this.window.performance.now() - notificationObj.timeShown;
       if (!notificationObj.wasDismissed &&
           !notificationObj.recordedTelemetryMainAction) {
         notificationObj._recordTelemetry("POPUP_NOTIFICATION_DISMISSAL_MS",
                                          timeSinceShown);
       }
-      notificationObj._recordTelemetryStat(this.nextDismissReason);
 
       // Do not mark the notification as dismissed or fire NOTIFICATION_EVENT_DISMISSED
       // if the notification is removed.
       if (notificationObj.options.removeOnDismissal) {
+        notificationObj._recordTelemetryStat(this.nextRemovalReason);
         this._remove(notificationObj);
       } else {
         notificationObj.dismissed = true;
         this._fireCallback(notificationObj, NOTIFICATION_EVENT_DISMISSED);
       }
     }
   },