Initial implementation #2
Conversation
There was a problem hiding this comment.
r+ from me after fixing or commenting the two code issues. Lets also plan down the road for a fix to webrtcui [given input from @Standard8 or someone more knowledgable with the webrtc code]
| "resource:///modules/webrtcUI.jsm"); | ||
|
|
||
| let origReceiveMessage = null; | ||
| let boundRecv = null; |
| XPCOMUtils.defineLazyModuleGetter(this, "webrtcUI", | ||
| "resource:///modules/webrtcUI.jsm"); | ||
|
|
||
| let origReceiveMessage = null; |
There was a problem hiding this comment.
The underlying webrtcui.jsm makes this necessary, but doing a monkey patch like this is quite brittle. It feels like webrtcui should be fixed prior to moving this api beyond experiment phase. Maybe @Standard8 could give some input on this issue. https://dxr.mozilla.org/mozilla-central/source/browser/modules/webrtcUI.jsm#177
There was a problem hiding this comment.
I've only touched that module lightly. I think @fqueze is a better person to answer that.
There was a problem hiding this comment.
Agreed, I had a separate conversation with @jan-ivar about making the fix in webrtcUI.jsm. He's on board with fixing it there and that's in my queue of things to do. That change will need to land in central and ride the trains though, I'll aim to get it done before 52 is cut.
There was a problem hiding this comment.
Right, @aswan did you file a bug for that? I forget.
There was a problem hiding this comment.
Filed https://bugzilla.mozilla.org/show_bug.cgi?id=1310355 and moved the blocking callback handling over there.
| origReceiveMessage = webrtcUI.receiveMessage; | ||
| webrtcUI.receiveMessage = receiveMessage; | ||
| } | ||
| } |
There was a problem hiding this comment.
what happens if hookCount > 0 and webrtcUI.receiveMessage != receiveMessage?
There was a problem hiding this comment.
If hookCount > 0 then we previously hooked our version of receiveMessage in. If webrtcUI.receiveMessage is not our receiveMessage then somebody else has also hooked receiveMessage. I think the only reasonable thing is to assume that the other hook is a good citizen and is passing incoming messages through to us when appropriate.
This of course can/will get cleaner if we improve the hooking facilities in webrtcUI.jsm as discussed in a previous comment.
This is the companion to the Firefox patches in https://bugzilla.mozilla.org/show_bug.cgi?id=1310355 That patch moves the handling of blocking callbacks into webrtcUI.jsm so the code here gets much simpler as a result.
This still needs tests but has been tested manually.