Closed Bug 982777 Opened 7 years ago Closed 7 years ago

Prompt for permission to use the Camera API in privileged apps

Categories

(Core :: DOM: Device Interfaces, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
1.4 S5 (11apr)

People

(Reporter: reuben, Assigned: reuben)

References

Details

(Whiteboard: [systemsfe])

Attachments

(3 files, 3 obsolete files)

Attached patch WIP Prompt for camera access (obsolete) — Splinter Review
No description provided.
Attached patch WIP Prompt for camera access v2 (obsolete) — Splinter Review
Works with Fake camera backend. Still need to see what happens in the device.
Attachment #8389980 - Attachment is obsolete: true
Uploading for archival, this is useful for testing things in B2G desktop.
Attachment #8390089 - Flags: review?(kaze)
Attached patch Prompt for camera access v3 (obsolete) — Splinter Review
Attachment #8390051 - Attachment is obsolete: true
Attachment #8390098 - Flags: review?(mhabicher)
Reuben, which b2g/Gecko version is this targeted at?
Jonas said he wanted to land this before the branch, so I guess 1.4? Is that right Jonas?
Comment on attachment 8390098 [details] [diff] [review]
Prompt for camera access v3

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

Overall this looks fine to me, but you should really ask someone who knows the permissions code to look it over.

::: dom/camera/DOMCameraManager.cpp
@@ +112,5 @@
>  }
>  
> +class CameraPermissionRequest : public nsIContentPermissionRequest,
> +                                public PCOMContentPermissionRequestChild,
> +                                public nsIRunnable

nit: the style in this file is to put the comma before the base class declaration:

class CameraPermissionRequest : public nsIContentPermissionRequest
                              , public PCOMContentPermissionRequestChild
                              , public nsIRunnable
Attachment #8390098 - Flags: review?(mhabicher)
Comment on attachment 8390098 [details] [diff] [review]
Prompt for camera access v3

Thanks Mike.
Attachment #8390098 - Flags: review?(jonas)
Addressed Mike's comment and changed some nsCOMPtrs that should have been nsRefPtrs.
Attachment #8390098 - Attachment is obsolete: true
Attachment #8390098 - Flags: review?(jonas)
Attachment #8390314 - Flags: review?(jonas)
Comment on attachment 8390314 [details] [diff] [review]
Prompt for camera access v4

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

This looks fine to me. Though it would be good to have someone that knows the nsIContentPermissionRequest API better than me go over that part.
Attachment #8390314 - Flags: review?(jonas) → review+
Comment on attachment 8390314 [details] [diff] [review]
Prompt for camera access v4

Code using this API hasn't been reviewed by a consistent set of people, but Doug wrote it originally.
Attachment #8390314 - Flags: review?(dougt)
Whiteboard: [systemsfe]
Target Milestone: --- → 1.4 S4 (28mar)
Comment on attachment 8390089 [details] [review]
Gaia PR: Add strings for Camera permission

LGTM
Attachment #8390089 - Flags: review?(kaze) → review+
Comment on attachment 8390314 [details] [diff] [review]
Prompt for camera access v4

Yoink.
Attachment #8390314 - Flags: review?(dougt) → review?(josh)
Target Milestone: 1.4 S4 (28mar) → 1.4 S5 (11apr)
Comment on attachment 8390314 [details] [diff] [review]
Prompt for camera access v4

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

Nice patch.

::: dom/camera/DOMCameraManager.cpp
@@ +354,5 @@
> +  mPermission = nsIPermissionManager::DENY_ACTION;
> +
> +  if (aOnError) {
> +    ErrorResult ignored;
> +    aOnError->Call(NS_LITERAL_STRING("Permission denied."), ignored);

What does this do if the error callback throws an exception?
Attachment #8390314 - Flags: review?(josh) → review+
(In reply to Josh Matthews [:jdm] from comment #14)
> What does this do if the error callback throws an exception?

Nothing, it's ignored.
(In reply to Reuben Morais [:reuben] from comment #15)
> (In reply to Josh Matthews [:jdm] from comment #14)
> > What does this do if the error callback throws an exception?
> 
> Nothing, it's ignored.

Ok, but that doesn't sound like behaviour we actually want.
https://hg.mozilla.org/mozilla-central/rev/ab0b0b08b120
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
(In reply to Josh Matthews [:jdm] from comment #17)
> Ok, but that doesn't sound like behaviour we actually want.

What should we do? Report the exception to the caller script?
We want to ensure that the error gets reported to the console/debugger/whatever; silently dropping it on the floor is a bad practice.
FWIW, we found out on IRC that the error actually is reported, we don't have to do anything with the ErrorResult.
Just noticed this - whats the target release here - is it 1.4 as it says in the milestone above?
Flags: sec-review?(ptheriault)
Flags: sec-review?(ptheriault)
You need to log in before you can comment on or make changes to this bug.