315277 - Convert Toolkit Preference Calls to Fuel

Status: RESOLVED WONTFIX

(Toolkit :: Preferences, defect)

Description

[Mike Connor [:mconnor]]

JSLib and other bits and pieces around the tree reimplement wrappers to pref code (and other code, but that's a second step).  We should reuse some of this work to clean up our code, and provide something that extensions can reuse safely for the forseeable future.

Comment 1

[Mike Connor [:mconnor]]

Attachment: first cut

this doesn't address XBL bindings, that's the next step

Comment 2

[Simon Bünzli]

This seems a promising start. Still, some aspects to think about:
* Is XPCU really necessary? If it is, shouldn't it belong somewhere more general and get a more descriptive name (at least gXPCUtils)?
* Wouldn't it be possible to derive the type for setPref from aValue if aType is undefined and the pref doesn't exist yet (see GreaseMonkey's prefmanager.js)? This would also make the not-so-beautiful nsIPrefBranch2.PREF_* values less needed.
* What about !!aValue and parseInt(aValue) for boolean and integer prefs in setPref? Alternatively or additionally, I'd rather have the exception passed on to the caller - otherwise this might cost many programmers several hours of sleep.

Comment 3

[Davide Ficano]

Is it really necessary to call if (!this.mPrefs) this.init(); in every methods?
Should be better to make a static function called at module load time

Comment 4

[Asaf Romano (gone)]

http://lxr.mozilla.org/seamonkey/source/xpfe/components/prefwindow/resources/content/nsPrefWindow.js#168  (getPref)

Comment 5

[Nickolay_Ponomarev]

Agreed about XPCU with Simon. If we decide to use this everywhere, it should be put in a more general place and more thought should be put into it. (And btw: "aURL" should be "aContractID" or something, shouldn't it?)

In my opinion trying to guess the type of a preference is a bad idea, because prefs backend has the concept of 'complex' types which doesn't map well on the simple types as returned by getPrefType, nor on JS variable types.

The previous issue would be more obvious if you tried to implement file prefs, which I think should be done.

>+  addObserver: function(aDomain, aFunction)
It's not a function, but an object implementing nsIObserver, right?

+const nsIPrefBranch2 = Components.interfaces.nsIPrefBranch2;
Wouldn't this cause problems in case this is already declared in parent window, for example when the helper is included in multiple overlays for the same document (think extensions).

I wrote a similar wrapper a while ago: http://mozilla.doslash.org/prefutils/prefutils.js
Apart from the dynamic creation of methods (get*PrefDef), I believe it's a nice wrapper, and something similar (in terms of functionality) should be implemented in toolkit.

Comment 6

[Mark Finkle (:mfinkle) (use needinfo?)]

could be useful to move this into the FUEL js library

Comment 7

[Mike Connor [:mconnor]]

Mark, if you guys can pull this into FUEL I would love that, I'm a long way from having time to work on this, but I'd love to convert our chrome code to using the wrappers.  There's a dozen different wrappers floating around, so we should just ship one and use it ourselves.

Comment 8

[John Resig]

Mark and I are going to take the work presented in this ticket and move it over to the new Fuel library (since Fuel is included with Firefox now). This should be on track for inclusion in Firefox 3.0a6.

Comment 9

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #8)
> Mark and I are going to take the work presented in this ticket and move it over
> to the new Fuel library (since Fuel is included with Firefox now). This should
> be on track for inclusion in Firefox 3.0a6.

This bug was filed for pref wrappers accessible to all toolkit apps, so I don't think it's appropriate to morph this bug to be Firefox-specific, even if for the moment FUEL is Firefox-only. I suggest filing a new bug for pref wrappers in FUEL, and making this bug depend on it, perhaps.

Comment 10

[Nickolay_Ponomarev]

I don't see why a wrapper like fuel should be part of firefox, but not the toolkit. Where was it discussed?

Comment 11

[John Resig]

(In reply to comment #10)
> I don't see why a wrapper like fuel should be part of firefox, but not the
> toolkit. Where was it discussed?

It's certainly going to be a part of both; but to start with, we're just focusing on making it work for Firefox Extension development. This was discussed in our planning of Fuel and our team agreed that we should tackle this goal first.

Comment 12

[John Resig]

Ok - so things have been reorganized now. There are two separate goals: Converting the toolkit to using the Fuel preference calls (this bug), and converting Firefox to use the calls (bug 380288). That means that this bug is no longer on track for any specific deadline (only at some point after the Firefox conversion is complete).

Comment 13

[Asaf Romano (gone)]

(See bug 644016)