Bug 1911683 - Remove shouldNavigate for dynamic result types and make UrlbarUtils.getUrlFromResult() fall back to payload.url for all results. r=daisuke

Please see the bug for info.

Differential Revision: https://phabricator.services.mozilla.com/D218607

browser/components/urlbar/UrlbarInput.sys.mjs

@@ -1238,19 +1238,13 @@ export class UrlbarInput {
         return;
       }
       case lazy.UrlbarUtils.RESULT_TYPE.DYNAMIC: {
-        if (url) {
-          break;
-        }
-        url = result.payload.url;
-        // Keep the searchMode for telemetry since handleRevert sets it to null.
-        const searchMode = this.searchMode;
-        // Do not revert the Urlbar if we're going to navigate. We want the URL
-        // populated so we can navigate to it.
-        if (!url || !result.payload.shouldNavigate) {
+        if (!url) {
+          // If we're not loading a URL, the engagement is done. First revert
+          // and then record the engagement since providers expect the urlbar to
+          // be reverted when they're notified of the engagement, but before
+          // reverting, copy the search mode since it's nulled on revert.
+          const { searchMode } = this;
           this.handleRevert();
-        }
-        // If we won't be navigating, this is the end of the engagement.
-        if (!url || !result.payload.shouldNavigate) {
           this.controller.engagementEvent.record(event, {
             result,
             element,

browser/components/urlbar/UrlbarMuxerUnifiedComplete.sys.mjs

@@ -816,10 +816,10 @@ class MuxerUnifiedComplete extends UrlbarMuxer {
           return false;
         }
 
-        // For tab-to-search results, result.payload.url is the engine's domain
-        // with the public suffix already stripped, for example "www.mozilla.".
+        // `searchUrlDomainWithoutSuffix` is the engine's domain with the public
+        // suffix already stripped, for example "www.mozilla.".
         let [engineDomain] = UrlbarUtils.stripPrefixAndTrim(
-          result.payload.url,
+          result.payload.searchUrlDomainWithoutSuffix,
           {
             stripWww: true,
           }

browser/components/urlbar/UrlbarProviderTabToSearch.sys.mjs

@@ -404,16 +404,12 @@ class ProviderTabToSearch extends UrlbarProvider {
 }
 
 function makeOnboardingResult(engine, satisfiesAutofillThreshold = false) {
-  let [url] = UrlbarUtils.stripPrefixAndTrim(engine.searchUrlDomain, {
-    stripWww: true,
-  });
-  url = url.substr(0, url.length - engine.searchUrlPublicSuffix.length);
   let result = new lazy.UrlbarResult(
     UrlbarUtils.RESULT_TYPE.DYNAMIC,
     UrlbarUtils.RESULT_SOURCE.SEARCH,
     {
       engine: engine.name,
-      url,
+      searchUrlDomainWithoutSuffix: searchUrlDomainWithoutSuffix(engine),
       providesSearchMode: true,
       icon: UrlbarUtils.ICON.SEARCH_GLASS,
       dynamicType: DYNAMIC_RESULT_TYPE,
@@ -426,17 +422,13 @@ function makeOnboardingResult(engine, satisfiesAutofillThreshold = false) {
 }
 
 function makeResult(context, engine, satisfiesAutofillThreshold = false) {
-  let [url] = UrlbarUtils.stripPrefixAndTrim(engine.searchUrlDomain, {
-    stripWww: true,
-  });
-  url = url.substr(0, url.length - engine.searchUrlPublicSuffix.length);
   let result = new lazy.UrlbarResult(
     UrlbarUtils.RESULT_TYPE.SEARCH,
     UrlbarUtils.RESULT_SOURCE.SEARCH,
     ...lazy.UrlbarResult.payloadAndSimpleHighlights(context.tokens, {
       engine: engine.name,
       isGeneralPurposeEngine: engine.isGeneralPurposeEngine,
-      url,
+      searchUrlDomainWithoutSuffix: searchUrlDomainWithoutSuffix(engine),
       providesSearchMode: true,
       icon: UrlbarUtils.ICON.SEARCH_GLASS,
       query: "",
@@ -447,5 +439,12 @@ function makeResult(context, engine, satisfiesAutofillThreshold = false) {
   return result;
 }
 
+function searchUrlDomainWithoutSuffix(engine) {
+  let [value] = UrlbarUtils.stripPrefixAndTrim(engine.searchUrlDomain, {
+    stripWww: true,
+  });
+  return value.substr(0, value.length - engine.searchUrlPublicSuffix.length);
+}
+
 export var UrlbarProviderTabToSearch = new ProviderTabToSearch();
 initializeDynamicResult();

browser/components/urlbar/UrlbarUtils.sys.mjs

@@ -570,38 +570,34 @@ export var UrlbarUtils = {
   },
 
   /**
-   * Extracts an url from a result, if possible.
+   * Extracts the URL from a result.
    *
-   * @param {UrlbarResult} result The result to extract from.
-   * @returns {object} a {url, postData} object, or null if a url can't be built
-   *          from this result.
+   * @param {UrlbarResult} result
+   *   The result to extract from.
+   * @returns {object}
+   *   An object: `{ url, postData }`
+   *   `url` will be null if the result doesn't have a URL. `postData` will be
+   *   null if the result doesn't have post data.
    */
   getUrlFromResult(result) {
-    switch (result.type) {
-      case UrlbarUtils.RESULT_TYPE.URL:
-      case UrlbarUtils.RESULT_TYPE.REMOTE_TAB:
-      case UrlbarUtils.RESULT_TYPE.TAB_SWITCH:
-        return { url: result.payload.url, postData: null };
-      case UrlbarUtils.RESULT_TYPE.KEYWORD:
-        return {
-          url: result.payload.url,
-          postData: result.payload.postData
-            ? this.getPostDataStream(result.payload.postData)
-            : null,
-        };
-      case UrlbarUtils.RESULT_TYPE.SEARCH: {
-        if (result.payload.engine) {
-          const engine = Services.search.getEngineByName(result.payload.engine);
-          let [url, postData] = this.getSearchQueryUrl(
-            engine,
-            result.payload.suggestion || result.payload.query
-          );
-          return { url, postData };
-        }
-        break;
-      }
+    if (
+      result.type == UrlbarUtils.RESULT_TYPE.SEARCH &&
+      result.payload.engine
+    ) {
+      const engine = Services.search.getEngineByName(result.payload.engine);
+      let [url, postData] = this.getSearchQueryUrl(
+        engine,
+        result.payload.suggestion || result.payload.query
+      );
+      return { url, postData };
     }
-    return { url: null, postData: null };
+
+    return {
+      url: result.payload.url ?? null,
+      postData: result.payload.postData
+        ? this.getPostDataStream(result.payload.postData)
+        : null,
+    };
   },
 
   /**
@@ -1722,6 +1718,9 @@ UrlbarUtils.RESULT_PAYLOAD_SCHEMA = {
       satisfiesAutofillThreshold: {
         type: "boolean",
       },
+      searchUrlDomainWithoutSuffix: {
+        type: "string",
+      },
       suggestion: {
         type: "string",
       },
@@ -2090,12 +2089,6 @@ UrlbarUtils.RESULT_PAYLOAD_SCHEMA = {
       dynamicType: {
         type: "string",
       },
-      // If `shouldNavigate` is `true` and the payload contains a `url`
-      // property, when the result is selected the browser will navigate to the
-      // `url`.
-      shouldNavigate: {
-        type: "boolean",
-      },
     },
   },
   [UrlbarUtils.RESULT_TYPE.RESTRICT]: {

browser/components/urlbar/docs/dynamic-result-types.rst

@@ -614,10 +614,9 @@ useful in this case.
 URL Navigation
 ~~~~~~~~~~~~~~
 
-If a result's payload includes a string ``url`` property and a boolean
-``shouldNavigate: true`` property, then picking the result will navigate to the
-URL. The ``onLegacyEngagement`` method of the result's provider will still be called
-before navigation.
+If a result's payload includes a string ``url`` property, picking the result
+will navigate to the URL. The ``onEngagement`` method of the result's provider
+will be called before navigation.
 
 Text Highlighting
 ~~~~~~~~~~~~~~~~~

browser/components/urlbar/private/FakespotSuggestions.sys.mjs

@@ -130,7 +130,6 @@ export class FakespotSuggestions extends BaseFeature {
       totalReviews: Number(suggestion.totalReviews),
       fakespotGrade: suggestion.fakespotGrade,
       fakespotProvider: this.#parseProvider(suggestion),
-      shouldNavigate: true,
       dynamicType: "fakespot",
     };
 

browser/components/urlbar/private/Weather.sys.mjs

@@ -361,7 +361,6 @@ export class Weather extends BaseFeature {
           forecast: suggestion.forecast.summary,
           high: suggestion.forecast.high[unit],
           low: suggestion.forecast.low[unit],
-          shouldNavigate: true,
         }
       ),
       {

browser/components/urlbar/tests/browser/browser_dynamicResults.js

@@ -460,7 +460,7 @@ add_task(async function pick() {
 // Tests picking elements in a dynamic result.
 add_task(async function shouldNavigate() {
   /**
-   * A dummy provider that providers results with a `shouldNavigate` property.
+   * A dummy provider that providers results with a `url` property.
    */
   class TestShouldNavigateProvider extends TestProvider {
     /**
@@ -470,7 +470,6 @@ add_task(async function shouldNavigate() {
     async startQuery(context, addCallback) {
       for (let result of this.results) {
         result.payload.searchString = context.searchString;
-        result.payload.shouldNavigate = true;
         result.payload.url = DUMMY_PAGE;
         addCallback(this, result);
       }

browser/components/urlbar/tests/quicksuggest/unit/head.js

@@ -425,7 +425,6 @@ function makeWeatherResult({
       forecast: MerinoTestUtils.WEATHER_SUGGESTION.forecast.summary,
       high: MerinoTestUtils.WEATHER_SUGGESTION.forecast.high[temperatureUnit],
       low: MerinoTestUtils.WEATHER_SUGGESTION.forecast.low[temperatureUnit],
-      shouldNavigate: true,
     },
   };
 

browser/components/urlbar/tests/quicksuggest/unit/test_quicksuggest.js

@@ -1526,7 +1526,9 @@ add_tasks_with_rust(async function tabToSearch() {
       makeSearchResult(context, {
         engineName: engine.name,
         engineIconUri: UrlbarUtils.ICON.SEARCH_GLASS,
-        uri: UrlbarUtils.stripPublicSuffixFromHost(engine.searchUrlDomain),
+        searchUrlDomainWithoutSuffix: UrlbarUtils.stripPublicSuffixFromHost(
+          engine.searchUrlDomain
+        ),
         providesSearchMode: true,
         query: "",
         providerName: "TabToSearch",

browser/components/urlbar/tests/quicksuggest/unit/test_quicksuggest_fakespot.js

@@ -856,7 +856,6 @@ function makeExpectedResult({
       totalReviews,
       fakespotGrade,
       fakespotProvider,
-      shouldNavigate: true,
       dynamicType: "fakespot",
       icon: null,
     },

browser/components/urlbar/tests/unit/head.js

@@ -711,6 +711,9 @@ function makeRemoteTabResult(
  *   If the window to test is a private window.
  * @param {boolean} [options.isPrivateEngine]
  *   If the engine is a private engine.
+ * @param {string} [options.searchUrlDomainWithoutSuffix]
+ *   For tab-to-search results, the search engine domain without the public
+ *   suffix.
  * @param {number} [options.type]
  *   The type of the search result. Defaults to UrlbarUtils.RESULT_TYPE.SEARCH.
  * @param {number} [options.source]
@@ -739,6 +742,7 @@ function makeSearchResult(
     providerName,
     inPrivateWindow,
     isPrivateEngine,
+    searchUrlDomainWithoutSuffix,
     heuristic = false,
     trending = false,
     isRichSuggestion = false,
@@ -786,10 +790,11 @@ function makeSearchResult(
     payload.url = uri;
   }
   if (providerName == "TabToSearch") {
-    payload.satisfiesAutofillThreshold = satisfiesAutofillThreshold;
-    if (payload.url.startsWith("www.")) {
-      payload.url = payload.url.substring(4);
+    if (searchUrlDomainWithoutSuffix.startsWith("www.")) {
+      searchUrlDomainWithoutSuffix = searchUrlDomainWithoutSuffix.substring(4);
     }
+    payload.searchUrlDomainWithoutSuffix = searchUrlDomainWithoutSuffix;
+    payload.satisfiesAutofillThreshold = satisfiesAutofillThreshold;
     payload.isGeneralPurposeEngine = false;
   }
 

browser/components/urlbar/tests/unit/test_autofill_origins_alt_frecency.js

@@ -106,7 +106,9 @@ add_task(
         makeSearchResult(context, {
           engineName: engine.name,
           engineIconUri: UrlbarUtils.ICON.SEARCH_GLASS,
-          uri: UrlbarUtils.stripPublicSuffixFromHost(engine.searchUrlDomain),
+          searchUrlDomainWithoutSuffix: UrlbarUtils.stripPublicSuffixFromHost(
+            engine.searchUrlDomain
+          ),
           providesSearchMode: true,
           query: "",
           providerName: "TabToSearch",

browser/components/urlbar/tests/unit/test_providerTabToSearch.js

@@ -49,7 +49,9 @@ add_task(async function basic() {
       makeSearchResult(context, {
         engineName: testEngine.name,
         engineIconUri: UrlbarUtils.ICON.SEARCH_GLASS,
-        uri: UrlbarUtils.stripPublicSuffixFromHost(testEngine.searchUrlDomain),
+        searchUrlDomainWithoutSuffix: UrlbarUtils.stripPublicSuffixFromHost(
+          testEngine.searchUrlDomain
+        ),
         providesSearchMode: true,
         query: "",
         providerName: "TabToSearch",
@@ -94,7 +96,9 @@ add_task(async function noAutofill() {
       makeSearchResult(context, {
         engineName: testEngine.name,
         engineIconUri: UrlbarUtils.ICON.SEARCH_GLASS,
-        uri: UrlbarUtils.stripPublicSuffixFromHost(testEngine.searchUrlDomain),
+        searchUrlDomainWithoutSuffix: UrlbarUtils.stripPublicSuffixFromHost(
+          testEngine.searchUrlDomain
+        ),
         providesSearchMode: true,
         query: "",
         providerName: "TabToSearch",
@@ -145,7 +149,9 @@ add_task(async function ignoreWww() {
       makeSearchResult(context, {
         engineName: testEngine.name,
         engineIconUri: UrlbarUtils.ICON.SEARCH_GLASS,
-        uri: UrlbarUtils.stripPublicSuffixFromHost(testEngine.searchUrlDomain),
+        searchUrlDomainWithoutSuffix: UrlbarUtils.stripPublicSuffixFromHost(
+          testEngine.searchUrlDomain
+        ),
         providesSearchMode: true,
         query: "",
         providerName: "TabToSearch",
@@ -179,7 +185,7 @@ add_task(async function ignoreWww() {
       makeSearchResult(context, {
         engineName: wwwTestEngine.name,
         engineIconUri: UrlbarUtils.ICON.SEARCH_GLASS,
-        uri: UrlbarUtils.stripPublicSuffixFromHost(
+        searchUrlDomainWithoutSuffix: UrlbarUtils.stripPublicSuffixFromHost(
           wwwTestEngine.searchUrlDomain
         ),
         providesSearchMode: true,
@@ -207,7 +213,7 @@ add_task(async function ignoreWww() {
       makeSearchResult(context, {
         engineName: wwwTestEngine.name,
         engineIconUri: UrlbarUtils.ICON.SEARCH_GLASS,
-        uri: UrlbarUtils.stripPublicSuffixFromHost(
+        searchUrlDomainWithoutSuffix: UrlbarUtils.stripPublicSuffixFromHost(
           wwwTestEngine.searchUrlDomain
         ),
         providesSearchMode: true,
@@ -266,7 +272,7 @@ add_task(async function conflictingEngines() {
       makeSearchResult(context, {
         engineName: fooTestEngine.name,
         engineIconUri: UrlbarUtils.ICON.SEARCH_GLASS,
-        uri: UrlbarUtils.stripPublicSuffixFromHost(
+        searchUrlDomainWithoutSuffix: UrlbarUtils.stripPublicSuffixFromHost(
           fooTestEngine.searchUrlDomain
         ),
         providesSearchMode: true,
@@ -298,7 +304,7 @@ add_task(async function conflictingEngines() {
       makeSearchResult(context, {
         engineName: fooBarTestEngine.name,
         engineIconUri: UrlbarUtils.ICON.SEARCH_GLASS,
-        uri: UrlbarUtils.stripPublicSuffixFromHost(
+        searchUrlDomainWithoutSuffix: UrlbarUtils.stripPublicSuffixFromHost(
           fooBarTestEngine.searchUrlDomain
         ),
         providesSearchMode: true,
@@ -361,7 +367,9 @@ add_task(async function multipleEnginesForHostname() {
       makeSearchResult(context, {
         engineName: testEngine.name,
         engineIconUri: UrlbarUtils.ICON.SEARCH_GLASS,
-        uri: UrlbarUtils.stripPublicSuffixFromHost(testEngine.searchUrlDomain),
+        searchUrlDomainWithoutSuffix: UrlbarUtils.stripPublicSuffixFromHost(
+          testEngine.searchUrlDomain
+        ),
         providesSearchMode: true,
         query: "",
         providerName: "TabToSearch",
@@ -396,7 +404,9 @@ add_task(async function test_casing() {
       makeSearchResult(context, {
         engineName: testEngine.name,
         engineIconUri: UrlbarUtils.ICON.SEARCH_GLASS,
-        uri: UrlbarUtils.stripPublicSuffixFromHost(testEngine.searchUrlDomain),
+        searchUrlDomainWithoutSuffix: UrlbarUtils.stripPublicSuffixFromHost(
+          testEngine.searchUrlDomain
+        ),
         providesSearchMode: true,
         query: "",
         providerName: "TabToSearch",
@@ -430,7 +440,9 @@ add_task(async function test_publicSuffix() {
       makeSearchResult(context, {
         engineName: engine.name,
         engineIconUri: UrlbarUtils.ICON.SEARCH_GLASS,
-        uri: UrlbarUtils.stripPublicSuffixFromHost(engine.searchUrlDomain),
+        searchUrlDomainWithoutSuffix: UrlbarUtils.stripPublicSuffixFromHost(
+          engine.searchUrlDomain
+        ),
         providesSearchMode: true,
         query: "",
         providerName: "TabToSearch",
@@ -505,7 +517,9 @@ add_task(async function test_disabledEngine() {
       makeSearchResult(context, {
         engineName: engine.name,
         engineIconUri: UrlbarUtils.ICON.SEARCH_GLASS,
-        uri: UrlbarUtils.stripPublicSuffixFromHost(engine.searchUrlDomain),
+        searchUrlDomainWithoutSuffix: UrlbarUtils.stripPublicSuffixFromHost(
+          engine.searchUrlDomain
+        ),
         providesSearchMode: true,
         query: "",
         providerName: "TabToSearch",

browser/components/urlbar/tests/unit/test_providerTabToSearch_partialHost.js

@@ -71,7 +71,7 @@ add_task(async function test() {
         makeSearchResult(context, {
           engineName: "TestEngine",
           engineIconUri: UrlbarUtils.ICON.SEARCH_GLASS,
-          uri: "en.example.",
+          searchUrlDomainWithoutSuffix: "en.example.",
           providesSearchMode: true,
           query: "",
           providerName: "TabToSearch",
@@ -114,7 +114,7 @@ add_task(async function test() {
         makeSearchResult(context, {
           engineName: engine2.name,
           engineIconUri: UrlbarUtils.ICON.SEARCH_GLASS,
-          uri: "www.it.mochi.",
+          searchUrlDomainWithoutSuffix: "www.it.mochi.",
           providesSearchMode: true,
           query: "",
           providerName: "TabToSearch",
@@ -162,7 +162,7 @@ add_task(async function test() {
         makeSearchResult(context, {
           engineName: "TestEngine3",
           engineIconUri: UrlbarUtils.ICON.SEARCH_GLASS,
-          uri: "search.foo.",
+          searchUrlDomainWithoutSuffix: "search.foo.",
           providesSearchMode: true,
           query: "",
           providerName: "TabToSearch",