Bug 925521 - Part 2: correctly record identifiers for non-pre-installed engines. r=gps

* * *
Bug 925521 - Review comments.

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.

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);
           }

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);
         ]]></body>
       </method>

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;
     }

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.

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));