commit a3c91c5ecb3c912b503aa5b968883e276f62e124
parent 1cd52458c1354d28d6562045e64e2701e0f66848
Author: Serban Stanca <sstanca@mozilla.com>
Date: Fri, 31 Oct 2025 05:32:54 +0200
Revert "Bug 1995894: Use geolocation in GeolocationUtils when detecting the local location r=home-newtab-reviewers,adw,reemhamz" for causing Desktop newtab trainshop tests failures.
This reverts commit 4fc528d3633d21e60c17cffdf95fae49a5a49ac2.
Diffstat:
7 files changed, 98 insertions(+), 353 deletions(-)
diff --git a/browser/components/urlbar/tests/engagementTelemetry/browser/browser_glean_telemetry_engagement_selected_result.js b/browser/components/urlbar/tests/engagementTelemetry/browser/browser_glean_telemetry_engagement_selected_result.js
@@ -487,9 +487,7 @@ add_task(async function selected_result_weather() {
const cleanupQuickSuggest = await ensureQuickSuggestInit();
await MerinoTestUtils.initWeather();
- const cleanupGeolocation = GeolocationTestUtils.stubGeolocation(
- GeolocationTestUtils.SAN_FRANCISCO
- );
+ const cleanupGeolocation = GeolocationTestUtils.stubGeolocation();
await doTest(async () => {
let provider = "UrlbarProviderQuickSuggest";
diff --git a/browser/components/urlbar/tests/quicksuggest/GeolocationTestUtils.sys.mjs b/browser/components/urlbar/tests/quicksuggest/GeolocationTestUtils.sys.mjs
@@ -13,14 +13,6 @@ const lazy = XPCOMUtils.declareLazy({
*
*/
class _GeolocationTestUtils {
- get SAN_FRANCISCO() {
- return {
- country_code: "US",
- city: "San Francisco",
- region_code: "CA",
- };
- }
-
/**
* Initializes the utils.
*
@@ -40,14 +32,24 @@ class _GeolocationTestUtils {
/**
* Setup stub for GeolocationUtils.geolocation() using given geolocation.
+ * If the geolocation parameter is null, set "San Francisco" geolocation.
+ * NOTE: This returns function to restore the dummy function.
*
- * @param {object} geolocation
+ * @param {object} [geolocation]
* @param {string} [geolocation.country_code]
* @param {string} [geolocation.city]
* @param {string} [geolocation.region_code]
* @returns {Function} function to restore the stub.
*/
stubGeolocation(geolocation) {
+ if (!geolocation) {
+ geolocation = {
+ country_code: "US",
+ city: "San Francisco",
+ region_code: "CA",
+ };
+ }
+
let sandbox = lazy.sinon.createSandbox();
sandbox.stub(lazy.GeolocationUtils, "geolocation").resolves(geolocation);
diff --git a/browser/components/urlbar/tests/quicksuggest/browser/browser_weather.js b/browser/components/urlbar/tests/quicksuggest/browser/browser_weather.js
@@ -23,7 +23,7 @@ add_setup(async function () {
],
});
await MerinoTestUtils.initWeather();
- GeolocationTestUtils.stubGeolocation(GeolocationTestUtils.SAN_FRANCISCO);
+ GeolocationTestUtils.stubGeolocation();
});
// Does a search, clicks the "Show less frequently" result menu command, and
diff --git a/browser/components/urlbar/tests/quicksuggest/unit/test_weather.js b/browser/components/urlbar/tests/quicksuggest/unit/test_weather.js
@@ -71,9 +71,7 @@ add_task(async function disableAndEnable() {
});
async function doBasicDisableAndEnableTest(pref) {
- let cleanup = GeolocationTestUtils.stubGeolocation(
- GeolocationTestUtils.SAN_FRANCISCO
- );
+ let cleanup = GeolocationTestUtils.stubGeolocation();
// Disable the feature. It should be immediately uninitialized.
UrlbarPrefs.set(pref, false);
@@ -128,9 +126,7 @@ add_task(async function noSuggestion() {
// When the Merino response doesn't include a `region_code` for the geolocated
// version of the suggestion, the suggestion title should only contain a city.
add_task(async function geolocationSuggestionNoRegion() {
- let cleanup = GeolocationTestUtils.stubGeolocation(
- GeolocationTestUtils.SAN_FRANCISCO
- );
+ let cleanup = GeolocationTestUtils.stubGeolocation();
let { suggestions } = MerinoTestUtils.server.response.body;
let s = { ...MerinoTestUtils.WEATHER_SUGGESTION };
@@ -163,9 +159,7 @@ add_task(async function geolocationSuggestionNoRegion() {
// the suggestion's URL, the suggestion should be shown and the history visit
// should not be shown.
add_task(async function urlAlreadyInHistory() {
- let cleanup = GeolocationTestUtils.stubGeolocation(
- GeolocationTestUtils.SAN_FRANCISCO
- );
+ let cleanup = GeolocationTestUtils.stubGeolocation();
// A visit to the weather suggestion's exact URL.
let suggestionVisit = {
@@ -319,9 +313,7 @@ async function doLocaleTest({ shouldRunTask, osUnit, unitsByLocale }) {
// enabled rather than being set according to region/locale.
skipSuggestReset: true,
callback: async () => {
- let cleanup = GeolocationTestUtils.stubGeolocation(
- GeolocationTestUtils.SAN_FRANCISCO
- );
+ let cleanup = GeolocationTestUtils.stubGeolocation();
info("Checking locale: " + locale);
await check_results({
@@ -497,9 +489,7 @@ async function doRegionTest({ homeRegion, locale, query, expectedTitleL10n }) {
// Tests dismissal.
add_task(async function dismissal() {
- let cleanup = GeolocationTestUtils.stubGeolocation(
- GeolocationTestUtils.SAN_FRANCISCO
- );
+ let cleanup = GeolocationTestUtils.stubGeolocation();
await doDismissAllTest({
result: QuickSuggestTestUtils.weatherResult(),
@@ -519,9 +509,7 @@ add_task(async function dismissal() {
// When a Nimbus experiment is installed, it should override the remote settings
// weather record.
add_task(async function nimbusOverride() {
- let cleanup = GeolocationTestUtils.stubGeolocation(
- GeolocationTestUtils.SAN_FRANCISCO
- );
+ let cleanup = GeolocationTestUtils.stubGeolocation();
let defaultResult = QuickSuggestTestUtils.weatherResult();
// Verify a search works as expected with the default remote settings weather
@@ -840,9 +828,7 @@ add_task(async function cityRegionQueries() {
// Tests weather queries that don't include a city.
add_task(async function noCityQuery() {
- let cleanup = GeolocationTestUtils.stubGeolocation(
- GeolocationTestUtils.SAN_FRANCISCO
- );
+ let cleanup = GeolocationTestUtils.stubGeolocation();
await doCityTest({
desc: "No city in query, so only one call to Merino should be made and Merino does the geolocation internally",
diff --git a/browser/components/urlbar/tests/quicksuggest/unit/test_weather_keywords.js b/browser/components/urlbar/tests/quicksuggest/unit/test_weather_keywords.js
@@ -391,9 +391,7 @@ async function doKeywordsTest({
info("Doing keywords test: " + desc);
info(JSON.stringify({ nimbusValues, settingsData, minKeywordLength }));
- let cleanup = GeolocationTestUtils.stubGeolocation(
- GeolocationTestUtils.SAN_FRANCISCO
- );
+ let cleanup = GeolocationTestUtils.stubGeolocation();
let nimbusCleanup;
if (nimbusValues) {
@@ -567,9 +565,7 @@ async function doShowLessFrequentlyTest({
info("Doing increment test: " + desc);
info(JSON.stringify({ weather, configuration, nimbusValues }));
- let cleanup = GeolocationTestUtils.stubGeolocation(
- GeolocationTestUtils.SAN_FRANCISCO
- );
+ let cleanup = GeolocationTestUtils.stubGeolocation();
let nimbusCleanup;
if (nimbusValues) {
diff --git a/browser/extensions/newtab/lib/WeatherFeed.sys.mjs b/browser/extensions/newtab/lib/WeatherFeed.sys.mjs
@@ -8,8 +8,6 @@ const lazy = {};
ChromeUtils.defineESModuleGetters(lazy, {
clearTimeout: "resource://gre/modules/Timer.sys.mjs",
setTimeout: "resource://gre/modules/Timer.sys.mjs",
- GeolocationUtils:
- "moz-src:///browser/components/urlbar/private/GeolocationUtils.sys.mjs",
PersistentCache: "resource://newtab/lib/PersistentCache.sys.mjs",
Region: "resource://gre/modules/Region.sys.mjs",
});
@@ -95,6 +93,37 @@ export class WeatherFeed {
this.fetchTimer = 0;
}
+ /**
+ * This thin wrapper around the fetch call makes it easier for us to write
+ * automated tests that simulate responses.
+ */
+ async fetchHelper(retries = 3, queryOverride = null) {
+ this.restartFetchTimer();
+ const weatherQuery = this.store.getState().Prefs.values[PREF_WEATHER_QUERY];
+ let suggestions = [];
+ let retry = 0;
+ const query = queryOverride ?? weatherQuery ?? "";
+ while (retry++ < retries && suggestions.length === 0) {
+ try {
+ suggestions = await this.merino.fetch({
+ query,
+ providers: MERINO_PROVIDER,
+ timeoutMs: 7000,
+ otherParams: {
+ request_type: "weather",
+ source: "newtab",
+ },
+ });
+ } catch (error) {
+ // We don't need to do anything with this right now.
+ }
+ }
+
+ // results from the API or empty array if null
+ this.suggestions = suggestions ?? [];
+ return this.suggestions;
+ }
+
async fetch() {
// Keep a handle on the `MerinoClient` instance that exists at the start of
// this fetch. If fetching stops or this `Weather` instance is uninitialized
@@ -104,7 +133,7 @@ export class WeatherFeed {
this.merino = await this.MerinoClient(MERINO_CLIENT_KEY);
}
- this.suggestions = await this._fetchHelper();
+ await this.fetchHelper();
if (this.suggestions.length) {
const hasLocationData =
@@ -198,6 +227,41 @@ export class WeatherFeed {
}
}
+ async fetchLocationByIP() {
+ if (!this.merino) {
+ this.merino = await this.MerinoClient(MERINO_CLIENT_KEY);
+ }
+
+ // First we fetch the forecast through user's IP Address
+ // which is done by not adding in a query parameter, but keeping the "weather" request_type.
+ // This method is mentioned in the AccuWeather docs:
+ // https://apidev.accuweather.com/developers/locationsAPIguide#IPAddress
+ try {
+ const ipLocation = await this.fetchHelper(3, "");
+
+ const ipData = ipLocation?.[0];
+
+ // Second, we use the city name that came from the IP look up to get the normalized merino response
+ // For context, the IP lookup response does not have the complete response data we need
+ const locationData = await this.merino.fetch({
+ query: ipData.city_name,
+ providers: MERINO_PROVIDER,
+ timeoutMs: 7000,
+ otherParams: {
+ request_type: "location",
+ source: "newtab",
+ },
+ });
+
+ const response = locationData?.[0]?.locations?.[0];
+ return response;
+ // return response
+ } catch (err) {
+ console.error("WeatherFeed failed to look up IP");
+ return null;
+ }
+ }
+
async onPrefChangedAction(action) {
switch (action.data.name) {
case PREF_WEATHER_QUERY:
@@ -270,10 +334,11 @@ export class WeatherFeed {
this.store.dispatch(ac.SetPref("weather.optInAccepted", true));
this.store.dispatch(ac.SetPref("weather.optInDisplayed", false));
- const detectedLocation = await this._fetchNormalizedLocation();
+ const detectedLocation = await this.fetchLocationByIP();
if (detectedLocation) {
// Build the payload exactly like manual search does
+
this.store.dispatch(
ac.BroadcastToContent({
type: at.WEATHER_LOCATION_DATA_UPDATE,
@@ -296,95 +361,6 @@ export class WeatherFeed {
}
}
}
-
- /**
- * This thin wrapper around the fetch call makes it easier for us to write
- * automated tests that simulate responses.
- */
- async _fetchHelper(retries = 3, queryOverride = null) {
- this.restartFetchTimer();
-
- const weatherQuery = this.store.getState().Prefs.values[PREF_WEATHER_QUERY];
- const query = queryOverride ?? weatherQuery ?? "";
- const otherParams = {
- request_type: "weather",
- source: "newtab",
- };
-
- if (!query) {
- let geolocation = await lazy.GeolocationUtils.geolocation();
- if (!geolocation) {
- return [];
- }
-
- if (geolocation.country_code) {
- otherParams.country = geolocation.country_code;
- }
- let region = geolocation.region_code || geolocation.region;
- if (region) {
- otherParams.region = region;
- }
- let city = geolocation.city || geolocation.region;
- if (city) {
- otherParams.city = city;
- }
- }
-
- let suggestions;
- let retry = 0;
-
- while (retry++ < retries && !suggestions?.length) {
- try {
- suggestions = await this.merino.fetch({
- query,
- providers: MERINO_PROVIDER,
- timeoutMs: 7000,
- otherParams,
- });
- } catch (error) {
- // We don't need to do anything with this right now.
- }
- }
-
- // results from the API or empty array if null
- return suggestions ?? [];
- }
-
- async _fetchNormalizedLocation() {
- const geolocation = await lazy.GeolocationUtils.geolocation();
- if (!geolocation) {
- return null;
- }
-
- // "region" might be able to be city if geolocation.city is null
- const city = geolocation.city || geolocation.region;
- if (!city) {
- return null;
- }
-
- if (!this.merino) {
- this.merino = await this.MerinoClient(MERINO_CLIENT_KEY);
- }
-
- try {
- // We use the given city name look up to get the normalized merino response
- const locationData = await this.merino.fetch({
- query: city,
- providers: MERINO_PROVIDER,
- timeoutMs: 7000,
- otherParams: {
- request_type: "location",
- source: "newtab",
- },
- });
-
- const response = locationData?.[0]?.locations?.[0];
- return response;
- } catch (err) {
- console.error("WeatherFeed failed to get normalized location");
- return null;
- }
- }
}
/**
@@ -392,10 +368,7 @@ export class WeatherFeed {
* This makes it easier for us to write automated tests that simulate responses.
*/
WeatherFeed.prototype.MerinoClient = (...args) => {
- return new lazy.MerinoClient({
- allowOhttp: true,
- ...args,
- });
+ return new lazy.MerinoClient(...args);
};
WeatherFeed.prototype.PersistentCache = (...args) => {
return new lazy.PersistentCache(...args);
diff --git a/browser/extensions/newtab/test/xpcshell/test_WeatherFeed.js b/browser/extensions/newtab/test/xpcshell/test_WeatherFeed.js
@@ -7,15 +7,12 @@ ChromeUtils.defineESModuleGetters(this, {
actionCreators: "resource://newtab/common/Actions.mjs",
actionTypes: "resource://newtab/common/Actions.mjs",
sinon: "resource://testing-common/Sinon.sys.mjs",
- GeolocationTestUtils:
- "resource://testing-common/GeolocationTestUtils.sys.mjs",
MerinoTestUtils: "resource://testing-common/MerinoTestUtils.sys.mjs",
WeatherFeed: "resource://newtab/lib/WeatherFeed.sys.mjs",
Region: "resource://gre/modules/Region.sys.mjs",
});
const { WEATHER_SUGGESTION } = MerinoTestUtils;
-GeolocationTestUtils.init(this);
const WEATHER_ENABLED = "browser.newtabpage.activity-stream.showWeather";
const SYS_WEATHER_ENABLED =
@@ -133,7 +130,8 @@ add_task(async function test_onAction_INIT() {
sandbox.stub(feed, "isEnabled").returns(true);
- sandbox.stub(feed, "_fetchHelper").resolves([WEATHER_SUGGESTION]);
+ sandbox.stub(feed, "fetchHelper");
+ feed.suggestions = [WEATHER_SUGGESTION];
feed.locationData = locationData;
feed.store = {
dispatch: sinon.spy(),
@@ -190,8 +188,8 @@ add_task(async function test_onAction_opt_in_location_success() {
},
};
- // Stub _fetchNormalizedLocation() to simulate a successful lookup
- sandbox.stub(feed, "_fetchNormalizedLocation").resolves({
+ // Stub fetchLocationByIP() to simulate a successful lookup
+ sandbox.stub(feed, "fetchLocationByIP").resolves({
localized_name: "Testville",
administrative_area: "Paris",
country: "FR",
@@ -254,8 +252,8 @@ add_task(async function test_onAction_opt_in_no_location_found() {
},
};
- // Test that _fetchNormalizedLocation doesn't return a location
- sandbox.stub(feed, "_fetchNormalizedLocation").resolves(null);
+ // Test that fetchLocationByIP doesn't return a location
+ sandbox.stub(feed, "fetchLocationByIP").resolves(null);
await feed.onAction({ type: actionTypes.WEATHER_USER_OPT_IN_LOCATION });
@@ -289,211 +287,3 @@ add_task(async function test_onAction_opt_in_no_location_found() {
sandbox.restore();
});
-
-// Test fetching weather information using GeolocationUtils.geolocation()
-add_task(async function test_fetch_weather_with_geolocation() {
- const TEST_DATA = [
- {
- geolocation: {
- country_code: "US",
- region_code: "CA",
- region: "Califolnia",
- city: "San Francisco",
- },
- expected: {
- country: "US",
- region: "CA",
- city: "San Francisco",
- },
- },
- {
- geolocation: {
- country_code: "JP",
- region_code: "14",
- region: "Kanagawa",
- city: "",
- },
- expected: {
- country: "JP",
- region: "14",
- city: "Kanagawa",
- },
- },
- {
- geolocation: {
- country_code: "TestCountry",
- region_code: "",
- region: "TestRegion",
- city: "TestCity",
- },
- expected: {
- country: "TestCountry",
- region: "TestRegion",
- city: "TestCity",
- },
- },
- {
- geolocation: {
- country_code: "TestCountry",
- },
- expected: {
- country: "TestCountry",
- },
- },
- {
- geolocation: {
- region_code: "TestRegionCode",
- },
- expected: {
- region: "TestRegionCode",
- },
- },
- {
- geolocation: {
- region: "TestRegion",
- },
- expected: {
- region: "TestRegion",
- city: "TestRegion",
- },
- },
- {
- geolocation: {
- city: "TestCity",
- },
- expected: {
- city: "TestCity",
- },
- },
- {
- geolocation: {},
- expected: {},
- },
- {
- geolocation: null,
- expected: false,
- },
- ];
-
- for (let { geolocation, expected } of TEST_DATA) {
- info(`Test for ${JSON.stringify(geolocation)}`);
-
- let sandbox = sinon.createSandbox();
- sandbox.stub(WeatherFeed.prototype, "PersistentCache").returns({
- set: () => {},
- get: () => {},
- });
-
- let feed = new WeatherFeed();
- sandbox.stub(feed, "isEnabled").returns(true);
- feed.store = {
- dispatch: sinon.spy(),
- getState() {
- return { Prefs: { values: {} } };
- },
- };
- feed.merino = { fetch: () => {} };
-
- // Stub merino client
- let stub = sandbox.stub(feed.merino, "fetch").resolves(["result"]);
- let cleanupGeolocationStub =
- GeolocationTestUtils.stubGeolocation(geolocation);
-
- await feed.onAction({ type: actionTypes.SYSTEM_TICK });
-
- if (expected) {
- sinon.assert.calledOnce(stub);
- sinon.assert.calledWith(stub, {
- otherParams: { request_type: "weather", source: "newtab", ...expected },
- providers: ["accuweather"],
- query: "",
- timeoutMs: 7000,
- });
- } else {
- sinon.assert.notCalled(stub);
- }
-
- await cleanupGeolocationStub();
- sandbox.restore();
- }
-});
-
-// Test detecting location using GeolocationUtils.geolocation()
-add_task(async function test_detect_location_with_geolocation() {
- const TEST_DATA = [
- {
- geolocation: {
- city: "San Francisco",
- },
- expected: "San Francisco",
- },
- {
- geolocation: {
- city: "",
- region: "Yokohama",
- },
- expected: "Yokohama",
- },
- {
- geolocation: {
- region: "Tokyo",
- },
- expected: "Tokyo",
- },
- {
- geolocation: {
- city: "",
- region: "",
- },
- expected: false,
- },
- {
- geolocation: {},
- expected: false,
- },
- {
- geolocation: null,
- expected: false,
- },
- ];
- for (let { geolocation, expected } of TEST_DATA) {
- info(`Test for ${JSON.stringify(geolocation)}`);
-
- let sandbox = sinon.createSandbox();
- sandbox.stub(WeatherFeed.prototype, "PersistentCache").returns({
- set: () => {},
- get: () => {},
- });
-
- let feed = new WeatherFeed();
- feed.store = {
- dispatch: sinon.spy(),
- getState() {
- return { Prefs: { values: {} } };
- },
- };
- feed.merino = { fetch: () => {} };
-
- // Stub merino client
- let stub = sandbox.stub(feed.merino, "fetch").resolves(null);
- // Stub geolocation
- let cleanupGeolocationStub =
- GeolocationTestUtils.stubGeolocation(geolocation);
- await feed.onAction({ type: actionTypes.WEATHER_USER_OPT_IN_LOCATION });
-
- if (expected) {
- sinon.assert.calledOnce(stub);
- sinon.assert.calledWith(stub, {
- otherParams: { request_type: "location", source: "newtab" },
- providers: ["accuweather"],
- query: expected,
- timeoutMs: 7000,
- });
- } else {
- sinon.assert.notCalled(stub);
- }
-
- await cleanupGeolocationStub();
- sandbox.restore();
- }
-});
