Workaround: set media.navigator.permission.disabled=true in about:config

This is an unfinished patch that's been sitting on my laptop for a few months. Just posting it so I don't lose it.

Oops, I guess I needed to qref first.

Brad -- Do you have any problems if we address this in Fx34 instead of Fx33?

(In reply to Maire Reavy [:mreavy] (Please needinfo me) from comment #6) > Brad -- Do you have any problems if we address this in Fx34 instead of Fx33? No, that's fine

Yoink.

First part (the easy one) is to handle the global indicators. This should be a good code starting point to base the other parts too. The other two parts to this bug are: the per-browser indicators, and the request UI (which may or may not be broken down further, we'll figure it out soon)

Comment on attachment 8483754 [details] [diff] [review] webrtc-globalindicators Review of attachment 8483754 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/base/content/content.js @@ +21,5 @@ > XPCOMUtils.defineLazyModuleGetter(this, "UITour", > "resource:///modules/UITour.jsm"); > XPCOMUtils.defineLazyModuleGetter(this, "FormSubmitObserver", > "resource:///modules/FormSubmitObserver.jsm"); > +XPCOMUtils.defineLazyModuleGetter(this, "ContentWebRTC", Do you still want to make this lazy if we are using it unconditionally? ::: browser/modules/ContentWebRTC.jsm @@ +23,5 @@ > + } > + this._initialized = true; > + > + Services.obs.addObserver(updateIndicators, "recording-device-events", false); > + }, nit: trailing comma. @@ +55,5 @@ > + else if (app.value && !state.showScreenSharingIndicator) > + state.showScreenSharingIndicator = "Application"; > + > + let mm = getMessageManagerForWindow(contentWindow); > + mm.sendAsyncMessage("webrtc:UpdateIndicators", state); You want to send the message outside of the for loop. Is there a way to send the message to the parent process without associating it with a specific browser? ::: browser/modules/webrtcUI.jsm @@ +31,5 @@ > + .getService(Ci.nsIMessageListenerManager); > + mm.addMessageListener("webrtc:UpdateIndicators", this); > + }, > + > + receiveMessage: function (message) { nit: other methods in this file don't have a space between 'function' and '(' (init and uninit are the exceptions here). @@ +34,5 @@ > + > + receiveMessage: function (message) { > + switch (message.name) { > + case "webrtc:UpdateIndicators": > + updateIndicators2(message.data, message.target) You haven't defined an updateIndicators2 function, and the updateIndicators function only takes one 'data' parameter. @@ +44,5 @@ > Services.obs.removeObserver(handleRequest, "getUserMedia:request"); > Services.obs.removeObserver(updateIndicators, "recording-device-events"); > Services.obs.removeObserver(removeBrowserSpecificIndicator, "recording-window-ended"); > Services.obs.removeObserver(maybeAddMenuIndicator, "browser-delayed-startup-finished"); > + mm.removeMessageListener("webrtc:UpdateIndicators", this); This won't work if you don't define mm here.

Moving old M2 P1 bugs to M3.

This patch fixes the url bar icons (including the "Stop sharing" feature), and shows the global indicators. To try this patch with e10s enabled, you still need to set media.navigator.permission.disabled to true. I'm working on the second part that will fix the permission prompts. To limit the scope of this bug, I'll file follow-up bugs for: - make the global indicators actually list the streams with e10s enabled (with the attached patch they work with e10s off, and show empty lists with e10s on). - handle removing the indicators if the child process crashed - make the browser_devices_get_user_media.js test pass with e10s.

Note: the browser_devices_get_user_media.js test is broken and will need a fix. I haven't finished debugging, but I've debugged enough to know the problem is in the test rather than in the patch I'm attaching. Part 3 will fix the test :).

Both audio and video devices are now stored in the same array, so hardcoding 0 as the device index didn't work anymore to re-enable a device type.

Comment on attachment 8491492 [details] [diff] [review] Part 1 - fix urlbar indicators, show global indicators Review of attachment 8491492 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/base/content/content.js @@ +638,5 @@ > }; > DOMFullscreenHandler.init(); > > +ContentWebRTC.init(); > +addMessageListener("webrtc:StopSharing", ContentWebRTC); let's verify that this does not need to be removed

I pushed the patches to try, and try seems happy except for a few existing intermitents and a devtools test that attachment 8491492 [details] [diff] [review] breaks (fix attached).

(In reply to :Felipe Gomes from comment #16) > ::: browser/base/content/content.js > > +addMessageListener("webrtc:StopSharing", ContentWebRTC); > > let's verify that this does not need to be removed Most of the addMessageListener calls from content.js don't have associated removeMessageListener calls, and the try push is green (except for a failure that now has an r+'ed fix), so I don't think we are leaking. https://hg.mozilla.org/integration/fx-team/rev/846759ed5d7c https://hg.mozilla.org/integration/fx-team/rev/ba660b435bb5 https://hg.mozilla.org/integration/fx-team/rev/7917631e88a4 https://hg.mozilla.org/integration/fx-team/rev/3a96b277afac

https://hg.mozilla.org/mozilla-central/rev/846759ed5d7c https://hg.mozilla.org/mozilla-central/rev/ba660b435bb5 https://hg.mozilla.org/mozilla-central/rev/7917631e88a4 https://hg.mozilla.org/mozilla-central/rev/3a96b277afac

(In reply to Florian Quèze [:florian] [:flo] from comment #13) > To limit the scope of this bug, I'll file follow-up bugs for: Done. > - make the global indicators actually list the streams with e10s enabled > (with the attached patch they work with e10s off, and show empty lists with > e10s on). Bug 1071626 > - handle removing the indicators if the child process crashed Bug 1071627 > - make the browser_devices_get_user_media.js test pass with e10s. Bug 1071623

We tested getUserMedia using latest Nightly across platforms and found two issues that are not related with getUserMedia. More details about our testing can be found in the etherpad we created: https://etherpad.mozilla.org/getUserMedia-e10s Marking this as verified fixed.