757614 - Settings API: allow to observe a single setting

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Gregor Wagner [:gwagner]]

We want to observe a single setting. Right now we only have a global onSettingChange observer.

Do we want something like that?
  void observeSetting(in DOMString name, in nsIDOMEventListener onsettingchange);
  void removeSettingObserver(in DOMString name);

Comment 1

[Gregor Wagner [:gwagner]]

Attachment: draft implementation

Comment 2

[Gregor Wagner [:gwagner]]

Attachment: draft implementation

now with the right attachment

Comment 3

[Gregor Wagner [:gwagner]]

Comment on attachment 626187 [details] [diff] [review]
draft implementation

Please ignore the pref and DEBUG change.

Comment 4

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 626187 [details] [diff] [review]
draft implementation

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

::: dom/interfaces/settings/nsIDOMSettingsManager.idl
@@ +25,5 @@
>  interface nsIDOMSettingsManager : nsISupports
>  {
>    nsIDOMSettingsLock getLock();
>  
> +  void observeSetting(in DOMString name, in nsIDOMEventListener onsettingchange);

We shouldn't use event interfaces here, that should be left for the real Event API. Just create a new interface which just has a single function instead.

@@ +26,5 @@
>  {
>    nsIDOMSettingsLock getLock();
>  
> +  void observeSetting(in DOMString name, in nsIDOMEventListener onsettingchange);
> +  void removeSettingObserver(in DOMString name);

This should take a name and a listener. So that you can attach multiple listeners for the same setting but remove them independent of each other.

::: dom/settings/SettingsManager.js
@@ +4,5 @@
>  
>  "use strict"
>  
>  /* static functions */
> +let DEBUG = 1;

Probably don't want to check this in.

@@ +223,5 @@
>  
> +  observeSetting: function observeSetting(aName, aCallback) {
> +    if (!this._callbacks)
> +      this._callbacks = {};
> +    this._callbacks[aName] = aCallback;

We should support multiple listeners for each setting. So make each property hold an array of callbacks instead.

@@ +275,5 @@
>  
> +        let event = new this._window.MozSettingsEvent("settingchanged", {
> +          settingName: data.key,
> +          settingValue: data.value
> +        });

Rather than creating an event, simply call the callback with an object like { name: x, value: y }.

Comment 5

[Gregor Wagner [:gwagner]]

(In reply to Jonas Sicking (:sicking) from comment #4)
> Comment on attachment 626187 [details] [diff] [review]
> draft implementation
> 
> Review of attachment 626187 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/interfaces/settings/nsIDOMSettingsManager.idl
> @@ +25,5 @@
> >  interface nsIDOMSettingsManager : nsISupports
> >  {
> >    nsIDOMSettingsLock getLock();
> >  
> > +  void observeSetting(in DOMString name, in nsIDOMEventListener onsettingchange);
> 
> We shouldn't use event interfaces here, that should be left for the real
> Event API. Just create a new interface which just has a single function
> instead.

So you mean keep the add, remove function in the nsIDOMSettingsManager but the
nsIDOMEventListener arguments should be replaced by a new callback?

Comment 6

[Gregor Wagner [:gwagner]]

Attachment: WiP

Comment 7

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 629427 [details] [diff] [review]
WiP

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

::: dom/settings/SettingsManager.js
@@ +295,5 @@
>  
> +        let event = new this._window.MozSettingsEvent("settingchanged", {
> +          settingName: data.key,
> +          settingValue: data.value
> +        });

You only need to do this in the |if (_onsettingschange)| branch.

@@ +304,5 @@
> +        if (this._callbacks && this._callbacks[data.key]) {
> +          debug("observe callback called! " + data.key + " " + this._callbacks[data.key].length);
> +          for (let cb in this._callbacks[data.key]) {
> +            debug("cb: " + cb + " " + JSON.stringify(this._callbacks[data.key][cb]));
> +            this._callbacks[data.key][cb].handle({settingName: data.key, settingValue: data.value});

Cleaner as:

this._callbacks[data.key].forEach(function(cb) {
  ...
});

Also, does the JSON.stringify call there really return a useful string?

@@ +330,5 @@
> +
> +function SettingsObserverCallback(aCallback)
> +{
> +  this.callback = aCallback;
> +  debug("ObserverCallback Constructor");

Why do you need this class? Can't you just store the callback references in the array? I especially don't understand why you need the XPCOM stuff?

Comment 8

[Gregor Wagner [:gwagner]]

Attachment: patch

Thanks for the good feedback. That's an updated patch.
I will rebase and ask for review once bug 753862 has landed.

Comment 9

[Gregor Wagner [:gwagner]]

Attachment: patch

Comment 10

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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

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

r=me with those things fixed.

::: dom/settings/SettingsManager.js
@@ +263,5 @@
> +  addObserver: function addObserver(aName, aCallback) {
> +    debug("addObserver " + aName);
> +    if (!this._callbacks)
> +      this._callbacks = {};
> +    let cb = new SettingsObserverCallback();

So get rid of this constructor and just add aCallback to the _callbacks array

@@ +276,5 @@
> +  removeObserver: function removeObserver(aName, aCallback) {
> +    debug("deleteObserver " + aName);
> +    if (this._callbacks && this._callbacks[aName]) {
> +      for (let cb in this._callbacks[aName]) {
> +        if (aCallback === this._callbacks[aName][cb].callback) {

And change this to just use _callbacks.indexOf(aCallback) to find the callback to remove (still handling the case of indexOf returning -1)

@@ +278,5 @@
> +    if (this._callbacks && this._callbacks[aName]) {
> +      for (let cb in this._callbacks[aName]) {
> +        if (aCallback === this._callbacks[aName][cb].callback) {
> +          debug("delete " + aName);
> +          delete this._callbacks[aName][cb];

You need to use this._callbacks[aName].splice(index, 1) here, otherwise you end up with a hole in the array. 

Right now you'll end up throwing an exception if you add two observers, remove the first one, and then change a setting.

You should add a test for that.

@@ +336,5 @@
>  
> +function SettingsObserverCallback(aCallback)
> +{
> +  this.callback = aCallback;
> +  debug("ObserverCallback Constructor");

I still don't understand why you need this class? Why can't you just stick the callbacks in the array directly?

Comment 11

[Gregor Wagner [:gwagner]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/da7e37de7307

Comment 12

[Gregor Wagner [:gwagner]]

kaze feel free to use it :)

Comment 13

[Graeme McCutcheon [:graememcc]]

https://hg.mozilla.org/mozilla-central/rev/da7e37de7307

(Merged by Ed Morley)