898343 - [B2G getUserMedia] Use ContentPermissionPrompt to display microphone permission prompt

Status: RESOLVED INVALID

(Core :: WebRTC, defect)

Description

[Alfredo Yang (:alfredo)]

Use ContentPermissionPrompt service to display prompt dialog on B2G process

Comment 1

[Alfredo Yang (:alfredo)]

Attachment: Add ContentPermissonRequest in MediaManager

Rought patch, it needs to be refined.

Implamentation:
Add a state machine in MediaPermissionRequest and running state machine for every 10ms:
  CHECK_DEVICE: check if request devices are available.
  PROMPT: display prompt dialog on b2g process via RPC.
  TIMEOUT: cancel request.
  CLOSE: close request.

1. It may be better to separate these classes to another files.
2. It follows the gUM permission prompt protocol on desktop, so there are many string convertions, observer notifications in codes. The codes could be clear if we don't need to follow the protocol.

Problems:
1. No way to pass device list to Gaia by currrent ContentPermissionPrompt. If fxOS 1.2 devices don't have front camera, maybe we don't need the list. Only display strings like "Permission to use microphone", "Permission to use camer", or "Permission to use WebRTC" on dialog. However, the device list is necessary for future projects.
2. The prompt dialog is permanent, there is no timeout-dismiss.
3. From prior discuss, for web content should be one-shot only. ContentPermissionPrompt doesn't support it. It probably needs to change PermissionManager too.

Comment 2

[Alfredo Yang (:alfredo)]

Attachment: gUM_Implementation

Simply state machine, refine code flow, correct some code nitch.

Comment 3

[Alfredo Yang (:alfredo)]

Attachment: gUM_Camera_Perm

Check webrtc, video-capture permission when open camera

Comment 4

[Alfredo Yang (:alfredo)]

Attachment: Implement ContentPermissionRequest for WebRTC on B2G

1. Add MediaPermissionRequest class implemented nsIContentPermissionRequest.
   It owns a state machine which will control all the flow of this request.
   A timer is setup to run the state machine for every 10ms. Once the state
   enter CLOSE, this instance can't be reused.

2. Add MediaDeviceErrorCallback and MediaDeviceSuccessCallback for 
   GetUserMediaDevices callback.

Note: the permission name is stil under discussing on bug 853356, I'll update 
      the permission name once we have the final conclusion.

Comment 5

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

Comment on attachment 783601 [details] [diff] [review]
Implement ContentPermissionRequest for WebRTC on B2G

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

As the implementation code is all for gonk and there's a moderate amount of it (unrelated to the other code in MediaManager), perhaps it should move to a separate file that's only built for GONK.  (MediaPermissionGonk.cpp?)

Regarding the 60-second timeout:
a) is this the norm for requests on B2G?
b) what happens when the user tries to accept right before the timeout?  Will they be able to know if it timed out or not?  Could they end up clicking yes, but it actually timing out due to the state machine running in a different context?  If they missed getting their selection, what options do they have to get a permission request back to change that?  Reload?  Can the JS request again?
c) what happens if the JS sends a second request with different (or the same) options while the first is pending?  I don't see a way for it to be updated here, or would the first get cancelled?  If they're serially presented, does the second timer start running when the request is sent? (I assume so).  If the first times out and the second appears as your finger is reaching for accept (or deny), is there a mis-selection issue the user may never be aware of?  (Example: request for audio only.  User right at 60s is about to touch accept.  Permission times out and is replaced by a request for video+audio.  User's finger hits Accept without seeing it had changed (or hits it before they can abort the motion)).

Some of this (especially c) get into the UX assumptions and implementation - is there selection feedback, gaps between actions, etc, and what the model for interaction is.

r- for a number of issues, plus the apparent design mis-match with the getUserMedia spec about accepting one but not the other.

::: dom/media/MediaManager.cpp
@@ +357,5 @@
> +
> +  nsresult rv;
> +  nsCOMPtr<nsPIDOMWindow> window(nsGlobalWindow::GetOuterWindowWithId(mWindowID));
> +  nsRefPtr<nsIGetUserMediaDevicesSuccessCallback> success = new MediaDeviceSuccessCallback(this, mOptions);
> +  nsRefPtr<nsIDOMGetUserMediaErrorCallback> error = new MediaDeviceErrorCallback(this, mOptions);

wrap lines after '='

@@ +360,5 @@
> +  nsRefPtr<nsIGetUserMediaDevicesSuccessCallback> success = new MediaDeviceSuccessCallback(this, mOptions);
> +  nsRefPtr<nsIDOMGetUserMediaErrorCallback> error = new MediaDeviceErrorCallback(this, mOptions);
> +  rv = mManager->GetUserMediaDevices(window, success, error);
> +  NS_ASSERTION(NS_SUCCEEDED(rv), "GetUserMediaDevices failed");
> +

remove blank line

@@ +374,5 @@
> +  mStartTime = TimeStamp::Now();
> +  if (XRE_GetProcessType() == GeckoProcessType_Content) {
> +    nsCOMPtr<nsPIDOMWindow> window(nsGlobalWindow::GetOuterWindowWithId(mWindowID));
> +    if (!window) {
> +      NS_ASSERTION(false, "Invaild Window");

spelling (same issue in multiple places)

@@ +430,5 @@
> +nsresult MediaPermissionRequest::SetAvailableDevice(nsISupports** aDevices, uint32_t aCount)
> +{
> +  nsresult rv;
> +  uint32_t VIDEO_DEVICE = 0x0001;
> +  uint32_t AUDIO_DEVICE = 0x0010;

These appear to be constants, and there's no reason to space the bits so far apart

@@ +438,5 @@
> +
> +  rv = mOptions->GetAudio(&reqAudio);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +  if (reqAudio) {
> +    reqDevices |= VIDEO_DEVICE;

AUDIO_DEVICE

@@ +444,5 @@
> +
> +  rv = mOptions->GetVideo(&reqVideo);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +  if (reqVideo) {
> +    reqDevices |= AUDIO_DEVICE;

VIDEO_DEVICE

@@ +466,5 @@
> +      hasDevices |= AUDIO_DEVICE;
> +    }
> +  }
> +
> +  if (reqDevices != hasDevices) {

Instead of reqDevices, just have hasDevices (or better yet two bools), and here call mOptions->GetVideo/Audio and check if we have a request for them.  Also, if we only care if we have a video device IF a video device was requested.  If it wasn't requested, we don't care if it's supported, so the test as written is incorrect.

Also, if there's a request for audio+video, and the device only supports audio, the request is NOT supposed to fail.  Users are allowed in getUserMedia to accept audio and reject video, and if the user has no video device, the video part of a request would be rejected but the user would be asked if they want to accept or deny audio access.

@@ +534,5 @@
> +  return NS_OK;
> +}
> +
> +NS_IMETHODIMP
> +MediaPermissionRequest::GetType(nsACString& aType)

GetType() is very generic is leads to confusion with the device GetType which returns video or audio.
Why does this not return video, audio, or audio-video?  ("webrtc" probably the wrong name here in any case)

::: dom/media/MediaManager.h
@@ +612,5 @@
>  #endif
> +
> +  // It will create one MediaPermissionRequest instance per getUserMedia call.
> +  // This is used on B2G only.
> +  nsRefPtr<MediaRequestObserver> mRequestObserver;

If these are B2G only (per the comment), why is it not inside MOZ_WIDGET_GONK?  Or (per top-level-comment) have this be #ifdef'd, and have the definitions above in a separate file (though that's not strictly necessary even if we split the .cpp files)

Comment 6

[Alfredo Yang (:alfredo)]

(In reply to Randell Jesup [:jesup] from comment #5)
> Comment on attachment 783601 [details] [diff] [review]
> Implement ContentPermissionRequest for WebRTC on B2G
> 
> Review of attachment 783601 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> As the implementation code is all for gonk and there's a moderate amount of
> it (unrelated to the other code in MediaManager), perhaps it should move to
> a separate file that's only built for GONK.  (MediaPermissionGonk.cpp?)
> 
> Regarding the 60-second timeout:
> a) is this the norm for requests on B2G?

  It is unnecessary, prompt dialog will dismiss when screen timeout accordign to current
  b2g system app design. And MediaPermissionRequest::Recv__delete__() will be called via IPDL
  to response the request (in most case, it'll be cancel).

  I'll remove the timeout mechanism.

> b) what happens when the user tries to accept right before the timeout? 
> Will they be able to know if it timed out or not?  Could they end up
> clicking yes, but it actually timing out due to the state machine running in
> a different context?  If they missed getting their selection, what options
> do they have to get a permission request back to change that?  Reload?  Can
> the JS request again?

  1~3 questions should be addressed after removing timout mechanism.
  If user missed getting selection, the default action is cancel this request.
  User can reload the page to bring the prompt dialog again if the 'remember' button
  on the dialog is not selected. In current design, only app has 'remember' option.
  Web content doesn't have it[1].

  [1] https://bugzilla.mozilla.org/show_bug.cgi?id=898986#c9

> c) what happens if the JS sends a second request with different (or the
> same) options while the first is pending?  I don't see a way for it to be
> updated here, or would the first get cancelled?  If they're serially
> presented, does the second timer start running when the request is sent? (I
> assume so).  If the first times out and the second appears as your finger is
> reaching for accept (or deny), is there a mis-selection issue the user may
> never be aware of?  (Example: request for audio only.  User right at 60s is
> about to touch accept.  Permission times out and is replaced by a request
> for video+audio.  User's finger hits Accept without seeing it had changed
> (or hits it before they can abort the motion)).

  Removing timeout mechanism will address most part of this.
  When 2+ requests sent, each request will have each own MediaPermissionRequest
  instance. System ap will queue these requests and display the dialog after prior
  one dismissed.

> 
> AUDIO_DEVICE
> 
> @@ +444,5 @@
> > +
> > +  rv = mOptions->GetVideo(&reqVideo);
> > +  NS_ENSURE_SUCCESS(rv, rv);
> > +  if (reqVideo) {
> > +    reqDevices |= AUDIO_DEVICE;
> 
> VIDEO_DEVICE
> 
> @@ +466,5 @@
> > +      hasDevices |= AUDIO_DEVICE;
> > +    }
> > +  }
> > +
> > +  if (reqDevices != hasDevices) {
> 
> Instead of reqDevices, just have hasDevices (or better yet two bools), and
> here call mOptions->GetVideo/Audio and check if we have a request for them. 
> Also, if we only care if we have a video device IF a video device was
> requested.  If it wasn't requested, we don't care if it's supported, so the
> test as written is incorrect.
> 
> Also, if there's a request for audio+video, and the device only supports
> audio, the request is NOT supposed to fail.  Users are allowed in
> getUserMedia to accept audio and reject video, and if the user has no video
> device, the video part of a request would be rejected but the user would be
> asked if they want to accept or deny audio access.
> 
> @@ +534,5 @@
> > +  return NS_OK;
> > +}
> > +
> > +NS_IMETHODIMP
> > +MediaPermissionRequest::GetType(nsACString& aType)
> 
> GetType() is very generic is leads to confusion with the device GetType
> which returns video or audio.
> Why does this not return video, audio, or audio-video?  ("webrtc" probably
> the wrong name here in any case)

  GetType() is defined in nsIContentPermissionPrompt.idl [2]. It is used broadly
  in geolocation, desktp notification and device storage permission request.
  It will be out of scope of this bug changing this idl. I can file a bug to fix
  it later. Is it make sense to you?
  
  'webrtc' is the suggested permission name in [3]. However, it is still under 
  discussing now. I'll change the permission name when the conclusion is made. 
  Sorry for not explain it in advance.

  [2] http://dxr.mozilla.org/mozilla-central/source/dom/interfaces/base/nsIContentPermissionPrompt.idl#l23
  [3] https://bugzilla.mozilla.org/show_bug.cgi?id=853356#c38
  
  Others will be fixed. Thanks for your review.

Comment 7

[Alfredo Yang (:alfredo)]

Attachment: Implement ContentPermissionRequest for WebRTC on B2G

Draft of the modified patch according to review comments, it still needs to double check.

Comment 8

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

(In reply to Alfredo Yang from comment #6)

> 
> > b) what happens when the user tries to accept right before the timeout? 
> > Will they be able to know if it timed out or not?  Could they end up
> > clicking yes, but it actually timing out due to the state machine running in
> > a different context?  If they missed getting their selection, what options
> > do they have to get a permission request back to change that?  Reload?  Can
> > the JS request again?
> 
>   1~3 questions should be addressed after removing timout mechanism.
>   If user missed getting selection, the default action is cancel this
> request.
>   User can reload the page to bring the prompt dialog again if the
> 'remember' button
>   on the dialog is not selected. In current design, only app has 'remember'
> option.
>   Web content doesn't have it[1].

This auto-cancel has implications.  For apps, this means that to change the permission after an auto-cancel, you must go to the permissions UI for the app (I believe).  Users who start an app and then get distracted (by any number of things) may never realize a choice has been permanently stored for them.  

I would suggest that an automatic denial NOT be permanently stored; only actual user selections be stored.

It's worth considering (for a future phase) to have a non-intrusive indicator an app/website is asking for and being denied access due to a stored response.  In desktop, this would be "don't put up the doorhanger, but put up the icon in the URLbar for mic/camera access requests."

This would be worth considering with the general UX team and security/privacy teams.

> 
>   [1] https://bugzilla.mozilla.org/show_bug.cgi?id=898986#c9
> 
> > c) what happens if the JS sends a second request with different (or the
> > same) options while the first is pending?  I don't see a way for it to be
> > updated here, or would the first get cancelled?  If they're serially
> > presented, does the second timer start running when the request is sent? (I
> > assume so).  If the first times out and the second appears as your finger is
> > reaching for accept (or deny), is there a mis-selection issue the user may
> > never be aware of?  (Example: request for audio only.  User right at 60s is
> > about to touch accept.  Permission times out and is replaced by a request
> > for video+audio.  User's finger hits Accept without seeing it had changed
> > (or hits it before they can abort the motion)).
> 
>   Removing timeout mechanism will address most part of this.
>   When 2+ requests sent, each request will have each own
> MediaPermissionRequest
>   instance. System ap will queue these requests and display the dialog after
> prior
>   one dismissed.

My question is still "when do the timers start", and also "when a request is auto-denied, is there any temporary UI block to avoid the failures mentioned above.

> > > +NS_IMETHODIMP
> > > +MediaPermissionRequest::GetType(nsACString& aType)
> > 
> > GetType() is very generic is leads to confusion with the device GetType
> > which returns video or audio.
> > Why does this not return video, audio, or audio-video?  ("webrtc" probably
> > the wrong name here in any case)
> 
>   GetType() is defined in nsIContentPermissionPrompt.idl [2]. It is used
> broadly
>   in geolocation, desktp notification and device storage permission request.
>   It will be out of scope of this bug changing this idl. I can file a bug to
> fix
>   it later. Is it make sense to you?

I'm not sure it needs to be changed.  If it wasn't named that for any reason, then I would, but there is a reason.

>   
>   'webrtc' is the suggested permission name in [3]. However, it is still
> under 
>   discussing now. I'll change the permission name when the conclusion is
> made. 
>   Sorry for not explain it in advance.

It should at least be getUserMedia instead of webrtc (if it's meant to describe the type of service that made the request, and I do not think that is the case).  And here it's being used to mean "audio+video", so it's really wrong.  I thought that comment referred to a category of requests, not a description of what was being granted.

Comment 9

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

(In reply to Randell Jesup [:jesup] from comment #8)
> (In reply to Alfredo Yang from comment #6)
> 
> > 
> > > b) what happens when the user tries to accept right before the timeout? 
> > > Will they be able to know if it timed out or not?  Could they end up
> > > clicking yes, but it actually timing out due to the state machine running in
> > > a different context?  If they missed getting their selection, what options
> > > do they have to get a permission request back to change that?  Reload?  Can
> > > the JS request again?
> > 
> >   1~3 questions should be addressed after removing timout mechanism.
> >   If user missed getting selection, the default action is cancel this
> > request.
> >   User can reload the page to bring the prompt dialog again if the
> > 'remember' button
> >   on the dialog is not selected. In current design, only app has 'remember'
> > option.
> >   Web content doesn't have it[1].
> 
> This auto-cancel has implications.  For apps, this means that to change the
> permission after an auto-cancel, you must go to the permissions UI for the
> app (I believe).  Users who start an app and then get distracted (by any
> number of things) may never realize a choice has been permanently stored for
> them.  
> 
> I would suggest that an automatic denial NOT be permanently stored; only
> actual user selections be stored.

I should note that I don't know how B2G permissions and permission UX normally work in any detail (as is probably evident from my questions).  These UI/permission issues I raise are based on what I know of desktop and the needs/expectations of WebRTC and the working group; they may not apply to B2G directly.  I don't know what actions/UI are considered standard, and which are at the option of the UX for the feature, so please do not take my questions or suggestions as primary guidance - if they conflict with B2G norms, we might want to query the B2G UX people, Jonas, etc, but certainly don't change the UX on my say-so.

One item I snipped from this reply was to wonder how the UI design handles these timers when there are multiple requests; I assume the timer starts when the request becomes visible ("on the top").  (I suppose this means that in theory you could have a large number of pending requests stacked up.) 

I raised these questions because reviewing the patch made me realize I didn't know how this is intended to work on B2G; these questions are not the reason for my r-.

Comment 10

[Alfredo Yang (:alfredo)]

(In reply to Randell Jesup [:jesup] from comment #9)
> I should note that I don't know how B2G permissions and permission UX
> normally work in any detail (as is probably evident from my questions). 
> These UI/permission issues I raise are based on what I know of desktop and
> the needs/expectations of WebRTC and the working group; they may not apply
> to B2G directly.  I don't know what actions/UI are considered standard, and
> which are at the option of the UX for the feature, so please do not take my
> questions or suggestions as primary guidance - if they conflict with B2G
> norms, we might want to query the B2G UX people, Jonas, etc, but certainly
> don't change the UX on my say-so.
> 
> One item I snipped from this reply was to wonder how the UI design handles
> these timers when there are multiple requests; I assume the timer starts
> when the request becomes visible ("on the top").  (I suppose this means that
> in theory you could have a large number of pending requests stacked up.) 
> 
> I raised these questions because reviewing the patch made me realize I
> didn't know how this is intended to work on B2G; these questions are not the
> reason for my r-.

The UX wireframe is at [1], FYR (that's for 1.2 scope but I don't think we will implement it all).

The permission prompt dialog is used amongs apps (geolocation, disk-storage, desktop-notification and now gUM). I think the major different of prompt between desktop and b2g is: on b2g, the prompt is actually rendered by system app, not requested app itself. Back to the timeout problem, the b2g prompt should be dissmissed by system app, not by requested app (my first patch is wrong about timeout). However, on current system app implemetation, prompt doesn't have timeout mechanism (I guess that's because system app will dismiss prompt when screen timeout and screen timeout will happen in few seconds later).

For multiple requests, current design is to prompt the request one after another. That may work for different kinds of request like geolocation and disk-storage coming together. But for gUM, the spec suggests the multiple requests should be merge to a single one prompt. UX may need to aware of it.

Hi UX teams, durign the review of gUM patch, there are 2 things need your comments.
1. Should the prompt need auto timeout mechanism?
2. For multiple gUM requests from the same script, according to spec [2], it should be merged to one prompt. I don't see it in current wireframe. It may be worth to take it into consideration for future plan (maybe for 1.3).

Thanks.

[1] https://mozilla.app.box.com/s/44utizl9oz4eupyu3fuu/1/1030019930/9427588931/1
[2] http://www.w3.org/TR/2013/WD-mediacapture-streams-20130516/#implementation-suggestions

Comment 11

[Alfredo Yang (:alfredo)]

Attachment: Implement ContentPermissionRequest for WebRTC on B2G

Draft version 2.

known issue:
Blocks at http://dxr.mozilla.org/mozilla-central/source/content/media/webrtc/MediaEngineWebRTCVideo.cpp#l233, it looks like prompt time will cause some timing issue in b2g webrtc codes.

It needs b2g webrtc expert's help.

Comment 12

[StevenLee[:slee]]

Camera initialization successes but when it tries to callback result, it fails in http://mxr.mozilla.org/mozilla-central/source/dom/camera/DOMCameraControl.cpp#419.

Comment 13

[Stephany Wilkes]

Adding Jacqueline to this since she is working on the latest draft of the gUM/webRTC spec, which Esther began.

Comment 14

[Alfredo Yang (:alfredo)]

Attachment: Implement ContentPermissionRequest for WebRTC on B2G

Send inner window to camera instead of outer window.

Comment 15

[Alfredo Yang (:alfredo)]

Attachment: Implement ContentPermissionRequest for WebRTC on B2G

Add gonk flag in moz.build to include MediaPermissionGonk in B2G only.

Comment 16

[Alfredo Yang (:alfredo)]

Attachment: gUM, prompt request via ContentPermissionRequest PIDL

1. For timeout issue. The prompt will be dismissed when screen is off on b2g. IMHO, timeout mechanism is not required here.
2. Accoring to spec suggestion, when multiple requests were requested by script, the UI should merge them into 1 prompt. So instead of a new permission name "webrtc" for audio+video, we will send "audio-capture" and "video-capture" request respectively.

Flow:

MediaRequestObserver::Observe()
  -> Get request notfication.
  -> Create MediaPermissionMgr

MediaPermissionMgr::MediaPermissionMgr()
  -> Create audio/video requests

MediaPermissionMgr::SetState(CHECK_DEVICE)

MediaPermissionMgr::RunStateMachine::CHECK_DEVICE
  -> check if the device is available and add device to request.
  -> MediaPermissionMgr::SetState(PROMPT)

MediaPermissionMgr::RunStateMachine::PROMPT
  -> MediaPermissionRequest::DoPrompt(), send prompt ipdl to system app.

MediaPermissionRequest::Recv__delete__(const bool& allow)
  -> receive prompt dismiss ipdl and call Allow() or Cancel() depends on 'allow' 
     parameter
  -> call MediaPermissionMgr::RequestReply() to check if all requests are replied. 
     If yes, MediaPermissionMgr::SetState(CLOSE)

MediaPermissionMgr::RunStateMachine::CLOSE
  Call 'allow' notification if any request is granted. Otherwise, send 'deny'.

Comment 17

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

Comment on attachment 788815 [details] [diff] [review]
gUM, prompt request via ContentPermissionRequest PIDL

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

r+, but please verify that the UI has all the info it will need to merge the requests reliably

::: dom/media/MediaPermissionGonk.cpp
@@ +53,5 @@
> +{
> +  if (XRE_GetProcessType() == GeckoProcessType_Content) {
> +    nsCOMPtr<nsPIDOMWindow> window(nsGlobalWindow::GetOuterWindowWithId(aWindowID));
> +    if (!window) {
> +      NS_ENSURE_TRUE(window, NS_NOINTERFACE);

NS_ERROR_FAILURE

@@ +58,5 @@
> +    }
> +
> +    TabChild* child = GetTabChildFrom(window->GetDocShell());
> +    if (!child) {
> +      NS_ENSURE_TRUE(child, NS_NOINTERFACE);

NS_ERROR_FAILURE

@@ +64,5 @@
> +
> +    nsresult rv;
> +    nsCString type;
> +    rv = GetType(type);
> +    NS_ENSURE_TRUE(NS_SUCCEEDED(rv), NS_ERROR_UNEXPECTED);

NS_ENSURE_SUCCESS(rv, rv)
If these are really as impossible as I imagine they are, you may want to MOZ_ASSERT() on them instead (or do both)

@@ +68,5 @@
> +    NS_ENSURE_TRUE(NS_SUCCEEDED(rv), NS_ERROR_UNEXPECTED);
> +
> +    nsCString access;
> +    rv = GetAccess(access);
> +    NS_ENSURE_TRUE(NS_SUCCEEDED(rv), NS_ERROR_UNEXPECTED);

Ditto

@@ +75,5 @@
> +    // Corresponding release occurs in DeallocPContentPermissionRequest.
> +    AddRef();
> +    nsCOMPtr<nsIPrincipal> principal;
> +    rv = GetPrincipal(getter_AddRefs(principal));
> +    NS_ENSURE_TRUE(NS_SUCCEEDED(rv), NS_ERROR_UNEXPECTED);

Ditto

::: dom/media/MediaPermissionGonk.h
@@ +24,5 @@
> + * NOTE:
> + * If script requests an audio+video permission via a single gUM call, there
> + * will be 2 MediaPermissionRequest instances; one for audio, another one for video.
> + * It depends on UI/UX if these requests should be merged to one prompt or displays
> + * one by one.

I assume that there is data in the requests that can be used by the UI to merge these requests?  Are there timing issues in the UI -- to merge, it needs to know if another request is coming, or it needs to start with the first one and then revise to merge them.

@@ +50,5 @@
> +
> +protected:
> +  nsAutoString mType;
> +  bool mAllow;
> +  // True when this request is closed (it could be cancel or allow).

cancelled or allowed

@@ +143,5 @@
> +  nsRefPtr<MediaPermissionMgr> mMediaPermMgr;
> +};
> +
> +/**
> + * The observer to create the MediaPermissionMgr. This is the entry point of 

trailing space

Comment 18

[Alfredo Yang (:alfredo)]

(In reply to Randell Jesup [:jesup] from comment #17)
> 
> ::: dom/media/MediaPermissionGonk.h
> @@ +24,5 @@
> > + * NOTE:
> > + * If script requests an audio+video permission via a single gUM call, there
> > + * will be 2 MediaPermissionRequest instances; one for audio, another one for video.
> > + * It depends on UI/UX if these requests should be merged to one prompt or displays
> > + * one by one.
> 
> I assume that there is data in the requests that can be used by the UI to
> merge these requests?  Are there timing issues in the UI -- to merge, it
> needs to know if another request is coming, or it needs to start with the
> first one and then revise to merge them.

Yes, UI can get the app/url info from principal which can be retrieved via request->GetPrincipal(). There is another bug 898986 works with UI now.
On current system app implementation, there is an queue for permission prompt requests, and they will display prompt one by one popuping from the queue. IMO, they will need a function to scan the queue and merge the requests to currnet displaying prompt when:
1. Push a new gUM request to queue.
2. Popup a request from the queue to prompt.
However, the real implementation depends on Gaia.

Comment 19

[Alfredo Yang (:alfredo)]

Attachment: gUM, prompt request via ContentPermissionRequest PIDL

1. Update patch according to review comments.
2. Remove the camera permission check in MediaManager.cpp. This permission check should be for navigator.mozCamera, not for gUM. Since we are adding permission request now, it should be removed.

 #ifdef MOZ_B2G_CAMERA
   if (mCameraManager == nullptr) {
-    aPrivileged = nsDOMCameraManager::CheckPermission(aWindow);
-    if (aPrivileged) {
-      mCameraManager = nsDOMCameraManager::CreateInstance(aWindow);
-    }
+    mCameraManager = nsDOMCameraManager::CreateInstance(aWindow);
   }
 #endif

Comment 20

[Alfredo Yang (:alfredo)]

After discussing with :fabrice and :alive, we will do it all (pop 1 prompt for audio, video, or audio+video) on 1.2.
This bug is not required and back to bug 853356.