Currently when one changes the browser language to a locale with a bundled search engine, the engine list shown in the browser will not change until next browser restart. E.g, change to Chinese Simplified and you'd expect to see Baidu but you see our default en-US listing (Google/Yahoo/Bing/Amazon/Twitter/Wikipedia). We should fetch the appropriate engines without the need to restart.

Pretty much a sibling of Bug 997589.

This ought to do it. Untested.

This ought to do it, except that BrowserSearch isn't listening. 06-09 22:59:08.470 W/GeckoEventDispatcher(23059): No listeners for SearchEngines:Data

Raymond, Ryan: you two have the most blame on nsSearchService. From reading code, it looks to me like the correct course of action is for the search service itself to observe the general.useragent.locale pref, calling _asyncLoadEngines when the locale changes, just as if it were being initialized for the first time, and notifying observers accordingly. Do you agree with this analysis? The alternative, as far as I can see, is to have code elsewhere specifically ask nsSearchService to reinitialize itself. If you do agree, would you folks like to do that work, or are you free to review a patch from me?

This patch makes nsSearchService watch for changes in the general.useragent.locale pref, asynchronously reiniting itself when the pref changes. reinit-complete and reinit-failed events are broadcast on the usual search service topic.

This builds on top of part 1, assuming that: * If we're redisplaying the search bar, and * The locale has changed since we last fetched search results, then * We should re-request, and nsSearchService will be up-to-date, because humans don't click fast enough for this assumption to be validated. If the assumption chain is false, we'll get stale or no search engines until we fetch again. That's acceptable IMO.

Comment on attachment 8454790 [details] [diff] [review] Part 2: invalidate BrowserSearch engine list when locale has changed. v1 Review of attachment 8454790 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/base/home/BrowserSearch.java @@ +108,5 @@ > private volatile SuggestClient mSuggestClient; > > + // List of search engines from Gecko. > + // Do not mutate this list. > + private volatile List<SearchEngine> mSearchEngines = new ArrayList<SearchEngine>(); Why change this to volatile? If we actually access this on multiple threads, we're in trouble since we aren't using any form of synchronization, so I don't think volatile buys us anything. It looks like we're requiring mSearchEngines to be on the UI thread, so I think it would be preferable to: * Add a comment that this be used only on the UI thread. * Add ThreadUtils.assertOnUiThread() to any methods using it whose thread is unclear. @@ +233,5 @@ > super.onResume(); > > + // Fetch engines if we need to. > + if (mSearchEngines.isEmpty() || > + !Locale.getDefault().equals(mLastLocale)) { Nit: Keep this on the same line? It makes my eyes bleed when a multi-line condition is indented at the same level as the body.

(In reply to Brian Nicholson (:bnicholson) from comment #7) > Why change this to volatile? If we actually access this on multiple threads, > we're in trouble since we aren't using any form of synchronization, so I > don't think volatile buys us anything. We write to it from the message loop (background), and check it in onResume (UI). We don't mutate the interior of the list (indeed, it's unmodifiable), and it's never accessed via two places until long after it's built, so volatile is exactly the primitive we want: atomic dereference and assignment.

(In reply to Richard Newman [:rnewman] from comment #8) > We write to it from the message loop (background) I don't think so -- see http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/home/BrowserSearch.java?rev=36853e536d68#351. Note that mAdapter is tied to mSearchEngines, so we must follow modifications to mSearchEngines with notifyDataSetChanged() on the UI thread.

(In reply to Brian Nicholson (:bnicholson) from comment #9) > (In reply to Richard Newman [:rnewman] from comment #8) > > We write to it from the message loop (background) > > I don't think so -- see Oh, you're right! That's what I get for making assumptions. Good spot.

https://hg.mozilla.org/try/rev/1360128fb492 https://hg.mozilla.org/try/rev/03ffcf005ca3

Comment on attachment 8454780 [details] [diff] [review] Part 1: reinitialize nsSearchService when the browser locale changes. v1. Review of attachment 8454780 [details] [diff] [review]: ----------------------------------------------------------------- I don't know this code super well, but there are a couple of races here that I'm a little worried about. (1) If the locale is changed while the initial init is ongoing, it looks like we could run into trouble, at least with checkForSyncCompletion and the error it throws. That seems unlikely to happen in practice, though, but who knows. (2) If clients use the service while re-init is ongoing, then it looks like what will happen is it'll fall back to sync init, which logs a warning, and then the _asyncLoadEngines call from _asyncReInit may end up throwing an error during one of its several calls to checkForSyncCompletion. That seems more likely to happen than (1). It doesn't look like the consequences of either case are too bad. We'd end up with warnings or errors logged, but the service would ultimately be (re)inited. But it is a little dodgy, so I'm kind of inclined to not r+ this in favor of a more sound fix. But what do you think, Richard?

Clients are supposed to call nsIBrowserSearchService.init themselves -- in the one client I've written since async init was added, it was easiest just to assume the service had never been inited, so all my calls to the service are guarded by init calls. If that's a common pattern for clients, then maybe it's enough for the service to simply uninit itself when the locale changes and let clients re-init it. Just a thought.

(In reply to Drew Willcoxon :adw from comment #12) Thanks for the review! > It doesn't look like the consequences of either case are too bad. We'd end > up with warnings or errors logged, but the service would ultimately be > (re)inited. But it is a little dodgy, so I'm kind of inclined to not r+ > this in favor of a more sound fix. But what do you think, Richard? The main trigger for this (and the typical place where we'd see timing issues) would be Fennec's locale switching, which sets the newly observed pref in response to user action. It's not possible to switch locales until Gecko is up and running (=> the search service should either be initialized or not yet begun, depending on how lazy its consumers are), and locale switching is human-initiated, so it's impractical for a race to occur there. (Fingers are slow.) On desktop, my guess is that that pref isn't changed often (or else someone would have fixed this bug sooner!), but if it is changed it's likely to be changed by a human, with similar race characteristics. If I understand the desktop distribution story correctly, desktop users download a single-locale build, so they should have no cause to ever change general.useragent.locale. I agree wholeheartedly that this is something of an inexpert fix -- it pretty much stomps on state until it stops moving, then triggers init, and it has few better options given that it's in an observer. It would be great if someone with more knowledge of this code would make it bulletproof. My inclination is to kick that to a follow-up and get this into 32 with the rest of the locale switching code -- AFAICS the risk for desktop users is ~0, and the risk for Fennec users seems acceptable to me after testing. The alternative would be that we reach into the guts of nsSearchService from Fennec's code, which seems even more fragile. Re Comment 13: I deliberately triggered the init immediately, rather than relying on a consumer causing init, because it simply produces a better user experience to not have to wait for the search engines to load. We even have a bug on file to pre-load them the first time: Bug 927692. The reason for lazy init is to avoid doing work when cycles are precious; locale change probably isn't such a case, so it's better to front-load the work.

Comment on attachment 8454780 [details] [diff] [review] Part 1: reinitialize nsSearchService when the browser locale changes. v1. Review of attachment 8454780 [details] [diff] [review]: ----------------------------------------------------------------- OK.

https://hg.mozilla.org/integration/fx-team/rev/0234ad055a7b https://hg.mozilla.org/integration/fx-team/rev/476a0455e0e8

Misread my bugmail as requesting f? from Gavin, not clearing it, so missed 33. Darn. Will give this a few days to bake then request uplift. Thanks for the reviews, folks!

https://hg.mozilla.org/mozilla-central/rev/0234ad055a7b https://hg.mozilla.org/mozilla-central/rev/476a0455e0e8

Let's get this on Beta

Comment on attachment 8454780 [details] [diff] [review] Part 1: reinitialize nsSearchService when the browser locale changes. v1. ** Approval request for both parts ** Approval Request Comment [Feature/regressing bug #]: Long-standing limitation in nsSearchService and BrowserSearch. Needed to support locale switching work in 32. [User impact if declined]: Users switching locales will see the default search engines from their previous locale until Android next relaunches Fennec. [Describe test coverage new/current, TBPL]: Manually verified by QA. [Risks and why]: Low risk of incorrect reinitialization due to pref changes and races (nothing practical). Otherwise seems pretty robust. [String/UUID change made/needed]: None.

Oops - I wanted to look at this before it landed (hence the feedback), and I missed that being cleared. Can we make this re-init behavior Fennec-only? Reinitialization is a bit scary to me and I'd prefer if it not happen on Desktop, where there little value.

(In reply to :Gavin Sharp [email: gavin@gavinsharp.com] from comment #21) > Oops - I wanted to look at this before it landed (hence the feedback), and I > missed that being cleared. > > Can we make this re-init behavior Fennec-only? We can. How would you prefer to do this? Options, of varying complexities: * Preprocess nsSearchService.js. * Introspect at runtime to decide which app we are. Not sure if that's done in toolkit code right now. * Switch to using some explicit observer notification that desktop doesn't send (e.g., "locale-changed-reinit") instead of pref observing. * Moving internals-aware logic from nsSearchService into Fennec. Doesn't seem like a good solution from a maintenance standpoint. * Fork nsSearchService for Fennec.

nsSearchService.js is already preprocessed (for fennec-specific stuff even), you can just throw an #ifdef ANDROID in there. I filed bug 1043627.

Comment on attachment 8454780 [details] [diff] [review] Part 1: reinitialize nsSearchService when the browser locale changes. v1. Please also fill the uplift request in bug 1043627 when it is ready.

Comment on attachment 8454790 [details] [diff] [review] Part 2: invalidate BrowserSearch engine list when locale has changed. v1 > ** Approval request for both parts ** As requested.

https://hg.mozilla.org/releases/mozilla-aurora/rev/088b9010da18 https://hg.mozilla.org/releases/mozilla-aurora/rev/3adc5ff487a6 https://hg.mozilla.org/releases/mozilla-beta/rev/bbfebb4ec504 https://hg.mozilla.org/releases/mozilla-beta/rev/d1e5cb0fbe70