From 72e1ff6dbdaa6484e87008020954b92869cea666 Mon Sep 17 00:00:00 2001 From: Richard Newman Date: Fri, 18 Oct 2013 12:31:39 -0700 Subject: [PATCH] Bug 925521 - Part 2: correctly record identifiers for non-pre-installed engines. r=gps * * * Bug 925521 - Review comments. --- browser/base/content/browser.js | 3 +- browser/components/nsBrowserGlue.js | 5 ++- browser/components/search/content/search.xml | 2 +- browser/modules/AboutHome.jsm | 5 +-- services/healthreport/providers.jsm | 35 ++++--------------- .../tests/xpcshell/test_provider_searches.js | 16 ++++----- 6 files changed, 20 insertions(+), 46 deletions(-) diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js index 2e760b5a998..5f066f69026 100644 --- a/browser/base/content/browser.js +++ b/browser/base/content/browser.js @@ -3083,8 +3083,7 @@ const BrowserSearch = { * FHR records only search counts and nothing pertaining to the search itself. * * @param engine - * (string) The name of the engine used to perform the search. This - * is typically nsISearchEngine.name. + * (nsISearchEngine) The engine handling the search. * @param source * (string) Where the search originated from. See the FHR * SearchesProvider for allowed values. diff --git a/browser/components/nsBrowserGlue.js b/browser/components/nsBrowserGlue.js index efb2e60525c..ff030f5ce04 100644 --- a/browser/components/nsBrowserGlue.js +++ b/browser/components/nsBrowserGlue.js @@ -318,9 +318,8 @@ BrowserGlue.prototype = { reporter.onInit().then(function record() { try { - let name = subject.QueryInterface(Ci.nsISearchEngine).name; - reporter.getProvider("org.mozilla.searches").recordSearch(name, - "urlbar"); + let engine = subject.QueryInterface(Ci.nsISearchEngine); + reporter.getProvider("org.mozilla.searches").recordSearch(engine, "urlbar"); } catch (ex) { Cu.reportError(ex); } diff --git a/browser/components/search/content/search.xml b/browser/components/search/content/search.xml index ce3cdc15f17..58fd87e7d14 100644 --- a/browser/components/search/content/search.xml +++ b/browser/components/search/content/search.xml @@ -483,7 +483,7 @@ // null parameter below specifies HTML response for search var submission = this.currentEngine.getSubmission(aData, null, "searchbar"); - BrowserSearch.recordSearchInHealthReport(this.currentEngine.name, "searchbar"); + BrowserSearch.recordSearchInHealthReport(this.currentEngine, "searchbar"); openUILinkIn(submission.uri.spec, aWhere, null, submission.postData); ]]> diff --git a/browser/modules/AboutHome.jsm b/browser/modules/AboutHome.jsm index 98c443d11cd..4d357476d2d 100644 --- a/browser/modules/AboutHome.jsm +++ b/browser/modules/AboutHome.jsm @@ -165,11 +165,12 @@ let AboutHome = { Cu.reportError(ex); break; } + let engine = Services.search.currentEngine; #ifdef MOZ_SERVICES_HEALTHREPORT - window.BrowserSearch.recordSearchInHealthReport(data.engineName, "abouthome"); + window.BrowserSearch.recordSearchInHealthReport(engine, "abouthome"); #endif // Trigger a search through nsISearchEngine.getSubmission() - let submission = Services.search.currentEngine.getSubmission(data.searchTerms, null, "homepage"); + let submission = engine.getSubmission(data.searchTerms, null, "homepage"); window.loadURI(submission.uri.spec, null, submission.postData); break; } diff --git a/services/healthreport/providers.jsm b/services/healthreport/providers.jsm index 0b8075e0156..fdb6ae06f81 100644 --- a/services/healthreport/providers.jsm +++ b/services/healthreport/providers.jsm @@ -1248,7 +1248,6 @@ SearchCountMeasurement2.prototype = Object.freeze({ }); function SearchCountMeasurement3() { - this.nameMappings = null; SearchCountMeasurementBase.call(this); } @@ -1261,32 +1260,14 @@ SearchCountMeasurement3.prototype = Object.freeze({ return Services.search.getEngines(); }, - _initialize: function () { - this.nameMappings = {}; - let engines = this.getEngines(); - for (let engine of engines) { - let name = engine.name; - if (!name) { - // This is something we'd like to know, but we can't track it unless we - // rejig how recordSearchInHealthReport is implemented. - continue; - } - - let id = engine.identifier; - if (!id) { - continue; - } - - // TODO: again, we need to rejig this to avoid name collisions. - this.nameMappings[name] = id; + getEngineID: function (engine) { + if (!engine) { + return "other"; } - }, - - getEngineID: function (engineName) { - if (!this.nameMappings) { - this._initialize(); + if (engine.identifier) { + return engine.identifier; } - return this.nameMappings[engineName] || "other-" + engineName; + return "other-" + engine.name; }, }); @@ -1320,9 +1301,7 @@ this.SearchesProvider.prototype = Object.freeze({ * Record that a search occurred. * * @param engine - * (string) The search engine used. If the search engine is unknown, - * the search will be attributed to "other-$engine"; otherwise, its - * identifier will be used. + * (nsISearchEngine) The search engine used. * @param source * (string) Where the search was initiated from. Must be one of the * SearchCountMeasurement2.SOURCES values. diff --git a/services/healthreport/tests/xpcshell/test_provider_searches.js b/services/healthreport/tests/xpcshell/test_provider_searches.js index e90a2bb7e44..94ce7976085 100644 --- a/services/healthreport/tests/xpcshell/test_provider_searches.js +++ b/services/healthreport/tests/xpcshell/test_provider_searches.js @@ -21,10 +21,6 @@ function MockSearchCountMeasurement() { } MockSearchCountMeasurement.prototype = { __proto__: bsp.SearchCountMeasurement3.prototype, - - getEngines: function () { - return DEFAULT_ENGINES; - }, }; function MockSearchesProvider() { @@ -59,16 +55,16 @@ add_task(function test_record() { if (engine.identifier == "yahoo") { continue; } - yield provider.recordSearch(engine.name, "abouthome"); - yield provider.recordSearch(engine.name, "contextmenu"); - yield provider.recordSearch(engine.name, "searchbar"); - yield provider.recordSearch(engine.name, "urlbar"); + yield provider.recordSearch(engine, "abouthome"); + yield provider.recordSearch(engine, "contextmenu"); + yield provider.recordSearch(engine, "searchbar"); + yield provider.recordSearch(engine, "urlbar"); } // Invalid sources should throw. let errored = false; try { - yield provider.recordSearch(DEFAULT_ENGINES[0].name, "bad source"); + yield provider.recordSearch(DEFAULT_ENGINES[0], "bad source"); } catch (ex) { errored = true; } finally { @@ -98,7 +94,7 @@ add_task(function test_record() { // Also, check that our non-default engine contributed, with a computed // identifier. - let identifier = "other-Not Default"; + let identifier = "notdef"; for (let source of ["abouthome", "contextmenu", "searchbar", "urlbar"]) { let field = identifier + "." + source; do_check_true(day.has(field));