Bug 1318333 - Fix SEARCH_COUNTS not counting one-off searches. r=mak, a=gchang FIREFOX_51_0b3_BUILD1 FIREFOX_51_0b3_RELEASE
authorAlessio Placitelli <alessio.placitelli@gmail.com>
Mon, 21 Nov 2016 04:56:00 +0100
changeset 356748 f37e99ebc6e0c682003b52573f415e5fd78d425a
parent 356747 ebddbe716951a92bc9104d85e56a68ed31258ed0
child 356749 427a01f3d6a48626cb1081be8d7ccb94c50e74f4
push id6614
push usercbook@mozilla.com
push dateThu, 24 Nov 2016 15:33:03 +0000
treeherdermozilla-beta@f37e99ebc6e0 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmak, gchang
bugs1318333
milestone51.0
Bug 1318333 - Fix SEARCH_COUNTS not counting one-off searches. r=mak, a=gchang MozReview-Commit-ID: 2HTJBzbpUK2
browser/modules/BrowserUsageTelemetry.jsm
browser/modules/test/browser_UsageTelemetry_content.js
browser/modules/test/browser_UsageTelemetry_content_aboutHome.js
browser/modules/test/browser_UsageTelemetry_searchbar.js
browser/modules/test/browser_UsageTelemetry_urlbar.js
browser/modules/test/head.js
--- a/browser/modules/BrowserUsageTelemetry.jsm
+++ b/browser/modules/BrowserUsageTelemetry.jsm
@@ -265,33 +265,35 @@ let BrowserUsageTelemetry = {
    * @param {Boolean} [details.isAlias=false]
    *        true if this event was generated by a search using an alias.
    * @param {Object} [details.type=null]
    *        The object describing the event that triggered the search.
    * @throws if source is not in the known sources list.
    */
   recordSearch(engine, source, details={}) {
     const isOneOff = !!details.isOneOff;
+    const countId = getSearchEngineId(engine) + "." + source;
 
     if (isOneOff) {
       if (!KNOWN_ONEOFF_SOURCES.includes(source)) {
         // Silently drop the error if this bogus call
         // came from 'urlbar' or 'searchbar'. They're
         // calling |recordSearch| twice from two different
-        // code paths.
+        // code paths because they want to record the search
+        // in SEARCH_COUNTS.
         if (['urlbar', 'searchbar'].includes(source)) {
+          Services.telemetry.getKeyedHistogramById("SEARCH_COUNTS").add(countId);
           return;
         }
         throw new Error("Unknown source for one-off search: " + source);
       }
     } else {
       if (!KNOWN_SEARCH_SOURCES.includes(source)) {
         throw new Error("Unknown source for search: " + source);
       }
-      let countId = getSearchEngineId(engine) + "." + source;
       Services.telemetry.getKeyedHistogramById("SEARCH_COUNTS").add(countId);
     }
 
     // Dispatch the search signal to other handlers.
     this._handleSearchAction(source, details);
   },
 
   _handleSearchAction(source, details) {
--- a/browser/modules/test/browser_UsageTelemetry_content.js
+++ b/browser/modules/test/browser_UsageTelemetry_content.js
@@ -18,31 +18,33 @@ add_task(function* setup() {
   let engineDefault = Services.search.getEngineByName("MozSearch");
   let originalEngine = Services.search.currentEngine;
   Services.search.currentEngine = engineDefault;
 
   // Move the second engine at the beginning of the one-off list.
   let engineOneOff = Services.search.getEngineByName("MozSearch2");
   Services.search.moveEngine(engineOneOff, 0);
 
-  // We want select events to be fired.
-  yield new Promise(resolve =>
-    SpecialPowers.pushPrefEnv({"set": [["dom.select_events.enabled", true]]}, resolve));
+  yield SpecialPowers.pushPrefEnv({"set": [
+    ["dom.select_events.enabled", true], // We want select events to be fired.
+    ["toolkit.telemetry.enabled", true]  // And Extended Telemetry to be enabled.
+  ]});
 
   // Make sure to restore the engine once we're done.
   registerCleanupFunction(function* () {
     Services.search.currentEngine = originalEngine;
     Services.search.removeEngine(engineDefault);
     Services.search.removeEngine(engineOneOff);
   });
 });
 
 add_task(function* test_context_menu() {
   // Let's reset the counts.
   Services.telemetry.clearScalars();
+  let search_hist = getSearchCountsHistogram();
 
   // Open a new tab with a page containing some text.
   let tab =
     yield BrowserTestUtils.openNewForegroundTab(gBrowser, "data:text/plain;charset=utf8,test%20search");
 
   info("Select all the text in the page.");
   yield ContentTask.spawn(tab.linkedBrowser, "", function*() {
     return new Promise(resolve => {
@@ -64,24 +66,28 @@ add_task(function* test_context_menu() {
 
   info("Validate the search counts.");
   const scalars =
     Services.telemetry.snapshotKeyedScalars(Ci.nsITelemetry.DATASET_RELEASE_CHANNEL_OPTIN, false);
   checkKeyedScalar(scalars, SCALAR_CONTEXT_MENU, "search", 1);
   Assert.equal(Object.keys(scalars[SCALAR_CONTEXT_MENU]).length, 1,
                "This search must only increment one entry in the scalar.");
 
+  // Make sure SEARCH_COUNTS contains identical values.
+  checkKeyedHistogram(search_hist, 'other-MozSearch.contextmenu', 1);
+
   contextMenu.hidePopup();
   yield BrowserTestUtils.removeTab(gBrowser.selectedTab);
   yield BrowserTestUtils.removeTab(tab);
 });
 
 add_task(function* test_about_newtab() {
   // Let's reset the counts.
   Services.telemetry.clearScalars();
+  let search_hist = getSearchCountsHistogram();
 
   let tab = yield BrowserTestUtils.openNewForegroundTab(gBrowser, "about:newtab", false);
   yield ContentTask.spawn(tab.linkedBrowser, null, function* () {
     yield ContentTaskUtils.waitForCondition(() => !content.document.hidden);
   });
 
   info("Trigger a simple serch, just text + enter.");
   let p = BrowserTestUtils.browserLoaded(tab.linkedBrowser);
@@ -91,10 +97,13 @@ add_task(function* test_about_newtab() {
 
   // Check if the scalars contain the expected values.
   const scalars =
     Services.telemetry.snapshotKeyedScalars(Ci.nsITelemetry.DATASET_RELEASE_CHANNEL_OPTIN, false);
   checkKeyedScalar(scalars, SCALAR_ABOUT_NEWTAB, "search_enter", 1);
   Assert.equal(Object.keys(scalars[SCALAR_ABOUT_NEWTAB]).length, 1,
                "This search must only increment one entry in the scalar.");
 
+  // Make sure SEARCH_COUNTS contains identical values.
+  checkKeyedHistogram(search_hist, 'other-MozSearch.newtab', 1);
+
   yield BrowserTestUtils.removeTab(tab);
 });
--- a/browser/modules/test/browser_UsageTelemetry_content_aboutHome.js
+++ b/browser/modules/test/browser_UsageTelemetry_content_aboutHome.js
@@ -22,27 +22,31 @@ add_task(function* setup() {
   let engineDefault = Services.search.getEngineByName("MozSearch");
   let originalEngine = Services.search.currentEngine;
   Services.search.currentEngine = engineDefault;
 
   // Move the second engine at the beginning of the one-off list.
   let engineOneOff = Services.search.getEngineByName("MozSearch2");
   Services.search.moveEngine(engineOneOff, 0);
 
+  // Enable Extended Telemetry.
+  yield SpecialPowers.pushPrefEnv({"set": [["toolkit.telemetry.enabled", true]]});
+
   // Make sure to restore the engine once we're done.
   registerCleanupFunction(function* () {
     Services.search.currentEngine = originalEngine;
     Services.search.removeEngine(engineDefault);
     Services.search.removeEngine(engineOneOff);
   });
 });
 
 add_task(function* test_abouthome_simpleQuery() {
   // Let's reset the counts.
   Services.telemetry.clearScalars();
+  let search_hist = getSearchCountsHistogram();
 
   let tab = yield BrowserTestUtils.openNewForegroundTab(gBrowser);
 
   info("Setup waiting for AboutHomeLoadSnippetsCompleted.");
   let promiseAboutHomeLoaded = new Promise(resolve => {
     tab.linkedBrowser.addEventListener("AboutHomeLoadSnippetsCompleted", function loadListener(event) {
       tab.linkedBrowser.removeEventListener("AboutHomeLoadSnippetsCompleted", loadListener, true);
       resolve();
@@ -62,10 +66,13 @@ add_task(function* test_abouthome_simple
 
   // Check if the scalars contain the expected values.
   const scalars =
     Services.telemetry.snapshotKeyedScalars(Ci.nsITelemetry.DATASET_RELEASE_CHANNEL_OPTIN, false);
   checkKeyedScalar(scalars, SCALAR_ABOUT_HOME, "search_enter", 1);
   Assert.equal(Object.keys(scalars[SCALAR_ABOUT_HOME]).length, 1,
                "This search must only increment one entry in the scalar.");
 
+  // Make sure SEARCH_COUNTS contains identical values.
+  checkKeyedHistogram(search_hist, 'other-MozSearch.abouthome', 1);
+
   yield BrowserTestUtils.removeTab(tab);
 });
--- a/browser/modules/test/browser_UsageTelemetry_searchbar.js
+++ b/browser/modules/test/browser_UsageTelemetry_searchbar.js
@@ -63,49 +63,57 @@ add_task(function* setup() {
   let engineDefault = Services.search.getEngineByName("MozSearch");
   let originalEngine = Services.search.currentEngine;
   Services.search.currentEngine = engineDefault;
 
   // Move the second engine at the beginning of the one-off list.
   let engineOneOff = Services.search.getEngineByName("MozSearch2");
   Services.search.moveEngine(engineOneOff, 0);
 
+  // Enable Extended Telemetry.
+  yield SpecialPowers.pushPrefEnv({"set": [["toolkit.telemetry.enabled", true]]});
+
   // Make sure to restore the engine once we're done.
   registerCleanupFunction(function* () {
     Services.search.currentEngine = originalEngine;
     Services.search.removeEngine(engineDefault);
     Services.search.removeEngine(engineOneOff);
   });
 });
 
 add_task(function* test_plainQuery() {
   // Let's reset the counts.
   Services.telemetry.clearScalars();
+  let search_hist = getSearchCountsHistogram();
 
   let tab = yield BrowserTestUtils.openNewForegroundTab(gBrowser, "about:blank");
 
   info("Simulate entering a simple search.");
   let p = BrowserTestUtils.browserLoaded(tab.linkedBrowser);
   yield searchInSearchbar("simple query");
   EventUtils.sendKey("return");
   yield p;
 
   // Check if the scalars contain the expected values.
   const scalars =
     Services.telemetry.snapshotKeyedScalars(Ci.nsITelemetry.DATASET_RELEASE_CHANNEL_OPTIN, false);
   checkKeyedScalar(scalars, SCALAR_SEARCHBAR, "search_enter", 1);
   Assert.equal(Object.keys(scalars[SCALAR_SEARCHBAR]).length, 1,
                "This search must only increment one entry in the scalar.");
 
+  // Make sure SEARCH_COUNTS contains identical values.
+  checkKeyedHistogram(search_hist, 'other-MozSearch.searchbar', 1);
+
   yield BrowserTestUtils.removeTab(tab);
 });
 
 add_task(function* test_oneOff() {
   // Let's reset the counts.
   Services.telemetry.clearScalars();
+  let search_hist = getSearchCountsHistogram();
 
   let tab = yield BrowserTestUtils.openNewForegroundTab(gBrowser, "about:blank");
 
   info("Perform a one-off search using the first engine.");
   let p = BrowserTestUtils.browserLoaded(tab.linkedBrowser);
   yield searchInSearchbar("query");
 
   info("Pressing Alt+Down to highlight the first one off engine.");
@@ -115,22 +123,26 @@ add_task(function* test_oneOff() {
 
   // Check if the scalars contain the expected values.
   const scalars =
     Services.telemetry.snapshotKeyedScalars(Ci.nsITelemetry.DATASET_RELEASE_CHANNEL_OPTIN, false);
   checkKeyedScalar(scalars, SCALAR_SEARCHBAR, "search_oneoff", 1);
   Assert.equal(Object.keys(scalars[SCALAR_SEARCHBAR]).length, 1,
                "This search must only increment one entry in the scalar.");
 
+  // Make sure SEARCH_COUNTS contains identical values.
+  checkKeyedHistogram(search_hist, 'other-MozSearch2.searchbar', 1);
+
   yield BrowserTestUtils.removeTab(tab);
 });
 
 add_task(function* test_suggestion() {
   // Let's reset the counts.
   Services.telemetry.clearScalars();
+  let search_hist = getSearchCountsHistogram();
 
   // Create an engine to generate search suggestions and add it as default
   // for this test.
   const url = getRootDirectory(gTestPath) + "usageTelemetrySearchSuggestions.xml";
   let suggestionEngine = yield new Promise((resolve, reject) => {
     Services.search.addEngine(url, null, "", false, {
       onSuccess(engine) { resolve(engine) },
       onError() { reject() }
@@ -151,12 +163,15 @@ add_task(function* test_suggestion() {
 
   // Check if the scalars contain the expected values.
   const scalars =
     Services.telemetry.snapshotKeyedScalars(Ci.nsITelemetry.DATASET_RELEASE_CHANNEL_OPTIN, false);
   checkKeyedScalar(scalars, SCALAR_SEARCHBAR, "search_suggestion", 1);
   Assert.equal(Object.keys(scalars[SCALAR_SEARCHBAR]).length, 1,
                "This search must only increment one entry in the scalar.");
 
+  // Make sure SEARCH_COUNTS contains identical values.
+  checkKeyedHistogram(search_hist, 'other-' + suggestionEngine.name + '.searchbar', 1);
+
   Services.search.currentEngine = previousEngine;
   Services.search.removeEngine(suggestionEngine);
   yield BrowserTestUtils.removeTab(tab);
 });
--- a/browser/modules/test/browser_UsageTelemetry_urlbar.js
+++ b/browser/modules/test/browser_UsageTelemetry_urlbar.js
@@ -53,72 +53,84 @@ add_task(function* setup() {
 
   // And the first one-off engine.
   Services.search.moveEngine(engine, 0);
 
   // Enable search suggestions in the urlbar.
   Services.prefs.setBoolPref(SUGGEST_URLBAR_PREF, true);
   Services.prefs.setBoolPref(ONEOFF_URLBAR_PREF, true);
 
+  // Enable Extended Telemetry.
+  yield SpecialPowers.pushPrefEnv({"set": [["toolkit.telemetry.enabled", true]]});
+
   // Make sure to restore the engine once we're done.
   registerCleanupFunction(function* () {
     Services.search.currentEngine = originalEngine;
     Services.search.removeEngine(engine);
     Services.prefs.clearUserPref(SUGGEST_URLBAR_PREF, true);
     Services.prefs.clearUserPref(ONEOFF_URLBAR_PREF);
   });
 });
 
 add_task(function* test_simpleQuery() {
   // Let's reset the counts.
   Services.telemetry.clearScalars();
+  let search_hist = getSearchCountsHistogram();
 
   let tab = yield BrowserTestUtils.openNewForegroundTab(gBrowser, "about:blank");
 
   info("Simulate entering a simple search.");
   let p = BrowserTestUtils.browserLoaded(tab.linkedBrowser);
   yield searchInAwesomebar("simple query");
   EventUtils.sendKey("return");
   yield p;
 
   // Check if the scalars contain the expected values.
   const scalars =
     Services.telemetry.snapshotKeyedScalars(Ci.nsITelemetry.DATASET_RELEASE_CHANNEL_OPTIN, false);
   checkKeyedScalar(scalars, SCALAR_URLBAR, "search_enter", 1);
   Assert.equal(Object.keys(scalars[SCALAR_URLBAR]).length, 1,
                "This search must only increment one entry in the scalar.");
 
+  // Make sure SEARCH_COUNTS contains identical values.
+  checkKeyedHistogram(search_hist, 'other-MozSearch.urlbar', 1);
+
   yield BrowserTestUtils.removeTab(tab);
 });
 
 add_task(function* test_searchAlias() {
   // Let's reset the counts.
   Services.telemetry.clearScalars();
+  let search_hist = getSearchCountsHistogram();
 
   let tab = yield BrowserTestUtils.openNewForegroundTab(gBrowser, "about:blank");
 
   info("Search using a search alias.");
   let p = BrowserTestUtils.browserLoaded(tab.linkedBrowser);
   yield searchInAwesomebar("mozalias query");
   EventUtils.sendKey("return");
   yield p;
 
   // Check if the scalars contain the expected values.
   const scalars =
     Services.telemetry.snapshotKeyedScalars(Ci.nsITelemetry.DATASET_RELEASE_CHANNEL_OPTIN, false);
   checkKeyedScalar(scalars, SCALAR_URLBAR, "search_alias", 1);
   Assert.equal(Object.keys(scalars[SCALAR_URLBAR]).length, 1,
                "This search must only increment one entry in the scalar.");
 
+  // Make sure SEARCH_COUNTS contains identical values.
+  checkKeyedHistogram(search_hist, 'other-MozSearch.urlbar', 1);
+
   yield BrowserTestUtils.removeTab(tab);
 });
 
 add_task(function* test_oneOff() {
   // Let's reset the counts.
   Services.telemetry.clearScalars();
+  let search_hist = getSearchCountsHistogram();
 
   let tab = yield BrowserTestUtils.openNewForegroundTab(gBrowser, "about:blank");
 
   info("Perform a one-off search using the first engine.");
   let p = BrowserTestUtils.browserLoaded(tab.linkedBrowser);
   yield searchInAwesomebar("query");
 
   info("Pressing Alt+Down to take us to the first one-off engine.");
@@ -128,22 +140,26 @@ add_task(function* test_oneOff() {
 
   // Check if the scalars contain the expected values.
   const scalars =
     Services.telemetry.snapshotKeyedScalars(Ci.nsITelemetry.DATASET_RELEASE_CHANNEL_OPTIN, false);
   checkKeyedScalar(scalars, SCALAR_URLBAR, "search_oneoff", 1);
   Assert.equal(Object.keys(scalars[SCALAR_URLBAR]).length, 1,
                "This search must only increment one entry in the scalar.");
 
+  // Make sure SEARCH_COUNTS contains identical values.
+  checkKeyedHistogram(search_hist, 'other-MozSearch.urlbar', 1);
+
   yield BrowserTestUtils.removeTab(tab);
 });
 
 add_task(function* test_suggestion() {
   // Let's reset the counts.
   Services.telemetry.clearScalars();
+  let search_hist = getSearchCountsHistogram();
 
   // Create an engine to generate search suggestions and add it as default
   // for this test.
   const url = getRootDirectory(gTestPath) + "usageTelemetrySearchSuggestions.xml";
   let suggestionEngine = yield new Promise((resolve, reject) => {
     Services.search.addEngine(url, null, "", false, {
       onSuccess(engine) { resolve(engine) },
       onError() { reject() }
@@ -164,12 +180,15 @@ add_task(function* test_suggestion() {
 
   // Check if the scalars contain the expected values.
   const scalars =
     Services.telemetry.snapshotKeyedScalars(Ci.nsITelemetry.DATASET_RELEASE_CHANNEL_OPTIN, false);
   checkKeyedScalar(scalars, SCALAR_URLBAR, "search_suggestion", 1);
   Assert.equal(Object.keys(scalars[SCALAR_URLBAR]).length, 1,
                "This search must only increment one entry in the scalar.");
 
+  // Make sure SEARCH_COUNTS contains identical values.
+  checkKeyedHistogram(search_hist, 'other-' + suggestionEngine.name + '.urlbar', 1);
+
   Services.search.currentEngine = previousEngine;
   Services.search.removeEngine(suggestionEngine);
   yield BrowserTestUtils.removeTab(tab);
 });
--- a/browser/modules/test/head.js
+++ b/browser/modules/test/head.js
@@ -72,8 +72,26 @@ let typeInSearchField = Task.async(funct
       content.wrappedJSObject.gContentSearchController.remoteTimeout = 5000;
     }
     // Put the focus on the search box.
     let searchInput = content.document.getElementById(fieldName);
     searchInput.focus();
     searchInput.value = text;
   });
 });
+
+/**
+ * Clear and get the SEARCH_COUNTS histogram.
+ */
+function getSearchCountsHistogram() {
+  let search_hist = Services.telemetry.getKeyedHistogramById("SEARCH_COUNTS");
+  search_hist.clear();
+  return search_hist;
+}
+
+/**
+ * Check that the keyed histogram contains the right value.
+ */
+function checkKeyedHistogram(h, key, expectedValue) {
+  const snapshot = h.snapshot();
+  Assert.ok(key in snapshot, `The histogram must contain ${key}.`);
+  Assert.equal(snapshot[key].sum, expectedValue, `The key ${key} must contain ${expectedValue}.`);
+}