Closed
Bug 915705
Opened 11 years ago
Closed 11 years ago
[B2G getUserMedia] Display microphone permission acquisition prompt
Categories
(Core :: WebRTC, defect)
Tracking
()
Tracking | Status | |
---|---|---|
firefox26 | --- | fixed |
People
(Reporter: schien, Assigned: schien)
References
Details
(Keywords: dev-doc-complete, Whiteboard: [b2g-gum+])
Attachments
(2 files, 3 obsolete files)
2.25 KB,
patch
|
schien
:
review+
|
Details | Diff | Splinter Review |
16.20 KB,
patch
|
schien
:
review+
|
Details | Diff | Splinter Review |
For Media Recording 1.2 feature, we'll need add permission for microphone only.
Updated•11 years ago
|
Keywords: dev-doc-needed
Assignee | ||
Comment 1•11 years ago
|
||
Add audio-capture into permission table and prompt list.
Attachment #803754 -
Flags: review?(fabrice)
Updated•11 years ago
|
Updated•11 years ago
|
Attachment #803754 -
Flags: review?(fabrice) → review+
Assignee | ||
Updated•11 years ago
|
Summary: [B2G getUserMedia] Display camera/ microphone permission acquisition prompt → [B2G getUserMedia] Display microphone permission acquisition prompt
Assignee | ||
Comment 2•11 years ago
|
||
Handle |getUserMedia:request| on FirefoxOS.
Attachment #804069 -
Flags: review?(rjesup)
Comment 3•11 years ago
|
||
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+
Assignee | ||
Comment 4•11 years ago
|
||
(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.
Assignee | ||
Comment 5•11 years ago
|
||
Update commit comment, carry r+
Attachment #803754 -
Attachment is obsolete: true
Attachment #804389 -
Flags: review+
Assignee | ||
Comment 6•11 years ago
|
||
Update according to review comments, carry r+.
Attachment #804069 -
Attachment is obsolete: true
Attachment #804390 -
Flags: review+
Assignee | ||
Comment 7•11 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=8d4f5d0cc501
Depends on: 882145
Assignee | ||
Comment 8•11 years ago
|
||
rebase to latest m-c, carry r+
Attachment #804390 -
Attachment is obsolete: true
Attachment #804826 -
Flags: review+
Assignee | ||
Comment 9•11 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=2ebb005264b9
Comment 11•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/5bdec1cb1428 https://hg.mozilla.org/integration/mozilla-inbound/rev/f6402ce2b67a
Keywords: checkin-needed
Comment 12•11 years ago
|
||
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•11 years ago
|
||
relanded since bug 882145 relanded https://hg.mozilla.org/mozilla-central/rev/078d8f290879
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
Updated•11 years ago
|
status-firefox26:
--- → fixed
Comment 14•11 years ago
|
||
Verified through initial test pass of gUM. See dependencies on bug 894848 for potential followups.
Status: RESOLVED → VERIFIED
Keywords: verifyme
Updated•11 years ago
|
Blocks: b2g-getusermedia
Assignee | ||
Comment 15•11 years ago
|
||
MDN is updated. https://developer.mozilla.org/en-US/Apps/Developing/App_permissions
Keywords: dev-doc-needed
Updated•11 years ago
|
Keywords: dev-doc-complete
You need to log in
before you can comment on or make changes to this bug.
Description
•