mozilla / memchaser

Firefox extension to chase the memory usage and garbage collector activity
https://wiki.mozilla.org/QA/Automation_Services/Projects/Addons/MemChaser
29 stars 23 forks source link

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

Closed xabolcs closed 10 years ago

xabolcs commented 10 years ago

A fix for #185.

As you can see

Also changed some parameter name data to aData.

xabolcs commented 10 years ago

@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?

xabolcs commented 10 years ago

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 commented 10 years ago

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 commented 10 years ago

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 commented 10 years ago

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 commented 10 years ago

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

Ok, I'll do it soon.

whimboo commented 10 years ago

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 commented 10 years ago

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 commented 10 years ago

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

Just now, @whimboo.

xabolcs commented 10 years ago

@whimboo, ping.

whimboo commented 10 years ago

@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 commented 10 years ago

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

xabolcs commented 10 years ago

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 commented 10 years ago

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 commented 10 years ago

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 commented 10 years ago

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 commented 10 years ago

@whimboo , the PR is ready again to review.

xabolcs commented 10 years ago

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

whimboo commented 10 years ago

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 commented 10 years ago

@whimboo, commits were rebased and squashed.