Closed Bug 1831971 Opened 1 year ago Closed 1 year ago

The weather suggestion will be shown on zero prefix until remote settings are synced

Categories

(Firefox :: Address Bar, defect, P1)

defect

Tracking

()

VERIFIED FIXED
115 Branch
Tracking Status
firefox114 + verified
firefox115 --- verified

People

(Reporter: adw, Assigned: adw)

References

Details

Attachments

(1 file)

If remote settings and Nimbus don't define a weather config, Firefox will show the suggestion on zero prefix. That means if remote settings contain a config but it hasn't been synced yet, Firefox will go from showing the suggestion on zero prefix to showing it on keywords after settings are synced.

Firefox shouldn't show the suggestion at all if there's no config.

[Tracking Requested - why for this release]: This bug is necessary for the Firefox Suggest weather suggestion feature we intend to ship in 114.

This fixes the bug by not starting fetches until a config is set from either
remote settings or Nimbus. By "config" I mean keywords basically, but we sync
more than keywords -- the min keyword length and min keyword length cap -- so
that's why I use different term. So, before remote settings is synced, no config
will be set, so no fetches will happen, so the suggestion will be null. When the
urlbar provider starts a query, it will see the suggestion is null and not add a
result. Once a config is set from RS or Nimbus, we'll start fetching.

Currently we allow zero prefix when keywords or the min keyword length aren't
set. This patch removes that functionality because on second thought, there's
not a safe and obvious way to support zero prefix using these keyword-related
config properties/variables by themselves, and zero prefix isn't a feature
requirement anymore anyway. If we wanted to keep supporting it with these
properties/variables, there are a few options, and I don't like any of them:

  • If keywords is undefined or null, use zero prefix. This is dangerous because
    we may make a mistake in RS or Nimbus and forget to set it. Also, we use null
    as the default value for the Nimbus var, and since we use UrlbarPrefs to
    access Nimbus vars, there's no good way to tell whether null was set
    intentionally or not.
  • If keywords is an empty array, use zero prefix. This is awkward because the
    user can now increment the min keyword length, which means the keywords array
    kept by Weather can become empty when the min keyword length is incremented
    a lot. In that case, no suggestion should be shown at all.
  • If min_keyword_length is zero/falsey, use zero prefix. This has the same
    problems as the first point above.

If we do need to support zero prefix again in the future, I think we should add
an RS property/Nimbus variable that makes it clear what's happening, e.g., a
useZeroPrefix boolean.

I removed the exposure scalar because it's entirely based on zero prefix. We can
use Glean for that now anyway.

I also noticed weather suggestions are case insenstive, so I fixed that and
added a test task.

Pushed by dwillcoxon@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/a39f9cd81a42 Remove zero-prefix functionality from the weather suggestion and don't start fetches until a config is set. r=daisuke

Backed out for causing bc failures on browser_glean_telemetry_engagement_selected_result.js

Backout link

Push with failures

Failure log

Flags: needinfo?(adw)
Pushed by dwillcoxon@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/c77c18e12926 Remove zero-prefix functionality from the weather suggestion and don't start fetches until a config is set. r=daisuke
Flags: needinfo?(adw)
Status: ASSIGNED → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → 115 Branch

STR for QA

Please create a new profile and enable the weather suggestion and urlbar Debug logging. If you can inject the prefs into the profile before starting it so that the weather feature is enabled immediately when you do start it, that would be ideal.

browser.urlbar.weather.featureGate = true
browser.urlbar.weather.ignoreVPN = true
browser.urlbar.loglevel = Debug
  1. Start Firefox with the new profile
  2. Open the browser console
  3. Click in the urlbar several times and verify the weather suggestion is not shown. Before Firefox syncs remote settings in the next step, the suggestion should never be shown.
  4. Within about half a minute, Firefox should sync remote settings for the first time. To tell when this happens, filter on "quicksuggest.weather" in the browser console and you should see a message similar to this:

1683663568414 urlbar DEBUG QuickSuggest.Weather :: Got weather records: [{"type":"weather","schema":1683294047906,"weather":{"keywords":["weather","forecast","windy","humidity","monsoon","flooding","rain in","storms","storm in","forcast","wether","wather","weahter","weater","weaher","vindy"],"min_keyword_length":3,"min_keyword_length_cap":6},"id":"weather","last_modified":1683313881574}]

  1. Click in the urlbar again and verify the suggestion is not shown
  2. Type "weather" and verify the suggestion is shown
Flags: qe-verify+
Flags: in-testsuite+

Comment on attachment 9332297 [details]
Bug 1831971 - Remove zero-prefix functionality from the weather suggestion and don't start fetches until a config is set.

Beta/Release Uplift Approval Request

  • User impact if declined: This bug is necessary for the Firefox Suggest weather suggestion feature we intend to ship in 114.
  • Is this code covered by automated tests?: Yes
  • Has the fix been verified in Nightly?: No
  • Needs manual test from QE?: Yes
  • If yes, steps to reproduce: Please see comment 7
  • List of other uplifts needed: None
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): This patch only affects the Firefox Suggest weather suggestion feature. Has automated tests.
  • String changes made/needed:
  • Is Android affected?: No
Attachment #9332297 - Flags: approval-mozilla-beta?

I have verified this issue on the latest Firefox Nightly 115.0a1 (Build ID: 20230510094206), on Windows 10 x64, macOS 12.6.1, and Ubuntu 20.04 x64.

  • The weather suggestion is no longer shown in the zero prefix panel.
  • The weather suggestion is only triggered using specific keywords from Remote Settings.
  • I confirm that using the STR from comment 7, I also got the following log in the Browser Console:
    1683729000813 urlbar DEBUG QuickSuggest.Weather :: Got weather records: [{"type":"weather","schema":1683294047906,"weather":{"keywords":["weather","forecast","windy","humidity","monsoon","flooding","rain in","storms","storm in","forcast","wether","wather","weahter","weater","weaher","vindy"],"min_keyword_length":3,"min_keyword_length_cap":6},"id":"weather","last_modified":1683313881574}]

Comment on attachment 9332297 [details]
Bug 1831971 - Remove zero-prefix functionality from the weather suggestion and don't start fetches until a config is set.

Approved for 114 beta 3, thanks.

Attachment #9332297 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
QA Whiteboard: [qa-triaged]

I have verified this issue on the latest Firefox Beta 114.0b3 (Build ID: 20230511191645), on Windows 10 x64, macOS 12.6.1, and Ubuntu 20.04 x64.

  • The weather suggestion is no longer shown in the zero prefix panel.
  • The weather suggestion is only triggered using specific keywords from Remote Settings.
  • I confirm that using the STR from comment 7, I also got the following log in the Browser Console:
    1683729000813 urlbar DEBUG QuickSuggest.Weather :: Got weather records: [{"type":"weather","schema":1683294047906,"weather":{"keywords":["weather","forecast","windy","humidity","monsoon","flooding","rain in","storms","storm in","forcast","wether","wather","weahter","weater","weaher","vindy"],"min_keyword_length":3,"min_keyword_length_cap":6},"id":"weather","last_modified":1683313881574}]
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: