1685729 - Refresh top sites on region change

Status: VERIFIED FIXED

(Firefox :: Top Sites, enhancement, P1)

Description

[Mike de Boer [:mikedeboer]]

When Region.jsm notifies the region-updated message, which happens when the user's home location changes, TopSitesFeed.jsm should reload the list of (default) Top Sites, because they're filtered on region (usually).

It's not absolutely necessary for all open New Tab pages to pick up this change right away. If it cheap enough, great!

Comment 1

[Dão Gottwald [:dao]]

Attachment: Bug 1685729 - Refresh top sites on region change. r=DaleHarvey

Comment 2

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ccbeb11eca6f
Refresh top sites on region change. r=daleharvey

Comment 3

[Alexandru Michis [:malexandru]]

https://hg.mozilla.org/mozilla-central/rev/ccbeb11eca6f

Comment 4

[Adrian Florinescu [:aflorinescu]]

This fix is testable using a snippet that forces the region change:

var {Region} = ChromeUtils.import("resource://gre/modules/Region.jsm")
Region._setHomeRegion("RU")

Discussing with :dharvey and :standard8, the region change happens as soon as the region change timer hits - which might be in the middle of the session. When the region change hits, the search-config also triggers the search engine update based on the updated region.

For the Topsites with RS on and the fix -Nightly 86 (Mac/Windows) - most of the time, the region change will not trigger the topsite update for subsequent newly open about:newtab or about:home: it needs restart for the Topsites to pick the region change. However, we have had some small amout of tests in which the region update has triggered the Topsites update as well in the session (for subsequent new tabs), but we're unsure about the pre-requisites to hit that scenario.

Comment 5

[Dão Gottwald [:dao]]

Sigh. Turns out the notification is called "browser-region", not "region-updated". "region-updated" is only added as extra data and basically redundant. (IMHO, the notification being called "browser-region-updated" and data being null would have been more in line with how notifications are normally set up.)

Comment 6

[Dão Gottwald [:dao]]

Attachment: Bug 1685729 - Use correct notification name for region changes. r=daleharvey

Sigh. Turns out the notification is called "browser-region", not "region-updated". "region-updated" is only added as extra data and basically redundant. (IMHO, the notification being called "browser-region-updated" and data being null would have been more in line with how notifications are normally set up.)

Comment 7

[Mike de Boer [:mikedeboer]]

(In reply to Dão Gottwald [::dao] from comment #6)

Created attachment 9198617 [details]
Bug 1685729 - Use correct notification name for region changes. r=daleharvey

Sigh. Turns out the notification is called "browser-region", not "region-updated". "region-updated" is only added as extra data and basically redundant. (IMHO, the notification being called "browser-region-updated" and data being null would have been more in line with how notifications are normally set up.)

FWIW, feel free to change it while it's hot! There's no consumer for this notification atm, so should be relatively straightforward.

Comment 8

[Dão Gottwald [:dao]]

(In reply to Mike de Boer [:mikedeboer] from comment #7)

Sigh. Turns out the notification is called "browser-region", not "region-updated". "region-updated" is only added as extra data and basically redundant. (IMHO, the notification being called "browser-region-updated" and data being null would have been more in line with how notifications are normally set up.)

FWIW, feel free to change it while it's hot! There's no consumer for this notification atm, so should be relatively straightforward.

Filed bug 1688220.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9755a79772b5
Use correct notification name for region changes. r=Standard8

Comment 10

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/9755a79772b5

Comment 11

[Adrian Florinescu [:aflorinescu]]

Verified as fixed on Ubuntu 20.04, Mac 10.13.6 and Windows 10 using 86.0a1 (2021-01-25).
In regards to verification details, the tests were done using comment 3 snippet for region change and browser.topsites.useRemoteSetting -true.

As a functionality sumup, with the fix, topsites will now update and pick up the update on region change in all new tabs. (topsites with RS - on)