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.

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

Conversation

mandysGit
Copy link

@mandysGit mandysGit commented Jan 18, 2025

Pull Request checklist

  • Breaking changes: This PR follows our breaking change policy
    • This PR follows the breaking change policy:
      • This PR has no breaking API changes, or
      • There are corresponding PRs for our consumer applications that resolve the breaking changes and have been approved
  • Quality: This PR builds and tests run cleanly
    • Note:
      • For changes that need extra cross-platform testing, consider adding [ci full] to the PR title.
      • If this pull request includes a breaking change, consider cutting a new release after merging.
  • Tests: This PR includes thorough tests or an explanation of why it does not
  • Changelog: This PR includes a changelog entry in CHANGELOG.md or an explanation of why it does not need one
    • Any breaking changes to Swift or Kotlin binding APIs are noted explicitly
  • Dependencies: This PR follows our dependency management guidelines
    • Any new dependencies are accompanied by a summary of the due diligence applied in selecting them.

Branch builds: add [firefox-android: branch-name] to the PR title.

@mandysGit mandysGit requested a review from Standard8 January 18, 2025 02:43
@mandysGit mandysGit changed the title Bug 1930985 - Implement search engine order handling on the Rust base… Bug 1930985 - Implement search engine order handling on the Rust based SearchEngineSelector. Jan 18, 2025
@mandysGit
Copy link
Author

This PR is still work in progress and there will be more changes soon.
I need a first pass review, please.

I've added the implemention for orderHint property. I included setting the sort priority and sorting the engines orders.

The part I need some feedback on right now is the first test I wrote, and of course everything else if there's any suggestions.

I converted this test into Rust in the selector.rs:

https://searchfox.org/mozilla-central/source/toolkit/components/search/tests/xpcshell/test_engine_selector_engine_orders.js#163-173

And I'm copying over this engine config from the top of the file:

https://searchfox.org/mozilla-central/source/toolkit/components/search/tests/xpcshell/test_engine_selector_engine_orders.js#15-43

As I was following the test structure in selector.rs I noticed the config and expected_engines parts are duplicated and very long. The way we've done it in Desktop is that we hadn't need to write all of the properties in. I wanted some feedback if I should continue the tests in this format even though they're quite long and has lots of duplication?

My aim is to continue writing tests for starts_with and defaults engines. But that would mean I'll need to create another json config and another expected_engines list -- which I think is fine to do but there's a lot of duplication.

Curious to know what your thoughts on this are?

components/search/src/configuration_types.rs Show resolved Hide resolved
) -> 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.

@@ -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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants