823453 - Media manager needs to accept an array of devices to approve when receiving the getUserMedia:response:allow notification

Status: RESOLVED FIXED

(Core :: WebRTC: Audio/Video, defect, P1)

Description

[Dão Gottwald [:dao]]

See bug 802421 comment 7.

Who can take this? Randell?

Comment 1

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Change gUM permissions response to be an array

Comment 2

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 694399 [details] [diff] [review]
Change gUM permissions response to be an array

Gold star to the first reviewer to step up!
Dao, if you're comfortable reviewing this feel free to step in and do so.

Comment 3

[Randell Jesup [:jesup] (needinfo me)]

FYI, tested with the patch from bug 802421 and appears happy

Comment 4

[Dão Gottwald [:dao]]

Comment on attachment 694399 [details] [diff] [review]
Change gUM permissions response to be an array

>     if (aSubject) {
>-      // A particular device was chosen by the user.
>-      // NOTE: does not allow setting a device to null; assumes nullptr
>-      nsCOMPtr<nsIMediaDevice> device = do_QueryInterface(aSubject);
>-      if (device) {
>+      nsCOMPtr<nsISupportsArray> array(do_QueryInterface(aSubject));
>+      if (array) {
>+        // A particular device or devices were chosen by the user.

The array should always be there -- if it's missing, you can treat that as a failure.

I'm curious what happens when the array is empty, which can happen if we add the "No Video"/"No Audio" options.

Comment 5

[Randell Jesup [:jesup] (needinfo me)]

If No Audio and No Video are selected, that's denying access.  We can do that on the UI side, or in the backend code - or both.  Right now, count of 0 will select default devices instead of no devices, so we need one or the other to consider it a Deny.

Comment 6

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Change gUM permissions response to be an array

treating empty array as Deny here; could be done as well in UI.  Can't test easily since 'No audio' and 'No video' don't appear in the dropdown - they should be added

Comment 7

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 694423 [details] [diff] [review]
Change gUM permissions response to be an array

Depending on what we do in the UI we could lose the MOZ_ASSERT(len) as well

Comment 8

[Anant Narayanan [:anant]]

Comment on attachment 694423 [details] [diff] [review]
Change gUM permissions response to be an array

Review of attachment 694423 [details] [diff] [review]:
-----------------------------------------------------------------

Look good to me!

Comment 9

[Randell Jesup [:jesup] (needinfo me)]

Note: needs to land with bug 802421

Comment 10

[Dão Gottwald [:dao]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/faa37c5e3508

Comment 11

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/faa37c5e3508