Bug 1919541 - Implement variant handling in the search engine selector.

[Standard8]

This implements handling for search engine variants within the selector. It does not handle sub-variants at this time, that will be handled in a follow-up.

No changelog/API entries as this isn't currently used anywhere.

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]

Thank you. This looks good. I had a suggestion for renaming the parameters of the merge function and the other parts all look good to me.

[mandysGit]

I'm a bit confused with the parameter naming or lack of naming. It's confusing to figure out which Url is overriding the other Url because there's self and other: Self. I know the first one is just referring to the type self which is JSONEngineUrl. It wasn't obvious at first and I had mixed them up.

I'm wondering if we're able to change function definition to any of these?

fn merge(baseUrl: Self, variantUrl: Self) -> Self 

fn merge(baseUrl: Self, overrideUrl: Self) -> Self 

I was reading about Associated Functions and if we change the function definition with a explicit parameter name, then this changes to an associated function and we might need to call it in a different way:
JSONEngineUrl::merge(url1, url2) rather than url1.merge(url2)

My other thought is, if you'll like to keep it as an instance method, maybe rename other to overrideUrl or variantUrl?

[Standard8]

I thought about it a bit, and decided to go with associated functions, since they seemed better for what was actually going on - merging two objects & returning a new one (rather than modifying one of them in-place).

I've also switched to using original and preferred as I think that is hopefully clear than base/variant - the merge functions don't care about what is being merged.

[mandysGit]

Question:
I was reading the docs I wrote on variants and sub-variants as a refresher, but it didn't say anything about not matching any sub-variants and what happens in that scenario.

I know this PR isn't about sub-variants, but I was just curious and wanted to verify. If there's a list of sub-variants but none of them match, we match the last top-level variant that was matched. Is that right?

[Standard8]

I know this PR isn't about sub-variants, but I was just curious and wanted to verify. If there's a list of sub-variants but none of them match, we match the last top-level variant that was matched. Is that right?

If a top-level variant matches, then it is applied, regardless of if there's a matching sub-variant or not. If there is a matching sub-variant, then that will be applied on top i.e. take the base, merge the variant, (maybe) merge the sub-variant.

[linabutler]

Oops, I had these small pending style suggestions, and never submitted them!

[Standard8]

Oops, I had these small pending style suggestions, and never submitted them!

Thank you!