Closed Bug 448107 Opened 12 years ago Closed 12 years ago

Revise EnableTextbox to be more element generic

Categories

(SeaMonkey :: Preferences, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
seamonkey2.0a1

People

(Reporter: iann_bugzilla, Assigned: iann_bugzilla)

References

Details

Attachments

(2 files)

At the moment EnableTextbox in preferences.js gives the impression (by name) that it is just for enabling textboxes whereas it could be used for most elements. There is also scope for allowing an element rather than just its id to be passed if need be.
This has been spun off from bug 441403
This patch:
* Changes name of EnableTextbox to EnableElementById
* Adds function EnableElement which EnableElementById calls
* Changes consumers to use EnableElementById

Carrying forward sr= from bug 441403
Attachment #331416 - Flags: superreview+
Attachment #331416 - Flags: review?(mnyromyr)
Blocks: 445011
Blocks: 444157, 445010
Blocks: 429143
Blocks: 428705
Comment on attachment 331416 [details] [diff] [review]
patch for preferences.js v0.1 (Checkin: Comment 3)

r=me by code inspection; courtesy of Vancouver Airport Wifi Mozcamp. ;-)
Attachment #331416 - Flags: review?(mnyromyr) → review+
Comment on attachment 331416 [details] [diff] [review]
patch for preferences.js v0.1 (Checkin: Comment 3)

http://hg.mozilla.org/comm-central/index.cgi/rev/4a777e17bf30
Attachment #331416 - Attachment description: patch for preferences.js v0.1 → patch for preferences.js v0.1 (Checkin: Comment 3)
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Hmm. You could just have one function "EnableElement" and test if the first parameter is either a string or an object. e.g.

function EnableElement(aItem, aEnable, aFocus)
{
  let aElement = (typeof aItem) == "string" ? document.getElementById(aItem) : aItem;
...etc..

I've seen it done this way in other parts of the Mozilla codebase.
<http://mxr.mozilla.org/comm-central/search?string===+%22string%22>
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
As suggested this patch:
* Determines what to do depending on typeof aItem
* Changes callers to use new name of EnableElement
Attachment #332051 - Flags: superreview?(neil)
Attachment #332051 - Flags: review?(mnyromyr)
Status: REOPENED → ASSIGNED
Comment on attachment 332051 [details] [diff] [review]
EnableElement patch v0.2

I'm not convinced.
Attachment #332051 - Flags: superreview?(neil)
Comment on attachment 332051 [details] [diff] [review]
EnableElement patch v0.2

>+  let element = (typeof aItem) == "string" ? document.getElementById(aItem) :
>+                                             aItem;
>+  let pref = document.getElementById(element.getAttribute("preference"));

I disagree.
Having different functions for different parameter types is much more readable and much less error-prone, see the problems the preferences base binding has with insufficient type checking...
Attachment #332051 - Flags: review?(mnyromyr) → review-
> see the problems the preferences base binding has
> with insufficient type checking...

Does the patch in attachment 331416 [details] [diff] [review] do *any* type checking at all?
(In reply to comment #8)
> Does the patch in attachment 331416 [details] [diff] [review] do *any* type checking at all?

I know this is just a rhetorical question, but: no, of course not, because we know the callers and about the existence of the parameters there.
Doing weird type checking is just lazy and error-prone.

Status: ASSIGNED → RESOLVED
Closed: 12 years ago12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.