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

Status: RESOLVED FIXED

(Thunderbird :: General, task, P3)

Description

[Magnus Melin [:mkmelin]]

+++ 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.

Comment 1

[Geoff Lankow (:darktrojan)]

Attachment: 1562560-sync-to-from-preference-1.diff

Comment 2

[Jorg K (CEST = GMT+2)]

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

Comment 3

[:aceman]

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 4

[:aceman]

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.

Comment 5

[Geoff Lankow (:darktrojan)]

(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.

Comment 6

[Geoff Lankow (:darktrojan)]

Attachment: 1562560-sync-to-from-preference-2.diff

Comment 7

[Pulsebot]

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