1041421 - UnifiedComplete: issues in preferences code

Status: RESOLVED WONTFIX

(Toolkit :: Places, defect)

Description

[Asaf Romano (gone)]

***
  let prefs = new Preferences(PREF_BRANCH);
...
  let store = {
    observe: function (subject, topic, data) {
      loadPrefs();
    },
    QueryInterface: XPCOMUtils.generateQI([ Ci.nsIObserver ])
  };
  loadPrefs();
  prefs.observe("", store);

Preferences(.jsm).observe takes a callback, not an nsIObserver.

****

I didn't check all of them, but for all of those I did, it seems the defaults for autocomplete preferences is set in all.js and firefox.js, as it should. This code can, and should, assume that, and not set its own default fallbacks.

And unless there's a good reason not to do so, if there's a preference for which the default is not set in all.js/firefox, it should be fixed.

***

The preferences constants seem pointless. There should be a simple array of preferences names that is mapped to an object/map of branch-stripped-pref-name => value pairs.

***

I don't think the observer-callback should load all prefs on every change,

Comment 1

[Marco Bonardo [:mak]]

(In reply to Mano (needinfo? for any questions; not reading general bugmail) from comment #0)
> Preferences(.jsm).observe takes a callback, not an nsIObserver.

This is untrue, the argument is called "callback" but look at the javadoc:
268    * @param   callback    {Function|Object}
and
http://mxr.mozilla.org/mozilla-central/source/toolkit/modules/Preferences.jsm#417

basically, it can take nsIObserver or a function (and makes sense since nSIObserver has [function] as well.


> I didn't check all of them, but for all of those I did, it seems the
> defaults for autocomplete preferences is set in all.js and firefox.js, as it
> should. This code can, and should, assume that, and not set its own default
> fallbacks.

Those fallbacks are only used if the defaults are missing, that might happen for third parties. Preferences.jsm API wants fallbacks and I'm not sure I see what damage they are doing...

> I don't think the observer-callback should load all prefs on every change,

We might fix that, but what's the downside since prefs are in memory?

Comment 2

[Asaf Romano (gone)]

(In reply to Marco Bonardo [:mak] from comment #1)
> (In reply to Mano (needinfo? for any questions; not reading general bugmail)
> from comment #0)
> > Preferences(.jsm).observe takes a callback, not an nsIObserver.
> 
> This is untrue, the argument is called "callback" but look at the javadoc:
> 268    * @param   callback    {Function|Object}
> and
> http://mxr.mozilla.org/mozilla-central/source/toolkit/modules/Preferences.
> jsm#417
> 
> basically, it can take nsIObserver or a function (and makes sense since
> nSIObserver has [function] as well.
>

Ah! That's ok then (though I think passing a callback is better anyway). In any case, no need to implement QI.

 
> > I didn't check all of them, but for all of those I did, it seems the
> > defaults for autocomplete preferences is set in all.js and firefox.js, as it
> > should. This code can, and should, assume that, and not set its own default
> > fallbacks.
> 
> Those fallbacks are only used if the defaults are missing, that might happen
> for third parties. Preferences.jsm API wants fallbacks and I'm not sure I
> see what damage they are doing...

My concern is that the preference code in this file is much more lengthy - and somewhat complex - than necessary.

But, yeah, it seems Preferences.jsm makes it impossible to catch unset defaults. That's very disappointing, because it renders either firefox.js/all.js obsolete, or forces all callers to pass "fake defaults" which will never be used.

Given that you've already implemented nsIObserver, I'd really consider just using Services.prefs. That's just me though. I just don't like maintaining this misleading default pref values list. It's either unused or accidentally used.

> > I don't think the observer-callback should load all prefs on every change,
> 
> We might fix that, but what's the downside since prefs are in memory?

I wasn't concerned of performance. It's just that when I read that I exepeted loadPrefs to do something more than resetting the variables, because if it's just that, there's no reason to ignore the callback arguments.

Comment 3

[Marco Bonardo [:mak]]

(In reply to Mano (needinfo? for any questions; not reading general bugmail) from comment #2)
> I wasn't concerned of performance. It's just that when I read that I
> exepeted loadPrefs to do something more than resetting the variables,
> because if it's just that, there's no reason to ignore the callback
> arguments.

That needs more complex code though, since the prefs mapping it's currently from varname => pref, but we'll be notified for a pref, so then we'd need to build another mapping from pref => varname to be able to update only a single value.

Comment 4

[Marco Bonardo [:mak]]

So, I don't think it's worth to address this bug.

1. the [function] decorator has already solved the first issue.
2. I don't think we should drop Preferences.jsm cause it makes the code much simpler, and that requires us to provide defaults, that by itself is not a negative thing (imo)
3. While we could update a single pref when we get a notification, it would require for each pref to add a property => pref_const relation to a map, that would double the prefs code in size. Since there is no perf downside I don't think it's worth it.