Closed Bug 915705 Opened 6 years ago Closed 6 years ago

[B2G getUserMedia] Display microphone permission acquisition prompt

Categories

(Core :: WebRTC, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla26
blocking-b2g koi+
Tracking Status
firefox26 --- fixed

People

(Reporter: schien, Assigned: schien)

References

Details

(Keywords: dev-doc-complete, Whiteboard: [b2g-gum+])

Attachments

(2 files, 3 obsolete files)

For Media Recording 1.2 feature, we'll need add permission for microphone only.
Add audio-capture into permission table and prompt list.
Attachment #803754 - Flags: review?(fabrice)
Blocks: 894848
No longer blocks: koi-media-recording
Attachment #803754 - Flags: review?(fabrice) → review+
Summary: [B2G getUserMedia] Display camera/ microphone permission acquisition prompt → [B2G getUserMedia] Display microphone permission acquisition prompt
Handle |getUserMedia:request| on FirefoxOS.
Attachment #804069 - Flags: review?(rjesup)
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)
Attachment #804069 - Flags: review?(rjesup) → review+
(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.
Update commit comment, carry r+
Attachment #803754 - Attachment is obsolete: true
Attachment #804389 - Flags: review+
Update according to review comments, carry r+.
Attachment #804069 - Attachment is obsolete: true
Attachment #804390 - Flags: review+
rebase to latest m-c, carry r+
Attachment #804390 - Attachment is obsolete: true
Attachment #804826 - Flags: review+
Ready to check-in!
Keywords: checkin-needed
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
relanded since bug 882145 relanded
https://hg.mozilla.org/mozilla-central/rev/078d8f290879
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
Keywords: verifyme
QA Contact: jsmith
Verified through initial test pass of gUM. See dependencies on bug 894848 for potential followups.
Status: RESOLVED → VERIFIED
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.