Bug 1728057 - fix captive portal telemetry, r=nhnt11 a=RyanVM
authorDan Mosedale <dmose@mozilla.org>
Mon, 30 Aug 2021 13:49:25 +0000
changeset 661148 1416bfa15a5b247149d8c0dcc214deaa2eefc650
parent 661147 280aaaa2dc4259c83f9ac5a9c8ad8ab3095b1ac1
child 661149 16fa80893f36c520a2afbbc2784142baddc71640
push id2647
push userryanvm@gmail.com
push dateWed, 01 Sep 2021 14:21:58 +0000
treeherdermozilla-release@3049159544ae [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersnhnt11, RyanVM
bugs1728057
milestone92.0
Bug 1728057 - fix captive portal telemetry, r=nhnt11 a=RyanVM Differential Revision: https://phabricator.services.mozilla.com/D123911
browser/base/content/browser-captivePortal.js
toolkit/components/captivedetect/CaptiveDetect.jsm
toolkit/components/captivedetect/test/unit/head_setprefs.js
toolkit/components/captivedetect/test/unit/test_captive_portal_found.js
toolkit/components/captivedetect/test/unit/test_multiple_requests.js
--- a/browser/base/content/browser-captivePortal.js
+++ b/browser/base/content/browser-captivePortal.js
@@ -205,21 +205,16 @@ var CaptivePortalWatcher = {
     // so they can log in before continuing to browse.
     if (win != Services.focus.activeWindow) {
       this._delayedCaptivePortalDetectedInProgress = true;
       window.addEventListener("activate", this, { once: true });
       Services.obs.addObserver(this, "delayed-captive-portal-handled");
     }
 
     this._showNotification();
-    Services.telemetry.recordEvent(
-      "networking.captive_portal",
-      "login_infobar_shown",
-      "infobar"
-    );
   },
 
   /**
    * Called after we regain focus if we detect a portal while a browser window
    * doesn't have focus. Triggers a portal recheck to reaffirm state, and adds
    * the tab if needed after a short delay to allow the recheck to complete.
    */
   _delayedCaptivePortalDetected() {
@@ -274,24 +269,16 @@ var CaptivePortalWatcher = {
       tab &&
       tab.linkedBrowser &&
       tab.linkedBrowser.currentURI.equalsExceptRef(canonicalURI)
     ) {
       this._previousCaptivePortalTab = null;
       gBrowser.removeTab(tab);
     }
     this._captivePortalTab = null;
-
-    if (aSuccess) {
-      Services.telemetry.recordEvent(
-        "networking.captive_portal",
-        "login_successful",
-        "detector"
-      );
-    }
   },
 
   _cancelDelayedCaptivePortal() {
     if (this._delayedCaptivePortalDetectedInProgress) {
       this._delayedCaptivePortalDetectedInProgress = false;
       Services.obs.removeObserver(this, "delayed-captive-portal-handled");
       window.removeEventListener("activate", this);
     }
@@ -371,16 +358,22 @@ var CaptivePortalWatcher = {
       message,
       this.PORTAL_NOTIFICATION_VALUE,
       "",
       gNotificationBox.PRIORITY_INFO_MEDIUM,
       buttons,
       closeHandler
     );
 
+    Services.telemetry.recordEvent(
+      "networking.captive_portal",
+      "login_infobar_shown",
+      "infobar"
+    );
+
     gBrowser.tabContainer.addEventListener("TabSelect", this);
   },
 
   _removeNotification() {
     let n = this._captivePortalNotification;
     if (!n || !n.parentNode) {
       return;
     }
--- a/toolkit/components/captivedetect/CaptiveDetect.jsm
+++ b/toolkit/components/captivedetect/CaptiveDetect.jsm
@@ -237,16 +237,17 @@ function LoginObserver(captivePortalDete
 
   return observer;
 }
 
 function CaptivePortalDetector() {
   // Load preference
   this._canonicalSiteURL = null;
   this._canonicalSiteExpectedContent = null;
+  this._telemetryService = Services.telemetry;
 
   try {
     this._canonicalSiteURL = Services.prefs.getCharPref(
       "captivedetect.canonicalURL"
     );
     this._canonicalSiteExpectedContent = Services.prefs.getCharPref(
       "captivedetect.canonicalContent"
     );
@@ -276,17 +277,17 @@ function CaptivePortalDetector() {
   // Create HttpObserver for monitoring the login procedure
   this._loginObserver = LoginObserver(this);
 
   this._nextRequestId = 0;
   this._runningRequest = null;
   this._requestQueue = []; // Maintain a progress table, store callbacks and the ongoing XHR
   this._interfaceNames = {}; // Maintain names of the requested network interfaces
 
-  Services.telemetry.setEventRecordingEnabled(
+  this._telemetryService.setEventRecordingEnabled(
     "networking.captive_portal",
     true
   );
 
   debug(
     "CaptiveProtalDetector initiated, waiting for network connection established"
   );
 }
@@ -437,16 +438,22 @@ CaptivePortalDetector.prototype = {
       debug("callback executed");
       if (this._runningRequest.hasOwnProperty("callback")) {
         this._runningRequest.callback.complete(success);
       }
 
       // Only when the request has a event id and |success| is true
       // do we need to notify the login-success event.
       if (this._runningRequest.hasOwnProperty("eventId") && success) {
+        this._telemetryService.recordEvent(
+          "networking.captive_portal",
+          "login_successful",
+          "detector"
+        );
+
         let details = {
           type: kCaptivePortalLoginSuccessEvent,
           id: this._runningRequest.eventId,
         };
         this._sendEvent(kCaptivePortalLoginSuccessEvent, details);
       }
 
       // Continue the following request
--- a/toolkit/components/captivedetect/test/unit/head_setprefs.js
+++ b/toolkit/components/captivedetect/test/unit/head_setprefs.js
@@ -2,16 +2,17 @@
 /* This Source Code Form is subject to the terms of the Mozilla Public
  * License, v. 2.0. If a copy of the MPL was not distributed with this file,
  * You can obtain one at http://mozilla.org/MPL/2.0/. */
 "use strict";
 
 var { XPCOMUtils } = ChromeUtils.import(
   "resource://gre/modules/XPCOMUtils.jsm"
 );
+const { sinon } = ChromeUtils.import("resource://testing-common/Sinon.jsm");
 var { Services } = ChromeUtils.import("resource://gre/modules/Services.jsm");
 var {
   HTTP_400,
   HTTP_401,
   HTTP_402,
   HTTP_403,
   HTTP_404,
   HTTP_405,
@@ -31,22 +32,19 @@ var {
   HTTP_502,
   HTTP_503,
   HTTP_504,
   HTTP_505,
   HttpError,
   HttpServer,
 } = ChromeUtils.import("resource://testing-common/httpd.js");
 
-XPCOMUtils.defineLazyServiceGetter(
-  this,
-  "gCaptivePortalDetector",
-  "@mozilla.org/toolkit/captive-detector;1",
-  "nsICaptivePortalDetector"
-);
+const fakeTelemetryService = {
+  recordEvent: sinon.spy(),
+};
 
 const kCanonicalSitePath = "/canonicalSite.html";
 const kCanonicalSiteContent = "true";
 const kPrefsCanonicalURL = "captivedetect.canonicalURL";
 const kPrefsCanonicalContent = "captivedetect.canonicalContent";
 const kPrefsMaxWaitingTime = "captivedetect.maxWaitingTime";
 const kPrefsPollingTime = "captivedetect.pollingTime";
 
@@ -58,20 +56,29 @@ function setupPrefs() {
     kPrefsCanonicalURL,
     gServerURL + kCanonicalSitePath
   );
   Services.prefs.setCharPref(kPrefsCanonicalContent, kCanonicalSiteContent);
   Services.prefs.setIntPref(kPrefsMaxWaitingTime, 0);
   Services.prefs.setIntPref(kPrefsPollingTime, 1);
 }
 
+let gCaptivePortalDetector;
 function run_captivedetect_test(xhr_handler, fakeUIResponse, testfun) {
   gServer = new HttpServer();
   gServer.registerPathHandler(kCanonicalSitePath, xhr_handler);
   gServer.start(-1);
   gServerURL = "http://localhost:" + gServer.identity.primaryPort;
 
   setupPrefs();
 
+  // Instead of getting the XPCOM service, we need the real JS object, so that
+  // we can give it a stubbed out telemetry service.
+  const { CaptivePortalDetector } = ChromeUtils.import(
+    "resource:///modules/CaptiveDetect.jsm"
+  );
+  gCaptivePortalDetector = new CaptivePortalDetector();
+  gCaptivePortalDetector._telemetryService = fakeTelemetryService;
+
   fakeUIResponse();
 
   testfun();
 }
--- a/toolkit/components/captivedetect/test/unit/test_captive_portal_found.js
+++ b/toolkit/components/captivedetect/test/unit/test_captive_portal_found.js
@@ -31,16 +31,17 @@ function fakeUIResponse() {
       Assert.equal(++step, 2);
     }
   }, "captive-portal-login");
 
   Services.obs.addObserver(function observe(subject, topic, data) {
     if (topic === "captive-portal-login-success") {
       Assert.equal(++step, 4);
       gServer.stop(do_test_finished);
+      Assert.ok(fakeTelemetryService.recordEvent.calledOnce);
     }
   }, "captive-portal-login-success");
 }
 
 function test_portal_found() {
   do_test_pending();
 
   let callback = {
--- a/toolkit/components/captivedetect/test/unit/test_multiple_requests.js
+++ b/toolkit/components/captivedetect/test/unit/test_multiple_requests.js
@@ -38,16 +38,17 @@ function fakeUIResponse() {
     if (topic === "captive-portal-login-success") {
       loginSuccessCount++;
       if (loginSuccessCount > 1) {
         throw new Error(
           "We should only receive 'captive-portal-login-success' once"
         );
       }
       Assert.equal(++step, 4);
+      Assert.ok(fakeTelemetryService.recordEvent.calledOnce);
     }
   }, "captive-portal-login-success");
 }
 
 function test_multiple_requests() {
   do_test_pending();
 
   let callback = {