From 5d0da08d55cb88cc7d13c979d840f0d86f0bb494 Mon Sep 17 00:00:00 2001 From: Drew Willcoxon Date: Wed, 2 Dec 2015 16:26:07 -0800 Subject: [PATCH] Bug 1226629 - Increment "urlbar" BrowserUITelemetry/FHR for all searchengine results clicked in the urlbar. r=mak --- browser/base/content/test/general/browser.ini | 1 + .../content/test/general/browser_aboutHome.js | 56 +----- .../general/browser_urlbarSearchTelemetry.js | 177 ++++++++++++++++++ browser/base/content/test/general/head.js | 58 ++++++ browser/base/content/urlbarBindings.xml | 29 +-- browser/components/nsBrowserGlue.js | 31 +-- 6 files changed, 258 insertions(+), 94 deletions(-) create mode 100644 browser/base/content/test/general/browser_urlbarSearchTelemetry.js diff --git a/browser/base/content/test/general/browser.ini b/browser/base/content/test/general/browser.ini index ab9f47f4d1c..d18049f1772 100644 --- a/browser/base/content/test/general/browser.ini +++ b/browser/base/content/test/general/browser.ini @@ -472,6 +472,7 @@ skip-if = os == "linux" || e10s # Bug 1073339 - Investigate autocomplete test un [browser_urlbarSearchSingleWordNotification.js] [browser_urlbarSearchSuggestions.js] [browser_urlbarSearchSuggestionsNotification.js] +[browser_urlbarSearchTelemetry.js] [browser_urlbarStop.js] [browser_urlbarTrimURLs.js] [browser_urlbar_autoFill_backspaced.js] diff --git a/browser/base/content/test/general/browser_aboutHome.js b/browser/base/content/test/general/browser_aboutHome.js index 441c3f5a9a2..c32eb95ebd7 100644 --- a/browser/base/content/test/general/browser_aboutHome.js +++ b/browser/base/content/test/general/browser_aboutHome.js @@ -114,7 +114,7 @@ var gTests = [ // Get the current number of recorded searches. let searchStr = "a search"; - getNumberOfSearches(engineName).then(num => { + getNumberOfSearchesInFHR(engineName, "abouthome").then(num => { numSearchesBefore = num; info("Perform a search."); @@ -126,7 +126,7 @@ var gTests = [ getSubmission(searchStr, null, "homepage"). uri.spec; let loadPromise = waitForDocLoadAndStopIt(expectedURL).then(() => { - getNumberOfSearches(engineName).then(num => { + getNumberOfSearchesInFHR(engineName, "abouthome").then(num => { is(num, numSearchesBefore + 1, "One more search recorded."); searchEventDeferred.resolve(); }); @@ -601,58 +601,6 @@ function promiseSetupSnippetsMap(aTab, aSetupFn) return deferred.promise; } -/** - * Retrieves the number of about:home searches recorded for the current day. - * - * @param aEngineName - * name of the setup search engine. - * - * @return {Promise} Returns a promise resolving to the number of searches. - */ -function getNumberOfSearches(aEngineName) { - let reporter = Components.classes["@mozilla.org/datareporting/service;1"] - .getService() - .wrappedJSObject - .healthReporter; - ok(reporter, "Health Reporter instance available."); - - return reporter.onInit().then(function onInit() { - let provider = reporter.getProvider("org.mozilla.searches"); - ok(provider, "Searches provider is available."); - - let m = provider.getMeasurement("counts", 3); - return m.getValues().then(data => { - let now = new Date(); - let yday = new Date(now); - yday.setDate(yday.getDate() - 1); - - // Add the number of searches recorded yesterday to the number of searches - // recorded today. This makes the test not fail intermittently when it is - // run at midnight and we accidentally compare the number of searches from - // different days. Tests are always run with an empty profile so there - // are no searches from yesterday, normally. Should the test happen to run - // past midnight we make sure to count them in as well. - return getNumberOfSearchesByDate(aEngineName, data, now) + - getNumberOfSearchesByDate(aEngineName, data, yday); - }); - }); -} - -function getNumberOfSearchesByDate(aEngineName, aData, aDate) { - if (aData.days.hasDay(aDate)) { - let id = Services.search.getEngineByName(aEngineName).identifier; - - let day = aData.days.getDay(aDate); - let field = id + ".abouthome"; - - if (day.has(field)) { - return day.get(field) || 0; - } - } - - return 0; // No records found. -} - function waitForLoad(cb) { let browser = gBrowser.selectedBrowser; browser.addEventListener("load", function listener() { diff --git a/browser/base/content/test/general/browser_urlbarSearchTelemetry.js b/browser/base/content/test/general/browser_urlbarSearchTelemetry.js new file mode 100644 index 00000000000..73adec47f7c --- /dev/null +++ b/browser/base/content/test/general/browser_urlbarSearchTelemetry.js @@ -0,0 +1,177 @@ +"use strict"; + +Cu.import("resource:///modules/BrowserUITelemetry.jsm"); + +const SUGGEST_URLBAR_PREF = "browser.urlbar.suggest.searches"; +const TEST_ENGINE_BASENAME = "searchSuggestionEngine.xml"; + +// Must run first. +add_task(function* prepare() { + Services.prefs.setBoolPref(SUGGEST_URLBAR_PREF, true); + let engine = yield promiseNewSearchEngine(TEST_ENGINE_BASENAME); + let oldCurrentEngine = Services.search.currentEngine; + Services.search.currentEngine = engine; + registerCleanupFunction(function* () { + Services.prefs.clearUserPref(SUGGEST_URLBAR_PREF); + Services.search.currentEngine = oldCurrentEngine; + + // Clicking urlbar results causes visits to their associated pages, so clear + // that history now. + yield PlacesTestUtils.clearHistory(); + + // Make sure the popup is closed for the next test. + gURLBar.blur(); + Assert.ok(!gURLBar.popup.popupOpen, "popup should be closed"); + }); +}); + +add_task(function* heuristicResult() { + yield compareCounts(function* () { + yield promiseAutocompleteResultPopup("heuristicResult"); + let action = getActionAtIndex(0); + Assert.ok(!!action, "there should be an action at index 0"); + Assert.equal(action.type, "searchengine", "type should be searchengine"); + let item = gURLBar.popup.richlistbox.getItemAtIndex(0); + let loadPromise = promiseTabLoaded(gBrowser.selectedTab); + item.click(); + yield loadPromise; + }); +}); + +add_task(function* searchSuggestion() { + yield compareCounts(function* () { + yield promiseAutocompleteResultPopup("searchSuggestion"); + let idx = getFirstSuggestionIndex(); + Assert.ok(idx >= 0, "there should be a first suggestion"); + let item = gURLBar.popup.richlistbox.getItemAtIndex(idx); + let loadPromise = promiseTabLoaded(gBrowser.selectedTab); + item.click(); + yield loadPromise; + }); +}); + +/** + * This does three things: gets current telemetry/FHR counts, calls + * clickCallback, gets telemetry/FHR counts again to compare them to the old + * counts. + * + * @param clickCallback Use this to open the urlbar popup and choose and click a + * result. + */ +function* compareCounts(clickCallback) { + // Search events triggered by clicks (not the Return key in the urlbar) are + // recorded in three places: + // * BrowserUITelemetry + // * Telemetry histogram named "SEARCH_COUNTS" + // * FHR + + let engine = Services.search.currentEngine; + let engineID = "org.mozilla.testsearchsuggestions"; + + // First, get the current counts. + + // BrowserUITelemetry + let uiTelemCount = 0; + let bucket = BrowserUITelemetry.currentBucket; + let events = BrowserUITelemetry.getToolbarMeasures().countableEvents; + if (events[bucket] && + events[bucket].search && + events[bucket].search.urlbar) { + uiTelemCount = events[bucket].search.urlbar; + } + + // telemetry histogram SEARCH_COUNTS + let histogramCount = 0; + let histogramKey = engineID + ".urlbar"; + let histogram; + try { + histogram = Services.telemetry.getKeyedHistogramById("SEARCH_COUNTS"); + } catch (ex) { + // No searches performed yet, not a problem. + } + if (histogram) { + let snapshot = histogram.snapshot(); + if (histogramKey in snapshot) { + histogramCount = snapshot[histogramKey].sum; + } + } + + // FHR -- first make sure the engine has an identifier so that FHR is happy. + Object.defineProperty(engine.wrappedJSObject, "identifier", + { value: engineID }); + let fhrCount = yield getNumberOfSearchesInFHR(engine.name, "urlbar"); + + gURLBar.focus(); + yield clickCallback(); + + // Now get the new counts and compare them to the old. + + // BrowserUITelemetry + events = BrowserUITelemetry.getToolbarMeasures().countableEvents; + Assert.ok(bucket in events, "bucket should be recorded"); + events = events[bucket]; + Assert.ok("search" in events, "search should be recorded"); + events = events.search; + Assert.ok("urlbar" in events, "urlbar should be recorded"); + Assert.equal(events.urlbar, uiTelemCount + 1, + "clicked suggestion should be recorded"); + + // telemetry histogram SEARCH_COUNTS + histogram = Services.telemetry.getKeyedHistogramById("SEARCH_COUNTS"); + let snapshot = histogram.snapshot(); + Assert.ok(histogramKey in snapshot, "histogram with key should be recorded"); + Assert.equal(snapshot[histogramKey].sum, histogramCount + 1, + "histogram sum should be incremented"); + + // FHR + let newFHRCount = yield getNumberOfSearchesInFHR(engine.name, "urlbar"); + Assert.equal(newFHRCount, fhrCount + 1, "should be recorded in FHR"); +} + +/** + * Returns the "action" object at the given index in the urlbar results: + * { type, params: {}} + * + * @param index The index in the urlbar results. + * @return An action object, or null if index >= number of results. + */ +function getActionAtIndex(index) { + let controller = gURLBar.popup.input.controller; + if (controller.matchCount <= index) { + return null; + } + let url = controller.getValueAt(index); + let mozActionMatch = url.match(/^moz-action:([^,]+),(.*)$/); + if (!mozActionMatch) { + let msg = "result at index " + index + " is not a moz-action: " + url; + Assert.ok(false, msg); + throw new Error(msg); + } + let [, type, paramStr] = mozActionMatch; + return { + type: type, + params: JSON.parse(paramStr), + }; +} + +/** + * Returns the index of the first search suggestion in the urlbar results. + * + * @return An index, or -1 if there are no search suggestions. + */ +function getFirstSuggestionIndex() { + let controller = gURLBar.popup.input.controller; + let matchCount = controller.matchCount; + for (let i = 0; i < matchCount; i++) { + let url = controller.getValueAt(i); + let mozActionMatch = url.match(/^moz-action:([^,]+),(.*)$/); + if (mozActionMatch) { + let [, type, paramStr] = mozActionMatch; + let params = JSON.parse(paramStr); + if (type == "searchengine" && "searchSuggestion" in params) { + return i; + } + } + } + return -1; +} diff --git a/browser/base/content/test/general/head.js b/browser/base/content/test/general/head.js index 2f20399e19a..4e5f98f5a07 100644 --- a/browser/base/content/test/general/head.js +++ b/browser/base/content/test/general/head.js @@ -1202,3 +1202,61 @@ function promiseCrashReport(expectedExtra) { } }); } + +/** + * Retrieves the number of searches recorded in FHR for the current day. + * + * @param aEngineName + * name of the setup search engine. + * @param aSource + * The FHR "source" name for the search, like "abouthome" or "urlbar". + * + * @return {Promise} Returns a promise resolving to the number of searches. + */ +function getNumberOfSearchesInFHR(aEngineName, aSource) { + let reporter = Components.classes["@mozilla.org/datareporting/service;1"] + .getService() + .wrappedJSObject + .healthReporter; + ok(reporter, "Health Reporter instance available."); + + return reporter.onInit().then(function onInit() { + let provider = reporter.getProvider("org.mozilla.searches"); + ok(provider, "Searches provider is available."); + + let m = provider.getMeasurement("counts", 3); + return m.getValues().then(data => { + let now = new Date(); + let yday = new Date(now); + yday.setDate(yday.getDate() - 1); + + // Add the number of searches recorded yesterday to the number of searches + // recorded today. This makes the test not fail intermittently when it is + // run at midnight and we accidentally compare the number of searches from + // different days. Tests are always run with an empty profile so there + // are no searches from yesterday, normally. Should the test happen to run + // past midnight we make sure to count them in as well. + return getNumberOfSearchesInFHRByDate(aEngineName, aSource, data, now) + + getNumberOfSearchesInFHRByDate(aEngineName, aSource, data, yday); + }); + }); +} + +/** + * Helper for getNumberOfSearchesInFHR. You probably don't want to call this + * directly. + */ +function getNumberOfSearchesInFHRByDate(aEngineName, aSource, aData, aDate) { + if (aData.days.hasDay(aDate)) { + let id = Services.search.getEngineByName(aEngineName).identifier; + + let day = aData.days.getDay(aDate); + let field = id + "." + aSource; + + if (day.has(field)) { + return day.get(field) || 0; + } + } + + return 0; // No records found. +} diff --git a/browser/base/content/urlbarBindings.xml b/browser/base/content/urlbarBindings.xml index e34c83ea13b..37eeaab95e9 100644 --- a/browser/base/content/urlbarBindings.xml +++ b/browser/base/content/urlbarBindings.xml @@ -361,13 +361,7 @@ file, You can obtain one at http://mozilla.org/MPL/2.0/. } else if (action.type == "keyword") { url = action.params.url; } else if (action.type == "searchengine") { - let engine = Services.search.getEngineByName(action.params.engineName); - let query = action.params.searchSuggestion || - action.params.searchQuery; - let submission = engine.getSubmission(query, null, "keyword"); - - url = submission.uri.spec; - postData = submission.postData; + [url, postData] = this._parseAndRecordSearchEngineAction(action); } else if (action.type == "visiturl") { url = action.params.url; } @@ -455,6 +449,19 @@ file, You can obtain one at http://mozilla.org/MPL/2.0/. ]]> + + + + + @@ -1455,12 +1462,8 @@ file, You can obtain one at http://mozilla.org/MPL/2.0/. break; } case "searchengine": { - let engine = Services.search.getEngineByName(action.params.engineName); - let query = action.params.searchSuggestion || - action.params.searchQuery; - let submission = engine.getSubmission(query, null, "keyword"); - url = submission.uri.spec; - options.postData = submission.postData; + [url, options.postData] = + this._parseAndRecordSearchEngineAction(action); break; } default: { diff --git a/browser/components/nsBrowserGlue.js b/browser/components/nsBrowserGlue.js index e4bd610874d..e8faeeedbc6 100644 --- a/browser/components/nsBrowserGlue.js +++ b/browser/components/nsBrowserGlue.js @@ -422,39 +422,16 @@ BrowserGlue.prototype = { this._dispose(); break; case "keyword-search": - // This is very similar to code in - // browser.js:BrowserSearch.recordSearchInHealthReport(). The code could - // be consolidated if there is will. We need the observer in - // nsBrowserGlue to prevent double counting. - let win = RecentWindow.getMostRecentBrowserWindow(); - BrowserUITelemetry.countSearchEvent("urlbar", win.gURLBar.value); - + // This notification is broadcast by the docshell when it "fixes up" a + // URI that it's been asked to load into a keyword search. let engine = null; try { engine = subject.QueryInterface(Ci.nsISearchEngine); } catch (ex) { Cu.reportError(ex); } - - win.BrowserSearch.recordSearchInTelemetry(engine, "urlbar"); -#ifdef MOZ_SERVICES_HEALTHREPORT - let reporter = Cc["@mozilla.org/datareporting/service;1"] - .getService() - .wrappedJSObject - .healthReporter; - - if (!reporter) { - return; - } - - reporter.onInit().then(function record() { - try { - reporter.getProvider("org.mozilla.searches").recordSearch(engine, "urlbar"); - } catch (ex) { - Cu.reportError(ex); - } - }); -#endif + let win = RecentWindow.getMostRecentBrowserWindow(); + win.BrowserSearch.recordSearchInHealthReport(engine, "urlbar"); break; case "browser-search-engine-modified": // Ensure we cleanup the hiddenOneOffs pref when removing