Bug 1775470: Expose the most important source for a recommendation. r=Standard8
authorDave Townsend <dtownsend@oxymoronical.com>
Mon, 27 Jun 2022 16:09:07 +0000
changeset 622175 b253c011bfb2f3ff801f8be1ba51e9a5c2ac4ce6
parent 622174 5ee1d64fb04a25fa3a0570807d40c4a56a361eac
child 622176 72f7919cc0ebf424c0ce97fbe4dace1e6affa520
push id165269
push userdtownsend@mozilla.com
push dateMon, 27 Jun 2022 16:19:42 +0000
treeherderautoland@b253c011bfb2 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersStandard8
bugs1775470
milestone103.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1775470: Expose the most important source for a recommendation. r=Standard8 Differential Revision: https://phabricator.services.mozilla.com/D150231
browser/components/places/SnapshotScorer.jsm
browser/components/places/SnapshotSelector.jsm
browser/components/places/tests/unit/interactions/head_interactions.js
browser/components/places/tests/unit/interactions/test_snapshotscorer.js
browser/components/places/tests/unit/interactions/test_snapshotscorer_combining.js
browser/components/places/tests/unit/interactions/test_snapshotselection_overlapping.js
browser/components/places/tests/unit/interactions/test_snapshotselection_recent.js
--- a/browser/components/places/SnapshotScorer.jsm
+++ b/browser/components/places/SnapshotScorer.jsm
@@ -30,21 +30,25 @@ XPCOMUtils.defineLazyGetter(lazy, "logCo
 
 /**
  * @typedef {object} Recommendation
  *   A snapshot recommendation with an associated score.
  * @property {Snapshot} snapshot
  *   The recommended snapshot.
  * @property {number} score
  *   The score for this snapshot.
+ * @property {string | undefined} source
+ *   The source that provided the largest score for this snapshot.
  */
 
 /**
  * @typedef {object} RecommendationGroup
  *   A set of recommendations with an associated weight to apply to their scores.
+ * @property {string} source
+ *   The source of the group.
  * @property {Recommendation[]} recommendations
  *   The recommended snapshot.
  * @property {number} weight
  *   The weight for the scores in these recommendations.
  */
 
 /**
  * The snapshot scorer receives sets of snapshots and scores them based on the
@@ -99,68 +103,83 @@ const SnapshotScorer = new (class Snapsh
   combineAndScore(selectionContext, ...recommendationGroups) {
     /**
      * @typedef {object} SnapshotScore
      *   A currently known score for a snapshot.
      * @property {Snapshot} snapshot
      *   The snapshot.
      * @property {number} snapshotScore
      *   The score generated from this snapshot.
-     * @property {number} sourceScore
-     *   The score from the source of the recommendation.
+     * @property {Map<string, number>} sourceScore
+     *   The score from the sources of the recommendation.
      */
 
     /**
      * Maintains the current score for each seen snapshot.
      * @type {Map<string, SnapshotScore>}
      */
     let combined = new Map();
 
     let currentDate = this.#dateOverride ?? Date.now();
     let currentSessionUrls = selectionContext.getCurrentSessionUrls();
 
-    for (let { recommendations, weight } of recommendationGroups) {
+    for (let { source, recommendations, weight } of recommendationGroups) {
       for (let { snapshot, score } of recommendations) {
         if (
           selectionContext.filterAdult &&
           lazy.FilterAdult.isAdultUrl(snapshot.url)
         ) {
           continue;
         }
 
         let currentScore = combined.get(snapshot.url);
         if (currentScore) {
           // We've already generated the snapshot specific score, update the
-          // source specific score.
-          currentScore.sourceScore += score * weight;
+          // source specific scores.
+          currentScore.sourceScore.set(source, score * weight);
         } else {
           currentScore = {
             snapshot,
             snapshotScore: this.#score(
               snapshot,
               currentDate,
               currentSessionUrls
             ),
-            sourceScore: score * weight,
+            sourceScore: new Map([[source, score * weight]]),
           };
 
           combined.set(snapshot.url, currentScore);
         }
       }
     }
 
     let recommendations = [];
     for (let currentScore of combined.values()) {
       let recommendation = {
         snapshot: currentScore.snapshot,
-        score: currentScore.snapshotScore + currentScore.sourceScore,
+        score: currentScore.snapshotScore,
+        source: undefined,
       };
 
+      // Add up all the source scores and identify the largest.
+      let maxScore = null;
+      let source = null;
+      for (let [id, score] of currentScore.sourceScore) {
+        recommendation.score += score;
+
+        if (maxScore === null || maxScore < score) {
+          maxScore = score;
+          source = id;
+        }
+      }
+
+      recommendation.source = source;
+
       lazy.logConsole.debug(
-        `Scored ${recommendation.score} for ${recommendation.snapshot.url}`
+        `Scored ${recommendation.score} for ${recommendation.snapshot.url} from source ${recommendation.source}`
       );
 
       if (recommendation.score >= this.snapshotThreshold) {
         recommendations.push(recommendation);
       }
     }
 
     return this.dedupeSnapshots(recommendations).sort(
--- a/browser/components/places/SnapshotSelector.jsm
+++ b/browser/components/places/SnapshotSelector.jsm
@@ -55,16 +55,18 @@ XPCOMUtils.defineLazyGetter(lazy, "logCo
 
 /**
  * @typedef {object} Recommendation
  *   The details of a specific recommendation for a snapshot.
  * @property {Snapshot} snapshot
  *   The snapshot this recommendation relates to.
  * @property {number} score
  *   The score for the snapshot.
+ * @property {string | undefined} source
+ *   The source that provided the largest score for this snapshot.
  */
 
 /**
  * A snapshot selector is responsible for generating a list of snapshots based
  * on the current context. The context initially is just the url of the page
  * being viewed but will evolve to include things like the search terms that
  * brought the user to that page etc.
  *
@@ -180,29 +182,29 @@ class SnapshotSelector extends EventEmit
     }
 
     this.#task.arm();
   }
 
   /**
    * Called internally when the set of snapshots has been generated.
    *
-   * @param {Snapshot[]} snapshots
+   * @param {Recommendation[]} recommendations
    */
-  #snapshotsGenerated(snapshots) {
+  #snapshotsGenerated(recommendations) {
     // If this instance has been destroyed then do nothing.
     if (!this.#task) {
       return;
     }
 
     lazy.logConsole.debug(
-      "Generated snapshots",
-      snapshots.map(s => s.url)
+      "Generated recommendations",
+      recommendations.map(s => s.snapshot.url)
     );
-    this.emit("snapshots-updated", snapshots);
+    this.emit("snapshots-updated", recommendations);
   }
 
   /**
    * Starts the process of building snapshots.
    */
   async #buildSnapshots() {
     if (this.#context.sourceWeights) {
       await this.#buildRelevancySnapshots();
@@ -229,28 +231,31 @@ class SnapshotSelector extends EventEmit
 
     snapshots = snapshots.filter(snapshot => {
       if (snapshot.url == context.url) {
         return false;
       }
       return !context.filterAdult || !lazy.FilterAdult.isAdultUrl(snapshot.url);
     });
 
-    snapshots = lazy.SnapshotScorer.dedupeSnapshots(
-      snapshots.map(s => ({
-        snapshot: s,
-      }))
-    )
-      .slice(0, context.count)
-      .map(s => s.snapshot)
-      .slice();
+    let recommendations = snapshots.map((snapshot, index) => ({
+      source: "recent",
+      score: snapshots.length - index,
+      snapshot,
+    }));
 
-    lazy.PlacesUIUtils.insertTitleStartDiffs(snapshots);
+    recommendations = lazy.SnapshotScorer.dedupeSnapshots(
+      recommendations
+    ).slice(0, context.count);
 
-    this.#snapshotsGenerated(snapshots);
+    lazy.PlacesUIUtils.insertTitleStartDiffs(
+      recommendations.map(s => s.snapshot)
+    );
+
+    this.#snapshotsGenerated(recommendations);
   }
 
   /**
    * Build snapshots based on relevancy heuristsics.
    *  These include overlapping visits and common referrer, defined by the context options.
    *
    */
   async #buildRelevancySnapshots() {
@@ -279,33 +284,33 @@ class SnapshotSelector extends EventEmit
             recommendations.map(
               r =>
                 `${r.snapshot.url} (score: ${r.score}${
                   r.data ? ", data: " + JSON.stringify(r.data) : ""
                 })`
             )
           );
 
-          return { recommendations, weight };
+          return { source: key, recommendations, weight };
         }
       )
     );
 
     let recommendations = lazy.SnapshotScorer.combineAndScore(
       context,
       ...recommendationGroups
     );
 
-    let snapshots = recommendations
-      .slice(0, context.count)
-      .map(r => r.snapshot);
+    recommendations = recommendations.slice(0, context.count);
 
-    lazy.PlacesUIUtils.insertTitleStartDiffs(snapshots);
+    lazy.PlacesUIUtils.insertTitleStartDiffs(
+      recommendations.map(r => r.snapshot)
+    );
 
-    this.#snapshotsGenerated(snapshots);
+    this.#snapshotsGenerated(recommendations);
   }
 
   /**
    * Update context details and start a rebuild.
    * Undefined properties are ignored, thus pass null to nullify a property.
    * @param {string} [url]
    *  The url of the current context.
    * @param {number} [time]
--- a/browser/components/places/tests/unit/interactions/head_interactions.js
+++ b/browser/components/places/tests/unit/interactions/head_interactions.js
@@ -167,18 +167,20 @@ function assertRecentDate(date, threshol
  * @param {Snapshot|Recommendation} actual
  *  The snapshot to test.
  * @param {Snapshot} expected
  *  The snapshot to test against.
  */
 function assertSnapshot(actual, expected) {
   // This may be a recommendation.
   let score = 0;
+  let source = null;
   if ("snapshot" in actual) {
     score = actual.score;
+    source = actual.source;
     actual = actual.snapshot;
   }
 
   Assert.equal(actual.url, expected.url, "Should have the expected URL");
   let expectedTitle = `test visit for ${expected.url}`;
   if (expected.hasOwnProperty("title")) {
     // We set title in this statement instead of with an OR so that consumers
     // can pass an explicit null.
@@ -213,16 +215,23 @@ function assertSnapshot(actual, expected
   }
   if (expected.commonName) {
     Assert.equal(
       actual.commonName,
       expected.commonName,
       "Should have the Snapshot URL's common name."
     );
   }
+  if (expected.source) {
+    Assert.equal(
+      source,
+      expected.source,
+      "Should have the correct recommendation source."
+    );
+  }
   if (expected.scoreEqualTo != null) {
     Assert.equal(
       score,
       expected.scoreEqualTo,
       "Should have a score equal to the expected score"
     );
   }
   if (expected.scoreGreaterThan != null) {
@@ -427,16 +436,24 @@ function assertRecommendations(recommend
       expected[i].url,
       "Should have returned the expected URL for the recommendation"
     );
     Assert.equal(
       recommendations[i].score,
       expected[i].score,
       `Should have set the expected score for ${expected[i].url}`
     );
+
+    if (expected[i].source) {
+      Assert.equal(
+        recommendations[i].source,
+        expected[i].source,
+        `Should have set the correct source for ${expected[i].url}`
+      );
+    }
   }
 }
 
 /**
  * Abstracts getting the right moz-page-thumb url depending on enabled features.
  * @param {string} url
  *  The page url to get a screenshot for.
  * @returns {string} a moz-page-thumb:// url
--- a/browser/components/places/tests/unit/interactions/test_snapshotscorer.js
+++ b/browser/components/places/tests/unit/interactions/test_snapshotscorer.js
@@ -208,24 +208,26 @@ add_task(async function test_scores() {
     let url = `https://example.com/${i}`;
     let snapshot = await Snapshots.get(url, true);
 
     handleSnapshotSetup(data, snapshot, sessionUrls);
 
     let snapshots = await SnapshotScorer.combineAndScore(
       { getCurrentSessionUrls: () => sessionUrls },
       {
+        source: "foo",
         recommendations: [{ snapshot, score: data.sourceScore ?? 0 }],
         weight: 3.0,
       }
     );
 
     assertRecommendations(snapshots, [
       {
         url,
+        source: "foo",
         score: data.score,
       },
     ]);
   }
 });
 
 add_task(async function test_score_threshold() {
   const THRESHOLD = 4;
@@ -242,22 +244,23 @@ add_task(async function test_score_thres
     sourceRecommendations.push({
       snapshot,
       score: THRESHOLD_TESTS[i].sourceScore,
     });
   }
 
   let snapshots = await SnapshotScorer.combineAndScore(
     { getCurrentSessionUrls: () => sessionUrls },
-    { recommendations: sourceRecommendations, weight: 3.0 }
+    { source: "bar", recommendations: sourceRecommendations, weight: 3.0 }
   );
 
   assertRecommendations(
     snapshots,
     // map before filter so that we get the url values correct.
     THRESHOLD_TESTS.map((t, i) => {
       return {
         url: `https://example.com/${i + SCORE_TESTS.length}`,
+        source: "bar",
         score: t.score,
       };
     }).filter(t => t.score > THRESHOLD)
   );
 });
--- a/browser/components/places/tests/unit/interactions/test_snapshotscorer_combining.js
+++ b/browser/components/places/tests/unit/interactions/test_snapshotscorer_combining.js
@@ -28,63 +28,71 @@ add_task(async function setup() {
 
 add_task(async function test_combining_throw_away_first() {
   let snapshot1 = await Snapshots.get(TEST_URL1);
   let snapshot2 = await Snapshots.get(TEST_URL2);
 
   let combined = SnapshotScorer.combineAndScore(
     { getCurrentSessionUrls: () => new Set([TEST_URL1, TEST_URL2]) },
     {
+      source: "foo",
       recommendations: [{ snapshot: snapshot1, score: 0.5 }],
       weight: 3.0,
     },
     {
+      source: "bar",
       recommendations: [
         { snapshot: snapshot2, score: 0.5 },
         { snapshot: snapshot1, score: 1 },
       ],
       weight: 3.0,
     }
   );
 
   assertRecommendations(combined, [
     {
       url: TEST_URL1,
       score: 7.5,
+      source: "bar",
     },
     {
       url: TEST_URL2,
       score: 4.5,
+      source: "bar",
     },
   ]);
 });
 
 add_task(async function test_combining_throw_away_second_and_sort() {
   // We swap the snapshots around a bit here to additionally test the sort.
   let snapshot1 = await Snapshots.get(TEST_URL1);
   let snapshot2 = await Snapshots.get(TEST_URL2);
 
   let combined = SnapshotScorer.combineAndScore(
     { getCurrentSessionUrls: () => new Set([TEST_URL1, TEST_URL2]) },
     {
       recommendations: [{ snapshot: snapshot2, score: 1 }],
       weight: 3.0,
+      source: "foo",
     },
     {
       recommendations: [
         { snapshot: snapshot1, score: 0.5 },
         { snapshot: snapshot2, score: 0.5 },
       ],
       weight: 3.0,
+      source: "bar",
     }
   );
 
   assertRecommendations(combined, [
     {
       url: TEST_URL2,
       score: 7.5,
+      source: "foo",
     },
     {
       url: TEST_URL1,
       score: 4.5,
+      source: "bar",
     },
   ]);
 });
--- a/browser/components/places/tests/unit/interactions/test_snapshotselection_overlapping.js
+++ b/browser/components/places/tests/unit/interactions/test_snapshotselection_overlapping.js
@@ -63,17 +63,19 @@ add_task(async function test_enable_over
   await Snapshots.add({ url: TEST_URL2 });
   await Snapshots.add({ url: TEST_URL3 });
   await Snapshots.add({ url: TEST_URL4 });
 
   selector.updateDetailsAndRebuild({ url: TEST_URL1 });
   snapshots = await snapshotPromise;
 
   // Only snapshots with overlapping interactions should be selected
-  await assertSnapshotList(snapshots, [{ url: TEST_URL2 }]);
+  await assertSnapshotList(snapshots, [
+    { url: TEST_URL2, source: "Overlapping" },
+  ]);
 });
 
 add_task(async function test_overlapping_with_scoring() {
   // Reset the threshold, the snapshot should be lower than the required score.
   Services.prefs.clearUserPref("browser.places.snapshots.threshold");
 
   let snapshotPromise = selector.once("snapshots-updated");
   selector.rebuild();
@@ -84,10 +86,12 @@ add_task(async function test_overlapping
   // Boost the score of the expected snapshot by adding it to the current url
   // set.
   currentSessionUrls.add(TEST_URL2);
 
   snapshotPromise = selector.once("snapshots-updated");
   selector.rebuild();
   snapshots = await snapshotPromise;
 
-  await assertSnapshotList(snapshots, [{ url: TEST_URL2 }]);
+  await assertSnapshotList(snapshots, [
+    { url: TEST_URL2, source: "Overlapping" },
+  ]);
 });
--- a/browser/components/places/tests/unit/interactions/test_snapshotselection_recent.js
+++ b/browser/components/places/tests/unit/interactions/test_snapshotselection_recent.js
@@ -26,51 +26,60 @@ add_task(async function test_interaction
   let snapshots = await snapshotPromise;
 
   await assertSnapshotList(snapshots, []);
 
   snapshotPromise = selector.once("snapshots-updated");
   await Snapshots.add({ url: TEST_URL1 });
   snapshots = await snapshotPromise;
 
-  await assertSnapshotList(snapshots, [{ url: TEST_URL1 }]);
+  await assertSnapshotList(snapshots, [{ url: TEST_URL1, source: "recent" }]);
 
   // Changing the url should generate new snapshots and should exclude the
   // current url.
   snapshotPromise = selector.once("snapshots-updated");
   selector.updateDetailsAndRebuild({ url: TEST_URL1 });
   snapshots = await snapshotPromise;
 
   await assertSnapshotList(snapshots, []);
 
   snapshotPromise = selector.once("snapshots-updated");
   selector.updateDetailsAndRebuild({ url: TEST_URL2 });
   snapshots = await snapshotPromise;
 
-  await assertSnapshotList(snapshots, [{ url: TEST_URL1 }]);
+  await assertSnapshotList(snapshots, [{ url: TEST_URL1, source: "recent" }]);
 
   snapshotPromise = selector.once("snapshots-updated");
   await Snapshots.add({ url: TEST_URL2 });
   snapshots = await snapshotPromise;
 
-  await assertSnapshotList(snapshots, [{ url: TEST_URL1 }]);
+  await assertSnapshotList(snapshots, [{ url: TEST_URL1, source: "recent" }]);
 
   snapshotPromise = selector.once("snapshots-updated");
   await Snapshots.add({ url: TEST_URL3 });
   snapshots = await snapshotPromise;
 
-  await assertSnapshotList(snapshots, [{ url: TEST_URL1 }, { url: TEST_URL3 }]);
+  await assertSnapshotList(snapshots, [
+    { url: TEST_URL1, source: "recent" },
+    { url: TEST_URL3, source: "recent" },
+  ]);
 
   snapshotPromise = selector.once("snapshots-updated");
   selector.updateDetailsAndRebuild({ url: TEST_URL3 });
   snapshots = await snapshotPromise;
 
-  await assertSnapshotList(snapshots, [{ url: TEST_URL2 }, { url: TEST_URL1 }]);
+  await assertSnapshotList(snapshots, [
+    { url: TEST_URL2, source: "recent" },
+    { url: TEST_URL1, source: "recent" },
+  ]);
 
   snapshotPromise = selector.once("snapshots-updated");
   selector.updateDetailsAndRebuild({ url: TEST_URL4 });
   snapshots = await snapshotPromise;
 
   // The snapshot count is limited to 2.
-  await assertSnapshotList(snapshots, [{ url: TEST_URL2 }, { url: TEST_URL1 }]);
+  await assertSnapshotList(snapshots, [
+    { url: TEST_URL2, source: "recent" },
+    { url: TEST_URL1, source: "recent" },
+  ]);
 
   await reset();
 });