1896673 - Home and newtab weather system tick and caching fixes

Status: RESOLVED FIXED

(Firefox :: New Tab Page, defect)

Description

[Scott [:thecount] Downe]

We do a weather fetch on startup and on every update/system tick. We then store the result in memory, until the next system tick/update.

We can probably store the result in a cache, so restarts don't need to fetch again.
We can probably have a cache timeout that's not dependent on system tick interval.
If the user turns off weather, we might want to clear weather cache and what weather is in memory.

Comment 1

[Scott [:thecount] Downe]

[Tracking Requested - why for this release]: We intend to launch a weather experiment in 127. This fix reduces the number of requests the client would make to the server, improving performance. It should be a low risk, small set of changes.

Comment 2

[Scott [:thecount] Downe]

Attachment: Bug 1896673 - Home and newtab weather widget adding cache timer

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1703afba4c18
Home and newtab weather widget adding cache timer r=home-newtab-reviewers,nbarrett

Comment 4

[Norisz Fay [:noriszfay]]

https://hg.mozilla.org/mozilla-central/rev/1703afba4c18

Comment 5

[BugBot [:suhaib / :marco/ :calixte]]

The patch landed in nightly and beta is affected.
:thecount, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval.
• If no, please set status-firefox127 to wontfix.

For more information, please visit BugBot documentation.

Comment 6

[Scott [:thecount] Downe]

Comment on attachment 9402112 [details]
Bug 1896673 - Home and newtab weather widget adding cache timer

Beta/Release Uplift Approval Request

• User impact if declined: We intend to launch a weather experiment in 127. This fix reduces the number of requests the client would make to the server, improving performance. It should be a low risk, small set of changes.
• 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: 1. Ensure browser.newtabpage.activity-stream.showWeather is true

2. Ensure browser.newtabpage.activity-stream.system.showWeather is also true
3. Open newtab
   Expected: Should see weather widget with a weather report.
4. Restart the browser.
   Expected: Should still see weather and a weather report.

• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Most of these changes are some fairly basic refactoring, the new logic is the addition of an else statement in loadWeather, and it is all behind a pref. The refactoring was fixes issues related to the additional else statement.
• String changes made/needed: None
• Is Android affected?: No

Comment 7

[Pascal Chevrel:pascalc]

Comment on attachment 9402112 [details]
Bug 1896673 - Home and newtab weather widget adding cache timer

Approved for 127 beta 6, thanks.

Comment 8

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-beta/rev/99157832c4e9

Comment 9

[Peter Magyari (Desktop QA)]

Hi Scott,

I'm trying to verify the fix but I'm not sure if I got the expected result correctly, the weather widget re-appears on the newtab page 5 minutes after restarting the browser, should it appear right away? I've tested this on both Nightly 128.0a1 (20240523205926) and Beta 127.0b6 (20240523121911) which include the fix but also on an outdated Beta, and the result was the same. (If I disable->enable @about:preferences the widget appears right away)

Thank you!

Comment 10

[Scott [:thecount] Downe]

Not sure I am following.

To clarify, on step 3 it shows the weather report, but after restarting on step 4, it no longer shows the weather report?

It would make some sense that it wouldn't show weather on step 4 if it also didn't show on step 3, but it's weird it would go away after restarting if you saw it in step 3.

I'll try and see what I can reproduce.

Comment 11

[Peter Magyari (Desktop QA)]

step 3 it shows the weather report, but after restarting on step 4, it no longer shows the weather report?

Yes, so the weather widget is no longer visible after restarting, it does re-appear after 5 minutes though.

Comment 12

[Scott [:thecount] Downe]

Hm, so if you see it in step 3 then no longer after restarting, in both nightly and beta, it actually sounds like something else is happening unrelated to the patch we're QAing.

I couldn't reproduce what I you're seeing.

Does it still happen if you try again? Is this issue consistent?

The fact that it reappears after 5 minutes makes sense and is expected but not helpful in narrowing down why it is displayed in step 3, and why it goes away after restarting.

Comment 13

[Peter Magyari (Desktop QA)]

Attachment: Screenshot 2024-05-27 at 10.57.57.png

I can consistently reproduce the issue, using Nightly and Beta both on Windows 10 and MacOS 14.4.
The attached error is presented for 5 minutes post restart, then the widget seems to "refresh" and works as expected.

I am using NordVPN in order to test this since I'm not located in the US but it is automatically connected when the browser starts.