We plugin permissions which are supposed to auto-renew for a certain period. Implementing this as a permission manager client is in fact pretty clunky and inefficient. This API lets you test-and-renew all in one motion.

Comment on attachment 762984 [details] [diff] [review] Bug 883420 - Add an API to set-and-renew a permission in one call Review of attachment 762984 [details] [diff] [review]: ----------------------------------------------------------------- Isn't there all the needed tools to do that using the PermissionManager API? It seems to be a very specific need and for the moment I do not feel comfortable adding this in the PermissionManager unless it is proven to be a real pain to make this as a client. Even though, in that case, I would prefer to fix the pain points.

Doing this as a client is possible, but it is pretty expensive. You have to do your initial test using .getPermissionObject (and do your own test for system-principal), then you have to call .add again based on the expireType and expireTime and domain values from the original permission object, but in order to do that you have to construct a principal from the domain. Then the permission manager has to redo its hash lookups. Whether or not this was an API directly on the permission manager, we would certainly be hiding it behind a helper function, because renewing permissions are a feature we're hoping to use for many other kinds of page features in the future, and I definitely wouldn't want to have that code duplicated around the codebase. So overall I think that adding this rather specific API on the permission manager is better than the alternative.

Comment on attachment 762984 [details] [diff] [review] Bug 883420 - Add an API to set-and-renew a permission in one call I think we agreed on IRC to go for a solution where we add a .updateExpireTime() method to nsIPermissionManager that will take three arguments: principal, exactMatch and new expireTime. The clients of this API could simply do: if (pm.testPermissionFromPrincipal(principal, "foo") == pm.ALLOW_ACTION) { pm.updateExpireTime(principal, false, 42); } If the permission associated with the principal isn't of time EXPIRE_TIME, the call should simply be ignored.

It will need to be a 4-argument function, since the expire time for a session permission will be different from that for a permanent. Implementing now.

Comment on attachment 765155 [details] [diff] [review] Add an API to set a new expireTime for an existing permission, r?jdm sr?mounir Review of attachment 765155 [details] [diff] [review]: ----------------------------------------------------------------- sr=me with the non-nit comments applied. ::: extensions/cookie/nsPermissionManager.cpp @@ +1850,5 @@ > + > + int32_t idx = entry->GetPermissionIndex(typeIndex); > + if (-1 == idx) { > + return NS_OK; > + } nit: I'm pretty sure all the code above could be refactored. Don't we have the same code in ::GetPermissionObject() ? @@ +1860,5 @@ > + perm.mExpireTime = aSessionExpireTime; > + } > + break; > + case EXPIRE_TIME: > + perm.mExpireTime = aPersistentExpireTime; Can't we simply update perm.mExpireTime with aExpireTime if perm.ExpireType == EXPIRE_SESSION || EXPIRE_TIME ? ::: netwerk/base/public/nsIPermissionManager.idl @@ +141,5 @@ > */ > uint32_t testPermission(in nsIURI uri, > in string type); > > + nit: no need for this new line. @@ +232,5 @@ > void removePermissionsForApp(in unsigned long appId, > in boolean browserOnly); > + > + /** > + * If the current permission is set to expire, reset the expiration time. Add that if there is no associated permission or if it doesn't involve time expiration, the call will be ignored. @@ +247,5 @@ > + void updateExpireTime(in nsIPrincipal principal, > + in string type, > + in boolean exactHost, > + in uint64_t sessionExpireTime, > + in uint64_t persistentExpireTime); I believe one parameter for expireTime should be enough.

> I believe one parameter for expireTime should be enough. It is definitely not enough for my use case. We have two types of permissions: * session permissions that expire after one hour * persistent permissions that expire after 90 days

Nits fixed.