1612824 - Search Services shouldn't use reInit for when the locale is dynamically changed.

Status: RESOLVED FIXED

(Firefox :: Search, task, P3)

Description

[Mark Banner (:standard8)]

_reInit was only really designed for use in tests. However, we are currently using it for when locales are dynamically changed.

The good thing is, that dynamically changing locale also currently forces a restart of the browser.

At some stage, once Fluent is mature enough, that is likely to change and there will be no restart.

The bad thing with _reInit is that we don't send out notifications of any changes.

At the moment, the worst case I've found is that if the default browser should be changed as a result of the switch, then we will not pick that change up until a subsequent restart - potentially confusing the user. This is because reInit doesn't send out any notifications.

Ideally, we should be using something like a simplified maybeReloadEngines (i.e. after bug 1610293), and ensure that the notifications get sent out.

Comment 2

[Axel Hecht [:Pike]]

Right now, we restart on locale change through prefs as a "better be safe than sorry". It's not that restarts are really required for most of our code, it's just niches.

I think the bigger problem here is that we have more and more code paths that do stuff in response to locale changes, and no signal on the Firefox-level that indicates "we're done switching locales, go on with life".

In the browser, that might be OK, but I wouldn't be surprised if this could lead to intermittent test failures at some point down the road.

Do we have anything that allows us to trigger async tasks across all kinds of places and processes, and wait for all of that to finish?

Comment 3

[Mark Banner (:standard8)]

(In reply to Axel Hecht [:Pike] from comment #2)

Right now, we restart on locale change through prefs as a "better be safe than sorry". It's not that restarts are really required for most of our code, it's just niches.

I thought it was because full language switching wasn't in place yet.

Do we have anything that allows us to trigger async tasks across all kinds of places and processes, and wait for all of that to finish?

Not as far as I know.

What the search service is doing here is bad though - it is doing more work than necessary, and not actually doing all of the work it should be doing.

Comment 4

[Florian Quèze [:florian]]

(In reply to Mark Banner (:standard8) from comment #0)

_reInit was only really designed for use in tests. However, we are currently using it for when locales are dynamically changed.

My recollection is that it happened the other way around: the search service reInit code was introduced for locale switching in Fennec, and then I abused it for tests to test absearch stuff using normal xpcshell tests instead of needing marionette tests.

Comment 5

[Mark Banner (:standard8)]

Attachment: Bug 1612824 - Search Services shouldn't use reInit for when the locale is changed or the ignore list is updated. r?daleharvey!

reInit is unsafe as it completely removes the existing data before reloading. If something interrupts the process that can cause dataloss.

_maybeReloadEngines is safer as it does changes progressively, it also now handles removing engines, which it didn't before.

Depends on D88023

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/abf8d8b24845
Search Services shouldn't use reInit for when the locale is changed or the ignore list is updated. r=daleharvey

Comment 7

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/db7cb1b70eef
Backed out 15 changesets (bug 1612824, bug 1661234, bug 1619926, bug 1612380, bug 1559530, bug 1642990) for Xpc failures. CLOSED TREE

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b384152d8539
Search Services shouldn't use reInit for when the locale is changed or the ignore list is updated. r=daleharvey

Comment 9

[Cristian Brindusan [:cbrindusan]]

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