Bug 1575038 - Quantumbar: Record engagement event telemetry when openViewOnFocus is triggered without mouse clicks. r=dao, a=RyanVM DEVEDITION_69_0b16_BUILD1 DEVEDITION_69_0b16_RELEASE FIREFOX_69_0b16_BUILD1 FIREFOX_69_0b16_RELEASE
authorDrew Willcoxon <adw@mozilla.com>
Thu, 22 Aug 2019 14:50:42 +0000
changeset 545254 91c666707c219c5215117da97cbbec3bf07f42e1
parent 545253 2a4622ac0b5ed2447782ce7f1a8260b16e55d6ed
child 545255 32798d0cb7ab0dc7b2f575b5b4733b49f56f5874
push id2131
push userffxbld-merge
push dateMon, 26 Aug 2019 18:30:20 +0000
treeherdermozilla-release@b19ffb3ca153 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersdao, RyanVM
bugs1575038
milestone69.0
Bug 1575038 - Quantumbar: Record engagement event telemetry when openViewOnFocus is triggered without mouse clicks. r=dao, a=RyanVM We need to start engagement event recording when the view opens due to `openViewOnFocus`. We already do for mouse clicks since we call `engagementEvent.start` from `_on_mousedown`. But we don't for the Ctrl/Command-L key shortcut. The shortcut command calls `openLocation` in browser.js, which calls `gURLBar.startQuery` but not `engagementEvent.start`. Every time we call `engagementEvent.start`, we do it before calling `input.startQuery`. The one exception is in `input._on_drop` because there we just handle the dropped value directly instead of starting a new query with it. The inverse is also mostly true, i.e., every time we call `input.startQuery`, we also call `engagementEvent.start`. The three exceptions are: in UITour (where it looks like we should be calling `urlbar.search` instead), in `UrlbarInput` after picking a keyword offer result, and in `openLocation` in browser.js (mentioned above). So really the only valid place is after picking a keyword entry. So, it makes sense to move `engagementEvent.start()` into `input.startQuery` so that callers don't have to call it. I added an `event` param to `startQuery`, since `engagementEvent.start` needs one. I considered removing that need. It's possible, but then we would need a way to avoid calling `engagementEvent.start` in the keyword offer case, so `startQuery` would need something like a `suppressEngagementEvent` param. `event` basically functions as that, so I left it. Another thing to point out about this patch is that I chose to record a "typed" value when the pageproxystate is invalid and the view opens due to `openViewOnFocus`. The view does not show the user's top sites in that case, so "topsites" seems wrong. Differential Revision: https://phabricator.services.mozilla.com/D42749
browser/base/content/browser-sets.inc
browser/base/content/browser.js
browser/components/urlbar/UrlbarController.jsm
browser/components/urlbar/UrlbarInput.jsm
browser/components/urlbar/tests/UrlbarTestUtils.jsm
browser/components/urlbar/tests/browser/browser_urlbar_event_telemetry.js
--- a/browser/base/content/browser-sets.inc
+++ b/browser/base/content/browser-sets.inc
@@ -76,17 +76,17 @@
     <command id="Browser:ShowAllTabs" oncommand="gTabsPanel.showAllTabsPanel();"/>
     <command id="cmd_fullZoomReduce"  oncommand="FullZoom.reduce()"/>
     <command id="cmd_fullZoomEnlarge" oncommand="FullZoom.enlarge()"/>
     <command id="cmd_fullZoomReset"   oncommand="FullZoom.reset()"/>
     <command id="cmd_fullZoomToggle"  oncommand="ZoomManager.toggleZoom();"/>
     <command id="cmd_gestureRotateLeft" oncommand="gGestureSupport.rotate(event.sourceEvent)"/>
     <command id="cmd_gestureRotateRight" oncommand="gGestureSupport.rotate(event.sourceEvent)"/>
     <command id="cmd_gestureRotateEnd" oncommand="gGestureSupport.rotateEnd()"/>
-    <command id="Browser:OpenLocation" oncommand="openLocation();"/>
+    <command id="Browser:OpenLocation" oncommand="openLocation(event);"/>
     <command id="Browser:RestoreLastSession" oncommand="SessionStore.restoreLastSession();" disabled="true"/>
     <command id="Browser:NewUserContextTab" oncommand="openNewUserContextTab(event.sourceEvent);"/>
     <command id="Browser:OpenAboutContainers" oncommand="openPreferences('paneContainers');"/>
     <command id="Tools:Search" oncommand="BrowserSearch.webSearch();"/>
     <command id="Tools:Downloads" oncommand="BrowserDownloadsUI();"/>
     <command id="Tools:Addons" oncommand="BrowserOpenAddonsMgr();"/>
     <command id="Tools:Sanitize" oncommand="Sanitizer.showUI(window);"/>
     <command id="Tools:PrivateBrowsing"
--- a/browser/base/content/browser.js
+++ b/browser/base/content/browser.js
@@ -2837,21 +2837,21 @@ function focusAndSelectUrlBar(userInitia
     FullScreen.showNavToolbox();
   }
 
   gURLBar.userInitiatedFocus = userInitiatedFocus;
   gURLBar.select();
   gURLBar.userInitiatedFocus = false;
 }
 
-function openLocation() {
+function openLocation(event) {
   if (window.location.href == AppConstants.BROWSER_CHROME_URL) {
     focusAndSelectUrlBar(true);
     if (gURLBar.openViewOnFocus && !gURLBar.view.isOpen) {
-      gURLBar.startQuery();
+      gURLBar.startQuery({ event });
     }
     return;
   }
 
   // If there's an open browser window, redirect the command there.
   let win = getTopWin();
   if (win) {
     win.focus();
--- a/browser/components/urlbar/UrlbarController.jsm
+++ b/browser/components/urlbar/UrlbarController.jsm
@@ -336,18 +336,20 @@ class UrlbarController {
             );
           }
         } else {
           if (this.keyEventMovesCaret(event)) {
             break;
           }
           if (executeAction) {
             this.userSelectionBehavior = "arrow";
-            this.engagementEvent.start(event);
-            this.input.startQuery({ searchString: this.input.textValue });
+            this.input.startQuery({
+              searchString: this.input.textValue,
+              event,
+            });
           }
         }
         event.preventDefault();
         break;
       case KeyEvent.DOM_VK_LEFT:
       case KeyEvent.DOM_VK_RIGHT:
       case KeyEvent.DOM_VK_HOME:
       case KeyEvent.DOM_VK_END:
@@ -595,50 +597,65 @@ class UrlbarController {
  * @see Events.yaml
  */
 class TelemetryEvent {
   constructor(category) {
     this._category = category;
   }
 
   /**
-   * Start measuring the elapsed time from an input event.
+   * Start measuring the elapsed time from a user-generated event.
    * After this has been invoked, any subsequent calls to start() are ignored,
    * until either record() or discard() are invoked. Thus, it is safe to keep
-   * invoking this on every input event.
-   * @param {event} event A DOM input event.
+   * invoking this on every input event as the user is typing, for example.
+   * @param {event} event A DOM event.
+   * @param {string} [searchString] Pass a search string related to the event if
+   *        you have one.  The event by itself sometimes isn't enough to
+   *        determine the telemetry details we should record.
    * @note This should never throw, or it may break the urlbar.
    */
-  start(event) {
-    // Start is invoked at any input, but we only count the first one.
-    // Once an engagement or abandoment happens, we clear the _startEventInfo.
+  start(event, searchString = null) {
+    // start is invoked on a user-generated event, but we only count the first
+    // one.  Once an engagement or abandoment happens, we clear _startEventInfo.
     if (!this._category || this._startEventInfo) {
       return;
     }
     if (!event) {
       Cu.reportError("Must always provide an event");
       return;
     }
-    if (!["input", "drop", "mousedown", "keydown"].includes(event.type)) {
+    const validEvents = ["command", "drop", "input", "keydown", "mousedown"];
+    if (!validEvents.includes(event.type)) {
       Cu.reportError("Can't start recording from event type: " + event.type);
       return;
     }
 
-    // "typed" is used when the user types something, while "pasted" and
-    // "dropped" are used when the text is inserted at once, by a paste or drop
-    // operation. "topsites" is a bit special, it is used when the user opens
-    // the empty search dropdown, that is supposed to show top sites. That
-    // happens by clicking on the urlbar dropmarker, or pressing DOWN with an
-    // empty input field. Even if the user later types something, we still
-    // report "topsites", with a positive numChars.
+    // Possible interaction types:
+    //
+    // typed:
+    //   The user typed something and the view opened.  We also use this when
+    //   the user has opened the view without typing anything (by clicking the
+    //   dropdown arrow, for example) after having left the pageproxystate
+    //   invalid.  In both cases, the view reflects what the user typed.
+    // pasted:
+    //   The user pasted text.
+    // dropped:
+    //   The user dropped text.
+    // topsites:
+    //   The user opened the view with an empty search string (for example,
+    //   after deleting all text, or by clicking the dropdown arrow when the
+    //   pageproxystate is valid).  The view shows the user's top sites.
+
     let interactionType = "topsites";
     if (event.type == "input") {
       interactionType = UrlbarUtils.isPasteEvent(event) ? "pasted" : "typed";
     } else if (event.type == "drop") {
       interactionType = "dropped";
+    } else if (searchString) {
+      interactionType = "typed";
     }
 
     this._startEventInfo = {
       timeStamp: event.timeStamp || Cu.now(),
       interactionType,
     };
   }
 
@@ -731,19 +748,19 @@ class TelemetryEvent {
       method,
       action,
       value,
       extra
     );
   }
 
   /**
-   * Resets the currently tracked input event, that was registered via start(),
-   * so it won't be recorded.
-   * If there's no tracked input event, this is a no-op.
+   * Resets the currently tracked user-generated event that was registered via
+   * start(), so it won't be recorded.  If there's no tracked event, this is a
+   * no-op.
    */
   discard() {
     this._startEventInfo = null;
   }
 
   /**
    * Extracts a type from a result, to be used in the telemetry event.
    * @param {UrlbarResult} result The result to analyze.
--- a/browser/components/urlbar/UrlbarInput.jsm
+++ b/browser/components/urlbar/UrlbarInput.jsm
@@ -734,37 +734,45 @@ class UrlbarInput {
    *   Otherwise, the current input value must start with this value.
    * @param {boolean} [options.resetSearchState]
    *   If this is the first search of a user interaction with the input, set
    *   this to true (the default) so that search-related state from the previous
    *   interaction doesn't interfere with the new interaction.  Otherwise set it
    *   to false so that state is maintained during a single interaction.  The
    *   intended use for this parameter is that it should be set to false when
    *   this method is called due to input events.
+   * @param {event} [options.event]
+   *   The user-generated event that triggered the query, if any.  If given, we
+   *   will record engagement event telemetry for the query.
    */
   startQuery({
     allowAutofill = true,
     searchString = null,
     resetSearchState = true,
+    event = null,
   } = {}) {
+    if (!searchString) {
+      searchString =
+        this.getAttribute("pageproxystate") == "valid" ? "" : this.textValue;
+    } else if (!this.textValue.startsWith(searchString)) {
+      throw new Error("The current value doesn't start with the search string");
+    }
+
+    if (event) {
+      this.controller.engagementEvent.start(event, searchString);
+    }
+
     if (this._suppressStartQuery) {
       return;
     }
 
     if (resetSearchState) {
       this._resetSearchState();
     }
 
-    if (!searchString) {
-      searchString =
-        this.getAttribute("pageproxystate") == "valid" ? "" : this.textValue;
-    } else if (!this.textValue.startsWith(searchString)) {
-      throw new Error("The current value doesn't start with the search string");
-    }
-
     this._lastSearchString = searchString;
     this._textValueOnLastSearch = this.textValue;
 
     // TODO (Bug 1522902): This promise is necessary for tests, because some
     // tests are not listening for completion when starting a query through
     // other methods than startQuery (input events for example).
     this.lastQueryContextPromise = this.controller.startQuery(
       new UrlbarQueryContext({
@@ -1545,35 +1553,35 @@ class UrlbarInput {
       if (event.button != 0) {
         return;
       }
 
       if (event.detail == 2 && UrlbarPrefs.get("doubleClickSelectsAll")) {
         this.editor.selectAll();
         event.preventDefault();
       } else if (this.openViewOnFocusForCurrentTab && !this.view.isOpen) {
-        this.controller.engagementEvent.start(event);
         this.startQuery({
           allowAutofill: false,
+          event,
         });
       }
       return;
     }
 
     if (
       event.originalTarget.classList.contains("urlbar-history-dropmarker") &&
       event.button == 0
     ) {
       if (this.view.isOpen) {
         this.view.close();
       } else {
         this.focus();
-        this.controller.engagementEvent.start(event);
         this.startQuery({
           allowAutofill: false,
+          event,
         });
         this._maybeSelectAll();
       }
     }
   }
 
   _on_input(event) {
     let value = this.textValue;
@@ -1615,29 +1623,28 @@ class UrlbarInput {
     if (
       compositionState == UrlbarUtils.COMPOSITION.COMPOSING ||
       (compositionState == UrlbarUtils.COMPOSITION.CANCELED &&
         !compositionClosedPopup)
     ) {
       return;
     }
 
-    this.controller.engagementEvent.start(event);
-
     // Autofill only when text is inserted (i.e., event.data is not empty) and
     // it's not due to pasting.
     let allowAutofill =
       !!event.data &&
       !UrlbarUtils.isPasteEvent(event) &&
       this._maybeAutofillOnInput(value);
 
     this.startQuery({
       searchString: value,
       allowAutofill,
       resetSearchState: false,
+      event,
     });
   }
 
   _on_select(event) {
     if (
       !this.window.windowUtils.isHandlingUserInput ||
       !Services.clipboard.supportsSelectionClipboard()
     ) {
--- a/browser/components/urlbar/tests/UrlbarTestUtils.jsm
+++ b/browser/components/urlbar/tests/UrlbarTestUtils.jsm
@@ -215,17 +215,17 @@ var UrlbarTestUtils = {
     }
     return BrowserTestUtils.waitForCondition(
       () => httpserver.connectionNumber == count,
       "Waiting for speculative connection setup"
     );
   },
 
   /**
-   * Waits for the popup to be hidden.
+   * Waits for the popup to be shown.
    * @param {object} win The window containing the urlbar
    * @param {function} openFn Function to be used to open the popup.
    * @returns {Promise} resolved once the popup is closed
    */
   promisePopupOpen(win, openFn) {
     if (!openFn) {
       throw new Error("openFn should be supplied to promisePopupOpen");
     }
--- a/browser/components/urlbar/tests/browser/browser_urlbar_event_telemetry.js
+++ b/browser/components/urlbar/tests/browser/browser_urlbar_event_telemetry.js
@@ -419,16 +419,137 @@ const tests = [
         elapsed: val => parseInt(val) > 0,
         numChars: "1",
         selType: "search",
         selIndex: "0",
       },
     };
   },
 
+  async function() {
+    info(
+      "With pageproxystate=valid, open the panel with openViewOnFocus, select with DOWN, Enter."
+    );
+    gURLBar.value = "";
+    let promise = BrowserTestUtils.browserLoaded(gBrowser.selectedBrowser);
+    Services.prefs.setBoolPref("browser.urlbar.openViewOnFocus", true);
+    await UrlbarTestUtils.promisePopupOpen(window, () => {
+      window.document.getElementById("Browser:OpenLocation").doCommand();
+    });
+    Services.prefs.clearUserPref("browser.urlbar.openViewOnFocus");
+    await UrlbarTestUtils.promiseSearchComplete(window);
+    while (gURLBar.value != "http://mochi.test:8888/") {
+      EventUtils.synthesizeKey("KEY_ArrowDown");
+    }
+    EventUtils.synthesizeKey("VK_RETURN");
+    await promise;
+    return {
+      category: "urlbar",
+      method: "engagement",
+      object: "enter",
+      value: "topsites",
+      extra: {
+        elapsed: val => parseInt(val) > 0,
+        numChars: "0",
+        selType: "history",
+        selIndex: val => parseInt(val) >= 0,
+      },
+    };
+  },
+
+  async function() {
+    info(
+      "With pageproxystate=valid, open the panel with openViewOnFocus, click on entry."
+    );
+    gURLBar.value = "";
+    let promise = BrowserTestUtils.browserLoaded(gBrowser.selectedBrowser);
+    Services.prefs.setBoolPref("browser.urlbar.openViewOnFocus", true);
+    await UrlbarTestUtils.promisePopupOpen(window, () => {
+      window.document.getElementById("Browser:OpenLocation").doCommand();
+    });
+    Services.prefs.clearUserPref("browser.urlbar.openViewOnFocus");
+    while (gURLBar.value != "http://mochi.test:8888/") {
+      EventUtils.synthesizeKey("KEY_ArrowDown");
+    }
+    let element = UrlbarTestUtils.getSelectedElement(window);
+    EventUtils.synthesizeMouseAtCenter(element, {});
+    await promise;
+    return {
+      category: "urlbar",
+      method: "engagement",
+      object: "click",
+      value: "topsites",
+      extra: {
+        elapsed: val => parseInt(val) > 0,
+        numChars: "0",
+        selType: "history",
+        selIndex: "0",
+      },
+    };
+  },
+
+  async function() {
+    info(
+      "With pageproxystate=invalid, open the panel with openViewOnFocus, Enter."
+    );
+    gURLBar.value = "mochi.test";
+    gURLBar.setAttribute("pageproxystate", "invalid");
+    let promise = BrowserTestUtils.browserLoaded(gBrowser.selectedBrowser);
+    Services.prefs.setBoolPref("browser.urlbar.openViewOnFocus", true);
+    await UrlbarTestUtils.promisePopupOpen(window, () => {
+      window.document.getElementById("Browser:OpenLocation").doCommand();
+    });
+    Services.prefs.clearUserPref("browser.urlbar.openViewOnFocus");
+    await UrlbarTestUtils.promiseSearchComplete(window);
+    EventUtils.synthesizeKey("VK_RETURN");
+    await promise;
+    return {
+      category: "urlbar",
+      method: "engagement",
+      object: "enter",
+      value: "typed",
+      extra: {
+        elapsed: val => parseInt(val) > 0,
+        numChars: "10",
+        selType: "autofill",
+        selIndex: "0",
+      },
+    };
+  },
+
+  async function() {
+    info(
+      "With pageproxystate=invalid, open the panel with openViewOnFocus, click on entry."
+    );
+    gURLBar.value = "mochi.test";
+    gURLBar.setAttribute("pageproxystate", "invalid");
+    let promise = BrowserTestUtils.browserLoaded(gBrowser.selectedBrowser);
+    Services.prefs.setBoolPref("browser.urlbar.openViewOnFocus", true);
+    await UrlbarTestUtils.promisePopupOpen(window, () => {
+      window.document.getElementById("Browser:OpenLocation").doCommand();
+    });
+    Services.prefs.clearUserPref("browser.urlbar.openViewOnFocus");
+    await UrlbarTestUtils.promiseSearchComplete(window);
+    let element = UrlbarTestUtils.getSelectedElement(window);
+    EventUtils.synthesizeMouseAtCenter(element, {});
+    await promise;
+    return {
+      category: "urlbar",
+      method: "engagement",
+      object: "click",
+      value: "typed",
+      extra: {
+        elapsed: val => parseInt(val) > 0,
+        numChars: "10",
+        selType: "autofill",
+        selIndex: "0",
+      },
+    };
+  },
+
   /*
    * Abandonment tests.
    */
 
   async function() {
     info("Type something, blur.");
     gURLBar.select();
     EventUtils.synthesizeKey("x");
@@ -490,16 +611,63 @@ const tests = [
       value: "topsites",
       extra: {
         elapsed: val => parseInt(val) > 0,
         numChars: "1",
       },
     };
   },
 
+  async function() {
+    info(
+      "With pageproxystate=valid, open the panel with openViewOnFocus, don't type, blur it."
+    );
+    gURLBar.value = "";
+    Services.prefs.setBoolPref("browser.urlbar.openViewOnFocus", true);
+    await UrlbarTestUtils.promisePopupOpen(window, () => {
+      window.document.getElementById("Browser:OpenLocation").doCommand();
+    });
+    Services.prefs.clearUserPref("browser.urlbar.openViewOnFocus");
+    gURLBar.blur();
+    return {
+      category: "urlbar",
+      method: "abandonment",
+      object: "blur",
+      value: "topsites",
+      extra: {
+        elapsed: val => parseInt(val) > 0,
+        numChars: "0",
+      },
+    };
+  },
+
+  async function() {
+    info(
+      "With pageproxystate=invalid, open the panel with openViewOnFocus, don't type, blur it."
+    );
+    gURLBar.value = "mochi.test";
+    gURLBar.setAttribute("pageproxystate", "invalid");
+    Services.prefs.setBoolPref("browser.urlbar.openViewOnFocus", true);
+    await UrlbarTestUtils.promisePopupOpen(window, () => {
+      window.document.getElementById("Browser:OpenLocation").doCommand();
+    });
+    Services.prefs.clearUserPref("browser.urlbar.openViewOnFocus");
+    gURLBar.blur();
+    return {
+      category: "urlbar",
+      method: "abandonment",
+      object: "blur",
+      value: "typed",
+      extra: {
+        elapsed: val => parseInt(val) > 0,
+        numChars: "10",
+      },
+    };
+  },
+
   /*
    * No event tests.
    */
 
   async function() {
     info("Type something, click on search settings.");
     await BrowserTestUtils.withNewTab(
       { gBrowser, url: "about:blank" },
@@ -580,15 +748,17 @@ add_task(async function test() {
     await PlacesUtils.keywords.remove("kw");
     await PlacesUtils.bookmarks.remove(bm);
     await PlacesUtils.history.clear();
   });
 
   // This is not necessary after each loop, because assertEvents does it.
   Services.telemetry.clearEvents();
 
-  for (let testFn of tests) {
+  for (let i = 0; i < tests.length; i++) {
+    info(`Running test at index ${i}`);
+    let testFn = tests[i];
     let expectedEvents = [await testFn()].filter(e => !!e);
     // Always blur to ensure it's not accounted as an additional abandonment.
     gURLBar.blur();
     TelemetryTestUtils.assertEvents(expectedEvents, { category: "urlbar" });
   }
 });