Closed
Bug 828963
Opened 12 years ago
Closed 12 years ago
WebRTC gUM camera/microphone permissions aren't popped up
Categories
(Firefox for Android Graveyard :: General, defect)
Tracking
(Not tracked)
VERIFIED
FIXED
Firefox 21
People
(Reporter: gcp, Assigned: gcp)
References
Details
(Keywords: uiwanted, Whiteboard: [getUserMedia] [blocking-gum-])
Attachments
(1 file)
9.57 KB,
patch
|
mfinkle
:
review+
|
Details | Diff | Splinter Review |
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•12 years ago
|
||
We also need to add audio recording permission to the Android Manifest (and document why we need it).
Assignee | ||
Comment 2•12 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.
Comment 3•12 years ago
|
||
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•12 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•12 years ago
|
Whiteboard: [getUserMedia] [blocking-gum-]
Comment 5•12 years ago
|
||
(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•12 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.
Comment 7•12 years ago
|
||
(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•12 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•12 years ago
|
||
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•12 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 | ||
Comment 11•12 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•12 years ago
|
||
Bug 830829 is the meta-bug for the definite UX.
Comment 13•12 years ago
|
||
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•12 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
Comment 15•12 years ago
|
||
Assignee: nobody → gpascutto
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 21
Updated•4 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•