Closed
Bug 757614
Opened 13 years ago
Closed 13 years ago
Settings API: allow to observe a single setting
Categories
(Core :: DOM: Device Interfaces, defect)
Tracking
()
RESOLVED
FIXED
mozilla16
People
(Reporter: gwagner, Assigned: gwagner)
References
Details
Attachments
(1 file, 4 obsolete files)
12.88 KB,
patch
|
sicking
:
review+
|
Details | Diff | Splinter Review |
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);
Assignee | ||
Comment 1•13 years ago
|
||
Assignee | ||
Comment 2•13 years ago
|
||
now with the right attachment
Attachment #626183 -
Attachment is obsolete: true
Assignee | ||
Comment 3•13 years ago
|
||
Comment on attachment 626187 [details] [diff] [review]
draft implementation
Please ignore the pref and DEBUG change.
Attachment #626187 -
Flags: feedback?(jonas)
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 }.
Attachment #626187 -
Flags: feedback?(jonas) → feedback-
Assignee | ||
Comment 5•13 years ago
|
||
(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?
Assignee | ||
Comment 6•13 years ago
|
||
Attachment #626187 -
Attachment is obsolete: true
Attachment #629427 -
Flags: feedback?(jonas)
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?
Attachment #629427 -
Flags: feedback?(jonas) → feedback+
Assignee | ||
Comment 8•13 years ago
|
||
Thanks for the good feedback. That's an updated patch.
I will rebase and ask for review once bug 753862 has landed.
Attachment #629427 -
Attachment is obsolete: true
Assignee | ||
Comment 9•13 years ago
|
||
Attachment #629855 -
Attachment is obsolete: true
Assignee | ||
Updated•13 years ago
|
Attachment #630010 -
Flags: review?(jonas)
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?
Attachment #630010 -
Flags: review?(jonas) → review+
Assignee | ||
Comment 11•13 years ago
|
||
Assignee | ||
Comment 12•13 years ago
|
||
kaze feel free to use it :)
Comment 13•13 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/da7e37de7307
(Merged by Ed Morley)
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla16
You need to log in
before you can comment on or make changes to this bug.
Description
•