982777 - Prompt for permission to use the Camera API in privileged apps

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Reuben Morais [:reuben]]

Attachment: WIP Prompt for camera access

Comment 1

[Reuben Morais [:reuben]]

Attachment: WIP Prompt for camera access v2

Works with Fake camera backend. Still need to see what happens in the device.

Comment 2

[Reuben Morais [:reuben]]

Attachment: Fake camera backend

Uploading for archival, this is useful for testing things in B2G desktop.

Comment 3

[Reuben Morais [:reuben]]

Attachment: Gaia PR: Add strings for Camera permission

Comment 4

[Reuben Morais [:reuben]]

Attachment: Prompt for camera access v3

Comment 5

[(no longer active)]

Reuben, which b2g/Gecko version is this targeted at?

Comment 6

[Reuben Morais [:reuben]]

Jonas said he wanted to land this before the branch, so I guess 1.4? Is that right Jonas?

Comment 7

[Mike Habicher [:mikeh] (high bugzilla latency)]

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

Comment 8

[Reuben Morais [:reuben]]

Comment on attachment 8390098 [details] [diff] [review]
Prompt for camera access v3

Thanks Mike.

Comment 9

[Reuben Morais [:reuben]]

Attachment: Prompt for camera access v4

Addressed Mike's comment and changed some nsCOMPtrs that should have been nsRefPtrs.

Comment 10

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 11

[Reuben Morais [:reuben]]

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.

Comment 12

[Fabien Cazenave [:kaze]]

Comment on attachment 8390089 [details] [review]
Gaia PR: Add strings for Camera permission

LGTM

Comment 13

[Josh Matthews [:jdm]]

Comment on attachment 8390314 [details] [diff] [review]
Prompt for camera access v4

Yoink.

Comment 14

[Josh Matthews [:jdm]]

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?

Comment 15

[Reuben Morais [:reuben]]

(In reply to Josh Matthews [:jdm] from comment #14)
> What does this do if the error callback throws an exception?

Nothing, it's ignored.

Comment 16

[Reuben Morais [:reuben]]

https://hg.mozilla.org/integration/b2g-inbound/rev/ab0b0b08b120

Comment 17

[Josh Matthews [:jdm]]

(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.

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ab0b0b08b120

Comment 19

[Reuben Morais [:reuben]]

(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?

Comment 20

[Josh Matthews [:jdm]]

We want to ensure that the error gets reported to the console/debugger/whatever; silently dropping it on the floor is a bad practice.

Comment 21

[Reuben Morais [:reuben]]

FWIW, we found out on IRC that the error actually is reported, we don't have to do anything with the ErrorResult.

Comment 22

[Paul Theriault [:pauljt] (no longer reading bugmail)]

Just noticed this - whats the target release here - is it 1.4 as it says in the milestone above?