Skip to content
This repository was archived by the owner on Jun 17, 2024. It is now read-only.

Bug 1804594 - Differentiate search engines between "general" and "topic specific" when they are loaded#273

Merged
mergify[bot] merged 2 commits intomozilla-mobile:mainfrom
gabrielluong:1804594
Dec 12, 2022
Merged

Bug 1804594 - Differentiate search engines between "general" and "topic specific" when they are loaded#273
mergify[bot] merged 2 commits intomozilla-mobile:mainfrom
gabrielluong:1804594

Conversation

@gabrielluong
Copy link
Copy Markdown
Member

@gabrielluong gabrielluong commented Dec 8, 2022

Pull Request checklist

  • Quality: This PR builds and passes detekt/ktlint checks (A pre-push hook is recommended)
  • Tests: This PR includes thorough tests or an explanation of why it does not
  • Changelog: This PR includes a changelog entry or does not need one
  • Accessibility: The code in this PR follows accessibility best practices or does not include any user facing features

After merge

  • Milestone: Make sure issues closed by this pull request are added to the milestone of the version currently in development.
  • Breaking Changes: If this is a breaking change, please push a draft PR on Reference Browser to address the breaking issues.

GitHub Automation

Used by GitHub Actions.

@gabrielluong gabrielluong added the 🕵️‍♀️ needs review PRs that need to be reviewed label Dec 8, 2022
@gabrielluong gabrielluong requested a review from Mugurell December 8, 2022 03:35
gabrielluong
gabrielluong commented Dec 8, 2022
View reviewed changes
Comment thread ...feature/search/src/main/java/mozilla/components/feature/search/storage/SearchEngineReader.kt
// List of general search engine ids, taken from
// https://searchfox.org/mozilla-central/rev/ef0aa879e94534ffd067a3748d034540a9fc10b0/toolkit/components/search/SearchUtils.sys.mjs#200
internal val GENERAL_SEARCH_ENGINE_IDS = setOf(
GOOGLE_ID,
Copy link
Copy Markdown
Member Author

@gabrielluong gabrielluong Dec 8, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I think we could've also just put google-b-m and google-com-nocodes instead of doing a startsWith as well. I went with startsWith because I saw there was google-b-amzes for Echo Show in my search https://searchfox.org/mozilla-mobile/search?q=google-&path=&case=false&regexp=false

No strong opinions here - happy to make the switch.

gabrielluong
gabrielluong commented Dec 8, 2022
View reviewed changes
Comment thread ...ure/search/src/test/java/mozilla/components/feature/search/storage/SearchEngineReaderTest.kt Outdated
assertEquals(searchEngine.name, readSearchEngine.name)
assertEquals(searchEngine.type, readSearchEngine.type)
assertEquals(searchEngine.resultUrls, readSearchEngine.resultUrls)
assertFalse(readSearchEngine.isGeneral)
Copy link
Copy Markdown
Member Author

@gabrielluong gabrielluong Dec 8, 2022

Choose a reason for hiding this comment

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

Snuck the false check here

Mugurell
Mugurell reviewed Dec 8, 2022
View reviewed changes
Comment thread ...mponents/browser/state/src/main/java/mozilla/components/browser/state/search/SearchEngine.kt
val type: Type,
val resultUrls: List<String> = emptyList(),
val suggestUrl: String? = null,
val isGeneral: Boolean = false,
Copy link
Copy Markdown
Contributor

@Mugurell Mugurell Dec 8, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Wanted to ask about the direction to take in regards to user added search engines.
Because based on https://mozilla-hub.atlassian.net/browse/FNXV2-22038 only "general" search engines can be set as the default:

  • should the default here be true or
  • should we amend Fenix to go with "only general or user added search engines can be set as the default"?

cc @DreVla

Copy link
Copy Markdown
Member Author

@gabrielluong gabrielluong Dec 12, 2022

Choose a reason for hiding this comment

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

So, I thinking we will just set isGeneral when we call createSearchEngine in Fenix https://searchfox.org/mozilla-mobile/source/fenix/app/src/main/java/org/mozilla/fenix/settings/search/AddSearchEngineFragment.kt#180

@mergify
Copy link
Copy Markdown
Contributor

mergify bot commented Dec 8, 2022

This pull request has conflicts when rebasing. Could you fix it @gabrielluong? 🙏

@DreVla DreVla mentioned this pull request Dec 9, 2022
Merged
4 tasks
@gabrielluong gabrielluong force-pushed the 1804594 branch 2 times, most recently from f081f55 to 67baabd Compare December 12, 2022 05:58
@gabrielluong gabrielluong marked this pull request as draft December 12, 2022 06:05
@gabrielluong gabrielluong added work in progress Not ready to land yet. Work in progress (WIP). and removed 🕵️‍♀️ needs review PRs that need to be reviewed labels Dec 12, 2022
@gabrielluong gabrielluong added 🕵️‍♀️ needs review PRs that need to be reviewed and removed work in progress Not ready to land yet. Work in progress (WIP). labels Dec 12, 2022
@gabrielluong gabrielluong marked this pull request as ready for review December 12, 2022 06:50
gabrielluong
gabrielluong commented Dec 12, 2022
View reviewed changes
Comment thread ...feature/search/src/main/java/mozilla/components/feature/search/storage/SearchEngineReader.kt Outdated
type = type,
resultUrls = resultsUrls,
suggestUrl = suggestUrl,
isGeneral = type == SearchEngine.Type.CUSTOM || isGeneralSearchEngine(identifier),
Copy link
Copy Markdown
Member Author

@gabrielluong gabrielluong Dec 12, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I don't know if I am happy with this check type == SearchEngine.Type.CUSTOM with all custom search engines being considered general. However, I am assuming all custom search engines are user added search engines. I could also do something similar to above with the variables name, suggestUrl, etc which will set isGeneral based on the value is that is saved in the file instead of deriving isGeneral from the type.

@gabrielluong
Copy link
Copy Markdown
Member Author

gabrielluong commented Dec 12, 2022 •
edited
Loading

@Mugurell I am wondering if we even need this patch if we could just derive that the search engine is general when it's a CUSTOM type or if it's a BUNDLED and part of the GENERAL_SEARCH_ENGINE_IDS, which could go directly into Fenix as well.

@Mugurell
Copy link
Copy Markdown
Contributor

Mugurell commented Dec 12, 2022

@Mugurell I am wondering if we even need this patch if we could just derive that the search engine is general when it's a CUSTOM type or if it's a BUNDLED and part of the GENERAL_SEARCH_ENGINE_IDS, which could go directly into Fenix as well.

That would work also and is what @DreVla has been doing in his Fenix drafts.
It could be an extension property or we could promote this property in the SearchEngine API. By adding this differentiation in AC this patch helps with encapsulating the logic for what a general/topic specific search engine is (clients should not need to be responsable for this (checking against that list of general search ids)) and allows to easily adapt behavior anywhere in Fenix and AC.
Both approaches seem okay to me but I would incline a bit more to going with with patch with the difference between an extension property and a new internal property to me being related to:

  • whether we want to give more visibility to this by promoting it in the SearchEngine API and as I understand based on the recent discussions this separation will indeed serve as the basis for other important functionalities in the future on all platforms so for me it would make sense to be part of the "official" API,
  • whether we want one eager (this patch) vs multiple lazy type inferences (an extension property). It's probably hard to track the performance differences but based on the above point I would still prefer the property to already be available.

Mugurell
Mugurell approved these changes Dec 12, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@Mugurell Mugurell left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@Mugurell Mugurell removed the 🕵️‍♀️ needs review PRs that need to be reviewed label Dec 12, 2022
@DreVla
Copy link
Copy Markdown
Contributor

DreVla commented Dec 12, 2022

I have been following this patch and also tested it for this ticket: mozilla-mobile/fenix#28112. I have to agree with what @Mugurell said, rather taking this one patch as basis for the many other tasks involving the separation between the two search engines categories in fenix.

@gabrielluong
Copy link
Copy Markdown
Member Author

gabrielluong commented Dec 12, 2022

Sounds good, I will land this after the cut. We'll need to update the changelog.

@gabrielluong gabrielluong added the 🛬 needs landing PRs that are ready to land label Dec 12, 2022
@gabrielluong gabrielluong reopened this Dec 12, 2022
@mergify mergify bot merged commit f99a255 into mozilla-mobile:main Dec 12, 2022
@gabrielluong gabrielluong deleted the 1804594 branch December 12, 2022 21:53
This was referenced Dec 13, 2022
Merged
Merged
SkewedZeppelin added a commit to Divested-Mobile/Mull-Fenix that referenced this pull request Sep 15, 2023
@SkewedZeppelin
Fennec 115.0.0
5ce9bce 
ref:
https://bugzilla.mozilla.org/show_bug.cgi?id=1830530
mozilla-mobile/firefox-android#273

Signed-off-by: Tad <tad@spotco.us>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@Mugurell Mugurell Mugurell approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@Mugurell Mugurell

Labels

🛬 needs landing PRs that are ready to land

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@gabrielluong @Mugurell @DreVla