645000 - Extend preferences-service to support pref-observers

Status: RESOLVED FIXED

(Add-on SDK Graveyard :: General, enhancement, P2)

Description

[Zibi Braniecki [:zbraniecki][:gandalf]]

Quite often the single decision on whenever any given piece of code should be enabled or disabled depends on a preference. It makes sense then, to make it easy to observer preferences and currently observer-service does not give an easy way to do that.

I'm suggesting extending preference-service with add_observer and remove_observer methods that will give such functionality to addon authors.

WIP patch coming

Comment 1

[Zibi Braniecki [:zbraniecki][:gandalf]]

Attachment: WIP

example use:

  var foo = function(data) {
    console.log(data)
    prefs.remove_observer('privacy.donottrackheader', foo);
  }
  prefs.add_observer('privacy.donottrackheader', foo);

Tried to stay close to how observer-service API and nsPrefservice.addObserver API.

Comment 2

[Zibi Braniecki [:zbraniecki][:gandalf]]

actually, prefs.observers.add and prefs.observers.remove feels more right to me, but I'll wait for a general feedback first on if it's something AddonSDK team might be interested in

Comment 3

[Zibi Braniecki [:zbraniecki][:gandalf]]

Attachment: WIP v2

updated WIP to the new API proposal

Comment 4

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 522645 [details] [diff] [review]
WIP v2

Erm, you only asked for feedback, but I didn't realize that and did a review at the same time!


>+function PrefListener(branchName, func) {

Nit: the role of `func` would be more obvious if it was called `callback` or `listener`.


>+    var prefService = Cc["@mozilla.org/preferences-service;1"]
>+                                .getService(Ci.nsIPrefService);

Nit: this should be cached globally (probably in prefSvc, with the current value of that global changing to something like prefRoot).
Nit: var -> let


>+    this.register = function() {
>+        branch.addObserver("", this, false);
>+        branch.getChildList("", { })
>+              .forEach(function (name) { func(branch, name); });

This calls the callback for every observed preference upon registration, but it passes the callback a different set of arguments from the set it normally passes when an observed preference changes.  That seems wonky, as it seems like consumers will expect a registered callback to only get called when an observed preference actually changes and for its arguments to be the same every time it is called.


>+    if (observer) {
>+      console.log('removing observer')
>+      observer.unregister();
>+      cache.splice(cache.indexOf(observer), 1);
>+    }

This module shouldn't log expected behavior to the console.

Also, if I remember correctly, the last time we discussed it, we decided to make an API throw when a consumer tries to unregister something that isn't registered.

This needs to unregister all observers on unload using the `unload` module.

And it needs tests!  And docs!


Otherwise, this looks fine, and the API seems ok, although I would prefer either an EventEmitter-style API (with "on" and "removeEventListener" methods on the `exports` object, registration for specific prefs rather than branches, and the argument to the callback being the new value of the preference) or an observe/ignore-style API like the one in the JS module on which this CommonJS module is based <http://hg.mozilla.org/labs/jsmodules/file/tip/Preferences.js>.

Note: in both cases, the API would allow consumers to observe only specific preferences rather than preference branches.  That seems like the common case, so I think it's the right one to provide the initial and most optimized API for, with some additional API or a special pref name syntax ("foo.bar.*"?) down the road to handle the branch case if it proves to be necessary.

Comment 5

[Zibi Braniecki [:zbraniecki][:gandalf]]

(In reply to comment #4)
> Comment on attachment 522645 [details] [diff] [review]
> WIP v2
> 
> Erm, you only asked for feedback, but I didn't realize that and did a review at
> the same time!

How dare you! :)

> 
> >+function PrefListener(branchName, func) {
> 
> Nit: the role of `func` would be more obvious if it was called `callback` or
> `listener`.

fixed

> 
> >+    var prefService = Cc["@mozilla.org/preferences-service;1"]
> >+                                .getService(Ci.nsIPrefService);
> 
> Nit: this should be cached globally (probably in prefSvc, with the current
> value of that global changing to something like prefRoot).
> Nit: var -> let

Sure.
 
> >+    if (observer) {
> >+      console.log('removing observer')
> >+      observer.unregister();
> >+      cache.splice(cache.indexOf(observer), 1);
> >+    }
> 
> This module shouldn't log expected behavior to the console.
> 

fixed.

> Also, if I remember correctly, the last time we discussed it, we decided to
> make an API throw when a consumer tries to unregister something that isn't
> registered.

fixed.
 
> This needs to unregister all observers on unload using the `unload` module.

fixed.

> And it needs tests!  And docs!

absolutely. That's why it was WIP, not a patch ;)
 
> Otherwise, this looks fine, and the API seems ok, although I would prefer
> either an EventEmitter-style API (with "on" and "removeEventListener" methods
> on the `exports` object, registration for specific prefs rather than branches,
> and the argument to the callback being the new value of the preference) or an
> observe/ignore-style API like the one in the JS module on which this CommonJS
> module is based <http://hg.mozilla.org/labs/jsmodules/file/tip/Preferences.js>.

Umm... who's there to make the call? I see no reason to switch to either of them :)

Comment 6

[Zibi Braniecki [:zbraniecki][:gandalf]]

Attachment: WIP v3

patch with most of myk's comments addressed.

The big questions now are:
* should we switch to add/removeEventListener or observe/ignore? I like the latter more
* should we switch from branches to single preferences? Not sure if it adds value and it would require to do more checks cause our XPCOM service is tuned for branches, the way to switch to nodes, we'll just have to block branches :)

once you make a call, I'll be happy to provide the patch for review :)

Comment 7

[Myk Melez [:myk] [@mykmelez]]

(In reply to comment #6)
> * should we switch to add/removeEventListener or observe/ignore? I like the
> latter more

The latter is fine.


> * should we switch from branches to single preferences? Not sure if it adds
> value and it would require to do more checks cause our XPCOM service is tuned
> for branches, the way to switch to nodes, we'll just have to block branches :)

Does code actually use branches much?  In my experience, code almost always interacts with individual preferences, which is why the current API doesn't expose branches.

I think it'd be better for the Preferences API to deal with individual preferences, and then, if we identify a need to deal with branches, we can implement an explicit pref branch API for that.

Comment 8

[Myk Melez [:myk] [@mykmelez]]

(automatic reprioritization of 1.0 bugs)

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Marking anything that potentially looks like a feature request as "enhancement", sorry for the bugspam. :)

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Re-prioritizing all 1.1-targeted feature requests to 1.2.

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

(Pushing all open bugs to the --- milestone for the new triage system)

Comment 13

[Erik Vold]

Is it alright if I handle this in bug 709382? @myk & @gandalf ?

Comment 14

[Myk Melez [:myk] [@mykmelez]]

It's fine by me!

Comment 15

[Zibi Braniecki [:zbraniecki][:gandalf]]

Umm, two questions:

1) it seems that bug 709382 is about simple-prefs, not preferences-service. Are those just two different API's for the same backend data?

2) Bug 709382 is about iteration over prefs, this bug is about observing pref changes and firing events. Do you intend to extend that bug to accommodate this feature as well?

Comment 16

[Zibi Braniecki [:zbraniecki][:gandalf]]

ah, ok, I found the answer to my first question in the sources.

Comment 17

[Tomislav Jovanovic :zombie]

this was fixed in bug 709382.  

https://developer.mozilla.org/en-US/Add-ons/SDK/High-Level_APIs/simple-prefs#Globals