From 4f3bdad9cbab520264e5565f9fe2f238ddc15635 Mon Sep 17 00:00:00 2001 From: Mark Banner Date: Thu, 24 Mar 2022 18:44:27 +0000 Subject: [PATCH] Bug 1759452 - When querying snapshot groups, take into account hidden snapshots. r=mak Differential Revision: https://phabricator.services.mozilla.com/D141374 --- browser/components/places/SnapshotGroups.jsm | 12 ++++- .../unit/interactions/test_snapshot_groups.js | 50 ++++++++++++++++++- 2 files changed, 58 insertions(+), 4 deletions(-) diff --git a/browser/components/places/SnapshotGroups.jsm b/browser/components/places/SnapshotGroups.jsm index b2d640d35a37..82ab89a5ede6 100644 --- a/browser/components/places/SnapshotGroups.jsm +++ b/browser/components/places/SnapshotGroups.jsm @@ -284,6 +284,8 @@ const SnapshotGroups = new (class SnapshotGroups { * Use -1 to specify no limit. * @param {boolean} [options.hidden] * Pass true to also return hidden groups. + * @param {boolean} [options.countHidden] + * Pass true to include hidden snapshots in the count. * @param {string} [options.builder] * Limit searching snapshot groups to results from a particular builder. * @param {boolean} [options.skipMinimum] @@ -297,6 +299,7 @@ const SnapshotGroups = new (class SnapshotGroups { limit = 50, builder = undefined, hidden = false, + countHidden = false, skipMinimum = false, } = {}) { let db = await PlacesUtils.promiseDBConnection(); @@ -304,6 +307,7 @@ const SnapshotGroups = new (class SnapshotGroups { let params = {}; let sizeFragment = []; let limitFragment = ""; + let joinFragment = ""; if (!skipMinimum) { sizeFragment.push("HAVING snapshot_count >= :minGroupSize"); params.minGroupSize = MIN_GROUP_SIZE; @@ -329,6 +333,10 @@ const SnapshotGroups = new (class SnapshotGroups { whereTerms.push("g.hidden = 0"); } + if (!countHidden) { + joinFragment = "AND s.hidden = 0"; + } + let where = whereTerms.length ? `WHERE ${whereTerms.join(" AND ")}` : ""; let rows = await db.executeCached( @@ -342,14 +350,14 @@ const SnapshotGroups = new (class SnapshotGroups { SELECT preview_image_url, url FROM moz_places_metadata_snapshots sns JOIN moz_places_metadata_groups_to_snapshots gs USING(place_id) - JOIN moz_places h ON h.id = gs.place_id + JOIN moz_places h ON h.id = gs.place_id AND gs.hidden = 0 WHERE gs.group_id = g.id ORDER BY sns.last_interaction_at ASC LIMIT 2 ) ) AS image_urls FROM moz_places_metadata_snapshots_groups g - LEFT JOIN moz_places_metadata_groups_to_snapshots s ON s.group_id = g.id + LEFT JOIN moz_places_metadata_groups_to_snapshots s ON s.group_id = g.id ${joinFragment} LEFT JOIN moz_places_metadata_snapshots sn ON sn.place_id = s.place_id ${where} GROUP BY g.id ${sizeFragment.join(" ")} diff --git a/browser/components/places/tests/unit/interactions/test_snapshot_groups.js b/browser/components/places/tests/unit/interactions/test_snapshot_groups.js index 578fac5bbe3e..755d25eac939 100644 --- a/browser/components/places/tests/unit/interactions/test_snapshot_groups.js +++ b/browser/components/places/tests/unit/interactions/test_snapshot_groups.js @@ -6,7 +6,9 @@ */ const TEST_URL1 = "https://example.com/"; +const TEST_IMAGE_URL1 = "https://example.com/preview1.png"; const TEST_URL2 = "https://example.com/12345"; +const TEST_IMAGE_URL2 = "https://example.com/preview2.png"; const TEST_URL3 = "https://example.com/67890"; const TEST_URL4 = "https://example.com/135246"; const TEST_URL5 = "https://example.com/531246"; @@ -92,7 +94,7 @@ add_task(async function test_add_and_query() { }); info("add a featured image for second oldest snapshot"); - let previewImageURL = "https://example.com/preview2.png"; + let previewImageURL = TEST_IMAGE_URL2; await PlacesUtils.history.update({ url: TEST_URL2, previewImageURL, @@ -108,7 +110,7 @@ add_task(async function test_add_and_query() { }); info("add a featured image for the oldest snapshot"); - previewImageURL = "https://example.com/preview1.png"; + previewImageURL = TEST_IMAGE_URL1; await PlacesUtils.history.update({ url: TEST_URL1, previewImageURL, @@ -444,9 +446,28 @@ add_task(async function test_get_snapshots_sortBy() { add_task(async function test_get_snapshots_hidden() { let groups = await SnapshotGroups.query({ skipMinimum: true }); Assert.equal(groups.length, 1, "Should return 1 SnapshotGroup"); + Assert.equal( + groups[0].snapshotCount, + 3, + "Should have the correct count of snapshots" + ); await SnapshotGroups.setUrlHidden(groups[0].id, TEST_URL2, true); + groups = await SnapshotGroups.query({ skipMinimum: true }); + Assert.equal( + groups[0].snapshotCount, + 2, + "Should have the correct count of snapshots when not counting hidden ones" + ); + + groups = await SnapshotGroups.query({ skipMinimum: true, countHidden: true }); + Assert.equal( + groups[0].snapshotCount, + 3, + "Should have the correct count of snapshots when counting hidden ones" + ); + let snapshots = await SnapshotGroups.getSnapshots({ id: groups[0].id, sortBy: "last_interaction_at", @@ -467,6 +488,13 @@ add_task(async function test_get_snapshots_hidden() { await SnapshotGroups.setUrlHidden(groups[0].id, TEST_URL2, false); + groups = await SnapshotGroups.query({ skipMinimum: true }); + Assert.equal( + groups[0].snapshotCount, + 3, + "Should have the correct count of snapshots when not counting hidden ones" + ); + snapshots = await SnapshotGroups.getSnapshots({ id: groups[0].id, sortBy: "last_interaction_at", @@ -479,6 +507,24 @@ add_task(async function test_get_snapshots_hidden() { ]); }); +add_task(async function test_snapshot_image_with_hidden() { + let groups = await SnapshotGroups.query({ skipMinimum: true }); + Assert.equal(groups.length, 1, "Should return 1 SnapshotGroup"); + + // The images were set on TEST_URL1/TEST_URL2 in an earlier test. + assertSnapshotGroup(groups[0], { + imageUrl: TEST_IMAGE_URL1, + }); + + await SnapshotGroups.setUrlHidden(groups[0].id, TEST_URL1, true); + + groups = await SnapshotGroups.query({ skipMinimum: true }); + info("Should use the oldest non-hidden image"); + assertSnapshotGroup(groups[0], { + imageUrl: TEST_IMAGE_URL2, + }); +}); + add_task(async function test_minimum_size() { let urls = [TEST_URL1, TEST_URL2, TEST_URL3]; let groupId = await SnapshotGroups.add(