Skip to content
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.

Sign up for GitHub

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 1930985 - Implement search engine order handling on the Rust based SearchEngineSelector. #6563

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+302 −7
Open

Bug 1930985 - Implement search engine order handling on the Rust based SearchEngineSelector. #6563

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 18 additions & 1 deletion components/search/src/configuration_types.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -240,11 +240,28 @@ pub(crate) struct JSONDefaultEnginesRecord {
pub specific_defaults: Vec<JSONSpecificDefaultRecord>,
}

#[derive(Debug, Deserialize, Clone)]
#[serde(rename_all = "camelCase")]
pub(crate) struct JSONEngineOrder {
/// The specific environment to match for this record.
pub environment: JSONVariantEnvironment,

/// The order of the engines for the associated environment. If engines are
/// present for the user but not included in this list, they will follow
/// after the ones in this list in alphabetical order. If an individual entry
/// is suffixed with a star, matching is applied on a \"starts with\" basis.
mandysGit marked this conversation as resolved.
Show resolved Hide resolved
#[serde(default)]
pub order: Vec<String>,
}

/// Represents the engine orders record.
#[derive(Debug, Deserialize, Clone)]
#[serde(rename_all = "camelCase")]
pub(crate) struct JSONEngineOrdersRecord {
// TODO: Implementation.
/// When a user's instance matches the defined environments, the associated
/// engine order will be applied. The array is ordered, when multiple entries
/// match on environments, the later entry will override earlier entries.
pub orders: Vec<JSONEngineOrder>,
}

/// Represents an individual record in the raw search configuration.
Expand Down
78 changes: 73 additions & 5 deletions components/search/src/filter.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,7 @@ pub(crate) fn filter_engine_configuration(
user_environment.version = user_environment.version.to_lowercase();

let mut default_engines_record = None;
let mut engine_orders_record = None;

for record in configuration {
match record {
Expand All @@ -126,8 +127,8 @@ pub(crate) fn filter_engine_configuration(
JSONSearchConfigurationRecords::DefaultEngines(default_engines) => {
default_engines_record = Some(default_engines);
}
JSONSearchConfigurationRecords::EngineOrders(_engine_orders) => {
// TODO: Implementation.
JSONSearchConfigurationRecords::EngineOrders(engine_orders) => {
engine_orders_record = Some(engine_orders)
}
JSONSearchConfigurationRecords::Unknown => {
// Prevents panics if a new record type is added in future.
Expand All @@ -138,6 +139,19 @@ pub(crate) fn filter_engine_configuration(
let (default_engine_id, default_private_engine_id) =
determine_default_engines(&engines, default_engines_record, &user_environment);


if let Some(orders_record) = engine_orders_record {
for order_data in &orders_record.orders {
if matches_user_environment(&order_data.environment, &user_environment) {
set_engine_order(&mut engines, &order_data.order);
}
}
}

engines.sort_by(|a, b| {
sort(default_engine_id.as_ref(), default_private_engine_id.as_ref(), a, b)
});

Ok(RefinedSearchConfig {
engines,
app_default_engine_id: default_engine_id,
Expand Down Expand Up @@ -184,11 +198,11 @@ fn determine_default_engines(
if let Some(specific_default) = specific_default {
// Check the engine is present in the list of engines before
// we return it as default.
if let Some(engine_id) = find_engine_with_match(engines, specific_default.default) {
if let Some(engine_id) = find_engine_id_with_match(engines, specific_default.default) {
default_engine_id.replace(engine_id);
}
if let Some(private_engine_id) =
find_engine_with_match(engines, specific_default.default_private)
find_engine_id_with_match(engines, specific_default.default_private)
{
default_engine_private_id.replace(private_engine_id);
}
Expand Down Expand Up @@ -221,7 +235,18 @@ fn find_engine_id(engines: &[SearchEngineDefinition], engine_id: String) -> Opti
}
}

fn find_engine_with_match(
fn set_engine_order(engines: &mut [SearchEngineDefinition], ordered_engines: &[String]) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm tempted to say we could put the sort related functions into a separate module, which might make it easier to write unit tests for them (and smaller module sizes), what do you think?

let mut order_number = ordered_engines.len();

for engine_id in ordered_engines {
if let Some(found_engine) = find_engine_with_match_mut(engines, engine_id) {
found_engine.order_hint = Some(order_number as u32);
order_number -= 1;
}
}
}

fn find_engine_id_with_match(
engines: &[SearchEngineDefinition],
engine_id_match: String,
) -> Option<String> {
Expand All @@ -241,6 +266,49 @@ fn find_engine_with_match(
.map(|e| e.identifier.clone())
}

fn find_engine_with_match_mut<'a>(
engines: &'a mut [SearchEngineDefinition],
engine_id_match: &String,
) -> Option<&'a mut SearchEngineDefinition> {
if engine_id_match.is_empty() {
return None;
}
if let Some(match_no_star) = engine_id_match.strip_suffix('*') {
return engines
.iter_mut()
.find(|e| e.identifier.starts_with(match_no_star))
}

engines
.iter_mut()
.find(|e| e.identifier == *engine_id_match)
}

fn get_priority(
engine: &SearchEngineDefinition,
default_engine_id: Option<&String>,
default_private_engine_id: Option<&String>,
) -> u32 {
if Some(&engine.identifier) == default_engine_id {
return u32::MAX;
}
if Some(&engine.identifier) == default_private_engine_id {
return u32::MAX - 1;
}
engine.order_hint.unwrap_or(0)
}

fn sort(
default_engine_id: Option<&String>,
default_private_engine_id: Option<&String>,
a: &SearchEngineDefinition,
b: &SearchEngineDefinition
) -> std::cmp::Ordering {
let b_index = get_priority(b, default_engine_id, default_private_engine_id);
let a_index = get_priority(a, default_engine_id, default_private_engine_id);
b_index.cmp(&a_index)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a piece missing here, which the engine selector on desktop doesn't do - the items which aren't default/have an order hint set, need to be sorted alphabetically by their display name.

This is the full function we use: https://searchfox.org/mozilla-central/rev/5b061cdc4d40d44988dc61aa941cfbd98e31791f/toolkit/components/search/SearchUtils.sys.mjs#362-440

One thing I'm not sure about is how we'll handle the locale-based sorting like we do on desktop. I haven't looked into the options for sort functions on Rust.

}

#[cfg(test)]
mod tests {
use std::vec;
Expand Down
210 changes: 210 additions & 0 deletions components/search/src/selector.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -961,4 +961,214 @@ mod tests {
"Should have selected the private default engine for the matching specific default"
);
}

#[test]
fn test_filter_engine_orders() {
let selector = Arc::new(SearchEngineSelector::new());

let config_result = Arc::clone(&selector).set_search_config(
json!({
"data": [
{
"recordType": "engine",
"identifier": "default-engine",
"base": {
"name": "default-engine",
"classification": "general",
"urls": {
"search": {
"base": "https://example.com",
"method": "GET",
}
}
},
"variants": [{
"environment": {
"allRegionsAndLocales": true
}
}],
},
{
"recordType": "engine",
"identifier": "b-engine",
"base": {
"name": "b-engine",
"classification": "general",
"urls": {
"search": {
"base": "https://example.com",
"method": "GET"
}
}
},
"variants": [{
"environment": {
"allRegionsAndLocales": true
}
}],
},
{
"recordType": "engine",
"identifier": "a-engine",
"base": {
"name": "a-engine",
"classification": "general",
"urls": {
"search": {
"base": "https://example.com",
"method": "GET"
}
}
},
"variants": [{
"environment": {
"allRegionsAndLocales": true,
}
}],
},
{
"recordType": "engine",
"identifier": "c-engine",
"base": {
"name": "c-engine",
"classification": "general",
"urls": {
"search": {
"base": "https://example.com",
"method": "GET"
}
}
},
"variants": [{
"environment": {
"allRegionsAndLocales": true,
}
}],
},
{
"recordType": "engineOrders",
"orders": [
{
"environment": {" distributions": ["distro"] },
"order": ["default-engine", "a-engine", "b-engine", "c-engine"],
},
{
"environment": {
"distributions": ["distro"],
"locales": ["en-CA"],
"regions": ["CA"],
},
"order": ["default-engine", "c-engine", "b-engine", "a-engine"],
},
{
"environment": {
"distributions": ["distro-2"],
},
"order": ["default-engine", "a-engine", "b-engine"],
},
],
},
]
})
.to_string(),
);
assert!(
config_result.is_ok(),
"Should have set the configuration successfully. {:?}",
config_result
);

let expected_engines = vec![
SearchEngineDefinition {
charset: "UTF-8".to_string(),
classification: SearchEngineClassification::General,
identifier: "default-engine".to_string(),
name: "default-engine".to_string(),
order_hint: Some(4),
urls: SearchEngineUrls {
search: SearchEngineUrl {
base: "https://example.com".to_string(),
method: "GET".to_string(),
params: Vec::new(),
search_term_param_name: None,
},
..Default::default()
},
..Default::default()
},
SearchEngineDefinition {
charset: "UTF-8".to_string(),
classification: SearchEngineClassification::General,
identifier: "a-engine".to_string(),
name: "a-engine".to_string(),
order_hint: Some(3),
urls: SearchEngineUrls {
search: SearchEngineUrl {
base: "https://example.com".to_string(),
method: "GET".to_string(),
params: Vec::new(),
search_term_param_name: None,
},
..Default::default()
},
..Default::default()
},
SearchEngineDefinition {
charset: "UTF-8".to_string(),
classification: SearchEngineClassification::General,
identifier: "b-engine".to_string(),
name: "b-engine".to_string(),
order_hint: Some(2),
urls: SearchEngineUrls {
search: SearchEngineUrl {
base: "https://example.com".to_string(),
method: "GET".to_string(),
params: Vec::new(),
search_term_param_name: None,
},
..Default::default()
},
..Default::default()
},
SearchEngineDefinition {
charset: "UTF-8".to_string(),
classification: SearchEngineClassification::General,
identifier: "c-engine".to_string(),
name: "c-engine".to_string(),
order_hint: Some(1),
urls: SearchEngineUrls {
search: SearchEngineUrl {
base: "https://example.com".to_string(),
method: "GET".to_string(),
params: Vec::new(),
search_term_param_name: None,
},
..Default::default()
},
..Default::default()
},
];

let result = Arc::clone(&selector).filter_engine_configuration(SearchUserEnvironment {
distribution_id: "distro".to_string(),
locale: "fr".into(),
region: "FR".into(),
..Default::default()
});
assert!(
result.is_ok(),
"Should have filtered the configuration without error. {:?}",
result
);

assert_eq!(
result.unwrap(),
RefinedSearchConfig {
engines: expected_engines,
app_default_engine_id: None,
app_private_default_engine_id: None
},
"Should match engine orders with the distro distribution."
);
}
}
2 changes: 1 addition & 1 deletion components/search/src/types.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -199,7 +199,7 @@ pub struct SearchEngineDefinition {
/// The higher the number, the nearer to the front it should be.
/// If the number is not specified, other methods of sorting may be relied
/// upon (e.g. alphabetical).
pub order_hint: Option<u8>,
pub order_hint: Option<u32>,
}

/// Details of the search engines to display to the user, generated as a result
Expand Down