Closed Bug 224327 Opened 21 years ago Closed 21 years ago

if is blocked, is blocked, but cannot be unblocked using remove


(Core :: Networking: Cookies, defect)

Not set





(Reporter: mconnor, Assigned: mvl)




(2 files, 1 obsolete file)

spun off from

"The problem is that you can't find out what to remove. For example, if you
block from setting cookies, TestPermissions will return blocked for But calling Remove with won't do anything. We
need to decide what should happen."
The current nsIPermissionManager api worked when it was written. When you add a
permission a site, you are visiting the site, so passing a uri to Add made sense.
When you are deleting a permission, you are in the manager, and know the string
that you want to remove. So, that made sense too.

Now, you can add permissions when you are not visiting a site, and remove
permissions when you are visiting. So it is not that clear anymore.

And when path matching, wildcards or regexps come into play, it is even more
complicated. So it clearly needs some thought.
-> mvl :)
Assignee: darin → mvl
bit of discussion with mvl on IRC about this led to the following plan:

1) turn domainwalking on for .remove, and if we have to domainwalk then prompt
before removing the perm on the parent domain.  The prompt is optional, we could
include a pref to not show again.

2) make sure throughout the UI to not set the same permission for a subdomain so
that we minimize situation where is getting blocked because of and  If is already set, don't bother setting  We could always not break on match for the domainwalk and continue
upwards, not sure if this is a perf issue or not.
Patch that does part 1, domainwalk when removing a permission.
Attachment #138502 - Flags: review?(dwitte)
covers part 2 above, the Tools->Cookie Manager options already won't allow you
to set the permission if its the same as the current permission.  This extends
this to cookiemgr

On further review of these changes, I do not believe anything here is of
sufficient severity to warrant a dialog, despite my earlier comments.
Attachment #138590 - Flags: review?(
Blocks: 216743
Comment on attachment 138590 [details] [diff] [review]
fix cookieviewer to not allow duplicating permissions

>Index: mozilla/extensions/wallet/cookieviewer/resources/content/CookieViewer.js
>+  if (!permissionmanager.testPermission(uri, "cookie") == action)

What is wrong with != ? :)
mvl: nothing, :)  I don't know why I did it that way, it can easily be changed
before landing
Let's see if I got this right:
if is blocked and is unblocked, then when you're
visiting and you say "set to default" it changes to blocked, then
when you say "set to default" again it sets it to default?
Comment on attachment 138590 [details] [diff] [review]
fix cookieviewer to not allow duplicating permissions

>-  permissionmanager.add(uri, dialogType, action);
>+  // only set the permission if the permission doesn't already exist
>+  if (!permissionmanager.testPermission(uri, "cookie") == action)
>+    permissionmanager.add(uri, dialogType, action);
In the unlikely event that you think you can persuade me that the literal
cookie string is correct feel free to rerequest review ;-)
Attachment #138590 - Flags: review?( → review-
Comment on attachment 138590 [details] [diff] [review]
fix cookieviewer to not allow duplicating permissions

Oh, and (!x == y) is silly.
Attachment #138590 - Attachment is obsolete: true
Attachment #139237 - Flags: review?(
Attachment #139237 - Flags: review?( → review+
Attachment #139237 - Flags: superreview?(alecf)
Comment on attachment 139237 [details] [diff] [review]
fix cookieviewer to not allow duplicating permissions

Attachment #139237 - Flags: superreview?(alecf) → superreview+
cookieviewer patch landed at 01/27/2004 08:42
Comment on attachment 138502 [details] [diff] [review]
domainwalk in remove

nice! just one nit.

>Index: extensions/cookie/nsPermissionManager.cpp
>+nsHostEntry *
>+nsPermissionManager::GetHostEntry(const nsAFlatCString &aHost,
>+                                  PRUint32              aType)
>   PRUint32 offset = 0;
>+  nsHostEntry *entry = nsnull;

no need to init |entry| to null here :)

Attachment #138502 - Flags: review?(dwitte) → review+
Attachment #138502 - Flags: superreview?(darin)
Attachment #138502 - Flags: superreview?(darin) → superreview+
backend patch checked in.
Closed: 21 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.