Backed out 2 changesets (bug 1628674, bug 1649881) for xpcshell failures at toolkit/modules/tests/xpcshell/test_Region_geocoding.js

Backed out changeset fa23e1b89874 (bug 1649881)
Backed out changeset 311823332e95 (bug 1628674)

toolkit/modules/Region.jsm

@@ -467,34 +467,21 @@ class RegionDetector {
   async _geoCode(location) {
     let plainMap = await this._getPlainMap();
     let polygons = this._getPolygonsContainingPoint(location, plainMap);
-    if (polygons.length == 1) {
-      log.info("Found in single exact region");
-      return polygons[0].properties.alpha2;
-    }
+    // The plain map doesnt have overlapping regions so return
+    // region straight away.
     if (polygons.length) {
-      log.info("Found in ", polygons.length, "overlapping exact regions");
-      return this._findFurthest(location, polygons);
+      return polygons[0].region;
     }
-
-    // We haven't found a match in the exact map, use the buffered map
-    // to see if the point is close to a region.
     let bufferedMap = await this._getBufferedMap();
     polygons = this._getPolygonsContainingPoint(location, bufferedMap);
-
+    // Only found one matching region, return.
     if (polygons.length === 1) {
-      log.info("Found in single buffered region");
-      return polygons[0].properties.alpha2;
+      return polygons[0].region;
     }
-
-    // Matched more than one region, which one of those regions
-    // is it closest to without the buffer.
-    if (polygons.length) {
-      log.info("Found in ", polygons.length, "overlapping buffered regions");
-      let regions = polygons.map(polygon => polygon.properties.alpha2);
-      let unBufferedRegions = plainMap.features.filter(feature =>
-        regions.includes(feature.properties.alpha2)
-      );
-      return this._findClosest(location, unBufferedRegions);
+    // Matched more than one region, find the longest distance
+    // from a border and return that region.
+    if (polygons.length > 1) {
+      return this._findLargestDistance(location, polygons);
     }
     return null;
   }
@@ -520,12 +507,18 @@ class RegionDetector {
       let coords = feature.geometry.coordinates;
       if (feature.geometry.type === "Polygon") {
         if (this._polygonInPoint(point, coords[0])) {
-          polygons.push(feature);
+          polygons.push({
+            coords: coords[0],
+            region: feature.properties.alpha2,
+          });
         }
       } else if (feature.geometry.type === "MultiPolygon") {
         for (const innerCoords of coords) {
           if (this._polygonInPoint(point, innerCoords[0])) {
-            polygons.push(feature);
+            polygons.push({
+              coords: innerCoords[0],
+              region: feature.properties.alpha2,
+            });
           }
         }
       }
@@ -534,74 +527,28 @@ class RegionDetector {
   }
 
   /**
-   * Find the largest distance between a point and any of the points that
-   * make up an array of regions.
+   * Find the largest distance between a point and and a border
+   * that contains it.
    *
    * @param {Object} location
    *   A lat + lng coordinate.
-   * @param {Array} regions
-   *   An array of GeoJSON region definitions.
+   * @param {Object} polygons
+   *   Array of polygons that define a border.
    *
    * @returns {String}
    *   A 2 character string representing a region.
    */
-  _findFurthest(location, regions) {
-    let max = { distance: 0, region: null };
-    this._traverse(regions, ({ lat, lng, region }) => {
+  _findLargestDistance(location, polygons) {
+    let maxDistance = { distance: 0, region: null };
+    for (const polygon of polygons) {
+      for (const [lng, lat] of polygon.coords) {
         let distance = this._distanceBetween(location, { lng, lat });
-      if (distance > max.distance) {
-        max = { distance, region };
-      }
-    });
-    return max.region;
-  }
-
-  /**
-   * Find the smallest distance between a point and any of the points that
-   * make up an array of regions.
-   *
-   * @param {Object} location
-   *   A lat + lng coordinate.
-   * @param {Array} regions
-   *   An array of GeoJSON region definitions.
-   *
-   * @returns {String}
-   *   A 2 character string representing a region.
-   */
-  _findClosest(location, regions) {
-    let min = { distance: Infinity, region: null };
-    this._traverse(regions, ({ lat, lng, region }) => {
-      let distance = this._distanceBetween(location, { lng, lat });
-      if (distance < min.distance) {
-        min = { distance, region };
-      }
-    });
-    return min.region;
-  }
-
-  /**
-   * Utility function to loop over all the coordinate points in an
-   * array of polygons and call a function on them.
-   *
-   * @param {Array} regions
-   *   An array of GeoJSON region definitions.
-   * @param {Function} fun
-   *   Function to call on individual coordinates.
-   */
-  _traverse(regions, fun) {
-    for (const region of regions) {
-      if (region.geometry.type === "Polygon") {
-        for (const [lng, lat] of region.geometry.coordinates[0]) {
-          fun({ lat, lng, region: region.properties.alpha2 });
-        }
-      } else if (region.geometry.type === "MultiPolygon") {
-        for (const innerCoords of region.geometry.coordinates) {
-          for (const [lng, lat] of innerCoords[0]) {
-            fun({ lat, lng, region: region.properties.alpha2 });
-          }
+        if (distance > maxDistance.distance) {
+          maxDistance = { distance, region: polygon.region };
         }
       }
     }
+    return maxDistance.region;
   }
 
   /**

toolkit/modules/docs/Region.rst

@@ -1,72 +0,0 @@
-.. _Region:
-
-======
-Region
-======
-
-Firefox monitors the users region in order to show relevant local
-search engines and content. The region is tracked in 2 properties:
-
- * ``Region.current`` - The most recent location we detected for the user.
- * ``Region.home`` - Where we consider the users home location.
-
-These are tracked separately as to avoid updating the users
-experience repeatedly as they travel for example. In general
-callers should use ``Region.home``.
-
-If the user is detected in a current region that is not there `home` region
-for a continuous period (current 2 weeks) then their `home` region
-will be updated.
-
-Testing
-=======
-
-To set the users region for testing you can use ``Region._setHomeRegion("US", false)``, the second parameter ``notify``
-will send a notification that the region has changed and trigger a
-reload of search engines and other content.
-
-Updating test_Region_geocoding.js data
---------------------------------------
-
-The test data used in this test is generated by running the MLS geocoding
-service locally:
-
-Follow the Ichnaea location development guide @ https://ichnaea.readthedocs.io/en/latest/local_dev.html.
-
-Make a list of test locations in a CSV format, for example:
-
-.. code-block:: shell
-
-  23.7818724,38.0531587
-  23.7728138,38.0572369
-  1.6780180,48.5973431
-  1.7034801,48.5979913
-  1.6978640,48.5919751
-
-You can use the MLS raw data files to get a large sample @ https://location.services.mozilla.com/downloads
-
-Save a script to run the geocoding in ``ichnaea/ichnaea``
-
-.. code-block:: python
-
-  import geocode
-  geocoder = geocode.Geocoder()
-
-  f = open("mls.csv", "r")
-  r = open("mls-lookup-results.csv", "a")
-
-  for x in f:
-    [lat, long] = x.strip().split(",")
-    region = geocoder.region(lat, long)
-    r.write("%s\n" % region)
-
-Run the script
-
-.. code-block:: shell
-
-  $ make shell
-  $ cd ichnaea
-  $ python run.py
-
-If you want to commit the new test data ~5000 seems to be a reasonable
-number of data points to test before running into issues with the test length.

toolkit/modules/docs/index.rst

@@ -9,4 +9,3 @@ The ``/toolkit/modules`` directory contains a number of self-contained toolkit m
 
   AsyncShutdown
   FirstStartup
-  Region

toolkit/modules/tests/xpcshell/test_Region_geocoding.js

@@ -23,7 +23,6 @@ async function stubMap(path, fun) {
 }
 
 add_task(async function test_setup() {
-  Services.prefs.setBoolPref("browser.region.log", true);
   await stubMap("regions/world.geojson", "_getPlainMap");
   await stubMap("regions/world-buffered.geojson", "_getBufferedMap");
 });
@@ -40,14 +39,7 @@ const LOCATIONS = [
   { lat: 35.4411368, lng: -41.5372973, expectedRegion: null },
 ];
 
-add_task(async function test_mls_results() {
-  setLocation({ lat: -5.066019, lng: 39.1026251 });
-  let expectedRegion = "TZ";
-  let region = await Region._getRegionLocally();
-  Assert.equal(region, expectedRegion);
-});
-
-/*add_task(async function test_basic() {
+add_task(async function test_basic() {
   for (const { lat, lng, expectedRegion } of LOCATIONS) {
     setLocation({ lat, lng });
     let region = await Region._getRegionLocally();
@@ -57,18 +49,4 @@ add_task(async function test_mls_results() {
       `Got the expected region at ${lat},${lng}`
     );
   }
-});*/
-
-add_task(async function test_mls_results() {
-  let data = await readFile(do_get_file("regions/mls-lookup-results.csv"));
-  for (const row of data.split("\n")) {
-    let [lat, lng, expectedRegion] = row.split(",");
-    setLocation({ lng: parseFloat(lng), lat: parseFloat(lat) });
-    let region = await Region._getRegionLocally();
-    Assert.equal(
-      region,
-      expectedRegion,
-      `Expected ${expectedRegion} at ${lat},${lng} got ${region}`
-    );
-  }
 });

toolkit/modules/tests/xpcshell/xpcshell.ini

@@ -7,7 +7,6 @@ support-files =
   chromeappsstore.sqlite
   corrupt.sqlite
   zips/zen.zip
-  regions/mls-lookup-results.csv
   regions/world.geojson
   regions/world-buffered.geojson