Closed Bug 1562560 Opened 5 years ago Closed 5 years ago

port bug 1547718 to thunderbird - killing onsync{to,from}preference - Remove `new Function` from preferencesBindings.js

Categories

(Thunderbird :: General, task, P3)

Product:
Thunderbird
Component:
General
Type:
task

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
Thunderbird 70.0

People

(Reporter: mkmelin, Assigned: darktrojan)

References

Details

Attachments

(1 file, 1 obsolete file)

1562560-sync-to-from-preference-2.diff
(deleted), patch
darktrojan
: review+
Details | Diff | Splinter Review

+++ This bug was initially created as a clone of Bug #1547718 +++

This bug is to handle killing onsync{to,from}preference in Thunderbird. m-c is doing this early (-ish) in the 70 cycle. WIP patch in https://phabricator.services.mozilla.com/D32326

Geoff, please have a look.

Attached patch 1562560-sync-to-from-preference-1.diff (obsolete) (deleted) — — Splinter Review
Attachment #9075873 - Flags: review?(acelists)

Bug 1547718 is about to land, so we need some action here, please.

Comment on attachment 9075873 [details] [diff] [review] 1562560-sync-to-from-preference-1.diff Review of attachment 9075873 [details] [diff] [review]: ----------------------------------------------------------------- (I assume this has to be applied with bug 1547718 too.) I played with this modifying e.g. the number of MBs for the disk cache size (on Advanced pane) and the "query OCSP" option. Why do I get both syncToPref and then syncFromPref functions called when modifying the value in the UI? ::: mail/components/preferences/chat.js @@ +48,5 @@ > window.addEventListener("paneSelected", this.paneSelectionChanged); > > + let element = document.getElementById("timeBeforeAway"); > + Preferences.addSyncFromPrefListener(element, > + () => Preferences.get("messenger.status.timeBeforeIdle").value / 60); Why can we limit this to the .instantApply == false case? ::: mail/components/preferences/connection.js @@ +75,1 @@ > // XXX: We can't init the DNS-over-HTTPs UI until the onsyncfrompreference for network.trr.mode Does this mention of "onsyncfrompreference" need any update? ::: mail/components/preferences/downloads.js @@ +113,5 @@ > + > +document.getElementById("paneApplications") > + .addEventListener("paneload", () => { > + Preferences.addSyncFromPrefListener(document.getElementById("saveWhere"), > + () => gDownloadDirSection.onReadUseDownloadDir()); Why is this moved into downloads.js? ::: mail/components/preferences/fonts.js @@ +130,5 @@ > }; > > document.addEventListener("dialogaccept", () => gFontsDialog.ondialogaccept()); > +window.addEventListener("load", () => { > + Preferences.addSyncFromPrefListener(document.getElementById("selectLangs"), Can you make an init() function to put all these? ::: mail/components/preferences/notifications.js @@ +11,5 @@ > { id: "alerts.totalOpenTime", type: "int" }, > ]); > + > +window.addEventListener("load", () => { > + let element = document.getElementById("totalOpenTime"); Can you make an init() function to put all these?
Comment on attachment 9075873 [details] [diff] [review] 1562560-sync-to-from-preference-1.diff Review of attachment 9075873 [details] [diff] [review]: ----------------------------------------------------------------- Actually I assume that calling both function on pref change is caused by bug 1547718, so that makes the patch here have r+. But please ask the m-c guys about it.
Attachment #9075873 - Flags: review?(acelists) → review+

(In reply to :aceman from comment #3)

(I assume this has to be applied with bug 1547718 too.)
I played with this modifying e.g. the number of MBs for the disk cache size
(on Advanced pane) and the "query OCSP" option. Why do I get both syncToPref
and then syncFromPref functions called when modifying the value in the UI?

It appears setting the value of a Preference object fires a change event and the listeners to it take no notice of what triggered the event, just go and get the value again. Seems a bit silly but ultimately it doesn't really matter and it's always done that.

Why can we limit this to the .instantApply == false case?

Actually that's a mistake (should be the other case) but it also doesn't really matter as it works either way. I'll fix it anyway.

+document.getElementById("paneApplications")

  •    .addEventListener("paneload", () => {

  •      Preferences.addSyncFromPrefListener(document.getElementById("saveWhere"),

  •        () => gDownloadDirSection.onReadUseDownloadDir());

Why is this moved into downloads.js?

Despite the name, downloads.js is part of this pane, and it contains the function we're calling.

Attachment #9075873 - Attachment is obsolete: true
Attachment #9076490 - Flags: review+

Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/2f5aab4c8902
Port bug 1547718: Remove onsync{to,from}preference. r=aceman

Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Regressions: 1567622
Target Milestone: --- → Thunderbird 70.0
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: