890507 - clean up mailnews/base/prefs/content/AccountManager.js::setFormElementValue function

Status: RESOLVED FIXED

(MailNews Core :: Account Manager, defect)

Description

[:aceman]

+++ This bug was initially created as a clone of Bug #881694 comment 7 +++

 [e-mail] 2013-07-05 15:56:55 CEST

So, this function (mailnews/base/prefs/content/AccountManager.js::setFormElementValue) makes very little sense, unfortunately it was written like this in 1999 and just monkeypatched every time.

* The defaultValue and defaultChecked properties have never existed.
  (The author may have been thinking of HTML input elements.)
* null compares == to undefined so there is no point comparing twice.
* Nobody passes undefined these days anyway.
* The function gets called for some radio buttons even though it also gets
  called for the radiogroup that chooses the value. The code here fortuitously
  avoids changing the radio's value in this case.

So I would really appreciate it if someone was to clean the code up.

Comment 1

[:aceman]

Attachment: patch

This does what Neil proposes. The last point is solved by removing wsm_persist from some <radio>s and even <radiogroup>s (autosync and retention) because those are stored and restored via their own dedicated code and so it looks like they do not need to go through the PageFormElements infrastructure.

Comment 2

[Magnus Melin [:mkmelin]]

Comment on attachment 8776280 [details] [diff] [review]
patch

Review of attachment 8776280 [details] [diff] [review]:
-----------------------------------------------------------------

::: mailnews/base/prefs/content/AccountManager.js
@@ +1415,1 @@
>        formElement.value = value;

these 4 lines could be just

formElement.value = value || "";

::: mailnews/base/prefs/content/am-offline.xul
@@ +59,3 @@
>               label="&allAutosync.label;" oncommand="onAutosyncChange();"/>
>        <hbox flex="1" align="center">
> +        <radio id="useAutosync.ByAge" accesskey="&ageAutosync.accesskey;" 

trailing space, remove while you're there

@@ +122,3 @@
>              label="&retentionKeepAll.label;" oncommand="onCheckKeepMsg();"/>
>          <hbox flex="1" align="center">
> +            <radio id="retention.keepNewMsg" accesskey="&retentionKeepRecent.accesskey;" 

trailing space

Comment 3

[:aceman]

Ok, thanks.
https://hg.mozilla.org/comm-central/rev/26b20089336b59dd89b893a5ba0b2460b8896472