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

RESOLVED FIXED in Firefox 42

Status

()

Core
Permission Manager
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: mystor, Assigned: mystor)

Tracking

({addon-compat, dev-doc-needed})

unspecified
mozilla42
addon-compat, dev-doc-needed
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox42 fixed)

Details

(Whiteboard: [addon-compat mentioned in comment 13])

Attachments

(2 attachments, 7 obsolete attachments)

7.46 KB, patch
Ehsan
: review+
Details | Diff | Splinter Review
85.42 KB, patch
Details | Diff | Splinter Review
(Assignee)

Description

3 years ago
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.
(Assignee)

Comment 1

3 years ago
Created attachment 8613569 [details] [diff] [review]
Part 1: Change the API for nsIPermissionManager::Remove() to accept a URI instead of a string
(Assignee)

Comment 2

3 years ago
Created attachment 8613571 [details] [diff] [review]
Part 2: Update JavaScript consumers of nsIPermissionManager::Remove

This patch should probably be split up to make reviewing it easier.
(Assignee)

Updated

3 years ago
Attachment #8613569 - Flags: review?(ehsan)
(Assignee)

Updated

3 years ago
Blocks: 1165263

Comment 4

3 years ago
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-

Comment 5

3 years ago
Also, note that this needs tests!
(Assignee)

Comment 6

3 years ago
Created attachment 8616888 [details] [diff] [review]
Part 1: Change the API for nsIPermissionManager::Remove() to accept a URI instead of a string
Attachment #8613569 - Attachment is obsolete: true
Attachment #8616888 - Flags: review?(ehsan)
(Assignee)

Comment 7

3 years ago
Created attachment 8616889 [details] [diff] [review]
Part 2: Update JavaScript consumers of nsIPermissionManager::Remove

Updated due to changes in Part 1
Attachment #8613571 - Attachment is obsolete: true
(Assignee)

Comment 8

3 years ago
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)
(Assignee)

Comment 9

3 years ago
Created attachment 8620403 [details] [diff] [review]
Part 1: Change the API for nsIPermissionManager::Remove() to accept a URI instead of a string

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 10

3 years ago
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+
(Assignee)

Comment 11

3 years ago
Created attachment 8623869 [details] [diff] [review]
Part 2: Update JavaScript consumers of nsIPermissionManager::Remove

This fixes another use which was added since I wrote the patch initially
Attachment #8616889 - Attachment is obsolete: true
Attachment #8623869 - Flags: review?(ehsan)

Comment 12

3 years ago
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()?
(Assignee)

Comment 13

3 years ago
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-
(Assignee)

Comment 15

3 years ago
Created attachment 8624720 [details] [diff] [review]
Part 2: Update JavaScript consumers of nsIPermissionManager::Remove

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+
(Assignee)

Comment 17

3 years ago
Created attachment 8624751 [details] [diff] [review]
Part 2: Update JavaScript consumers of nsIPermissionManager::Remove

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)

Updated

3 years ago
Attachment #8624751 - Flags: review?(ehsan) → review+
(Assignee)

Updated

3 years ago
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)

Updated

3 years ago
Keywords: checkin-needed
(Assignee)

Comment 21

3 years ago
Created attachment 8629391 [details] [diff] [review]
Part 2: Update JavaScript consumers of nsIPermissionManager::Remove

Rebased. Hopefully the patch should apply now :)
Attachment #8624751 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Flags: needinfo?(michael)
Keywords: checkin-needed

Updated

3 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/01c9c0047419
https://hg.mozilla.org/mozilla-central/rev/d716c5d113d8
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox42: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla42

Updated

3 years ago
Blocks: 1180871
Closing old needinfo request...
Flags: needinfo?(mozilla)
Keywords: dev-doc-needed
Blocks: 1231266
You need to log in before you can comment on or make changes to this bug.