Closed Bug 846057 Opened 7 years ago Closed 7 years ago

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

Categories

(Testing :: Mochitest, defect)

x86
macOS
defect
Not set

Tracking

(firefox20 wontfix, firefox21 wontfix, firefox22 fixed, b2g18 fixed, b2g18-v1.0.0 wontfix, b2g18-v1.0.1 wontfix)

RESOLVED FIXED
mozilla22
Tracking Status
firefox20 --- wontfix
firefox21 --- wontfix
firefox22 --- fixed
b2g18 --- fixed
b2g18-v1.0.0 --- wontfix
b2g18-v1.0.1 --- wontfix

People

(Reporter: onecyrenus, Assigned: onecyrenus)

References

Details

Attachments

(1 file, 1 obsolete file)

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.
Blocks: 843893
Generally need some feedback on if this approach seems the correct one.
Flags: needinfo?(jgriffin)
This sounds like you are describing a lot of the functionality provided in the navigator.mozPermissionSettings API.
(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 dclarke@mozilla.com  [: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.
Flags: needinfo?(jgriffin)
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.
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")
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)
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.
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");
Attachment #719978 - Flags: review?(jgriffin)
Attachment #719978 - Attachment is patch: true
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
Attachment #719978 - Flags: review?(jgriffin) → review+
Updated patch to include comments, should be ready for checkin.
Attachment #719978 - Attachment is obsolete: true
Comment on attachment 720825 [details] [diff] [review]
Updated: added testPermission / hasPermission api's

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

Thanks!
Attachment #720825 - Flags: review+
This needs to be nommed so we can resolve the dependency 843893.
blocking-b2g: --- → tef?
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
Assignee: nobody → dclarke
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/22288118672b
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
Ryan, got it.
This has to get checked into the v1-train, as i believe b2g tests run on the v1-train, and not the trunk .
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:
Attachment #720825 - Flags: approval-mozilla-b2g18?
Waiting on review of bug 843893 prior to considering this bug for approval.
(clearing tef? to get this out of the nom list until the review of bug 843893 happens)
blocking-b2g: tef? → ---
Attachment #720825 - Flags: approval-mozilla-b2g18? → approval-mozilla-b2g18+
You need to log in before you can comment on or make changes to this bug.