Use postMessage for widget and panel communication instead port object (#185) #186

Merged
merged 1 commit into from Mar 28, 2014

2 participants

@xabolcs

A fix for #185.

As you can see

  • object.postMessage({emitMessage: aData}, "*") is the new object.port.emit("emitMessage", {aData})
  • object.on("message", function ({emitMessage: aData}) { ... }) + early return is the new object.port.on("emitMessage", function (aData) { ... })

Also changed some parameter name data to aData.

@xabolcs

@whimboo, here is the fix for the postMessage upgrade.

I have just one question: should I insert a new line after the early return to improve readability?

@whimboo whimboo and 1 other commented on an outdated diff Feb 19, 2014
extension/data/panel/context.js
@@ -3,13 +3,14 @@
elements.forEach(function (element) {
element.onclick = function () {
- self.port.emit("command", { type: this.dataset.action});
+ self.postMessage({command: { type: this.dataset.action}}, "*");
@whimboo
whimboo added a line comment Feb 19, 2014

Given that we only communicate between modules in our own extension the second argument to postMessage should not be necessary.

@xabolcs
xabolcs added a line comment Feb 19, 2014

All the ui/toolbar and ui/frame example have this target too.
Also, ui/frame has postMessage(message, target) method. It's currently unused.

@whimboo
whimboo added a line comment Feb 19, 2014

Given that this parameter is not used at the moment I would say we don't add it here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@whimboo whimboo commented on an outdated diff Feb 19, 2014
extension/data/panel/context.js
};
});
})();
-self.port.on("update", function (aData) {
+self.on("message", function ({update: aData}) {
@whimboo
whimboo added a line comment Feb 19, 2014

Why do we have to add the dict here? I would say we should use a switch statement to differentiate between the different type of messages in the function body.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@whimboo whimboo and 1 other commented on an outdated diff Feb 19, 2014
extension/data/widget/widget.js
}
});
});
-self.port.on('update_cycle_collector', function (aData) {
+self.on("message", function ({update_cycle_collector: aData}) {
+ if (typeof(aData) === "undefined") return;
@whimboo
whimboo added a line comment Feb 19, 2014

Same as mentioned above. We should have a single self.on() definition for the messages and do the handling of messages inside a switch for each type of message, eg. update_memory, update_cylce_collector.

@xabolcs
xabolcs added a line comment Feb 19, 2014

My decision between switch and this was based on MDN article Communicating using "postMessage". Current implementation is shorter and more readable than switch, IMHO.

@whimboo
whimboo added a line comment Feb 19, 2014

No, this is way harder to understand because you are mangling a dict into the function parameter. I'm even not clear how you differentiate between different message type this way. I think we should really have a single self.on() method with switch message.type.

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

No, this is way harder to understand because you are mangling a dict into the function parameter. I'm even not clear how you differentiate between different message type this way. I think we should really have a single self.on() method with switch message.type.

I don't know what's so hard in this type of messaging. It's the same as the port.emit() and port.on(). This is one type of destructuring assignment.

I don't see why passing {someKey: keyValue, someOtherKey: data} as parameter is simpler and easier to understand than passing {key: data}. Did you check the mouseout + mouseover example on MDN what I linked above?

@xabolcs

Btw I put the keys in " to make editors color them.
Should I implement a lib/port.js and require it in the Australis code instead fixing #185?

@whimboo

I don't know what's so hard in this type of messaging. It's the same as the port.emit() and port.on(). This is one type of destructuring assignment.

It makes it simply harder to follow which objects are getting passed into methods. So I really want to avoid the usage of such constructs in memchaser. Please extract the necessary properties in the function body as needed.

@whimboo

Should I implement a lib/port.js and require it in the Australis code instead fixing #185?

Not sure what you mean here. We have to use postMessage and #185 has to be fixed. I don't see a reason why to have an extra port.js module.

@xabolcs

Please extract the necessary properties in the function body as needed.

Ok, I'll do it soon.

@whimboo

Hi @xabolcs. Do you have an ETA for this update? Aurora is going to be merged next week to Beta, and I would like to get out a new version of memchaser before that happens. Thanks.

@xabolcs

Hi @xabolcs. Do you have an ETA for this update?

Hi @whimboo, sorry for the silence, I'm going to address your comments and push the changes later tonight.

@xabolcs

I'm going to address your comments and push the changes later tonight.

Just now, @whimboo.

@whimboo whimboo and 1 other commented on an outdated diff Mar 19, 2014
extension/data/panel/context.js
@@ -3,16 +3,17 @@
elements.forEach(function (element) {
element.onclick = function () {
- self.port.emit("command", { type: this.dataset.action});
+ self.postMessage({"command": {"type": this.dataset.action}});
@whimboo
whimboo added a line comment Mar 19, 2014

I checked this and I have to say that this structure is not that scalable as I want. For an easier handling of messages we should always have a type and data property, whereby type points to e.g. command. So here an example: { type: "command", data: this.dataset.action }. With that we could easily have a switch statement depending on the type: switch (aEvent.data.type) { case "command": ... }

@whimboo
whimboo added a line comment Mar 19, 2014

Would you mind telling me which targets postMessage has? Will it be send to each of our modules, so we have to filter the appropriate types inside each on() method?

@xabolcs
xabolcs added a line comment Mar 20, 2014

..., so we have to filter the appropriate types inside each on() method?

I think, we don't have to. It should be * for now.

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

@xabolcs thanks for the update. In the future please make a comment too, so I get a notification for an update pull. I will have a look at it now.

@whimboo whimboo commented on an outdated diff Mar 21, 2014
extension/data/panel/context.js
};
});
})();
-self.port.on("update", function (aData) {
- let data = aData || { };
+self.on("message", function (aEvent) {
+ if (aEvent.data.type === "update") {
@whimboo
whimboo added a line comment Mar 21, 2014

As suggested in my last reviews, we should make that a switch statement, so we can more easily handle that in the future, and we are in sync with other modules here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@whimboo whimboo commented on an outdated diff Mar 21, 2014
extension/data/widget/widget.js
-self.port.on('update_cycle_collector', function (aData) {
- updateCollector('cc', aData);
-});
+ case "update_cycle_collector":
@whimboo
whimboo added a line comment Mar 21, 2014

Mind keeping the alphabetical order for the cases?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@whimboo whimboo commented on an outdated diff Mar 21, 2014
extension/lib/main.js
}
});
- // If user hovers over an entry the tooltip has to be updated
- widget.port.on("update_tooltip", function (data) {
- if (data === "logger" && logger.active) {
- data = "logger_enabled";
- }
- else if (data === "logger") {
- data = "logger_disabled";
+
+ widget.on("message", function (aEvent) {
+ switch (aEvent.data.type) {
+ case "update_tooltip":
+ // If user hovers over an entry the tooltip has to be updated
@whimboo
whimboo added a line comment Mar 21, 2014

I would put this above the case line.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@whimboo whimboo commented on an outdated diff Mar 21, 2014
extension/lib/main.js
- break;
- case "minimize_memory":
- memory.minimizeMemory(memory.reporter.retrieveStatistics);
- break;
- case "trigger_cc":
- garbage_collector.doCC();
- memory.reporter.retrieveStatistics();
- break;
- case "trigger_gc":
- garbage_collector.doGlobalGC();
- memory.reporter.retrieveStatistics();
+ contextPanel.on("message", function (aEvent) {
+ switch (aEvent.data.type) {
+ case "command":
+ // If user clicks a panel entry the appropriate command has to be executed
+ let command = aEvent.data.data;
@whimboo
whimboo added a line comment Mar 21, 2014

I would initially do let { type, command } = aEvent.data. That prevents the double .data and makes it clearer to follow.

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

Mostly nits. Otherwise that PR looks great now. Thanks for your work!

@xabolcs

Mostly nits. Otherwise that PR looks great now. Thanks for your work!

Thanks @whimboo, but it still doesn't work for me, could you test it please, and provide a quick feedback?

@whimboo

I added the following lines to the widget.js module at the top of the on() method:

dump('************ message: ' + JSON.stringify(aEvent) + '\n\n');
dump('************ type: ' + type + '\n\n');

As what you can see in the console, type is undefined. So what you actually want to do is:

let { type, data } = aEvent;

No need to access aEvent.data, which seems what gets pushed in as parameter.

@xabolcs

Thanks @whimboo, I found just it too!
But this is a bad news, because I'm reading the docs, and as I see ui/frame will send and receive {data: ..., source: ..., origin: ...} like objects ... :-/
I going to investigate soon.

@whimboo

Most likely due to policies so that you can make sure to only process messages which come from trusted sources. At the end we might only need data, but should at least check origin if it comes from ourself.

@xabolcs

@whimboo , the PR is ready again to review.

@xabolcs

@whimboo , Ihad to push one more fixup, but now really done. :)

@whimboo

I think that all looks great now! @xabolcs can you please squash the commits and update the commit message if necessary? Once done I will test it on my own again, and if all works we can get it landed.

@xabolcs

@whimboo, commits were rebased and squashed.

@whimboo whimboo merged commit 1005cf8 into mozilla:master Mar 28, 2014

1 check passed

Details default The Travis CI build passed
@xabolcs xabolcs deleted the xabolcs:branch-issue-185-bye-port-welcome-postmessage branch Mar 31, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment