Issue #8690: Specify the frecency threshold for the fetched top frecent sites #8806

gabrielluong · 2020-10-27T04:29:10Z

Fixes #8690. Requires AS bump with mozilla/application-services#3645.

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.

mergify · 2020-10-27T20:17:43Z

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

jonalmeida · 2020-10-27T20:23:21Z

...ature/top-sites/src/main/java/mozilla/components/feature/top/sites/DefaultTopSitesStorage.kt

@@ -67,7 +68,8 @@ class DefaultTopSitesStorage(

    override suspend fun getTopSites(
        totalSites: Int,
-        includeFrecent: Boolean
+        includeFrecent: Boolean,
+        frecencyThreshold: FrecencyThresholdOption


Does this option change? Maybe just put it in the constructor or leave it internal as part of our implicit feature expectations.

It doesn't change, but we also fetch the option from the TopSiteConfig

grigoryk

Overall this looks good! I think we should simplify the APIs a bit to avoid having illegal combinations around, and make sure to separately test the a-s bump (I'd prefer that in a separate PR, actually).

buildSrc/src/main/java/Dependencies.kt

components/browser/storage-sync/src/main/java/mozilla/components/browser/storage/sync/Types.kt

components/concept/storage/src/main/java/mozilla/components/concept/storage/HistoryStorage.kt

...nents/feature/top-sites/src/main/java/mozilla/components/feature/top/sites/TopSitesConfig.kt

...ents/feature/top-sites/src/main/java/mozilla/components/feature/top/sites/TopSitesStorage.kt

docs/changelog.md

mergify · 2020-11-05T19:01:03Z

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

mergify · 2020-11-06T20:17:31Z

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

grigoryk

Looks good! Just some nits and random musings.

🚢🚢🚢

...age-memory/src/main/java/mozilla/components/browser/storage/memory/InMemoryHistoryStorage.kt

components/browser/storage-sync/src/main/java/mozilla/components/browser/storage/sync/Types.kt

...e/top-sites/src/test/java/mozilla/components/feature/top/sites/DefaultTopSitesStorageTest.kt

grigoryk · 2020-11-09T18:16:10Z

@gabrielluong i'll do a round of testing for the a-s version bump before we land this.

mergify · 2020-11-11T10:55:39Z

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

mergify · 2020-11-16T22:45:31Z

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

mergify · 2020-11-17T16:49:49Z

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

grigoryk · 2020-11-18T18:06:33Z

...orage-sync/src/test/java/mozilla/components/browser/storage/sync/PlacesHistoryStorageTest.kt

        assertEquals(5, infos.size)
        assertEquals("https://www.mozilla.com/foo/bar/baz", infos[0].url)
        assertEquals("https://www.example.com/123", infos[1].url)
        assertEquals("https://news.ycombinator.com/", infos[2].url)
        assertEquals("https://mozilla.com/a1/b2/c3", infos[3].url)
        assertEquals("https://www.example.com/12345", infos[4].url)
+
+        infos = history.getTopFrecentSites(100, frecencyThreshold = FrecencyThresholdOption.SKIP_ONE_TIME_PAGES)


Will it work with negative values? e.g. history.getTopFrecentSites(-4, frecencyThreshold = FrecencyThresholdOption.NONE)

Textbook unit test :)

I added

if (numItems <= 0) { return emptyList() }

but I am wondering if we would prefer doing a require(numItems >= 0)

grigoryk · 2020-11-18T18:23:17Z

...e/top-sites/src/test/java/mozilla/components/feature/top/sites/DefaultTopSitesStorageTest.kt

@@ -213,15 +215,15 @@ class DefaultTopSitesStorageTest {

        val frecentSite2 = TopFrecentSiteInfo("https://example.com", "Example")
        val frecentSite3 = TopFrecentSiteInfo("https://getpocket.com", "Pocket")
-        whenever(historyStorage.getTopFrecentSites(anyInt())).thenReturn(
+        whenever(historyStorage.getTopFrecentSites(anyInt(), any())).thenReturn(


I really recommend doing away with the mocks, and just using the real storage implementation instead. It's a little bit more working setting things up, but really not much more - you'd need to actually insert history items and visits yourself to prep the storage.

For an example of how to set this up, see browser-storage-sync component. Specifically, you may need to copy-paste some bits from build.gradle:

// These dependencies are part of this module's public API. api(Dependencies.mozilla_places) { // Use our own version of the Glean dependency, // which might be different from the version declared by A-S. exclude group: 'org.mozilla.components', module: 'service-glean' } ... testImplementation Dependencies.mozilla_places testImplementation Dependencies.mozilla_full_megazord_forUnitTests testImplementation Dependencies.mozilla_glean_forUnitTests ...

(it may work without the glean stuff, try that first; i don't recall the details right now)

The result should be a much more robust and useful test suite which exercises our whole stack! This will become a good integration test, instead of a "we're calling the right APIs" unit test. The latter is helpful, but the former actually gives you confidence that the system is working correctly.

I spent some time on this, but ran into some issues that I have not managed to figure out yet. I will spin this task into a new issue #9501 to not block this any further.

mergify · 2021-01-07T15:28:02Z

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

codecov · 2021-01-25T23:32:20Z

Codecov Report

Merging #8806 (bca8f9b) into master (813db1d) will decrease coverage by 2.81%.
The diff coverage is 69.23%.

@@             Coverage Diff              @@
##             master    #8806      +/-   ##
============================================
- Coverage     74.57%   71.75%   -2.82%     
+ Complexity     5958     1957    -4001     
============================================
  Files           811      313     -498     
  Lines         29606     9662   -19944     
  Branches       4882     1630    -3252     
============================================
- Hits          22078     6933   -15145     
+ Misses         5055     1908    -3147     
+ Partials       2473      821    -1652

Impacted Files	Coverage Δ	Complexity Δ
...s/browser/storage/memory/InMemoryHistoryStorage.kt	`84.50% <ø> (ø)`	`28.00 <0.00> (ø)`
...lla/components/feature/top/sites/TopSitesConfig.kt	`0.00% <0.00%> (ø)`	`0.00 <0.00> (ø)`
...la/components/feature/top/sites/TopSitesStorage.kt	`0.00% <ø> (ø)`	`0.00 <0.00> (ø)`
...re/top/sites/presenter/DefaultTopSitesPresenter.kt	`53.33% <0.00%> (-8.21%)`	`6.00 <0.00> (ø)`
...nents/browser/storage/sync/PlacesHistoryStorage.kt	`69.23% <100.00%> (ø)`	`24.00 <2.00> (+1.00)`
...a/mozilla/components/browser/storage/sync/Types.kt	`80.43% <100.00%> (+1.36%)`	`0.00 <0.00> (ø)`
...onents/feature/top/sites/DefaultTopSitesStorage.kt	`70.27% <100.00%> (ø)`	`12.00 <0.00> (ø)`
...in/java/mozilla/components/lib/jexl/lexer/Token.kt
...components/feature/accounts/push/SendTabFeature.kt
.../components/browser/engine/system/NestedWebView.kt
... and 496 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 813db1d...dd3a69e. Read the comment docs.

…ched top frecent sites

gabrielluong added the a-s Application Services work needed label Oct 27, 2020

gabrielluong requested a review from grigoryk as a code owner October 27, 2020 04:29

gabrielluong force-pushed the frecency branch 2 times, most recently from 3a96f27 to 2d07c10 Compare October 27, 2020 04:49

jonalmeida reviewed Oct 27, 2020

View reviewed changes

gabrielluong force-pushed the frecency branch from 2d07c10 to f5bb79a Compare October 29, 2020 00:49

gabrielluong requested a review from a team as a code owner October 29, 2020 00:49

gabrielluong force-pushed the frecency branch from bf2583e to d96e557 Compare October 29, 2020 18:25

gabrielluong requested a review from jonalmeida October 29, 2020 20:36

gabrielluong added 🕵️‍♀️ needs review PRs that need to be reviewed and removed a-s Application Services work needed labels Oct 29, 2020

grigoryk suggested changes Oct 29, 2020

View reviewed changes

Amejia481 assigned grigoryk and jonalmeida Nov 2, 2020

gabrielluong force-pushed the frecency branch from d96e557 to 72713b6 Compare November 2, 2020 21:54

gabrielluong requested a review from grigoryk November 2, 2020 21:54

gabrielluong force-pushed the frecency branch 6 times, most recently from 7b76a38 to 5da0d86 Compare November 4, 2020 17:21

gabrielluong requested review from csadilek and pocmo as code owners November 4, 2020 17:21

gabrielluong mentioned this pull request Nov 4, 2020

For #14302 - Skip one time pages visit from frecent top sites mozilla-mobile/fenix#16379

Closed

3 tasks

gabrielluong removed request for pocmo and csadilek November 4, 2020 21:56

gabrielluong force-pushed the frecency branch from 5da0d86 to e24dc47 Compare November 6, 2020 15:55

gabrielluong force-pushed the frecency branch from e24dc47 to 5efb5c7 Compare November 9, 2020 15:21

grigoryk approved these changes Nov 9, 2020

View reviewed changes

gabrielluong force-pushed the frecency branch from 5efb5c7 to 18aeb2b Compare November 10, 2020 23:48

gabrielluong force-pushed the frecency branch from 18aeb2b to 609934f Compare November 16, 2020 16:08

gabrielluong force-pushed the frecency branch from 609934f to 4fe04e3 Compare November 16, 2020 23:29

gabrielluong force-pushed the frecency branch from 4fe04e3 to 391c20e Compare November 17, 2020 20:41

grigoryk reviewed Nov 18, 2020

View reviewed changes

gabrielluong force-pushed the frecency branch from 391c20e to a217d15 Compare January 25, 2021 23:23

Issue mozilla-mobile#8690: Specify the frecency threshold for the fet…

bca8f9b

…ched top frecent sites

gabrielluong force-pushed the frecency branch from a217d15 to bca8f9b Compare January 26, 2021 00:03

gabrielluong mentioned this pull request Jan 26, 2021

Add integration test for PlacesHistoryStorage in DefaultTopSitesStorageTest #9501

Closed

gabrielluong added the 🛬 needs landing PRs that are ready to land label Jan 26, 2021

gabrielluong force-pushed the frecency branch from 4f710ce to bca8f9b Compare January 26, 2021 14:58

Merge branch 'master' into frecency

dd3a69e

mergify bot merged commit bcba13e into mozilla-mobile:master Jan 26, 2021

gabrielluong mentioned this pull request Apr 26, 2022

Closes #24741: add top padding for download and bookmark fragment mozilla-mobile/fenix#24972

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue #8690: Specify the frecency threshold for the fetched top frecent sites #8806

Issue #8690: Specify the frecency threshold for the fetched top frecent sites #8806

gabrielluong commented Oct 27, 2020

mergify bot commented Oct 27, 2020

jonalmeida Oct 27, 2020

gabrielluong Oct 27, 2020

grigoryk left a comment

mergify bot commented Nov 5, 2020

mergify bot commented Nov 6, 2020

grigoryk left a comment

grigoryk commented Nov 9, 2020

mergify bot commented Nov 11, 2020

mergify bot commented Nov 16, 2020

mergify bot commented Nov 17, 2020

grigoryk Nov 18, 2020

gabrielluong Jan 25, 2021

grigoryk Nov 18, 2020

gabrielluong Jan 26, 2021

mergify bot commented Jan 7, 2021

codecov bot commented Jan 25, 2021 •

edited

Loading

Issue #8690: Specify the frecency threshold for the fetched top frecent sites #8806

Issue #8690: Specify the frecency threshold for the fetched top frecent sites #8806

Conversation

gabrielluong commented Oct 27, 2020

Pull Request checklist

After merge

mergify bot commented Oct 27, 2020

jonalmeida Oct 27, 2020

Choose a reason for hiding this comment

gabrielluong Oct 27, 2020

Choose a reason for hiding this comment

grigoryk left a comment

Choose a reason for hiding this comment

mergify bot commented Nov 5, 2020

mergify bot commented Nov 6, 2020

grigoryk left a comment

Choose a reason for hiding this comment

grigoryk commented Nov 9, 2020

mergify bot commented Nov 11, 2020

mergify bot commented Nov 16, 2020

mergify bot commented Nov 17, 2020

grigoryk Nov 18, 2020

Choose a reason for hiding this comment

gabrielluong Jan 25, 2021

Choose a reason for hiding this comment

grigoryk Nov 18, 2020

Choose a reason for hiding this comment

gabrielluong Jan 26, 2021

Choose a reason for hiding this comment

mergify bot commented Jan 7, 2021

codecov bot commented Jan 25, 2021 • edited Loading

Codecov Report

codecov bot commented Jan 25, 2021 •

edited

Loading