1664516 - Default topsites with %YYYYMMDDHH% should take current datetime regardless of the frecent/recent history

Status: RESOLVED FIXED

(Firefox :: Top Sites, defect, P1)

Description

[Adrian Florinescu [:aflorinescu]]

Given bug 1653934 fix, as per 09/11 discution with :dao there is still some work left in order to be able to use %YYYYMMDDHH% in the target topsite as intended.

From my understanding, %YYYYMMDDHH% need to be automatically pinned in order to function in the expected manner, option which is not available today: after the first click on the topsite, the translated current datetime becomes the topsite, since frecvent/recent history will kick in.

As a side note, it should also be considered that there also is a chance that the automatically pinned topsite with %YYYYMMDDHH% might be unpined, which should be taken into consideration.

Comment 1

[Jens Stutte [:jstutte]]

Hi :dao, I assume severity S1 is not really true here?

Comment 2

[Dão Gottwald [:dao]]

I picked S1 as this may require a dot release, but thinking about this again, that's not really true as we have a plan B: keep using the Amazon tile experiment code in 81.

Comment 3

[Dão Gottwald [:dao]]

Attachment: Bug 1664516 - Update frecency top sites from default top sites with %YYYYMMDDHH% to use the current datetime. r=mikedeboer

Comment 4

[Dão Gottwald [:dao]]

Comment on attachment 9176183 [details]
Bug 1664516 - Update frecency top sites from default top sites with %YYYYMMDDHH% to use the current datetime. r=mikedeboer

Beta/Release Uplift Approval Request

• User impact if declined: See comment 0. The datetime tag being current is a partner requirement.
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: Adrian has the steps
• List of other uplifts needed: Bug 1665757
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): The patch isn't that simple, but it doesn't change anything when remote settings are disabled, which they still are by default. We intend to roll this out after release to minimize risk, and I consider this bug a blocker for that. If something goes wrong, we can still go with the Amazon tile experiment code in 81. If we don't take this patch, this would limit us to that experiment route which has known flaws.
• String changes made/needed:

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/708170892bc8
Update frecency top sites from default top sites with %YYYYMMDDHH% to use the current datetime. r=mak

Comment 6

[Adrian Florinescu [:aflorinescu]]

[Tracking Requested - why for this release]: required in 81 in order to be able to use the feature for it's intended purpose.

Comment 7

[Adrian Florinescu [:aflorinescu]]

Using a try-build that contains RC2 + bug 1664516, I've tested the functionality and the frecency of the timestamp like dates. At this time I've only managed to cover Windows 10 (bug 1665757 took a bit of time to realize what was going on).

Few things I've noticed, but not entirely sure if it's a concern or not:

• frecency is recorded for the first translated timestamp and continue to increment that one[1].
• history will show an additional untranslated visit (https://abc.com/#%YYYYMMDDHH%) + the translated visit (https://abc.com/#2020091723)

[1]

Cu.import("resource://gre/modules/NewTabUtils.jsm");
(await NewTabUtils.activityStreamProvider.getTopFrecentSites({numItems: 25})).map(s => ${s.url} [${s.frecency}]).join("\n")
"https://www.baidu.com/ [10000]
https://abc.com/#2020091723 [8000]
https://www.example.com/#2020091723 [6000]
https://www.amazon.com/?tag=admarketus-20&ref=pd_sl_a77559ED8D65AA122020091723 [6000]"

Overall, I'm not sure if we would impact any general AS functionality with this fix: are there sites in the wild that map to the timestamp format we're messing around with which might accidentally be affected by this fix and get stuck in a frecency loop?

Comment 8

[Dão Gottwald [:dao]]

frecency is recorded for the first translated timestamp and continue to increment that one[1].

I don't think that's accurate. Frecency will increase for the URL you actually load, along with the unprocessed URL. As time changes, frecency wouldn't continue to increase for the URL using the first or some other old timestamp but the current one.

Other than that, what you wrote seems fine.

Comment 9

[Dão Gottwald [:dao]]

(In reply to Dão Gottwald [::dao] from comment #8)

frecency is recorded for the first translated timestamp and continue to increment that one[1].

I don't think that's accurate. Frecency will increase for the URL you actually load, along with the unprocessed URL. As time changes, frecency wouldn't continue to increase for the URL using the first or some other old timestamp but the current one.

Nevermind, you're right, there's a typo that I'm fixing in bug 1665757.

Comment 10

[Alexandru Michis [:malexandru]]

https://hg.mozilla.org/mozilla-central/rev/708170892bc8

Comment 11

[Adrian Florinescu [:aflorinescu]]

Verified as fixed with 82.0a1 2020-09-20 on Ubuntu 20.04, Windows 10 and Mac 10.13.6.

Comment 12

[Dão Gottwald [:dao]]

Attachment: patch for potential 81 dot release

Beta/Release Uplift Approval Request

• User impact if declined: See comment 0. The datetime tag being current is a partner requirement. This patch includes the one-line fix for bug 1665757 as well.
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed:
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): The patch isn't the most trivial, but it doesn't change anything when remote settings are disabled, which they are by default. We intend to roll this out after release to minimize risk, and this bug would be a blocker for that.
• String changes made/needed:

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9177136 [details] [diff] [review]
patch for potential 81 dot release

Per bkal, this isn't feature isn't targeting 81 anymore.