Bug 1560590 - Add Telemetry for Picture-in-Picture opening and closin…

…g mechanisms. data-review=chutten,r=JSON_voorhees Depends on D36358 Differential Revision: https://phabricator.services.mozilla.com/D36359 --HG-- extra : moz-landing-system : lando
dothq · Jul 2, 2019 · d38b355 · d38b355
1 parent 152ad9f
commit d38b355
Show file tree

Hide file tree

Showing 5 changed files with 71 additions and 11 deletions.
diff --git a/browser/actors/ContextMenuChild.jsm b/browser/actors/ContextMenuChild.jsm
@@ -125,6 +125,7 @@ class ContextMenuChild extends JSWindowActorChild {
                 }
                 break;
               case "pictureinpicture":
+                Services.telemetry.keyedScalarAdd("pictureinpicture.opened_method", "contextmenu", 1);
                 let event = new this.contentWindow.CustomEvent("MozTogglePictureInPicture", {
                   bubbles: true,
                 }, this.contentWindow);

diff --git a/toolkit/actors/PictureInPictureChild.jsm b/toolkit/actors/PictureInPictureChild.jsm
@@ -340,6 +340,9 @@ class PictureInPictureToggleChild extends ActorChild {
       state.isClickingToggle = true;
       state.clickedElement = Cu.getWeakReference(event.originalTarget);
       event.stopImmediatePropagation();
+
+      Services.telemetry.keyedScalarAdd("pictureinpicture.opened_method", "toggle", 1);
+
       let pipEvent =
         new this.content.CustomEvent("MozTogglePictureInPicture", {
           bubbles: true,
@@ -558,7 +561,7 @@ class PictureInPictureChild extends ActorChild {
       case "pagehide": {
         // The originating video's content document has unloaded,
         // so close Picture-in-Picture.
-        this.closePictureInPicture();
+        this.closePictureInPicture({ reason: "pagehide" });
         break;
       }
       case "play": {
@@ -601,13 +604,17 @@ class PictureInPictureChild extends ActorChild {
    */
   async togglePictureInPicture(video) {
     if (this.inPictureInPicture(video)) {
-      await this.closePictureInPicture();
+      // The only way we could have entered here for the same video is if
+      // we are toggling via the context menu, since we hide the inline
+      // Picture-in-Picture toggle when a video is being displayed in
+      // Picture-in-Picture.
+      await this.closePictureInPicture({ reason: "contextmenu" });
     } else {
       if (this.weakVideo) {
         // There's a pre-existing Picture-in-Picture window for a video
         // in this content process. Send a message to the parent to close
         // the Picture-in-Picture window.
-        await this.closePictureInPicture();
+        await this.closePictureInPicture({ reason: "new-pip" });
       }
 
       gWeakVideo = Cu.getWeakReference(video);
@@ -640,13 +647,14 @@ class PictureInPictureChild extends ActorChild {
    * @resolves {undefined} Once the pre-existing Picture-in-Picture
    * window has unloaded.
    */
-  async closePictureInPicture() {
+  async closePictureInPicture({ reason }) {
     if (this.weakVideo) {
       this.untrackOriginatingVideo(this.weakVideo);
     }
 
     this.mm.sendAsyncMessage("PictureInPicture:Close", {
       browingContextId: this.docShell.browsingContext.id,
+      reason,
     });
 
     if (this.weakPlayerContent) {
@@ -728,7 +736,7 @@ class PictureInPictureChild extends ActorChild {
       // If the video element has gone away before we've had a chance to set up
       // Picture-in-Picture for it, tell the parent to close the Picture-in-Picture
       // window.
-      await this.closePictureInPicture();
+      await this.closePictureInPicture({ reason: "setup-failure" });
       return;
     }
 

diff --git a/toolkit/components/pictureinpicture/PictureInPicture.jsm b/toolkit/components/pictureinpicture/PictureInPicture.jsm
@@ -12,6 +12,14 @@ const PLAYER_URI = "chrome://global/content/pictureinpicture/player.xhtml";
 const PLAYER_FEATURES = `chrome,titlebar=no,alwaysontop,lockaspectratio,resizable`;
 const WINDOW_TYPE = "Toolkit:PictureInPicture";
 
+/**
+ * If closing the Picture-in-Picture player window occurred for a reason that
+ * we can easily detect (user clicked on the close button, originating tab unloaded,
+ * user clicked on the unpip button), that will be stashed in gCloseReasons so that
+ * we can note it in Telemetry when the window finally unloads.
+ */
+let gCloseReasons = new WeakMap();
+
 /**
  * This module is responsible for creating a Picture in Picture window to host
  * a clone of a video element running in web content.
@@ -32,7 +40,8 @@ var PictureInPicture = {
         /**
          * Content has requested that its Picture in Picture window go away.
          */
-        this.closePipWindow();
+        let reason = aMessage.data.reason;
+        this.closePipWindow({ reason });
         break;
       }
       case "PictureInPicture:Playing": {
@@ -56,7 +65,7 @@ var PictureInPicture = {
     let gBrowser = this.browser.ownerGlobal.gBrowser;
     let tab = gBrowser.getTabForBrowser(this.browser);
     gBrowser.selectedTab = tab;
-    await this.closePipWindow();
+    await this.closePipWindow({ reason: "unpip" });
   },
 
   /**
@@ -73,7 +82,7 @@ var PictureInPicture = {
   /**
    * Find and close any pre-existing Picture in Picture windows.
    */
-  async closePipWindow() {
+  async closePipWindow({ reason }) {
     // This uses an enumerator, but there really should only be one of
     // these things.
     for (let win of Services.wm.getEnumerator(WINDOW_TYPE)) {
@@ -83,6 +92,7 @@ var PictureInPicture = {
       let closedPromise = new Promise(resolve => {
         win.addEventListener("unload", resolve, {once: true});
       });
+      gCloseReasons.set(win, reason);
       win.close();
       await closedPromise;
     }
@@ -110,7 +120,7 @@ var PictureInPicture = {
    */
   async handlePictureInPictureRequest(browser, videoData) {
     // If there's a pre-existing PiP window, close it first.
-    await this.closePipWindow();
+    await this.closePipWindow({ reason: "new-pip" });
 
     let parentWin = browser.ownerGlobal;
     this.browser = browser;
@@ -132,6 +142,10 @@ var PictureInPicture = {
    */
   unload(window) {
     TelemetryStopwatch.finish("FX_PICTURE_IN_PICTURE_WINDOW_OPEN_DURATION", window);
+
+    let reason = gCloseReasons.get(window) || "other";
+    Services.telemetry.keyedScalarAdd("pictureinpicture.closed_method", reason, 1);
+
     this.clearPipTabIcon();
     delete this.weakPipControls;
     delete this.browser;

diff --git a/toolkit/components/pictureinpicture/content/player.js b/toolkit/components/pictureinpicture/content/player.js
@@ -38,7 +38,7 @@ async function setupPlayer(originatingBrowser, videoData) {
   // If the content process hosting the video crashes, let's
   // just close the window for now.
   browser.addEventListener("oop-browser-crashed", () => {
-    window.close();
+    PictureInPicture.closePipWindow({ reason: "browser-crash" });
   });
 
   window.addEventListener("unload", () => {
@@ -47,7 +47,7 @@ async function setupPlayer(originatingBrowser, videoData) {
 
   let close = document.getElementById("close");
   close.addEventListener("click", () => {
-    window.close();
+    PictureInPicture.closePipWindow({ reason: "close-button" });
   });
 
   document.getElementById("controls").setAttribute("showing", true);

diff --git a/toolkit/components/telemetry/Scalars.yaml b/toolkit/components/telemetry/Scalars.yaml
@@ -584,6 +584,43 @@ sandbox:
     operating_systems:
       - "windows"
 
+pictureinpicture:
+  opened_method:
+    bug_numbers:
+      - 1560590
+    description: >
+      The number of times a Picture-in-Picture window was opened, per trigger
+      mechanism (e.g.: the video toggle, the context menu).
+    expires: "74"
+    kind: uint
+    keyed: true
+    notification_emails:
+      - [email protected]
+      - [email protected]
+    release_channel_collection: opt-in
+    record_in_processes:
+      - content
+    operating_systems:
+      - "windows"
+  closed_method:
+    bug_numbers:
+      - 1560590
+    description: >
+      The number of times a Picture-in-Picture window was closed, per trigger
+      mechanism (e.g.: the close button, the unpip button, originating tab going away,
+      process shutdown, etc).
+    expires: "74"
+    kind: uint
+    keyed: true
+    notification_emails:
+      - [email protected]
+      - [email protected]
+    release_channel_collection: opt-in
+    record_in_processes:
+      - main
+    operating_systems:
+      - "windows"
+
 preferences:
   created_new_user_prefs_file:
     bug_numbers: