superreview requested: [Bug 874196] Add nsIPermissionManager.getPermission : [Attachment 752181] Add nsIPermissionManager.getPermission, rev. 1

[bugzill...@mozilla.org]

Benjamin Smedberg [:bsmedberg] <benj...@smedbergs.us> has asked Mounir
Lamouri (:mounir) <mou...@lamouri.fr> for superreview:
Bug 874196: Add nsIPermissionManager.getPermission
https://bugzilla.mozilla.org/show_bug.cgi?id=874196

Attachment 752181: Add nsIPermissionManager.getPermission, rev. 1
https://bugzilla.mozilla.org/attachment.cgi?id=752181&action=edit


------- Additional Comments from Benjamin Smedberg [:bsmedberg]
<benj...@smedbergs.us>
Passes tryserver: https://tbpl.mozilla.org/?tree=Try&rev=b17a021dec36

[bugzill...@mozilla.org]

Mounir Lamouri (:mounir) <mou...@lamouri.fr> has not granted Benjamin Smedberg
[:bsmedberg] <benj...@smedbergs.us>'s request for superreview:

Bug 874196: Add nsIPermissionManager.getPermission
https://bugzilla.mozilla.org/show_bug.cgi?id=874196

Attachment 752181: Add nsIPermissionManager.getPermission, rev. 1
https://bugzilla.mozilla.org/attachment.cgi?id=752181&action=edit

------- Additional Comments from Mounir Lamouri (:mounir) <mou...@lamouri.fr>
Review of attachment 752181:
-----------------------------------------------------------------

The patch looks good but I would like a bit more testing. Could you create a
new test file (test_permmanager_getpermission.js) and do a bit more intensive
tests there?

::: extensions/cookie/nsPermission.cpp
@@ +8,4 @@
>
> // nsPermission Implementation
>
> +NS_IMPL_CLASSINFO(nsPermission, NULL, 0, {0})

Shouldn't that be nullptr?

::: extensions/cookie/nsPermissionManager.cpp
@@ +1055,5 @@
> + if (!entry) {
> + return NS_OK;
> + }
> +
> + int32_t idx = entry->GetPermissionIndex(typeIndex);

Could you write a comment explaining that you do that because
GetPermission(typeIndex) will return a fake UNKNOWN_ACTION permission if there
is no permission record for the given type.

@@ +1063,5 @@
> +
> + PermissionEntry& perm = entry->GetPermissions()[idx];
> + nsCOMPtr<nsIPermission> r = new nsPermission(entry->GetKey()->mHost,
> + entry->GetKey()->mAppId,
> +
entry->GetKey()->mIsInBrowserElement,

I think you could as well use host, appId and isInBrowserElement, no need to
use the Entry who anyway will have the same value but will require a bit more
work to get them.

::: netwerk/base/public/nsIPermissionManager.idl
@@ +188,5 @@
> in string type);
>
> + /**
> + * Get the permission object associated with the given URI and action.
> + * @param uri The principal

In those two lines, you speak about URI ("given URI" and "@param uri") but you
want "principal".

@@ +193,5 @@
> + * @param type A case-sensitive ASCII string identifying the consumer

> + * @param exactHost If true, only the specific host will be matched,
> + * @see testExactPermission. If false, subdomains will
> + * also be searched, @see testPermission.
> + * @returns The matching permission object, or null if not found.

I will try to be more verbose, whether there or in a @note explaining that it
returns a permission object iif the permission object has an entry for the
given parameters. If there is no returned object, that doesn't mean the
permission is denied but that there is nothing registered which could be as
well authorisation (eg. chrome code).

@@ +195,5 @@
> + * @see testExactPermission. If false, subdomains will
> + * also be searched, @see testPermission.
> + * @returns The matching permission object, or null if not found.
> + */
> + nsIPermission getPermission(in nsIPrincipal principal,

I wonder if "getPermissionObject()" wouldn't be a better name to make sure
consumers don't end up caling this to check for permissions. You can keep the
current name if you prefer it.

@@ +198,5 @@
> + */
> + nsIPermission getPermission(in nsIPrincipal principal,
> + in string type,
> + in boolean exactHost);
> +

nit: trailing ws

[bugzill...@mozilla.org]

Benjamin Smedberg [:bsmedberg] <benj...@smedbergs.us> has asked Mounir
Lamouri (:mounir) <mou...@lamouri.fr> for superreview:

Bug 874196: Add nsIPermissionManager.getPermission
https://bugzilla.mozilla.org/show_bug.cgi?id=874196

Attachment 756172: Add an API to get the specifics of a permission given a
host/type: this will allow the plugin click-to-activate UI to manage
permissions by the matching host and determine whether the current permission
is per-session or persistent. r?jdm sr?mounir
https://bugzilla.mozilla.org/attachment.cgi?id=756172&action=edit

[bugzill...@mozilla.org]

Mounir Lamouri (:mounir) <mou...@lamouri.fr> has granted Benjamin Smedberg
[:bsmedberg] <benj...@smedbergs.us>'s request for superreview:

Bug 874196: Add nsIPermissionManager.getPermission
https://bugzilla.mozilla.org/show_bug.cgi?id=874196

Attachment 756172: Add an API to get the specifics of a permission given a
host/type: this will allow the plugin click-to-activate UI to manage
permissions by the matching host and determine whether the current permission
is per-session or persistent. r?jdm sr?mounir
https://bugzilla.mozilla.org/attachment.cgi?id=756172&action=edit

------- Additional Comments from Mounir Lamouri (:mounir) <mou...@lamouri.fr>
Review of attachment 756172:
-----------------------------------------------------------------

Thanks :)