Bug 1676250 - Stop overcounting urlbar.tips.tabtosearch_onboard-shown. r=adw

Differential Revision: https://phabricator.services.mozilla.com/D97494

browser/components/urlbar/UrlbarMuxerUnifiedComplete.jsm

@@ -16,6 +16,8 @@ const { XPCOMUtils } = ChromeUtils.import(
 XPCOMUtils.defineLazyModuleGetters(this, {
   Services: "resource://gre/modules/Services.jsm",
   UrlbarPrefs: "resource:///modules/UrlbarPrefs.jsm",
+  UrlbarProviderTabToSearch:
+    "resource:///modules/UrlbarProviderTabToSearch.jsm",
   UrlbarMuxer: "resource:///modules/UrlbarUtils.jsm",
   UrlbarSearchUtils: "resource:///modules/UrlbarSearchUtils.jsm",
   UrlbarUtils: "resource:///modules/UrlbarUtils.jsm",
@@ -548,6 +550,14 @@ class MuxerUnifiedComplete extends UrlbarMuxer {
     // TODO (Bug 1670185): figure out better strategies to manage this case.
     if (result.providerName == "TabToSearch") {
       state.canAddTabToSearch = false;
+      // We want to record in urlbar.tips once per engagement per engine. Since
+      // whether these results are shown is dependent on the Muxer, we must
+      // add to `onboardingEnginesShown` here.
+      if (result.payload.dynamicType) {
+        UrlbarProviderTabToSearch.onboardingEnginesShown.add(
+          result.payload.engine
+        );
+      }
     }
   }
 }

browser/components/urlbar/UrlbarProviderTabToSearch.jsm

@@ -103,6 +103,7 @@ function initializeDynamicResult() {
 class ProviderTabToSearch extends UrlbarProvider {
   constructor() {
     super();
+    this.onboardingEnginesShown = new Set();
   }
 
   /**
@@ -241,6 +242,32 @@ class ProviderTabToSearch extends UrlbarProvider {
     }
   }
 
+  /**
+   * Called when the user starts and ends an engagement with the urlbar. We
+   * clear onboardingEnginesShown on engagement because we want to record in
+   * urlbar.tips once per engagement per engine. This has the unfortunate side
+   * effect of recording again when the user re-opens a view with a retained
+   * tab-to-search result. This is an acceptable tradeoff for not recording
+   * multiple times if the user backspaces autofill but then retypes the engine
+   * hostname, yielding the same tab-to-search result.
+   *
+   * @param {boolean} isPrivate True if the engagement is in a private context.
+   * @param {string} state The state of the engagement, one of: start,
+   *        engagement, abandonment, discard.
+   */
+  onEngagement(isPrivate, state) {
+    if (!this.onboardingEnginesShown.size) {
+      return;
+    }
+
+    Services.telemetry.keyedScalarAdd(
+      "urlbar.tips",
+      "tabtosearch_onboard-shown",
+      this.onboardingEnginesShown.size
+    );
+    this.onboardingEnginesShown.clear();
+  }
+
   /**
    * Defines whether the view should defer user selection events while waiting
    * for the first result from this provider.
@@ -295,7 +322,6 @@ class ProviderTabToSearch extends UrlbarProvider {
     const onboardingInteractionsLeft = UrlbarPrefs.get(
       "tabToSearch.onboard.interactionsLeft"
     );
-    let showedOnboarding = false;
 
     // If the engine host begins with the search string, autofill may happen
     // for it, and the Muxer will retain the result only if there's a matching
@@ -318,7 +344,6 @@ class ProviderTabToSearch extends UrlbarProvider {
       if (host.startsWith(searchStr.toLocaleLowerCase())) {
         if (onboardingInteractionsLeft > 0) {
           addCallback(this, makeOnboardingResult(engine));
-          showedOnboarding = true;
         } else {
           addCallback(this, makeResult(queryContext, engine));
         }
@@ -351,24 +376,12 @@ class ProviderTabToSearch extends UrlbarProvider {
       if (host) {
         let engine = partialMatchEnginesByHost.get(host);
         if (onboardingInteractionsLeft > 0) {
-          showedOnboarding = true;
           addCallback(this, makeOnboardingResult(engine, true));
         } else {
           addCallback(this, makeResult(queryContext, engine, true));
         }
       }
     }
-
-    // The muxer ensures we show at most one tab-to-search result per query.
-    // Thus, we increment this telemetry just once, even if we sent multiple
-    // results to the muxer.
-    if (showedOnboarding) {
-      Services.telemetry.keyedScalarAdd(
-        "urlbar.tips",
-        "tabtosearch_onboard-shown",
-        1
-      );
-    }
   }
 }
 

browser/components/urlbar/docs/telemetry.rst

@@ -103,9 +103,10 @@ urlbar.tips
   - ``intervention_update_web-shown``
     Incremented when the update_web search intervention is shown.
   - ``tabtosearch_onboard-shown``
-    Incremented when a tab-to-search onboarding result is shown. Please note
-    that the number of times tab-to-search onboarding results are picked is
-    the sum of all keys in ``urlbar.searchmode.tabtosearch_onboard``.
+    Incremented when a tab-to-search onboarding result is shown, once per engine
+    per engagement. Please note that the number of times tab-to-search
+    onboarding results are picked is the sum of all keys in
+    ``urlbar.searchmode.tabtosearch_onboard``.
   - ``searchTip_onboard-picked``
     Incremented when the user picks the onboarding search tip.
   - ``searchTip_onboard-shown``

browser/modules/test/browser/browser_UsageTelemetry_urlbar_searchmode.js

@@ -569,13 +569,6 @@ add_task(async function test_tabtosearch_onboard() {
     entry: "tabtosearch_onboard",
   });
 
-  const scalars = TelemetryTestUtils.getProcessScalars("parent", true, false);
-  TelemetryTestUtils.assertKeyedScalar(
-    scalars,
-    "urlbar.tips",
-    "tabtosearch_onboard-shown",
-    1
-  );
   await UrlbarTestUtils.promiseAutocompleteResultPopup({
     window,
     value: "foo",

browser/modules/test/browser/browser_UsageTelemetry_urlbar_tabtosearch.js

@@ -63,6 +63,32 @@ function assertTelemetryResults(histograms, type, index, method) {
   );
 }
 
+/**
+ * Checks to see if the second result in the Urlbar is an onboarding result
+ * with the correct engine.
+ */
+async function checkForOnboardingResult(engineName) {
+  Assert.ok(UrlbarTestUtils.isPopupOpen(window), "Popup should be open.");
+  let tabToSearchResult = (
+    await UrlbarTestUtils.waitForAutocompleteResultAt(window, 1)
+  ).result;
+  Assert.equal(
+    tabToSearchResult.providerName,
+    "TabToSearch",
+    "The second result is a tab-to-search result."
+  );
+  Assert.equal(
+    tabToSearchResult.payload.engine,
+    engineName,
+    "The tab-to-search result is for the first engine."
+  );
+  Assert.equal(
+    tabToSearchResult.payload.dynamicType,
+    "onboardTabToSearch",
+    "The tab-to-search result is an onboarding result."
+  );
+}
+
 add_task(async function setup() {
   await SpecialPowers.pushPrefEnv({
     set: [
@@ -141,3 +167,191 @@ add_task(async function test() {
     await PlacesUtils.history.clear();
   });
 });
+
+add_task(async function onboarding_impressions() {
+  await SpecialPowers.pushPrefEnv({
+    set: [["browser.urlbar.tabToSearch.onboard.interactionsLeft", 3]],
+  });
+
+  await BrowserTestUtils.withNewTab("about:blank", async browser => {
+    const firstEngineHost = "example";
+    let secondEngine = await Services.search.addEngineWithDetails(
+      `${ENGINE_NAME}2`,
+      { template: `http://${firstEngineHost}-2.com/?q={searchTerms}` }
+    );
+
+    for (let i = 0; i < 3; i++) {
+      await PlacesTestUtils.addVisits([`https://${firstEngineHost}-2.com`]);
+      await PlacesTestUtils.addVisits([`https://${ENGINE_DOMAIN}/`]);
+    }
+
+    // First do multiple searches for substrings of firstEngineHost. The view
+    // should show the same tab-to-search onboarding result the entire time, so
+    // we should not continue to increment urlbar.tips.
+    for (let i = 1; i < firstEngineHost.length; i++) {
+      info(
+        `Search for "${firstEngineHost.slice(
+          0,
+          i
+        )}". Only record one impression for this sequence.`
+      );
+      await UrlbarTestUtils.promiseAutocompleteResultPopup({
+        window,
+        value: firstEngineHost.slice(0, i),
+        fireInputEvent: true,
+      });
+      await checkForOnboardingResult(ENGINE_NAME);
+    }
+
+    await UrlbarTestUtils.promisePopupClose(window, () => gURLBar.blur());
+    TelemetryTestUtils.assertKeyedScalar(
+      TelemetryTestUtils.getProcessScalars("parent", true),
+      "urlbar.tips",
+      "tabtosearch_onboard-shown",
+      1
+    );
+
+    info("Type through autofill to second engine hostname. Record impression.");
+    await UrlbarTestUtils.promiseAutocompleteResultPopup({
+      window,
+      value: firstEngineHost,
+      fireInputEvent: true,
+    });
+    await checkForOnboardingResult(ENGINE_NAME);
+    await UrlbarTestUtils.promiseAutocompleteResultPopup({
+      window,
+      value: `${firstEngineHost}-`,
+      fireInputEvent: true,
+    });
+    await checkForOnboardingResult(`${ENGINE_NAME}2`);
+    await UrlbarTestUtils.promisePopupClose(window, () => gURLBar.blur());
+    // Since the user typed past the autofill for the first engine, we showed a
+    // different onboarding result and now we increment
+    // tabtosearch_onboard-shown.
+    TelemetryTestUtils.assertKeyedScalar(
+      TelemetryTestUtils.getProcessScalars("parent", true),
+      "urlbar.tips",
+      "tabtosearch_onboard-shown",
+      3
+    );
+
+    info("Make a typo and return to autofill. Do not record impression.");
+    await UrlbarTestUtils.promiseAutocompleteResultPopup({
+      window,
+      value: `${firstEngineHost}-`,
+      fireInputEvent: true,
+    });
+    await checkForOnboardingResult(`${ENGINE_NAME}2`);
+    await UrlbarTestUtils.promiseAutocompleteResultPopup({
+      window,
+      value: `${firstEngineHost}-3`,
+      fireInputEvent: true,
+    });
+    Assert.equal(
+      UrlbarTestUtils.getResultCount(window),
+      1,
+      "We are not showing a tab-to-search result."
+    );
+    await UrlbarTestUtils.promiseAutocompleteResultPopup({
+      window,
+      value: `${firstEngineHost}-2`,
+      fireInputEvent: true,
+    });
+    await checkForOnboardingResult(`${ENGINE_NAME}2`);
+    await UrlbarTestUtils.promisePopupClose(window, () => gURLBar.blur());
+    TelemetryTestUtils.assertKeyedScalar(
+      TelemetryTestUtils.getProcessScalars("parent", true),
+      "urlbar.tips",
+      "tabtosearch_onboard-shown",
+      4
+    );
+
+    info("Cancel then restart autofill. Do not record impression.");
+    await UrlbarTestUtils.promiseAutocompleteResultPopup({
+      window,
+      value: `${firstEngineHost}-2`,
+      fireInputEvent: true,
+    });
+    await checkForOnboardingResult(`${ENGINE_NAME}2`);
+    let searchPromise = UrlbarTestUtils.promiseSearchComplete(window);
+    EventUtils.synthesizeKey("KEY_Backspace");
+    await searchPromise;
+    Assert.greater(
+      UrlbarTestUtils.getResultCount(window),
+      1,
+      "Sanity check: we have more than one result."
+    );
+    let result = (await UrlbarTestUtils.waitForAutocompleteResultAt(window, 1))
+      .result;
+    Assert.notEqual(
+      result.type,
+      UrlbarUtils.RESULT_TYPE.SEARCH,
+      "The second result is not a tab-to-search result."
+    );
+    searchPromise = UrlbarTestUtils.promiseSearchComplete(window);
+    // Type the "." from `example-2.com`.
+    EventUtils.synthesizeKey(".");
+    await searchPromise;
+    await checkForOnboardingResult(`${ENGINE_NAME}2`);
+    await UrlbarTestUtils.promisePopupClose(window, () => gURLBar.blur());
+    TelemetryTestUtils.assertKeyedScalar(
+      TelemetryTestUtils.getProcessScalars("parent", true),
+      "urlbar.tips",
+      "tabtosearch_onboard-shown",
+      5
+    );
+
+    // See javadoc for UrlbarProviderTabToSearch.onEngagement for discussion
+    // about retained results.
+    info("Reopen the result set with retained results. Record impression.");
+    await UrlbarTestUtils.promisePopupOpen(window, () => {
+      EventUtils.synthesizeMouseAtCenter(gURLBar.inputField, {});
+    });
+    await checkForOnboardingResult(`${ENGINE_NAME}2`);
+    await UrlbarTestUtils.promisePopupClose(window, () => gURLBar.blur());
+    TelemetryTestUtils.assertKeyedScalar(
+      TelemetryTestUtils.getProcessScalars("parent", true),
+      "urlbar.tips",
+      "tabtosearch_onboard-shown",
+      6
+    );
+
+    info(
+      "Open a result page and then autofill engine host. Record impression."
+    );
+    await UrlbarTestUtils.promiseAutocompleteResultPopup({
+      window,
+      value: firstEngineHost,
+      fireInputEvent: true,
+    });
+    await checkForOnboardingResult(ENGINE_NAME);
+    // Press enter on the heuristic result so we visit example.com without
+    // doing an additional search.
+    let loadPromise = BrowserTestUtils.browserLoaded(browser);
+    EventUtils.synthesizeKey("KEY_Enter");
+    await loadPromise;
+    // Click the Urlbar and type to simulate what a user would actually do. If
+    // we use promiseAutocompleteResultPopup, no query would be made between
+    // this one and the previous tab-to-search query. Thus
+    // `onboardingEnginesShown` would not be cleared. This would not happen
+    // in real-world usage.
+    await UrlbarTestUtils.promisePopupOpen(window, () => {
+      EventUtils.synthesizeMouseAtCenter(gURLBar.inputField, {});
+    });
+    searchPromise = UrlbarTestUtils.promiseSearchComplete(window);
+    EventUtils.synthesizeKey(firstEngineHost.slice(0, 4));
+    await searchPromise;
+    await checkForOnboardingResult(ENGINE_NAME);
+    await UrlbarTestUtils.promisePopupClose(window, () => gURLBar.blur());
+    // We clear the scalar this time.
+    TelemetryTestUtils.assertKeyedScalar(
+      TelemetryTestUtils.getProcessScalars("parent", true, true),
+      "urlbar.tips",
+      "tabtosearch_onboard-shown",
+      8
+    );
+
+    await PlacesUtils.history.clear();
+    await Services.search.removeEngine(secondEngine);
+  });
+});