Last Comment Bug 757614 - Settings API: allow to observe a single setting
: Settings API: allow to observe a single setting
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM: Device Interfaces (show other bugs)
: unspecified
: x86 Mac OS X
: -- normal (vote)
: mozilla16
Assigned To: Gregor Wagner [:gwagner]
:
Mentors:
Depends on: 753862
Blocks:
  Show dependency treegraph
 
Reported: 2012-05-22 14:26 PDT by Gregor Wagner [:gwagner]
Modified: 2012-06-08 04:19 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
draft implementation (7.85 KB, patch)
2012-05-22 14:28 PDT, Gregor Wagner [:gwagner]
no flags Details | Diff | Splinter Review
draft implementation (8.64 KB, patch)
2012-05-22 14:36 PDT, Gregor Wagner [:gwagner]
jonas: feedback-
Details | Diff | Splinter Review
WiP (13.02 KB, patch)
2012-06-01 19:03 PDT, Gregor Wagner [:gwagner]
jonas: feedback+
Details | Diff | Splinter Review
patch (13.50 KB, patch)
2012-06-04 11:04 PDT, Gregor Wagner [:gwagner]
no flags Details | Diff | Splinter Review
patch (12.88 KB, patch)
2012-06-04 17:34 PDT, Gregor Wagner [:gwagner]
jonas: review+
Details | Diff | Splinter Review

Description Gregor Wagner [:gwagner] 2012-05-22 14:26:39 PDT
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] 2012-05-22 14:28:29 PDT
Created attachment 626183 [details] [diff] [review]
draft implementation
Comment 2 Gregor Wagner [:gwagner] 2012-05-22 14:36:24 PDT
Created attachment 626187 [details] [diff] [review]
draft implementation

now with the right attachment
Comment 3 Gregor Wagner [:gwagner] 2012-05-22 18:21:39 PDT
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 2012-05-31 23:51:23 PDT
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] 2012-06-01 13:44:04 PDT
(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] 2012-06-01 19:03:01 PDT
Created attachment 629427 [details] [diff] [review]
WiP
Comment 7 Jonas Sicking (:sicking) No longer reading bugmail consistently 2012-06-03 21:39:46 PDT
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] 2012-06-04 11:04:51 PDT
Created attachment 629855 [details] [diff] [review]
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] 2012-06-04 17:34:44 PDT
Created attachment 630010 [details] [diff] [review]
patch
Comment 10 Jonas Sicking (:sicking) No longer reading bugmail consistently 2012-06-06 18:19:41 PDT
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 12 Gregor Wagner [:gwagner] 2012-06-07 14:39:51 PDT
kaze feel free to use it :)
Comment 13 Graeme McCutcheon [:graememcc] 2012-06-08 04:19:25 PDT
https://hg.mozilla.org/mozilla-central/rev/da7e37de7307

(Merged by Ed Morley)

Note You need to log in before you can comment on or make changes to this bug.