846057 - SpecialPowers api needs to be able to check Permissions as well as add and remove

Status: RESOLVED FIXED

(Testing :: Mochitest, defect)

Description

[[e-mail] [:onecyrenus]]

The special powers api needs the ability to check Permissions as well as add and remove permissions. 

The below is *one* proposal for how to give the tester this ability. 

SpecialPowers.hasPermission("alarms", document);

which returns true or false. 

SpecialPowers.getPermission("alarms", document);

which returns the enumerated value from the permission Manager which is up to 
the test taker to compare against. 

  enum {
    UNKNOWN_ACTION = 0U,
    ALLOW_ACTION = 1U,
    DENY_ACTION = 2U,
    PROMPT_ACTION = 3U,
  };

The 95% usage case I'd imagine is the first instance, for people who want finer grained control they would want to use the second api.

Comment 1

[[e-mail] [:onecyrenus]]

Generally need some feedback on if this approach seems the correct one.

Comment 2

[Jason Smith [:jsmith]]

This sounds like you are describing a lot of the functionality provided in the navigator.mozPermissionSettings API.

Comment 3

[Jonathan Griffin (:jgriffin)]

(In reply to Jason Smith [:jsmith] from comment #2)
> This sounds like you are describing a lot of the functionality provided in
> the navigator.mozPermissionSettings API.

Both SpecialPowers.xxxPermission and mozPermissionSettings use the same API (nsIPermissionManager), but the former works without the "permissions" permissions.

(In reply to [e-mail]  [:onecyrenus] from comment #1)
> Generally need some feedback on if this approach seems the correct one.

Yes, I think that's the right approach.  A lot of SpecialPowers calls are direct copies of their API's, so we might want to use SpecialPowers.testPermission, ala https://developer.mozilla.org/en-US/docs/XPCOM_Interface_Reference/nsIPermissionManager#testPermission%28%29.

Comment 4

[[e-mail] [:onecyrenus]]

Yes I saw that, i was just thinking semantically 

SpecialPowers.hasPermission("alarms",document) makes a lot more sense than
SpecialPowers.testPermission("alarms", document)... 

The former seems more obvious to return a boolean true / false..

SpecialPowers.testPermission("alarms", PROMPT_ACTION, document) <-- this makes more sense to me than the former.

Comment 5

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

If I read the interface reference correctly, the API testPermission returns the permission value rather than a boolean. 

I wouldn't have two functions with the same name and purpose with different signatures--that would be confusing. So if we're going to align, seems like it'd be more like:

PROMPT_ACTION == SpecialPowers.testPermission(document, "alarms")

Comment 6

[[e-mail] [:onecyrenus]]

I think there is a bit of confusion there what i'm saying is 
PERMISSION_STATUS = SpecialPowers.testPermission(document, "alarms")

requires you to have a list of permission status'es stored somewhere in order to do the equality. 

The 90% case is that you just want to check if you have a permission or not. 

I was merely pointing out that an api with the name test_Permission means you are testing against a known value, whereas get_Permission would be easier to understand when reading the api. 

test_Permission and passing in the PROMPT_ACTION is also acceptable, but having a function called test_Permission which really returns an encoded value seems semantically incorrect...


NS_IMETHODIMP
nsPermissionManager::TestExactPermission(nsIURI     *aURI,
                                         const char *aType,
                                         uint32_t   *aPermission)

Comment 7

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

This makes sense, and I'm concerned about (convenient) access to the permissions constants as well.

More saying don't align the names unless the functions work the same--basic code comprehension thing. I'd go with hasPermissions for the simple boolean one precisely because the name is different.

Comment 8

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

er, hasPermission() that is.

Comment 9

[[e-mail] [:onecyrenus]]

Attachment: added testPermission / hasPermission api's

Example api usage: 

ok(SpecialPowers.hasPermission("alarms", document), "yeahh we have permission");

ok(SpecialPowers.testPermission("alarms", SpecialPowers.Ci.nsIPermissionManager.ALLOW_ACTION, document),"Allow Action should return true");

Comment 10

[Jonathan Griffin (:jgriffin)]

Comment on attachment 719978 [details] [diff] [review]
added testPermission / hasPermission api's

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

r+ with these changes.

::: testing/specialpowers/content/SpecialPowersObserverAPI.js
@@ +228,4 @@
>  
>          let secMan = Services.scriptSecurityManager;
>          let principal = secMan.getAppCodebasePrincipal(this._getURI(msg.url), msg.appId, msg.isInBrowserElement);
> +        let perm = null;

It's a little odd to declare a variable here just so we can use it in two different spots in the switch below.  I think it would be better to omit this and just use two differently-named vars in the switch, like hasPerm and testPerm.

::: testing/specialpowers/content/specialpowersAPI.js
@@ +1268,5 @@
> +  hasPermission: function (type, arg) {
> +   let [url, appId, isInBrowserElement] = this._getInfoFromPermissionArg(arg);
> +
> +    var msg = {
> +      'op': "has",

let's using single quotes for consistency

@@ +1276,5 @@
> +      'isInBrowserElement': isInBrowserElement
> +    };
> +
> +    return this._sendSyncMessage('SPPermissionManager', msg)[0];
> +   

nit:  extra spaces here

@@ +1282,5 @@
> +  testPermission: function (type, value, arg) {
> +   let [url, appId, isInBrowserElement] = this._getInfoFromPermissionArg(arg);
> +
> +    var msg = {
> +      'op': "test",

let's use single quotes for consistency

Comment 11

[[e-mail] [:onecyrenus]]

Attachment: Updated: added testPermission / hasPermission api's

Updated patch to include comments, should be ready for checkin.

Comment 12

[Jonathan Griffin (:jgriffin)]

Comment on attachment 720825 [details] [diff] [review]
Updated: added testPermission / hasPermission api's

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

Thanks!

Comment 13

[[e-mail] [:onecyrenus]]

This needs to be nommed so we can resolve the dependency 843893.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/22288118672b

David, can you please make sure you have hg configured so that your patches contain all the necessary checkin metadata by default? Thanks!
https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/22288118672b

Comment 16

[[e-mail] [:onecyrenus]]

Ryan, got it.

Comment 17

[[e-mail] [:onecyrenus]]

This has to get checked into the v1-train, as i believe b2g tests run on the v1-train, and not the trunk .

Comment 18

[[e-mail] [:onecyrenus]]

Comment on attachment 720825 [details] [diff] [review]
Updated: added testPermission / hasPermission api's

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 843893
User impact if declined: mochitest run cannot complete successfully
Testing completed: tested running mochitests
Risk to taking this patch (and alternatives if risky): 0
String or UUID changes made by this patch:

Comment 19

[Alex Keybl [:akeybl]]

Waiting on review of bug 843893 prior to considering this bug for approval.

Comment 20

[Michael Vines [:m1] [:evilmachines]]

(clearing tef? to get this out of the nom list until the review of bug 843893 happens)

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/bcba115a7cb0