Bug 925521 - Part 1: remove filter on recorded search engine identifiers. r=gps, a=bajaj
authorRichard Newman <rnewman@mozilla.com>
Fri, 18 Oct 2013 12:27:57 -0700
changeset 167347 aa8e43f9ec74dcdb38df9c3e0e0d87929068917c
parent 167346 7b2d872b4496f15bc9cbd8a526691dc5c7fd663c
child 167348 d2a2cfcd70fed195efce12318ba3da44dba00e7c
push id428
push userbbajaj@mozilla.com
push dateTue, 28 Jan 2014 00:16:25 +0000
treeherdermozilla-release@cd72a7ff3a75 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersgps, bajaj
bugs925521
milestone27.0a2
Bug 925521 - Part 1: remove filter on recorded search engine identifiers. r=gps, a=bajaj
services/healthreport/providers.jsm
services/healthreport/tests/xpcshell/test_provider_searches.js
--- a/services/healthreport/providers.jsm
+++ b/services/healthreport/providers.jsm
@@ -1181,81 +1181,30 @@ SearchCountMeasurement1.prototype = Obje
  * We want to record granular details for individual locale-specific search
  * providers, but only if they're Mozilla partners. In order to do this, we
  * track the nsISearchEngine identifier, which denotes shipped search engines,
  * and intersect those with our partner list.
  *
  * We don't use the search engine name directly, because it is shared across
  * locales; e.g., eBay-de and eBay both share the name "eBay".
  */
-function SearchCountMeasurement2() {
-  this._fieldSpecs = null;
-  this._interestingEngines = null;   // Name -> ID. ("Amazon.com" -> "amazondotcom")
-
+function SearchCountMeasurementBase() {
+  this._fieldSpecs = {};
   Metrics.Measurement.call(this);
 }
 
-SearchCountMeasurement2.prototype = Object.freeze({
+SearchCountMeasurementBase.prototype = Object.freeze({
   __proto__: Metrics.Measurement.prototype,
 
-  name: "counts",
-  version: 2,
 
-  /**
-   * Default implementation; can be overridden by test helpers.
-   */
-  getDefaultEngines: function () {
-    return Services.search.getDefaultEngines();
-  },
-
-  _initialize: function () {
-    // Don't create all of these for every profile.
-    // There are 61 partner engines, translating to 244 fields.
-    // Instead, compute only those that are possible -- those for whom the
-    // provider is one of the default search engines.
-    // This set can grow over time, and change as users run different localized
-    // Firefox instances.
-    this._fieldSpecs = {};
-    this._interestingEngines = {};
-
-    for (let source of this.SOURCES) {
-      this._fieldSpecs["other." + source] = DAILY_COUNTER_FIELD;
-    }
-
-    let engines = this.getDefaultEngines();
-    for (let engine of engines) {
-      let id = engine.identifier;
-      if (!id || (this.PROVIDERS.indexOf(id) == -1)) {
-        continue;
-      }
-
-      this._interestingEngines[engine.name] = id;
-      let fieldPrefix = id + ".";
-      for (let source of this.SOURCES) {
-        this._fieldSpecs[fieldPrefix + source] = DAILY_COUNTER_FIELD;
-      }
-    }
-  },
-
-  // Our fields are dynamic, so we compute them into _fieldSpecs by looking at
-  // the current set of interesting engines.
+  // Our fields are dynamic.
   get fields() {
-    if (!this._fieldSpecs) {
-      this._initialize();
-    }
     return this._fieldSpecs;
   },
 
-  get interestingEngines() {
-    if (!this._fieldSpecs) {
-      this._initialize();
-    }
-    return this._interestingEngines;
-  },
-
   /**
    * Override the default behavior: serializers should include every counter
    * field from the DB, even if we don't currently have it registered.
    *
    * Do this so we don't have to register several hundred fields to match
    * various Firefox locales.
    *
    * We use the "provider.type" syntax as a rudimentary check for validity.
@@ -1275,130 +1224,89 @@ SearchCountMeasurement2.prototype = Obje
     if (name in this.fields) {
       return this.fields[name].type;
     }
 
     // Default to a counter.
     return Metrics.Storage.FIELD_DAILY_COUNTER;
   },
 
-  // You can compute the total list of fields by unifying the entire l10n repo
-  // set with the list of partners:
-  //
-  //   sort -u */*/searchplugins/list.txt | tr -d '^M' | uniq | grep -f partners.txt
-  //
-  // where partners.txt contains
-  //
-  //   amazon
-  //   aol
-  //   bing
-  //   eBay
-  //   google
-  //   mailru
-  //   mercadolibre
-  //   seznam
-  //   twitter
-  //   yahoo
-  //   yandex
-  //
-  // Please update this list as the set of partners changes.
-  //
-  PROVIDERS: [
-    "amazon-co-uk",
-    "amazon-de",
-    "amazon-en-GB",
-    "amazon-france",
-    "amazon-it",
-    "amazon-jp",
-    "amazondotcn",
-    "amazondotcom",
-    "amazondotcom-de",
-
-    "aol-en-GB",
-    "aol-web-search",
-
-    "bing",
-
-    "eBay",
-    "eBay-de",
-    "eBay-en-GB",
-    "eBay-es",
-    "eBay-fi",
-    "eBay-france",
-    "eBay-hu",
-    "eBay-in",
-    "eBay-it",
-
-    "google",
-    "google-jp",
-    "google-ku",
-    "google-maps-zh-TW",
-
-    "mailru",
-
-    "mercadolibre-ar",
-    "mercadolibre-cl",
-    "mercadolibre-mx",
-
-    "seznam-cz",
-
-    "twitter",
-    "twitter-de",
-    "twitter-ja",
-
-    "yahoo",
-    "yahoo-NO",
-    "yahoo-answer-zh-TW",
-    "yahoo-ar",
-    "yahoo-bid-zh-TW",
-    "yahoo-br",
-    "yahoo-ch",
-    "yahoo-cl",
-    "yahoo-de",
-    "yahoo-en-GB",
-    "yahoo-es",
-    "yahoo-fi",
-    "yahoo-france",
-    "yahoo-fy-NL",
-    "yahoo-id",
-    "yahoo-in",
-    "yahoo-it",
-    "yahoo-jp",
-    "yahoo-jp-auctions",
-    "yahoo-mx",
-    "yahoo-sv-SE",
-    "yahoo-zh-TW",
-
-    "yandex",
-    "yandex-ru",
-    "yandex-slovari",
-    "yandex-tr",
-    "yandex.by",
-    "yandex.ru-be",
-  ],
-
   SOURCES: [
     "abouthome",
     "contextmenu",
     "searchbar",
     "urlbar",
   ],
 });
 
+function SearchCountMeasurement2() {
+  SearchCountMeasurementBase.call(this);
+}
+
+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 (engineName) {
+    if (!this.nameMappings) {
+      this._initialize();
+    }
+    return this.nameMappings[engineName] || "other-" + engineName;
+  },
+});
+
 this.SearchesProvider = function () {
   Metrics.Provider.call(this);
 };
 
 this.SearchesProvider.prototype = Object.freeze({
   __proto__: Metrics.Provider.prototype,
 
   name: "org.mozilla.searches",
   measurementTypes: [
     SearchCountMeasurement1,
     SearchCountMeasurement2,
+    SearchCountMeasurement3,
   ],
 
   /**
    * Initialize the search service before our measurements are touched.
    */
   preInit: function (storage) {
     // Initialize search service.
     let deferred = Promise.defer();
@@ -1408,36 +1316,50 @@ 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".
+   *        the search will be attributed to "other-$engine"; otherwise, its
+   *        identifier will be 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) {
-    let m = this.getMeasurement("counts", 2);
+    let m = this.getMeasurement("counts", 3);
 
     if (m.SOURCES.indexOf(source) == -1) {
       throw new Error("Unknown source for search: " + source);
     }
 
-    let id = m.interestingEngines[engine] || "other";
-    let field = id + "." + source;
-    return this.enqueueStorageOperation(function recordSearch() {
-      return m.incrementDailyCounter(field);
-    });
+    let field = m.getEngineID(engine) + "." + source;
+    if (this.storage.hasFieldFromMeasurement(m.id, field,
+                                             this.storage.FIELD_DAILY_COUNTER)) {
+      let fieldID = this.storage.fieldIDFromMeasurement(m.id, field);
+      return this.enqueueStorageOperation(function recordSearchKnownField() {
+        return this.storage.incrementDailyCounterFromFieldID(fieldID);
+      }.bind(this));
+    }
+
+    // Otherwise, we first need to create the field.
+    return this.enqueueStorageOperation(function recordFieldAndSearch() {
+      // This function has to return a promise.
+      return Task.spawn(function () {
+        let fieldID = yield this.storage.registerField(m.id, field,
+                                                       this.storage.FIELD_DAILY_COUNTER);
+        yield this.storage.incrementDailyCounterFromFieldID(fieldID);
+      }.bind(this));
+    }.bind(this));
   },
 });
 
 function HealthReportSubmissionMeasurement1() {
   Metrics.Measurement.call(this);
 }
 
 HealthReportSubmissionMeasurement1.prototype = Object.freeze({
--- a/services/healthreport/tests/xpcshell/test_provider_searches.js
+++ b/services/healthreport/tests/xpcshell/test_provider_searches.js
@@ -12,21 +12,22 @@ const DEFAULT_ENGINES = [
   {name: "Amazon.com",    identifier: "amazondotcom"},
   {name: "Bing",          identifier: "bing"},
   {name: "Google",        identifier: "google"},
   {name: "Yahoo",         identifier: "yahoo"},
   {name: "Foobar Search", identifier: "foobar"},
 ];
 
 function MockSearchCountMeasurement() {
-  bsp.SearchCountMeasurement2.call(this);
+  bsp.SearchCountMeasurement3.call(this);
 }
 MockSearchCountMeasurement.prototype = {
-  __proto__: bsp.SearchCountMeasurement2.prototype,
-  getDefaultEngines: function () {
+  __proto__: bsp.SearchCountMeasurement3.prototype,
+
+  getEngines: function () {
     return DEFAULT_ENGINES;
   },
 };
 
 function MockSearchesProvider() {
   SearchesProvider.call(this);
 }
 MockSearchesProvider.prototype = {
@@ -47,61 +48,76 @@ add_test(function test_constructor() {
 add_task(function test_record() {
   let storage = yield Metrics.Storage("record");
   let provider = new MockSearchesProvider();
 
   yield provider.init(storage);
 
   let now = new Date();
 
-  for (let engine of DEFAULT_ENGINES) {
+  // 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");
   }
 
   // Invalid sources should throw.
   let errored = false;
   try {
     yield provider.recordSearch(DEFAULT_ENGINES[0].name, "bad source");
   } catch (ex) {
     errored = true;
   } finally {
     do_check_true(errored);
   }
 
-  let m = provider.getMeasurement("counts", 2);
+  let m = provider.getMeasurement("counts", 3);
   let data = yield m.getValues();
   do_check_eq(data.days.size, 1);
   do_check_true(data.days.hasDay(now));
 
   let day = data.days.getDay(now);
   for (let engine of DEFAULT_ENGINES) {
     let identifier = engine.identifier;
-    if (identifier == "foobar") {
-      identifier = "other";
-    }
+    let expected = identifier != "yahoo";
 
     for (let source of ["abouthome", "contextmenu", "searchbar", "urlbar"]) {
       let field = identifier + "." + source;
-      do_check_true(day.has(field));
-      do_check_eq(day.get(field), 1);
+      if (expected) {
+        do_check_true(day.has(field));
+        do_check_eq(day.get(field), 1);
+      } else {
+        do_check_false(day.has(field));
+      }
     }
   }
 
+  // Also, check that our non-default engine contributed, with a computed
+  // identifier.
+  let identifier = "other-Not Default";
+  for (let source of ["abouthome", "contextmenu", "searchbar", "urlbar"]) {
+    let field = identifier + "." + source;
+    do_check_true(day.has(field));
+  }
+
   yield storage.close();
 });
 
 add_task(function test_includes_other_fields() {
   let storage = yield Metrics.Storage("includes_other_fields");
   let provider = new MockSearchesProvider();
 
   yield provider.init(storage);
-  let m = provider.getMeasurement("counts", 2);
+  let m = provider.getMeasurement("counts", 3);
 
   // Register a search against a provider that isn't live in this session.
   let id = yield m.storage.registerField(m.id, "test.searchbar",
                                          Metrics.Storage.FIELD_DAILY_COUNTER);
 
   let testField = "test.searchbar";
   let now = new Date();
   yield m.storage.incrementDailyCounterFromFieldID(id, now);