Bug 1730876 - Never discard quick suggest results and dedupe other URL results with a lower or same prefix rank. r=mak, a=RyanVM
authorDrew Willcoxon <adw@mozilla.com>
Thu, 16 Sep 2021 16:12:01 +0000
changeset 661190 3da3592431fea18186f09e641d3f6527e09ced32
parent 661189 d97a69bf7c941da02e1d029f28121f6b968244a4
child 661191 fc8363fb4e3eb4802304e3673bda3d46abda5ee2
push id2662
push userryanvm@gmail.com
push dateMon, 20 Sep 2021 20:26:13 +0000
treeherdermozilla-release@0531f02f1262 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmak, RyanVM
bugs1730876
milestone92.0.1
Bug 1730876 - Never discard quick suggest results and dedupe other URL results with a lower or same prefix rank. r=mak, a=RyanVM This makes a couple of changes: * Never discard quick suggest results. At some point we may want to revisit, but right now we should always show them. * Discard other URL results whose URLs dupe the URLs of quick suggest results or whose URLs have a lower prefix rank. The first change by itself fixes the bug, but without the second change, we'll still show the eBay history result above the eBay quick suggest, which I confirmed with Natalie is not what we want. We only want to show the quick suggest. Keep in mind that the results in this case have the same URLs, same prefixes. When a non-quick-suggest result has a *higher* prefix rank, I think we should *not* discard it. In that case we should show both the quick suggest with its lower prefix rank and the other result with its higher rank. Differential Revision: https://phabricator.services.mozilla.com/D125768
browser/components/urlbar/UrlbarMuxerUnifiedComplete.jsm
browser/components/urlbar/tests/unit/test_quicksuggest.js
--- a/browser/components/urlbar/UrlbarMuxerUnifiedComplete.jsm
+++ b/browser/components/urlbar/UrlbarMuxerUnifiedComplete.jsm
@@ -579,16 +579,22 @@ class MuxerUnifiedComplete extends Urlba
    * @param {UrlbarResult} result
    *   The result.
    * @param {object} state
    *   Global state that we use to make decisions during this sort.
    * @returns {boolean}
    *   True if the result can be added and false if it should be discarded.
    */
   _canAddResult(result, state) {
+    // Never discard quick suggest results. We may want to change this logic at
+    // some point, but for all current use cases, they should always be shown.
+    if (result.providerName == "UrlbarProviderQuickSuggest") {
+      return true;
+    }
+
     // We expect UrlbarProviderPlaces sent us the highest-ranked www. and non-www
     // origins, if any. Now, compare them to each other and to the heuristic
     // result.
     //
     // 1. If the heuristic result is lower ranked than both, discard the www
     //    origin, unless it has a different page title than the non-www
     //    origin. This is a guard against deduping when www.site.com and
     //    site.com have different content.
@@ -865,17 +871,26 @@ class MuxerUnifiedComplete extends Urlba
           stripHttps: true,
           stripWww: true,
           trimEmptyQuery: true,
         }
       );
       let prefixRank = UrlbarUtils.getPrefixRank(prefix);
       let topPrefixData = state.strippedUrlToTopPrefixAndTitle.get(strippedUrl);
       let topPrefixRank = topPrefixData ? topPrefixData.rank : -1;
-      if (topPrefixRank < prefixRank) {
+      if (
+        topPrefixRank < prefixRank ||
+        // If a quick suggest result has the same stripped URL and prefix rank
+        // as another result, store the quick suggest as the top rank so we
+        // discard the other during deduping. That happens after the user picks
+        // the quick suggest: The URL is added to history and later both a
+        // history result and the quick suggest may match a query.
+        (topPrefixRank == prefixRank &&
+          result.providerName == "UrlbarProviderQuickSuggest")
+      ) {
         // strippedUrl => { prefix, title, rank, providerName }
         state.strippedUrlToTopPrefixAndTitle.set(strippedUrl, {
           prefix,
           title: result.payload.title,
           rank: prefixRank,
           providerName: result.providerName,
         });
       }
--- a/browser/components/urlbar/tests/unit/test_quicksuggest.js
+++ b/browser/components/urlbar/tests/unit/test_quicksuggest.js
@@ -10,16 +10,20 @@ XPCOMUtils.defineLazyModuleGetters(this,
   UrlbarProviderQuickSuggest:
     "resource:///modules/UrlbarProviderQuickSuggest.jsm",
   UrlbarQuickSuggest: "resource:///modules/UrlbarQuickSuggest.jsm",
 });
 
 const SPONSORED_SEARCH_STRING = "frab";
 const NONSPONSORED_SEARCH_STRING = "nonspon";
 
+const HTTP_SEARCH_STRING = "http prefix";
+const HTTPS_SEARCH_STRING = "https prefix";
+const PREFIX_SUGGESTIONS_STRIPPED_URL = "example.com/prefix-test";
+
 const REMOTE_SETTINGS_DATA = [
   {
     id: 1,
     url: "http://test.com/q=frabbits",
     title: "frabbits",
     keywords: [SPONSORED_SEARCH_STRING],
     click_url: "http://click.reporting.test.com/",
     impression_url: "http://impression.reporting.test.com/",
@@ -30,16 +34,34 @@ const REMOTE_SETTINGS_DATA = [
     url: "http://test.com/?q=nonsponsored",
     title: "Non-Sponsored",
     keywords: [NONSPONSORED_SEARCH_STRING],
     click_url: "http://click.reporting.test.com/nonsponsored",
     impression_url: "http://impression.reporting.test.com/nonsponsored",
     advertiser: "TestAdvertiserNonSponsored",
     iab_category: "5 - Education",
   },
+  {
+    id: 3,
+    url: "http://" + PREFIX_SUGGESTIONS_STRIPPED_URL,
+    title: "http suggestion",
+    keywords: [HTTP_SEARCH_STRING],
+    click_url: "http://click.reporting.test.com/prefix",
+    impression_url: "http://impression.reporting.test.com/prefix",
+    advertiser: "TestAdvertiserPrefix",
+  },
+  {
+    id: 4,
+    url: "https://" + PREFIX_SUGGESTIONS_STRIPPED_URL,
+    title: "https suggestion",
+    keywords: [HTTPS_SEARCH_STRING],
+    click_url: "http://click.reporting.test.com/prefix",
+    impression_url: "http://impression.reporting.test.com/prefix",
+    advertiser: "TestAdvertiserPrefix",
+  },
 ];
 
 const EXPECTED_SPONSORED_RESULT = {
   type: UrlbarUtils.RESULT_TYPE.URL,
   source: UrlbarUtils.RESULT_SOURCE.SEARCH,
   heuristic: false,
   payload: {
     qsSuggestion: "frab",
@@ -72,16 +94,56 @@ const EXPECTED_NONSPONSORED_RESULT = {
     sponsoredAdvertiser: "testadvertisernonsponsored",
     isSponsored: false,
     helpUrl: UrlbarProviderQuickSuggest.helpUrl,
     helpL10nId: "firefox-suggest-urlbar-learn-more",
     displayUrl: "http://test.com/?q=nonsponsored",
   },
 };
 
+const EXPECTED_HTTP_RESULT = {
+  type: UrlbarUtils.RESULT_TYPE.URL,
+  source: UrlbarUtils.RESULT_SOURCE.SEARCH,
+  heuristic: false,
+  payload: {
+    qsSuggestion: HTTP_SEARCH_STRING,
+    title: "http suggestion",
+    url: "http://" + PREFIX_SUGGESTIONS_STRIPPED_URL,
+    icon: null,
+    sponsoredImpressionUrl: "http://impression.reporting.test.com/prefix",
+    sponsoredClickUrl: "http://click.reporting.test.com/prefix",
+    sponsoredBlockId: 3,
+    sponsoredAdvertiser: "testadvertiserprefix",
+    isSponsored: true,
+    helpUrl: UrlbarProviderQuickSuggest.helpUrl,
+    helpL10nId: "firefox-suggest-urlbar-learn-more",
+    displayUrl: "http://" + PREFIX_SUGGESTIONS_STRIPPED_URL,
+  },
+};
+
+const EXPECTED_HTTPS_RESULT = {
+  type: UrlbarUtils.RESULT_TYPE.URL,
+  source: UrlbarUtils.RESULT_SOURCE.SEARCH,
+  heuristic: false,
+  payload: {
+    qsSuggestion: HTTPS_SEARCH_STRING,
+    title: "https suggestion",
+    url: "https://" + PREFIX_SUGGESTIONS_STRIPPED_URL,
+    icon: null,
+    sponsoredImpressionUrl: "http://impression.reporting.test.com/prefix",
+    sponsoredClickUrl: "http://click.reporting.test.com/prefix",
+    sponsoredBlockId: 4,
+    sponsoredAdvertiser: "testadvertiserprefix",
+    isSponsored: true,
+    helpUrl: UrlbarProviderQuickSuggest.helpUrl,
+    helpL10nId: "firefox-suggest-urlbar-learn-more",
+    displayUrl: PREFIX_SUGGESTIONS_STRIPPED_URL,
+  },
+};
+
 add_task(async function init() {
   UrlbarPrefs.set("quicksuggest.enabled", true);
   UrlbarPrefs.set("quicksuggest.shouldShowOnboardingDialog", false);
   UrlbarPrefs.set("quicksuggest.remoteSettings.enabled", true);
   UrlbarPrefs.set("merino.enabled", false);
 
   // Install a default test engine.
   let engine = await addTestSuggestionsEngine();
@@ -476,8 +538,122 @@ add_task(async function generalBeforeSug
     ],
   });
 
   UrlbarPrefs.clear("browser.search.suggest.enabled");
   UrlbarPrefs.clear("suggest.searches");
   UrlbarPrefs.clear("showSearchSuggestionsFirst");
   await PlacesUtils.history.clear();
 });
+
+add_task(async function dedupeAgainstURL_samePrefix() {
+  await doDedupeAgainstURLTest({
+    searchString: HTTP_SEARCH_STRING,
+    expectedQuickSuggestResult: EXPECTED_HTTP_RESULT,
+    otherPrefix: "http://",
+    expectOther: false,
+  });
+});
+
+add_task(async function dedupeAgainstURL_higherPrefix() {
+  await doDedupeAgainstURLTest({
+    searchString: HTTPS_SEARCH_STRING,
+    expectedQuickSuggestResult: EXPECTED_HTTPS_RESULT,
+    otherPrefix: "http://",
+    expectOther: false,
+  });
+});
+
+add_task(async function dedupeAgainstURL_lowerPrefix() {
+  await doDedupeAgainstURLTest({
+    searchString: HTTP_SEARCH_STRING,
+    expectedQuickSuggestResult: EXPECTED_HTTP_RESULT,
+    otherPrefix: "https://",
+    expectOther: true,
+  });
+});
+
+/**
+ * Tests how the muxer dedupes URL results against quick suggest results.
+ * Depending on prefix rank, quick suggest results should be preferred over
+ * other URL results with the same stripped URL: Other results should be
+ * discarded when their prefix rank is lower than the prefix rank of the quick
+ * suggest. They should not be discarded when their prefix rank is higher, and
+ * in that case both results should be included.
+ *
+ * This function adds a visit to the URL formed by the given `otherPrefix` and
+ * `PREFIX_SUGGESTIONS_STRIPPED_URL`. The visit's title will be set to the given
+ * `searchString` so that both the visit and the quick suggest will match it.
+ *
+ * @param {string} searchString
+ *   The search string that should trigger one of the mock prefix-test quick
+ *   suggest results.
+ * @param {object} expectedQuickSuggestResult
+ *   The expected quick suggest result.
+ * @param {string} otherPrefix
+ *   The visit will be created with a URL with this prefix, e.g., "http://".
+ * @param {boolean} expectOther
+ *   Whether the visit result should appear in the final results.
+ */
+async function doDedupeAgainstURLTest({
+  searchString,
+  expectedQuickSuggestResult,
+  otherPrefix,
+  expectOther,
+}) {
+  // Disable search suggestions.
+  UrlbarPrefs.set("suggest.searches", false);
+
+  // Add a visit that will match our query below.
+  let otherURL = otherPrefix + PREFIX_SUGGESTIONS_STRIPPED_URL;
+  await PlacesTestUtils.addVisits({ uri: otherURL, title: searchString });
+
+  // First, do a search with quick suggest disabled to make sure the search
+  // string matches the visit.
+  info("Doing first query");
+  UrlbarPrefs.set("suggest.quicksuggest", false);
+  let context = createContext(searchString, { isPrivate: false });
+  await check_results({
+    context,
+    matches: [
+      makeSearchResult(context, {
+        heuristic: true,
+        query: searchString,
+        engineName: Services.search.defaultEngine.name,
+      }),
+      makeVisitResult(context, {
+        uri: otherURL,
+        title: searchString,
+      }),
+    ],
+  });
+
+  // Now do another search with quick suggest enabled.
+  UrlbarPrefs.set("suggest.quicksuggest", true);
+  UrlbarPrefs.set("suggest.quicksuggest.sponsored", true);
+
+  context = createContext(searchString, { isPrivate: false });
+
+  let expectedResults = [
+    makeSearchResult(context, {
+      heuristic: true,
+      query: searchString,
+      engineName: Services.search.defaultEngine.name,
+    }),
+  ];
+  if (expectOther) {
+    expectedResults.push(
+      makeVisitResult(context, {
+        uri: otherURL,
+        title: searchString,
+      })
+    );
+  }
+  expectedResults.push(expectedQuickSuggestResult);
+
+  info("Doing second query");
+  await check_results({ context, matches: expectedResults });
+
+  UrlbarPrefs.clear("suggest.quicksuggest");
+  UrlbarPrefs.clear("suggest.quicksuggest.sponsored");
+  UrlbarPrefs.clear("suggest.searches");
+  await PlacesUtils.history.clear();
+}