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

Status: RESOLVED FIXED

(Firefox :: Address Bar, task, P2)

Description

[Drew Willcoxon :adw]

I'd like to make these related changes:

• Make UrlbarUtils.getUrlFromResult() always fall back to result.payload.url.
• Remove payload.shouldNavigate from dynamic results. Instead, navigate if UrlbarUtils.getUrlFromResult() returns a non-null URL.
• Stop setting payload.url to invalid URLs in tab-to-search results and instead set a new payload property called searchUrlDomainWithoutSuffix.

Motivation:

While working on bug 1910209, I found that UrlbarTestUtils.getDetailsOfResultAt() doesn't return the expected URL for dynamic results since it relies on UrlbarUtils.getUrlFromResult(), which returns a null URL for result types that aren't specifically handled.

I think it makes sense for getUrlFromResult() to return result.payload.url for dynamic results, at least for dynamic results where payload.shouldNavigate is true. Actually, wouldn't it make sense for getUrlFromResult() to always fall back to result.payload.url regardless of the result type? We've pretty much standardized on setting payload.url if the result has a URL that should be loaded.

So I tried making that change, and the only test failures were due to onboarding tab-to-search results, which set payload.url to an engine domain without a public suffix, like "google.". With my change, when those results are picked, UrlbarInput attempts to load those invalid URLs, which ends up performing a search with the current engine. One way to fix that is to modify UrlbarInput.pickResult() so that it never attempts to load a url from a dynamic result when payload.shouldNavigate is falsey. It doesn't currently do that, which might be considered a bug.

But looking back, it's kind of unfortunate we added shouldNavigate. I think it would have been better to leave payload.url undefined for results that don't have valid URLs to load. Currently the only dynamic result that sets payload.url to an invalid URL is the onboarding tab-to-search result. It sets it to an engine domain without a public suffix, as mentioned above. OTOH there are two dynamic result types that set shouldNavigate: true (weather and Fakespot), and in general it's reasonable to expect that navigation is the common desired case for dynamic results that set url. We're penalizing the common case by requiring results to additionally set shouldNavigate.

So I'd like to remove shouldNavigate and stop setting url on onboarding TTS results. Instead we can set some other property.

Comment 1

[Drew Willcoxon :adw]

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

Please see the bug for info.

Comment 2

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0e5c054cf413
Remove shouldNavigate for dynamic result types and make UrlbarUtils.getUrlFromResult() fall back to payload.url for all results. r=daisuke

Comment 3

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/0e5c054cf413