-
Notifications
You must be signed in to change notification settings - Fork 236
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Bug 1955387 - Add locale fallback handling to the Rust based search engine selector. #6680
base: main
Are you sure you want to change the base?
Conversation
…ngine selector. - Introduce a new record type for a list of availablelocales in the search configuration - Implement logic to use the user's locale directly, or strip the regional part for fallback - Map unsupported English locales to en-US
bbb7e0a
to
e830bed
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've started looking at this, but haven’t had much time, will finish looking tomorrow but thought I'd submit the two comments in-line anyway in case you have time to have a look.
if locales_map_to_en_us.contains(&user_locale) { | ||
user_environment.locale = "en-us".to_string(); | ||
return; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we could simplify this to if user_locale starts with "en-" then make it "en-US".
The en-*
locales we care about will presumably have already been listed in available_locales_set
, so any other version of en-
should just fall back to en-US
. This would also avoid hard-coding the list of en-*
locales which could easily be incomplete or be missed later.
available_locales = locales_record.locales.clone(); | ||
} | ||
} | ||
let mut user_environment = user_environment.clone(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should pass in a cloned, mutable user_environment here - currently I think filter_engine_configuration_impl
would not get the updated locale, and hence it might determine the sort order/default engines incorrectly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, one additional comment to address. The tests look good.
let stringified = serde_json::to_string(&record.fields)?; | ||
if let Some(val) = record.fields.get("recordType") { | ||
if *val == "availableLocales" { | ||
let locales_record: Option<JSONAvailableLocalesRecord> = | ||
serde_json::from_str(&stringified)?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please can you put the serde_json::to_string
statement within the second if statement to avoid it being called unnecessarily.
Pull Request checklist
[ci full]
to the PR title.Branch builds: add
[firefox-android: branch-name]
to the PR title.