Bug 925521 - Part 2: correctly record identifiers for non-pre-installed engines. r=gps, a=bajaj
authorRichard Newman <rnewman@mozilla.com>
Fri, 18 Oct 2013 12:31:39 -0700
changeset 166396 d2a2cfcd70fed195efce12318ba3da44dba00e7c
parent 166395 aa8e43f9ec74dcdb38df9c3e0e0d87929068917c
child 166397 a018b84126922d020263fc8adeb418ad9cd70d7d
push id3066
push userakeybl@mozilla.com
push dateMon, 09 Dec 2013 19:58:46 +0000
treeherdermozilla-beta@a31a0dce83aa [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersgps, bajaj
bugs925521
milestone27.0a2
Bug 925521 - Part 2: correctly record identifiers for non-pre-installed engines. r=gps, a=bajaj
browser/base/content/browser.js
browser/components/nsBrowserGlue.js
browser/components/search/content/search.xml
browser/modules/AboutHome.jsm
services/healthreport/providers.jsm
services/healthreport/tests/xpcshell/test_provider_searches.js
--- a/browser/base/content/browser.js
+++ b/browser/base/content/browser.js
@@ -3076,18 +3076,17 @@ const BrowserSearch = {
   },
 
   /**
    * Helper to record a search with Firefox Health Report.
    *
    * 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.
    */
   recordSearchInHealthReport: function (engine, source) {
 #ifdef MOZ_SERVICES_HEALTHREPORT
     let reporter = Cc["@mozilla.org/datareporting/service;1"]
                      .getService()
--- a/browser/components/nsBrowserGlue.js
+++ b/browser/components/nsBrowserGlue.js
@@ -315,19 +315,18 @@ BrowserGlue.prototype = {
                          .healthReporter;
 
         if (!reporter) {
           return;
         }
 
         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);
           }
         });
         break;
 #endif
       case "browser-search-engine-modified":
         if (data != "engine-default" && data != "engine-current") {
--- a/browser/components/search/content/search.xml
+++ b/browser/components/search/content/search.xml
@@ -478,17 +478,17 @@
                 value : aData },
               { handleError : function(aError) {
                   Components.utils.reportError("Saving search to form history failed: " + aError.message);
               }});
           }
           
           // 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>
     </implementation>
 
     <handlers>
       <handler event="command"><![CDATA[
         const target = event.originalTarget;
--- a/browser/modules/AboutHome.jsm
+++ b/browser/modules/AboutHome.jsm
@@ -160,21 +160,22 @@ let AboutHome = {
       case "AboutHome:Search":
         let data;
         try {
           data = JSON.parse(aMessage.data.searchData);
         } catch(ex) {
           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;
     }
   },
 
   // Send all the chrome-privileged data needed by about:home. This
   // gets re-sent when the search engine changes.
   sendAboutHomeData: function(target) {
--- a/services/healthreport/providers.jsm
+++ b/services/healthreport/providers.jsm
@@ -1243,55 +1243,36 @@ function SearchCountMeasurement2() {
 
 SearchCountMeasurement2.prototype = Object.freeze({
   __proto__: SearchCountMeasurementBase.prototype,
   name: "counts",
   version: 2,
 });
 
 function SearchCountMeasurement3() {
-  this.nameMappings = null;
   SearchCountMeasurementBase.call(this);
 }
 
 SearchCountMeasurement3.prototype = Object.freeze({
   __proto__: SearchCountMeasurementBase.prototype,
   name: "counts",
   version: 3,
 
   getEngines: function () {
     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;
   },
 });
 
 this.SearchesProvider = function () {
   Metrics.Provider.call(this);
 };
 
 this.SearchesProvider.prototype = Object.freeze({
@@ -1315,19 +1296,17 @@ this.SearchesProvider.prototype = Object
     });
     return deferred.promise;
   },
 
   /**
    * 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.
    *
    * @return Promise<>
    *         The promise is resolved when the storage operation completes.
    */
   recordSearch: function (engine, source) {
--- a/services/healthreport/tests/xpcshell/test_provider_searches.js
+++ b/services/healthreport/tests/xpcshell/test_provider_searches.js
@@ -16,20 +16,16 @@ const DEFAULT_ENGINES = [
   {name: "Foobar Search", identifier: "foobar"},
 ];
 
 function MockSearchCountMeasurement() {
   bsp.SearchCountMeasurement3.call(this);
 }
 MockSearchCountMeasurement.prototype = {
   __proto__: bsp.SearchCountMeasurement3.prototype,
-
-  getEngines: function () {
-    return DEFAULT_ENGINES;
-  },
 };
 
 function MockSearchesProvider() {
   SearchesProvider.call(this);
 }
 MockSearchesProvider.prototype = {
   __proto__: SearchesProvider.prototype,
   measurementTypes: [MockSearchCountMeasurement],
@@ -54,26 +50,26 @@ add_task(function test_record() {
   let now = new Date();
 
   // Record searches for all but one of our defaults, and one engine that's
   // not a default.
   for (let engine of DEFAULT_ENGINES.concat([{name: "Not Default", identifier: "notdef"}])) {
     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 {
     do_check_true(errored);
   }
 
   let m = provider.getMeasurement("counts", 3);
   let data = yield m.getValues();
@@ -93,17 +89,17 @@ add_task(function test_record() {
       } else {
         do_check_false(day.has(field));
       }
     }
   }
 
   // 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));
   }
 
   yield storage.close();
 });