Issue #11483: Implement a ContileTopSitesProvider #11525
Conversation
gabrielluong
requested review from
Amejia481 and
csadilek
and removed request for
grigoryk
gabrielluong
force-pushed
the
11483
branch
4 times, most recently
from
4c540f3 to
57a8d71
Compare
...ature/top-sites/src/main/java/mozilla/components/feature/top/sites/DefaultTopSitesStorage.kt
Outdated
Show resolved
Hide resolved
|
This pull request has conflicts when rebasing. Could you fix it @gabrielluong? 🙏 |
| @@ -23,17 +23,21 @@ import kotlin.coroutines.CoroutineContext | |||
| * @param pinnedSitesStorage An instance of [PinnedSiteStorage], used for storing pinned sites. | |||
| * @param historyStorage An instance of [PlacesHistoryStorage], used for retrieving top frecent | |||
| * sites from history. | |||
| * @param topSitesProvider An optional instance of [TopSitesProvider], used for retrieving top | |||
| * sites from a provider. | |||
| * @param defaultTopSites A list containing a title to url pair of default top sites to be added | |||
| * to the [PinnedSiteStorage]. | |||
| */ | |||
| class DefaultTopSitesStorage( | |||
There was a problem hiding this comment.
This is not for now but we have a confusing design (or at least naming) here. The default storage is composed of other storages (PinnedSiteStorage, PlacesHistoryStorage) and now also a provider (TopSitesProvider). It's not clear to consumers at all how top sites are loaded / stored when using DefaultTopSitesStorage.
I could see us refactoring this into a top-level class TopSitesProvider or TopSitesManager :) that accepts a list of top site sources/services. Right now we're passing the storages and the provider to another storage which is quite confusing.
...ature/top-sites/src/main/java/mozilla/components/feature/top/sites/DefaultTopSitesStorage.kt
Outdated
Show resolved
Hide resolved
components/feature/top-sites/src/main/java/mozilla/components/feature/top/sites/TopSite.kt
Outdated
Show resolved
Hide resolved
.../service/contile/src/main/java/mozilla/components/service/contile/ContileTopSitesProvider.kt
Show resolved
Hide resolved
gabrielluong
added
🛬 needs landing
There was a problem hiding this comment.
Drive-by 🚗 : I left some comments that you may want to consider for #11529, which I discovered after reviewing mozilla-mobile/fenix#23410
Nothing urgent or required, just suggestions. 🙂
|
|
||
| private fun fetchTopSites(): List<TopSite.Provided> { | ||
| client.fetch( | ||
| Request(url = CONTILE_ENDPOINT_URL) |
There was a problem hiding this comment.
This endpoint could be good as a parameter with CONTILE_ENDPOINT_URL as the default so we aren't tied to this endpoint (say, if we need a separate one for Focus/Fenix).
| return try { | ||
| JSONObject(responseBody).getTopSites() | ||
| } catch (e: JSONException) { | ||
| throw IOException(e) |
There was a problem hiding this comment.
For JSONObject.toTopSite() we return null on exception, but for JSONObject.getTopSites() we throw here - it's not clear why we would want to throw it on only this one and then catch it to re-throw it above because the consuming app now needs to try/catch their call as well in order to be safe.
Might be prudent to log the exception and return null/emptyList so that we crash the app if the request is successful but the response has nothing in it.
Fixes #11483.
I am taking an incremental approach here. It's best to review commits separately. In followups, I will be looking at adding a WorkManager to periodically fetch along with a disk cache to store the results.
Pull Request checklist
After merge