Bug 1858789 - vastly more efficient counting of bookmarks for the hom… #4077

mhammond · 2023-10-13T13:54:32Z

…e activity.

Relies on mozilla/application-services#5853 which was very recently merged - hopefully the Nightlies job will see it 😅 .

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

Breaking Changes: If this is a breaking Android Components change, please push a draft PR on Reference Browser to address the breaking issues.

To download an APK when reviewing a PR (after all CI tasks finished running):

Click on Checks at the top of the PR page.
Click on the firefoxci-taskcluster group on the left to expand all tasks.
Click on the build-apk-{fenix,focus,klar}-debug task you're interested in.
Click on View task in Taskcluster in the new DETAILS section.
The APK links should be on the right side of the screen, named for each CPU architecture.

GitHub Automation

https://bugzilla.mozilla.org/show_bug.cgi?id=1858789

mhammond · 2023-10-13T13:56:03Z

fenix/app/src/main/java/org/mozilla/fenix/library/bookmarks/BookmarkItemMenu.kt

@@ -36,7 +36,7 @@ class BookmarkItemMenu(
    @VisibleForTesting
    @SuppressWarnings("LongMethod")
    internal suspend fun menuItems(itemType: BookmarkNodeType, itemId: String): List<TextMenuCandidate> {
-        val hasAtLeastOneChild = !context.bookmarkStorage.getTree(itemId)?.children.isNullOrEmpty()
+        val hasAtLeastOneChild = !context.bookmarkStorage.getTree(itemId, false)?.children.isNullOrEmpty()


This isn't technically part of the home activity changes, but while looking for other suspect uses of getTree I discovered this, which doesn't actually need the full content of each child but will work just fine with just the IDs of the children.

fenix/app/src/main/java/org/mozilla/fenix/library/bookmarks/DesktopFolders.kt

jonalmeida · 2023-10-18T20:43:38Z

@rahulsainani would you be interested in reviewing this patch? I see that you had written an extension function in this that does some clean-up so figured you might be curious to try this out.

As a further follow-up to BZ-1836447, we can reduce the HomeActivity by moving this out entirely and doing it some kind of telemetry observer that listens to lifecycle events. I'm not sure why we are doing this in onPause to begin with, but let's not change that part for now.

rahulsainani · 2023-11-10T13:16:16Z

Apologies, i haven't been able to look at this till now. Just got pinged by Jeff so I've added him as reviewer.

boek

Looks good! Could you add a Changelog entry before you merge? :)

fenix/app/src/main/java/org/mozilla/fenix/library/bookmarks/DesktopFolders.kt

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

mhammond · 2023-11-16T16:49:53Z

Thanks! Added a changelog entry and added a "needs landing" label, so I hope the rest takes care of itself :)

jonalmeida · 2023-11-16T19:54:17Z

Thanks! Added a changelog entry and added a "needs landing" label, so I hope the rest takes care of itself :)

Removing the label, sorry! I see some legitimate failures in the linter task and a unit test failure with DesktopFoldersTest. countBookmarks (ignore the intermittent failures from the other tasks).

mergify · 2023-11-20T16:09:06Z

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

mergify · 2023-11-22T16:28:22Z

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

jonalmeida · 2023-12-04T22:59:36Z

Spoke to mark offline and recommended two changes:

Let's remove the previous count() method so that we don't keep using it.
There is a failure in the unit test countBookmarks that is failing because of a long standing bug in mockk. Let's remove this test in that case, because it doesn't add enough value compared to trying to land it with a workaround for this test framework bug.

java.lang.ClassCastException: class java.lang.Integer cannot be cast to class kotlin.UInt (java.lang.Integer is in module java.base of loader 'bootstrap'; kotlin.UInt is in unnamed module of loader 'app')

cc: @boek

github-actions · 2023-12-12T15:59:45Z

🚧 Commit message is using the wrong format: remove count() method

The comment message should look like:

    Bug xxxx - Short description of your change
    Optionally, a longer description of the change.

mergify · 2023-12-12T16:11:11Z

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

mhammond · 2023-12-12T18:30:11Z

Best I can tell the only failures here are the ones main has.

jonalmeida

Cheers!

I left one comment below which I'll quickly fix myself so that we can land this now.

jonalmeida · 2023-12-12T22:04:42Z

fenix/app/src/test/java/org/mozilla/fenix/library/bookmarks/DesktopFoldersTest.kt

@@ -42,6 +43,7 @@ class DesktopFoldersTest {
    fun setup() {
        context = spyk(testContext)
        every { context.components.core.bookmarksStorage } returns mockk()
+        coEvery { context.components.core.bookmarksStorage.countBookmarksInTrees(any()) } returns 0U


This line will throw errors if someone tries to make an assertion that executes this line in a future test. It might be best to remove it to avoid a footgun in the future.

…e activity.

github-actions bot added the 🕵️‍♀️ needs review PRs that need to be reviewed label Oct 13, 2023

mhammond commented Oct 13, 2023

View reviewed changes

fenix/app/src/main/java/org/mozilla/fenix/library/bookmarks/DesktopFolders.kt Show resolved Hide resolved

rahulsainani requested a review from boek November 10, 2023 13:15

github-actions bot assigned boek Nov 10, 2023

boek approved these changes Nov 15, 2023

View reviewed changes

fenix/app/src/main/java/org/mozilla/fenix/library/bookmarks/DesktopFolders.kt Show resolved Hide resolved

...storage-sync/src/main/java/mozilla/components/browser/storage/sync/PlacesBookmarksStorage.kt Show resolved Hide resolved

boek added approved PR that has been approved and removed 🕵️‍♀️ needs review PRs that need to be reviewed labels Nov 15, 2023

mhammond added the 🛬 needs landing PRs that are ready to land label Nov 16, 2023

jonalmeida removed the 🛬 needs landing PRs that are ready to land label Nov 16, 2023

mhammond requested a review from boek December 12, 2023 18:30

jonalmeida approved these changes Dec 12, 2023

View reviewed changes

Bug 1858789 - vastly more efficient counting of bookmarks for the hom…

6e086ac

…e activity.

github-actions bot assigned jonalmeida Dec 12, 2023

jonalmeida added the 🛬 needs landing PRs that are ready to land label Dec 12, 2023

Merge branch 'main' into count-bookmarks-in-trees

d4a87a7

mergify bot merged commit 79047f7 into mozilla-mobile:main Dec 12, 2023
122 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug 1858789 - vastly more efficient counting of bookmarks for the hom… #4077

Bug 1858789 - vastly more efficient counting of bookmarks for the hom… #4077

mhammond commented Oct 13, 2023 •

edited by github-actions bot

Loading

mhammond Oct 13, 2023

jonalmeida commented Oct 18, 2023

rahulsainani commented Nov 10, 2023

boek left a comment

mhammond commented Nov 16, 2023

jonalmeida commented Nov 16, 2023 •

edited

Loading

mergify bot commented Nov 20, 2023

mergify bot commented Nov 22, 2023

jonalmeida commented Dec 4, 2023

github-actions bot commented Dec 12, 2023

mergify bot commented Dec 12, 2023

mhammond commented Dec 12, 2023

jonalmeida left a comment

jonalmeida Dec 12, 2023

Bug 1858789 - vastly more efficient counting of bookmarks for the hom… #4077

Bug 1858789 - vastly more efficient counting of bookmarks for the hom… #4077

Conversation

mhammond commented Oct 13, 2023 • edited by github-actions bot Loading

Pull Request checklist

After merge

To download an APK when reviewing a PR (after all CI tasks finished running):

GitHub Automation

mhammond Oct 13, 2023

Choose a reason for hiding this comment

jonalmeida commented Oct 18, 2023

rahulsainani commented Nov 10, 2023

boek left a comment

Choose a reason for hiding this comment

mhammond commented Nov 16, 2023

jonalmeida commented Nov 16, 2023 • edited Loading

mergify bot commented Nov 20, 2023

mergify bot commented Nov 22, 2023

jonalmeida commented Dec 4, 2023

github-actions bot commented Dec 12, 2023

mergify bot commented Dec 12, 2023

mhammond commented Dec 12, 2023

jonalmeida left a comment

Choose a reason for hiding this comment

jonalmeida Dec 12, 2023

Choose a reason for hiding this comment

mhammond commented Oct 13, 2023 •

edited by github-actions bot

Loading

jonalmeida commented Nov 16, 2023 •

edited

Loading