Bug 752108 Prefswildcard (v2) #439

Merged
merged 1 commit into from Oct 18, 2012

Projects

None yet

3 participants

@gregglind
Member

Solution:

add a special "*" emitter for any prefs change in the addon branch.

No other wildcards are supported. This is similar to what happens with the global observer service.

@Gozala
Member
Gozala commented Oct 3, 2012

@erikvold I think you have added support for * did not you ? If so could you please close this one ? Otherwise could you review this change ?

Thanks!

@erikvold
erikvold commented Oct 4, 2012

@gregglind use "" instead and this will be fine

@erikvold erikvold commented on an outdated diff Oct 5, 2012
packages/addon-kit/tests/test-simple-prefs.js
@@ -101,6 +101,19 @@ exports.testPrefListener = function(test) {
simplePrefs.on("test-listen", listener);
sp["test-listen"] = true;
+
+ // Wildcard listen
+ let toSet = ['wildcard1','wildcard.pref2','wildcard.even.longer.test'];
+ let observed = [];
+
+ simplePrefs.on('',function(prefName) { observed.push(prefName); });
@erikvold
erikvold Oct 5, 2012

we should add a test that this type of listener is removed on unload as part of the testPrefUnloadListener test. Also this listener needs to be removed in this test before it is done.

@erikvold erikvold commented on an outdated diff Oct 5, 2012
packages/addon-kit/docs/simple-prefs.md
@@ -270,6 +270,9 @@ sp.on("sayHello", function() {
require("simple-prefs").on("somePreference", onPrefChange);
require("simple-prefs").on("someOtherPreference", onPrefChange);
+ /* Empty-string `""` listens to all pref changes (in the extension's branch). */
+
@erikvold
erikvold Oct 5, 2012

this space doesn't seem necessary, and the comment style we use for one-liners is // like so

@erikvold erikvold commented on an outdated diff Oct 5, 2012
packages/addon-kit/tests/test-simple-prefs.js
@@ -158,7 +171,7 @@ exports.testPrefUnloadListener = function(test) {
// this may not execute after unload, but definitely shouldn't fire listener
sp.prefs["test-listen3"] = false;
// this should execute, but also definitely shouldn't fire listener
- require("simple-prefs").prefs["test-listen3"] = false; //
+ require("simple-prefs").prefs["test-listen3"] = false; //
@erikvold
erikvold Oct 5, 2012

can you just remove this trailing // or add a comment there?

@erikvold erikvold commented on an outdated diff Oct 5, 2012
packages/addon-kit/tests/test-simple-prefs.js
@@ -101,6 +101,19 @@ exports.testPrefListener = function(test) {
simplePrefs.on("test-listen", listener);
sp["test-listen"] = true;
+
+ // Wildcard listen
+ let toSet = ['wildcard1','wildcard.pref2','wildcard.even.longer.test'];
+ let observed = [];
+
+ simplePrefs.on('',function(prefName) { observed.push(prefName); });
+ toSet.forEach(function(pref) {
+ sp[pref] = true;
+ });
+
+ toSet.forEach(function(pref) {
+ test.assert(observed.indexOf(pref) > -1, "Wildcard observed " + pref);
+ });
@erikvold
erikvold Oct 5, 2012

hmm, I think this could be done a little differently to test that the order of events is correct. Perhaps just loop throu one array and make sure the element at the same index in the other array is the same.

@erikvold
erikvold commented Oct 5, 2012

looks good @gregglind I just made some small comments to address.

@erikvold erikvold commented on an outdated diff Oct 15, 2012
packages/addon-kit/tests/test-simple-prefs.js
@@ -101,6 +101,26 @@ exports.testPrefListener = function(test) {
simplePrefs.on("test-listen", listener);
sp["test-listen"] = true;
+
+ // Wildcard listen
+ let toSet = ['wildcard1','wildcard.pref2','wildcard.even.longer.test'];
+ let observed = [];
+
+ simplePrefs.on('',function(prefName) {
@erikvold
erikvold Oct 15, 2012

this listener should be removed at some point.

@erikvold

@gregglind this is looking good! thanks, and sorry for the delay.. There is just one small thing to do, and then you will need to merge this with master, but after these two remaining things I think it is good to go!

@erikvold

hey @gregglind this looks good, thanks! It just needs to be updated with master, which may be tricky with the layout changes, if you'd like me to do this then just let me know!

@erikvold erikvold merged commit ff28559 into mozilla:master Oct 18, 2012
@erikvold

Thanks @gregglind !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment