915705 - [B2G getUserMedia] Display microphone permission acquisition prompt

Status: VERIFIED FIXED

(Core :: WebRTC, defect)

Description

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

For Media Recording 1.2 feature, we'll need add permission for microphone only.

Comment 1

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: create-audio-capture-permission.patch

Add audio-capture into permission table and prompt list.

Comment 2

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: Part 2 - add handler for gUM request on FirefoxOS

Handle |getUserMedia:request| on FirefoxOS.

Comment 3

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

Comment on attachment 804069 [details] [diff] [review]
Part 2 - add handler for gUM request on FirefoxOS

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

All of these I think can be resolved without a re-review pass; please clean up as indicated.

::: dom/media/MediaPermissionGonk.cpp
@@ +25,5 @@
> +namespace mozilla {
> +
> +#ifdef PR_LOGGING
> +extern PRLogModuleInfo* GetMediaManagerLog();
> +#define MP_LOG(msg) PR_LOG(GetMediaManagerLog(), PR_LOG_DEBUG, msg)

Minor preference to keep with standard practice to make a #define like this be named "LOG()" unless there's a reason not to

@@ +43,5 @@
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  for (uint32_t i = 0; i < aDevices.Length(); ++i) {
> +    rv = array->AppendElement(aDevices.ElementAt(i));
> +    NS_ENSURE_SUCCESS(rv, rv);

SupportArray->AppendElement returns rv, but cannot actually fail since we added infallible allocators (GrowArrayBy() has no return) (and most if not all other code in gecko makes this assumption as well).

@@ +130,5 @@
> +}
> +
> +MediaPermissionRequest::~MediaPermissionRequest()
> +{
> +  MP_LOG(("~MediaPermissionRequest()"));

MP_LOG((__FUNCTION__));
Just an easier bit that can be pasted where needed, and avoids cut-and-paste errors.

@@ +162,5 @@
> +    aType = VIDEO_PERMISSION_NAME;
> +    return NS_OK;
> +  }
> +
> +  return NS_OK;

If this truly can only be one of mAudio or mVideo:

if (mAudio) {
  aType = ...;
} else {
  MOZ_ASSERT(mVideo);
  aType = ...;
}
return NS_OK;

Or get rid of mVideo entirely, and have bool mIsAudio and aType = mIsAudio ? xxx : xxx;

@@ +253,5 @@
> +  void* rawArray;
> +  uint32_t arrayLen;
> +
> +  nsresult rv;
> +  rv = devices->GetAsArray(&elementType, &elementIID, &arrayLen, &rawArray);

Isn't there a better way than getting raw array pointers, and using NS_Free(), NS_IF_RELEASE, etc?  Perhaps not after looking around.  Oh well.

@@ +266,5 @@
> +      nsCOMPtr<nsIMediaDevice> device(do_QueryInterface(supportsArray[i]));
> +      devices.AppendElement(device);
> +
> +      NS_IF_RELEASE(supportsArray[i]); // explicitly decrease reference count for raw pointer
> +      if (NS_FAILED(rv)) {

How could this fail?  We had NS_ENSURE_SUCCESS(rv,rv) up above and haven't touched it.  Remove the failure case

@@ +278,5 @@
> +    nsRefPtr<MediaPermissionRequest> req = new MediaPermissionRequest(mRequest, devices);
> +    rv = DoPrompt(req);
> +    NS_ENSURE_SUCCESS(rv, rv);
> +  }
> +  return NS_OK;

Move NS_ENSURE_SUCCESS to where "return NS_OK" is.   (Minor efficiency thing; not important if you think it's easier to understand like this)

@@ +292,5 @@
> +
> +    nsresult rv;
> +
> +    nsCOMPtr<nsPIDOMWindow> window(req->GetOwner());
> +    dom::TabChild* child = dom::GetTabChildFrom(window->GetDocShell());

null-check window before using it

@@ +300,5 @@
> +    req->GetType(type);
> +
> +    nsAutoCString access;
> +    rv = req->GetAccess(access);
> +    NS_ENSURE_SUCCESS(rv, rv);

GetAccess really can't fail - but if it needs to be checked, so should GetType, so either check both or neither.

@@ +380,5 @@
> +  nsCOMPtr<nsIObserverService> obs = services::GetObserverService();
> +  if (obs) {
> +    obs->AddObserver(this, "getUserMedia:request", false);
> +  }
> +  return NS_OK;

Init() can't fail, and is only called from the constructor.  move the code into the constructor.  (Init() calls are really for running fallible code)

Comment 4

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

(In reply to Randell Jesup [:jesup] from comment #3)
> Comment on attachment 804069 [details] [diff] [review]
> Part 2 - add handler for gUM request on FirefoxOS
> 
> Review of attachment 804069 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> All of these I think can be resolved without a re-review pass; please clean
> up as indicated.
> 
> ::: dom/media/MediaPermissionGonk.cpp
> @@ +25,5 @@
> > +namespace mozilla {
> > +
> > +#ifdef PR_LOGGING
> > +extern PRLogModuleInfo* GetMediaManagerLog();
> > +#define MP_LOG(msg) PR_LOG(GetMediaManagerLog(), PR_LOG_DEBUG, msg)
> 
> Minor preference to keep with standard practice to make a #define like this
> be named "LOG()" unless there's a reason not to
Simply remove it because no place except for the dtor is using MP_LOG().
> 
> @@ +43,5 @@
> > +  NS_ENSURE_SUCCESS(rv, rv);
> > +
> > +  for (uint32_t i = 0; i < aDevices.Length(); ++i) {
> > +    rv = array->AppendElement(aDevices.ElementAt(i));
> > +    NS_ENSURE_SUCCESS(rv, rv);
> 
> SupportArray->AppendElement returns rv, but cannot actually fail since we
> added infallible allocators (GrowArrayBy() has no return) (and most if not
> all other code in gecko makes this assumption as well).
Remove the dummy NS_ENSURE_SUCCESS().
> 
> @@ +130,5 @@
> > +}
> > +
> > +MediaPermissionRequest::~MediaPermissionRequest()
> > +{
> > +  MP_LOG(("~MediaPermissionRequest()"));
> 
> MP_LOG((__FUNCTION__));
> Just an easier bit that can be pasted where needed, and avoids cut-and-paste
> errors.
Removed.
> 
> @@ +162,5 @@
> > +    aType = VIDEO_PERMISSION_NAME;
> > +    return NS_OK;
> > +  }
> > +
> > +  return NS_OK;
> 
> If this truly can only be one of mAudio or mVideo:
> 
> if (mAudio) {
>   aType = ...;
> } else {
>   MOZ_ASSERT(mVideo);
>   aType = ...;
> }
> return NS_OK;
> 
> Or get rid of mVideo entirely, and have bool mIsAudio and aType = mIsAudio ?
> xxx : xxx;
I'll remove mVideo since this patch is only for audio-capture.
> 
> @@ +253,5 @@
> > +  void* rawArray;
> > +  uint32_t arrayLen;
> > +
> > +  nsresult rv;
> > +  rv = devices->GetAsArray(&elementType, &elementIID, &arrayLen, &rawArray);
> 
> Isn't there a better way than getting raw array pointers, and using
> NS_Free(), NS_IF_RELEASE, etc?  Perhaps not after looking around.  Oh well.
I didn't see any other way to do so in current code base, so I'll keep this.
> 
> @@ +266,5 @@
> > +      nsCOMPtr<nsIMediaDevice> device(do_QueryInterface(supportsArray[i]));
> > +      devices.AppendElement(device);
> > +
> > +      NS_IF_RELEASE(supportsArray[i]); // explicitly decrease reference count for raw pointer
> > +      if (NS_FAILED(rv)) {
> 
> How could this fail?  We had NS_ENSURE_SUCCESS(rv,rv) up above and haven't
> touched it.  Remove the failure case
Original code is tend to do |rv = devices.AppendElement|. I'll remove the failure case since nsTArray::AppendElement() will never fail.
> 
> @@ +278,5 @@
> > +    nsRefPtr<MediaPermissionRequest> req = new MediaPermissionRequest(mRequest, devices);
> > +    rv = DoPrompt(req);
> > +    NS_ENSURE_SUCCESS(rv, rv);
> > +  }
> > +  return NS_OK;
> 
> Move NS_ENSURE_SUCCESS to where "return NS_OK" is.   (Minor efficiency
> thing; not important if you think it's easier to understand like this)
Done.
> 
> @@ +292,5 @@
> > +
> > +    nsresult rv;
> > +
> > +    nsCOMPtr<nsPIDOMWindow> window(req->GetOwner());
> > +    dom::TabChild* child = dom::GetTabChildFrom(window->GetDocShell());
> 
> null-check window before using it
Done.
> 
> @@ +300,5 @@
> > +    req->GetType(type);
> > +
> > +    nsAutoCString access;
> > +    rv = req->GetAccess(access);
> > +    NS_ENSURE_SUCCESS(rv, rv);
> 
> GetAccess really can't fail - but if it needs to be checked, so should
> GetType, so either check both or neither.
Add check for GetType().
> 
> @@ +380,5 @@
> > +  nsCOMPtr<nsIObserverService> obs = services::GetObserverService();
> > +  if (obs) {
> > +    obs->AddObserver(this, "getUserMedia:request", false);
> > +  }
> > +  return NS_OK;
> 
> Init() can't fail, and is only called from the constructor.  move the code
> into the constructor.  (Init() calls are really for running fallible code)
Done.

Comment 5

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: Part 1, add audio-capture permission

Update commit comment, carry r+

Comment 6

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: Part 2 - add handler for gUM request on FirefoxOS

Update according to review comments, carry r+.

Comment 7

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

https://tbpl.mozilla.org/?tree=Try&rev=8d4f5d0cc501

Comment 8

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: Part 2 - add handler for gUM request on FirefoxOS

rebase to latest m-c, carry r+

Comment 9

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

https://tbpl.mozilla.org/?tree=Try&rev=2ebb005264b9

Comment 10

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Ready to check-in!

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5bdec1cb1428
https://hg.mozilla.org/integration/mozilla-inbound/rev/f6402ce2b67a

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Backed out because this depends on bug 882145 which was backed out at jsmith's request.
https://hg.mozilla.org/integration/mozilla-inbound/rev/4d39ed6fb69c

Comment 13

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

relanded since bug 882145 relanded
https://hg.mozilla.org/mozilla-central/rev/078d8f290879

Comment 14

[Jason Smith [:jsmith]]

Verified through initial test pass of gUM. See dependencies on bug 894848 for potential followups.

Comment 15

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

MDN is updated.
https://developer.mozilla.org/en-US/Apps/Developing/App_permissions