Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Bug 744514 - Observer callbacks get topic as argument #592

Closed
wants to merge 1 commit into from

2 participants

packages/api-utils/lib/system/events.js
@@ -59,7 +59,8 @@ const Observer = Class({
this.listener({
type: topic,
subject: subject,
- data: data
+ data: data,
+ topic: topic
@Gozala Owner
Gozala added a note

topic is already exposed via type property, is this really necessary ?

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

Gozala, this should be better.

packages/api-utils/tests/test-observer-service.js
@@ -78,3 +79,26 @@ exports.testObserverService = function(test) {
test.assertEqual(timesCalled, 2,
"observer-service.remove() should work");
};
+
+exports.testObserverWildcard = function(test) {
+ let uu = function() uuid().number.slice(1,-1);
+
+ var counter = {};
+ let uuids = [uu() for (ii in [0,1,2,3,4,5])];
@Gozala Owner
Gozala added a note

Could you please don't use comprehensions ? We banned them as it's was not obvious for people that are unfamiliar and they don't buy us much to be worth the traideoffs.

let uuids = Array(5).map(uu) 

Is both shorter and simpler.

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

Any progress on this one?

@Gozala
Owner

Hey I'm really sorry for the delay, please mention @gozala on the threads where you expect me to followup that way it'll be in the top of my message queue and I'll be getting to it sooner :wink:

I think it's ready to go, it just won't merge through github interface as pr is out of date I presume. Could you please update and land it just make sure to add r=@gozala if you don't have privileges to do that mention me and I'll land it!

Thanks

@Gozala Gozala commented on the diff
test/test-observer-service.js
@@ -6,6 +6,7 @@ const observers = require("sdk/deprecated/observer-service");
const { Cc, Ci } = require("chrome");
const { Loader } = require("sdk/test/loader");
const { PlainTextConsole } = require("sdk/console/plain-text");
+const {uuid} = require('sdk/util/uuid');
@Gozala Owner
Gozala added a note

Nit: We put spaces after brackets and try to be consistent in quotes at least with in the file.

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

At this point system/events can be used with a * event type, where listeners are called with event.type set to the topic of the notification. There for this is obsolete we'll be deprecating observer-service and removing it, so I don't think this is anymore relevant.

@Gozala Gozala closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Oct 18, 2012
  1. @gregglind

    Observer callbacks get topic as argument. bugzilla #744514

    gregglind authored
    Use case:  observe on ("*").
    
    https://bugzilla.mozilla.org/show_bug.cgi?id=744514
    
    (tests included for 4th argument)
This page is out of date. Refresh to see the latest.
View
8 lib/sdk/deprecated/observer-service.js
@@ -34,9 +34,9 @@ exports.topics = {
APPLICATION_READY: id + "_APPLICATION_READY"
};
-function Listener(callback, target) {
- return function listener({ subject, data }) {
- callback.call(target || callback, subject, data);
+function Listener(callback, target, topic) {
+ return function listener({ subject, data, type:topic }) {
+ callback.call(target || callback, subject, data, topic);
}
}
@@ -58,7 +58,7 @@ function Listener(callback, target) {
function add(topic, callback, target) {
let listeners = subscribers(callback);
if (!(topic in listeners)) {
- let listener = Listener(callback, target);
+ let listener = Listener(callback, target, topic);
listeners[topic] = listener;
// Cache callback unless it's already cached.
View
2  lib/sdk/system/events.js
@@ -12,7 +12,7 @@ const { Cc, Ci } = require('chrome');
const { Unknown } = require('../platform/xpcom');
const { Class } = require('../core/heritage');
const { ns } = require('../core/namespace');
-const { addObserver, removeObserver, notifyObservers } =
+const { addObserver, removeObserver, notifyObservers } =
Cc["@mozilla.org/observer-service;1"].getService(Ci.nsIObserverService);
const Subject = Class({
View
26 test/test-observer-service.js
@@ -6,6 +6,7 @@ const observers = require("sdk/deprecated/observer-service");
const { Cc, Ci } = require("chrome");
const { Loader } = require("sdk/test/loader");
const { PlainTextConsole } = require("sdk/console/plain-text");
+const {uuid} = require('sdk/util/uuid');
@Gozala Owner
Gozala added a note

Nit: We put spaces after brackets and try to be consistent in quotes at least with in the file.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
exports.testUnloadAndErrorLogging = function(test) {
var prints = [];
@@ -78,3 +79,28 @@ exports.testObserverService = function(test) {
test.assertEqual(timesCalled, 2,
"observer-service.remove() should work");
};
+
+exports.testObserverWildcard = function(test) {
+ let uu = function() uuid().number.slice(1,-1);
+
+ var counter = {};
+ let uuids = [0,1,2,3,4,5].map(uu);
+
+ var cb = function(subject, data, topic) {
+ if (counter[topic] === undefined) {counter[topic] = 0};
+ counter[topic] ++;
+ };
+
+ observers.add("*", cb);
+
+ uuids.forEach(function(id){
+ observers.notify(id, {},{});
+ })
+
+ uuids.forEach(function(id){
+ test.assertEqual(counter[id], 1, "wildcard counter detects correct topic, not '*' ")
+ });
+
+ observers.remove("*", cb);
+
+};
Something went wrong with that request. Please try again.