WebRTC gUM camera/microphone permissions aren't popped up

VERIFIED FIXED in Firefox 21

Status

()

Firefox for Android
General
VERIFIED FIXED
6 years ago
2 years ago

People

(Reporter: gcp, Assigned: gcp)

Tracking

({uiwanted})

Trunk
Firefox 21
ARM
Android
uiwanted
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [getUserMedia] [blocking-gum-])

Attachments

(1 attachment)

(Assignee)

Description

6 years ago
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.
(Assignee)

Comment 1

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

Comment 2

6 years ago
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.
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.
(Assignee)

Comment 4

6 years ago
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.

Updated

6 years ago
Whiteboard: [getUserMedia] [blocking-gum-]
(In reply to Mark Finkle (:mfinkle) from comment #3)
> I agree about the audio. No need to choose.

What about speaker versus earpiece?
(Assignee)

Comment 6

6 years ago
>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.
(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.
(Assignee)

Comment 8

6 years ago
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.
(Assignee)

Comment 9

6 years ago
Created attachment 701828 [details] [diff] [review]
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.
(Assignee)

Comment 10

6 years ago
>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.
(Assignee)

Updated

6 years ago
Keywords: uiwanted
(Assignee)

Comment 11

6 years ago
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.
Attachment #701828 - Flags: review?(mark.finkle)
(Assignee)

Comment 12

6 years ago
Bug 830829 is the meta-bug for the definite UX.
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
Attachment #701828 - Flags: review?(mark.finkle) → review+
(Assignee)

Comment 14

6 years ago
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
https://hg.mozilla.org/mozilla-central/rev/6344898b83c3
Assignee: nobody → gpascutto
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 21

Updated

6 years ago
Keywords: verifyme

Updated

5 years ago
Status: RESOLVED → VERIFIED
Keywords: verifyme
Duplicate of this bug: 929948
You need to log in before you can comment on or make changes to this bug.