828963 - WebRTC gUM camera/microphone permissions aren't popped up

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Gian-Carlo Pascutto [:gcp]]

When initiating a WebRTC call, Firefox for Android currently doesn't pop up a permission request to use the camera/microphone. We need this to pref on.

Comment 1

[Gian-Carlo Pascutto [:gcp]]

We also need to add audio recording permission to the Android Manifest (and document why we need it).

Comment 2

[Gian-Carlo Pascutto [:gcp]]

UX-wise I think an issue is that desktop shows a combobox with a list of devices to give access to, whereas the equivalent (Doorhanger) on Android doesn't have any possibility to do so.

Given that on Android there's only going to be 1 audio input, and likely for WebRTC only 1 relevant video input (front cam), I think we can live with doing away with the comboboxes for now.

Comment 3

[Mark Finkle (:mfinkle) (use needinfo?)]

On Android, since two cameras might exist, you could use the "select" prompt:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/config.xhtml#108

I agree about the audio. No need to choose.

Comment 4

[Gian-Carlo Pascutto [:gcp]]

I'll do a POC with a DoorHanger with multiple buttons (when appropriate), this seems most consistent with desktop UI & our current Android UI.

Desktop also changes the location bar to add camera icon so the user knows he has given his permission. IIRC we don't do this for Android right now (for example for geolocation). But leaving your camera and mic on is potentially way more disastrous than leaving geolocation on, so I think UX input will be needed here.

Comment 5

[Mark Banner (:standard8)]

(In reply to Mark Finkle (:mfinkle) from comment #3)
> I agree about the audio. No need to choose.

What about speaker versus earpiece?

Comment 6

[Gian-Carlo Pascutto [:gcp]]

>What about speaker versus earpiece?

Earpiece mutes the speaker on all devices I've seen so far. I'm not sure if that's even a choice for an Android app.

Comment 7

[Mark Banner (:standard8)]

(In reply to Gian-Carlo Pascutto (:gcp) from comment #6)
> >What about speaker versus earpiece?
> 
> Earpiece mutes the speaker on all devices I've seen so far. I'm not sure if
> that's even a choice for an Android app.

Ok, I meant the speaker held to your ear for "normal" phone calls, versus the speaker used for speakerphone/games etc.

Comment 8

[Gian-Carlo Pascutto [:gcp]]

I have code that displays all devices that are valid for a call and allows the user to select (our Doorhangers allow this), but the Android code doesn't seem to set the device name correctly. I think this, combined with the inability to show a "camera is enabled" notification on Android, will make it hard to just land a temporary UI.

Comment 9

[Gian-Carlo Pascutto [:gcp]]

Attachment: WIP. Pop up a permission request for WebRTC camera/audio

This will pop up a Doorhanger requesting access to camera and/or microphone. The camera part is untested as we don't have video working yet (technically, NumberOfCaptureDevices
bails with "AttachAndUseAndroidDeviceInfoObjects: SetAndroidObjects not called with a valid JVM.")

The OpenSLES backend currently isn't capable of identifying the audio device more precisely: http://dxr.mozilla.org/mozilla-central/media/webrtc/trunk/src/modules/audio_device/main/source/android/audio_device_android_opensles.cc.html#l683

The latter combined with the lack of an indicator that the camera is on makes me question if it is sensible to land this as a temporary. We probably can't really pref on WebRTC on mobile until we at least add a camera indicator, but maybe for development popping up the Doorhanger is already way friendlier than requiring a source patch/pref adjustment?

It's clear UX will need to think what to do about this (device selection, showing that camera is on) on mobile.

Comment 10

[Gian-Carlo Pascutto [:gcp]]

>Ok, I meant the speaker held to your ear for "normal" phone calls, versus the 
>speaker used for speakerphone/games etc.

This bug is only about inputs. We don't ask for permission to use outputs.

Comment 11

[Gian-Carlo Pascutto [:gcp]]

Comment on attachment 701828 [details] [diff] [review]
WIP. Pop up a permission request for WebRTC camera/audio

Brad told me he prefers to have this landed, and IMHO it certainly makes development easier, so let's land it. WebRTC is still defaulted off, so the issues aren't critical.

mfinkle: Because it's not so clear to me yet how much of the code is going to be shared between desktop and mobile, the code is still split. We can make a followup bug if it is sensible to merge them and move to toolkit/ as this is developed further.

Comment 12

[Gian-Carlo Pascutto [:gcp]]

Bug 830829 is the meta-bug for the definite UX.

Comment 13

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 701828 [details] [diff] [review]
WIP. Pop up a permission request for WebRTC camera/audio

>diff --git a/mobile/android/chrome/content/browser.js b/mobile/android/chrome/content/browser.js

>+var WebrtcUI = {

WebrtcUI is not may favorite captialization, but I see the dilemma.
 
>+  handleRequest: function handleRequest(aSubject, aTopic, aData) {
>+    let {windowID: windowID, callID: callID} = JSON.parse(aData);

nit: Can you add spaces inside the {} ?
So: { windowID: windowID, callID: callID }

>+    let someWindow = Services.wm.getMostRecentWindow(null);

Could you pass "navigator:browser" to be consistent with all other uses of getMostRecentWindow?
Also, if someWindow != window, we should return and exit.

>+    let contentWindow = someWindow.QueryInterface(Ci.nsIInterfaceRequestor)
>+          .getInterface(Ci.nsIDOMWindowUtils)
>+          .getOuterWindowWithId(windowID);
>+    let browser = contentWindow.QueryInterface(Ci.nsIInterfaceRequestor)
>+          .getInterface(Ci.nsIWebNavigation)
>+          .QueryInterface(Ci.nsIDocShell)
>+          .chromeEventHandler;

You could use BrowserApp.getBrowserForContent(...) to get browser, but this works too. We used to have a BrowserApp.getBrowserFromContentID too, but I guess we removed it. Also, I don't mind keeping the code on single lines too. Up to you.

>+  getDeviceButtons: function(aDevices, aCallID, stringBundle) {

>+          let allowedDevices = Cc["@mozilla.org/supports-array;1"]
>+                .createInstance(Ci.nsISupportsArray);

No wrap please

>+  prompt: function prompt(aBrowser, aCallID, aAudioRequested, aVideoRequested, aDevices) {

>+    let chromeDoc = aBrowser.ownerDocument;
>+    let chromeWin = chromeDoc.defaultView;

These two variables don't seem to be used

>+    let message = stringBundle.formatStringFromName("getUserMedia.share" + requestType + ".message",
>+                                                    [ host ], 1);

No wrap here please

>diff --git a/mobile/android/locales/en-US/chrome/browser.properties b/mobile/android/locales/en-US/chrome/browser.properties

>+# LOCALIZATION NOTE (getUserMedia.shareSelectedDevices.label):

Do we use this label? If no, remove this

>+# Semi-colon list of plural forms. See:
>+# http://developer.mozilla.org/en/docs/Localization_and_Plurals
>+# The number of devices can be either one or two.

Do we make any use of pluralized forms? If not, remove this

>+getUserMedia.denyRequest.accesskey = D

accesskey is not used on Android

>+getUserMedia.shareRequest.label = Share
>+getUserMedia.shareRequest.accesskey = S

same

r+ with changes

Comment 14

[Gian-Carlo Pascutto [:gcp]]

Landed with nits addressed. This is a temporary UX to make development easier. To be revised in bug 830829 before we pref on by default.

The extra permission in the manifest should probably be documented.

https://hg.mozilla.org/integration/mozilla-inbound/rev/6344898b83c3

Comment 15

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/6344898b83c3