Andrew McCreight [:mccr8] Assignee
	Description • 7 years ago

ContentWebRTC.jsm is imported for two reasons:

1. Add a bunch of observers, once per process.
2. Add a bunch of message listeners.

We shouldn't have to pay the price of a .jsm unless we're actually using WebRTC.

We can avoid the import for 1 by moving the addObserver calls into ContentObservers.jsm, and thunkifying them. Maybe avoiding the compartment for ContentObservers.jsm would be nice, but it is an easy way to have code only run once per process.

The import for 2 can be done just by thunkifying the message listeners. It is ugly but it works.

	Andrew McCreight [:mccr8] Assignee
	Comment 1 • 7 years ago

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2cb195247ca45e64cc0c93efa00374164c4c5333

	Florian Quèze [:florian]
	Comment 4 • 7 years ago

The webrtc:StartBrowserSharing code at https://hg.mozilla.org/try/rev/222fb449535fb8149197c3df8cefa23ff78a6f07#l1.30 seems to be deadcode. It was part of Firefox Hello and landed in bug 1133493.

	Florian Quèze [:florian]
	Comment 6 • 7 years ago
mozreview-review

Comment on attachment 8855534 [details]
Bug 1352522, part 3 - Remove the unused listener for webrtc:StartBrowserSharing.

https://reviewboard.mozilla.org/r/127362/#review130342

	Florian Quèze [:florian]
	Comment 7 • 7 years ago
mozreview-review

Comment on attachment 8855360 [details]
Bug 1352522, part 1 - Add ContentWebRTC observers in ContentObservers.js.

https://reviewboard.mozilla.org/r/127208/#review130344

r+ because I think this works and doesn't introduce bugs, but I wonder if it wouldn't be cleaner to have a single shim function used by all 5 observers, and to add an observe method in the ContentWebRTC object, with a switch/case in it, like receiveMessage does.
If all the observers use the same shim function, you can even define a constant array of the notification topics we care about, and loop over it both when adding and when removing observers.

Also, style wise, mixing methods directly inside the ContentWebRTC objects with method added using
ContentWebRTC.methodName = () => {
isn't exactly beautiful either.

	Florian Quèze [:florian]
	Comment 8 • 7 years ago
mozreview-review

Comment on attachment 8855361 [details]
Bug 1352522, part 2 - Use a shim for ContentWebRTC message listeners.

https://reviewboard.mozilla.org/r/127210/#review130346

::: browser/base/content/content.js:712
(Diff revision 1)
> -addMessageListener("rtcpeer:Deny", ContentWebRTC);
> -addMessageListener("webrtc:Allow", ContentWebRTC);
> -addMessageListener("webrtc:Deny", ContentWebRTC);
> -addMessageListener("webrtc:StopSharing", ContentWebRTC);
> +// it is actually needed.
> +var ContentWebRTCShim = {
> +  receiveMessage(aMessage) {
> +    ContentWebRTC.receiveMessage(aMessage);
> +  }
> +};

The nsIMessagerListener interface is defined with the 'function' keyword (http://searchfox.org/mozilla-central/rev/fcd9f1480d65f1a5df2acda97eb07a7e133f6ed4/dom/base/nsIMessageManager.idl#173), so you can use a function instead of an object as the second addMessageListener parameter, ie. simplify to:

var ContentWebRTCShim = message => ContentWebRTC.receiveMessage(message);

	Florian Quèze [:florian]
	Comment 9 • 7 years ago

I r+'ed, but don't hesitate to re-request review if you decide to make significant changes to follow my suggestions.

	Andrew McCreight [:mccr8] Assignee
	Comment 13 • 7 years ago

Thanks for the feedback. I reworked part 1 a bit, so that should probably get re-reviewed.

(In reply to Florian Quèze [:florian] [:flo] from comment #8)
> The nsIMessagerListener interface is defined with the 'function' keyword

Ah, neat, I didn't know about that.

	Andrew McCreight [:mccr8] Assignee
	Comment 14 • 7 years ago

Can you re-review part 1, please? I'm not sure how to request that in MozReview.

	Florian Quèze [:florian]
	Comment 15 • 7 years ago

(In reply to Andrew McCreight [:mccr8] from comment #14)
> Can you re-review part 1, please? I'm not sure how to request that in
> MozReview.

Looks good to me, I prefer this new version, thanks!

Please re-run at least the webrtc bc tests before landing. Eg. locally:
  mach mochitest browser/base/content/test/webrtc/

	Andrew McCreight [:mccr8] Assignee
	Comment 16 • 7 years ago

Thanks! Yeah, I did do a full Linux 64 debug try run because I didn't try myself to be able to find all the important tests for this code: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0b3038140466797cf9ffe087b293966a6ba625d9

	Pulsebot
	Comment 21 • 7 years ago

Pushed by amccreight@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c2fbf1f0dc9d
part 1 - Add ContentWebRTC observers in ContentObservers.js. r=florian
https://hg.mozilla.org/integration/autoland/rev/e62cc0fa0007
part 2 - Use a shim for ContentWebRTC message listeners. r=florian
https://hg.mozilla.org/integration/autoland/rev/2510319470b0
part 3 - Remove the unused listener for webrtc:StartBrowserSharing. r=florian

	Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)
	Comment 22 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/c2fbf1f0dc9d
https://hg.mozilla.org/mozilla-central/rev/e62cc0fa0007
https://hg.mozilla.org/mozilla-central/rev/2510319470b0