Closed Bug 1170200 Opened 5 years ago Closed 4 years ago

Change the API for nsIPermissionManager::Remove() to accept a URI instead of a string

Categories

(Core :: Permission Manager, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla42
Tracking Status
firefox42 --- fixed

People

(Reporter: Nika, Assigned: Nika)

References

Details

(Keywords: addon-compat, dev-doc-needed, Whiteboard: [addon-compat mentioned in comment 13])

Attachments

(2 files, 7 obsolete files)

7.46 KB, patch
ehsan
: review+
Details | Diff | Splinter Review
85.42 KB, patch
Details | Diff | Splinter Review
To allow for future changes to the implementation of nsPermissionManager to distinguish between http and https URIs, this patch changes the arguments to nsIPermissionManager::Remove to accept a nsIURI instead of a string.

This makes the API conform more closely to the API for nsIPermissionManager::Add, an allows for future changes to the backend to be made more easily.
This patch should probably be split up to make reviewing it easier.
Attachment #8613569 - Flags: review?(ehsan)
Blocks: 1165263
Comment on attachment 8613569 [details] [diff] [review]
Part 1: Change the API for nsIPermissionManager::Remove() to accept a URI instead of a string

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

::: extensions/cookie/nsPermissionManager.cpp
@@ +1077,5 @@
>  
>  NS_IMETHODIMP
> +nsPermissionManager::RemoveFromPermission(nsIPermission* aPerm)
> +{
> +  nsCString host;

Nit: nsAutoCString, so that we'd avoid allocating a buffer for small strings.

@@ +1085,5 @@
> +  nsCOMPtr<nsIPrincipal> principal;
> +  rv = GetPrincipal(host, getter_AddRefs(principal));
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  nsCString type;

Ditto.

@@ +1089,5 @@
> +  nsCString type;
> +  rv = aPerm->GetType(type);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  return RemoveFromPrincipal(principal, type.get());

This does something very different than what the comment says!

::: netwerk/base/nsIPermissionManager.idl
@@ +114,5 @@
>     * @param type   a case-sensitive ASCII string, identifying the consumer. 
>     *               The type must have been previously registered using the
>     *               add() method.
>     */
> +  void remove(in nsIURI uri,

Please rev the uuid of this interface.  You can use the ./mach update-uuids command for that purpose.

@@ +131,5 @@
> +   * This must be an exact permission, returned from, for example, the enumerator.
> +   *
> +   * @param perm   a permission returned from, for example, the enumerator
> +   */
> +  void removeFromPermission(in nsIPermission perm);

Nit: I think removePermission is a slightly better name.  Do you agree?
Attachment #8613569 - Flags: review?(ehsan) → review-
Also, note that this needs tests!
Updated due to changes in Part 1
Attachment #8613571 - Attachment is obsolete: true
Comment on attachment 8616888 [details] [diff] [review]
Part 1: Change the API for nsIPermissionManager::Remove() to accept a URI instead of a string

Writing tests before review
Attachment #8616888 - Flags: review?(ehsan)
Added a test to ensure that nsIPermissionManager::RemovePermisison() only removes 1 permission at a time.
Attachment #8616888 - Attachment is obsolete: true
Attachment #8620403 - Flags: review?(ehsan)
Comment on attachment 8620403 [details] [diff] [review]
Part 1: Change the API for nsIPermissionManager::Remove() to accept a URI instead of a string

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

Looks great!
Attachment #8620403 - Flags: review?(ehsan) → review+
This fixes another use which was added since I wrote the patch initially
Attachment #8616889 - Attachment is obsolete: true
Attachment #8623869 - Flags: review?(ehsan)
How are addons supposed to know which API to use? An addon, unlike Firefox core code, needs to support multiple versions of interfaces simultaneously, at a minimum over the lifespan of an ESR. Shouldn't this be a different method such as removeURI()?
This bug is part of the movement towards bug 1165263 - which would mean that a remove() method which accepts a host could no longer be supported, as it would not be meaningful (as internally we wouldn't be using a host anymore).

We can't support the old API for remove, so we are changing it to a new one which we will be able to support now and into the future.

Unfortunately, this change will break addons, but I don't think that there is a better way to do this.
Comment on attachment 8623869 [details] [diff] [review]
Part 2: Update JavaScript consumers of nsIPermissionManager::Remove

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

::: browser/components/preferences/advanced.js
@@ +567,5 @@
>      // remove the permission
>      var pm = Components.classes["@mozilla.org/permissionmanager;1"]
>                         .getService(Components.interfaces.nsIPermissionManager);
> +    pm.removePermission(perm);
> +    pm.removePermission(perm);

This line shouldn't be in here twice. Does pm.removePermission(perm) remove both the nsIPermissionManager.ALLOW_ACTION and nsIOfllineCacheUpdateService.ALLOW_NO_WARN permissions?
Attachment #8623869 - Flags: feedback-
I looked back at advanced.js file, and apparently it was late when I worked on it, because I did _everything_ wrong. Here's a new patch which should not have the problems of the old version. (hopefully)

With regard to your question about nsIPermissionManager.ALLOW_ACTION and nsIOfflineCacheUpdateService.ALLOW_NO_WARN, pm.remove() only takes 2 arguments. Those extra arguments to the remove method are actually ignored by the xpcom translation layer. (so in the old code, the same method was also being invoked twice).  This new version only invokes the method once, but performs some ugly code to get a URI back out of the host object. I improve it (in my opinion) in bug 1173523 when I get access to principals on nsIPermissions, which have better properties for this kind of stuff.

I'd appreciate it if you could take another look and make sure that I didn't do anything obviously wrong.
Attachment #8623869 - Attachment is obsolete: true
Attachment #8623869 - Flags: review?(ehsan)
Attachment #8624720 - Flags: feedback?(jaws)
Comment on attachment 8624720 [details] [diff] [review]
Part 2: Update JavaScript consumers of nsIPermissionManager::Remove

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

I only looked at advanced.js.

::: browser/components/preferences/in-content/advanced.js
@@ +606,5 @@
>      var pm = Components.classes["@mozilla.org/permissionmanager;1"]
>                         .getService(Components.interfaces.nsIPermissionManager);
> +    let uri;
> +    try {
> +      uri = ios.newURI(host, null, null);

Please add a comment that file:// URIs are stored with their scheme. Might also swap these (try putting the http:// scheme on first and if that throws then attempt without, since the http:// will be more common than file://).
Attachment #8624720 - Flags: feedback?(jaws) → feedback+
Add comment about file:// URIs to advanced.js.
Attachment #8624720 - Attachment is obsolete: true
Attachment #8624751 - Flags: review?(ehsan)
Keywords: addon-compat
Whiteboard: [addon-compat mentioned in comment 13]
(In reply to Ian Nartowicz from comment #12)
> How are addons supposed to know which API to use? An addon, unlike Firefox
> core code, needs to support multiple versions of interfaces simultaneously,
> at a minimum over the lifespan of an ESR. Shouldn't this be a different
> method such as removeURI()?

If I understand correctly, the add-on could attempt to use the new API in a try-catch block, and if that throws then in the catch block they could attempt to use the function with the old arguments. Does that help?
Flags: needinfo?(mozilla)
Attachment #8624751 - Flags: review?(ehsan) → review+
Keywords: checkin-needed
part 2 failed to apply: renamed 1170200 -> Bug-1170200---Part-2-Update-JavaScript-consumers-o.patch
applying Bug-1170200---Part-2-Update-JavaScript-consumers-o.patch
patching file browser/base/content/urlbarBindings.xml
Hunk #1 FAILED at 2395
1 out of 1 hunks FAILED -- saving rejects to file browser/base/content/urlbarBindings.xml.rej

could you take a look, thanks!
Flags: needinfo?(michael)
Rebased. Hopefully the patch should apply now :)
Attachment #8624751 - Attachment is obsolete: true
Flags: needinfo?(michael)
Keywords: checkin-needed
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/01c9c0047419
https://hg.mozilla.org/mozilla-central/rev/d716c5d113d8
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
Blocks: 1180871
Closing old needinfo request...
Flags: needinfo?(mozilla)
You need to log in before you can comment on or make changes to this bug.