363951 - Action menu button in Show Cookies sheet lacks "Add" (whitelist) option

Status: RESOLVED FIXED

(Camino Graveyard :: Preferences, defect)

Description

[Chris Lawson (gone)]

Use case: anyone who uses session cookies and decides to allow a site to specify cookie lifetimes (rather than forcing session cookies).

What they have to do now, after finding the site in the list:

1) Remove and block
2) Close sheet
3) Click "Edit Exceptions List"
4) Find site in deny list (hopefully you can remember what you just blocked)
5) Change "Deny" to "Allow"
6) Re-visit site to restore cookies to previous state (or as close as is possible, anyway)

That process is far more convoluted than it should be, and adding an "Allow" action to that menu button would resolve this entirely by reducing it to one step.

When bug 174070 is fixed, this is going to be even more of a problem than it is already because we'll be exposing UI that encourages the use case outlined above.

I dug through all the cookie bugs I could find -- I swear we talked about this at some point -- but I couldn't find anything about it in any open bugs or in any dupes or WFs.

Comment 1

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

I'm confused at how someone would need the STR you provided.  If they've done anything in the cookie sheet, there will be an entry for it (allow, session, deny) in the Exceptions List already.

For reference, there was a bug filed on allowing manual additions/editing to the Exceptions List (bug 269084) and it was fixed in such a way that indicated manual editing was not desirable.

Comment 2

[Chris Lawson (gone)]

(In reply to comment #1)
> I'm confused at how someone would need the STR you provided.  If they've done
> anything in the cookie sheet, there will be an entry for it (allow, session,
> deny) in the Exceptions List already.

That's true for people who only ever create session cookies as a result of being asked to choose. If you have network.cookie.lifetimePolicy set to 2, you get all cookies as session cookies and it doesn't ask (so you don't get an exceptions list entry).

cl

Comment 3

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Ooops, I meant to cc smfr when I commented before.

I'm not sure that's a reasonable situation, or rather a reasonable situation for Camino's target audience.  I'm not mortally opposed to this, though, just unsure of where it lies on the feature/bloat-confusion continuum.

Comment 4

[Luke deGruchy]

I vote to fix this.  Firefox allows you to manually enter exception sites for cookies.  Since I enable only cookies that are part of my whitelist, I use this feature extensively in Firefox.

I realize that I'm just a single largely irrelevant user, and that my vote alone definitely won't result in this getting fixed, which is fine.  I just wanted to offer my perspective, as a Firefox/Camino user who's maybe slightly overzealous about privacy.

Comment 5

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Those STR in comment 0 are still wrong; all a user needs to do is this:

1. "Block"
2. Close sheet
3. Open Exceptions List
4. Find site in list
5. Change "Deny" to "Allow"

There is no step 6; your cookies are preserved.

I don't know that adding "Add" there will be worth the clutter for the majority of Camino's users given how rare this case will be for them.

Comment 6

[Chris Lawson (gone)]

That's still a ridiculous set of steps to do what ought to be a one-click action.

Comment 7

[Luke deGruchy]

Workaround:

Edit ~/Library/Application Support/Camino/hostperm.1 

Camino preferences for allowed sites for cookies seems to respect the settings in that file, despite the onerous "Do not edit this file" disclaimer.

Comment 8

[Chris Lawson (gone)]

(In reply to comment #7)
> Workaround:
> 
> Edit ~/Library/Application Support/Camino/hostperm.1 

Sure, but that's horribly unfriendly and intimidating for most users. (Thus the "do not edit this file" warning.)

This bug is about putting a GUI on the feature. Camino uses -- and will continue to use -- standard Core APIs, including those that interface with hostperm.1, to accomplish such features.

Comment 9

[Chris Lawson (gone)]

(In reply to comment #5)
> I don't know that adding "Add" there will be worth the clutter for the majority
> of Camino's users given how rare this case will be for them.

By that logic, since "the majority of Camino's users" don't even know that menu exists or what the heck a cookie is in the first place, how about we just remove it entirely? :-p

Comment 10

[Mike Pinkerton (not reading bugmail)]

I don't see the harm in this, it's already hidden in an action menu, so adding more "actions" that streamline things seems reasonable to me. Users that don't know what it means won't go poking around in there and it makes it easier for users that do.

That said, this would never stop release of any product and we shouldn't explicitly allocate engineering resources to fix this, but if someone wants to in their spare time I don't think it will actively hurt anything.

Comment 11

[Chris Lawson (gone)]

I'll take this and fix it soon, then.

Comment 12

[Chris Lawson (gone)]

Attachment: fix

This will require nib and strings changes as well, to be posted in a second.

Comment 13

[Chris Lawson (gone)]

Attachment: nib file

Nib for Smokey to play with. (Ian, feel free to review this too.)

Comment 14

[Chris Lawson (gone)]

Attachment: new strings file for posterity, open to changes

Here are the strings changes. I'm open to discussion here, though, as "Allow" doesn't totally explain why you would want this oh-so-useful CM item.

Comment 15

[Chris Lawson (gone)]

--> 1.6 since we have code and just need to discuss wording at this point (most likely).

Comment 16

[froodian (Ian Leue)]

Comment on attachment 263311 [details] [diff] [review]
fix

As it stands, |allowCookiesFromSites| and |blockCookiesFromSites| have virtually identical code.  Can you move that into a shared method, and while you're there, clean up the formatting to comply with our current standards (I know the entire file is a mess, but since you'll be touching those lines as it is...)?

for the strings, how about:

"AllowCookieFromSite" = "Allow all cookies from sites matching “%@”";
"AllowCookiesFromSites" = "Allow all cookies from these sites";

I think the "all" makes it clearer that we're talking about a whitelist which will affect future cookie-accepting decisions.

Comment 17

[froodian (Ian Leue)]

Comment on attachment 263312 [details]
nib file

Oh, and the nib is fine.

Comment 18

[Chris Lawson (gone)]

Attachment: addresses review comments

I like the suggested strings changes. Here's fixed code that shares the logic for adding a permission for multiple selected hosts.

Comment 19

[Stuart Morgan]

Comment on attachment 267382 [details] [diff] [review]
addresses review comments

This needs a code review before sr?

Comment 20

[Chris Lawson (gone)]

Comment on attachment 267382 [details] [diff] [review]
addresses review comments

Changing r flag to murph since he has more time lately.

When this gets r, please set someone for sr too, probably smorgan.

Comment 21

[Sean Murphy]

Comment on attachment 267382 [details] [diff] [review]
addresses review comments

r=me, although the patch needs to be diff'ed again because hunk 4 has some trouble.  Also, a few small comments:

>+- (void)addPermissionForSelection:(int)inPermission
>+{
>+  if (mCachedCookies && mPermissionManager) {
>+    NSArray* rows = [[mCookiesTable selectedRowEnumerator] allObjects];
>+    NSEnumerator* e = [rows reverseObjectEnumerator];

I'd rather see |e| renamed to |rowEnumerator| or something more descriptive.

>+    NSNumber* index;
>+    while ((index = [e nextObject]))
>+    {
>+      int row = [index intValue];

I think the variable names |row| and |index| should actually be reversed, since at first we're enumerating selected rows then using that number as an index for the cached cookies.

>+
>+      nsCAutoString host, name, path;
>+      mCachedCookies->ObjectAt(row)->GetHost(host);
>+      [self addPermission:inPermission forHost:[NSString stringWith_nsACString:host]];

The variables |name| and |path| are unused.

Comment 22

[Chris Lawson (gone)]

Attachment: fresh diff

Here's a fresh diff that should work fine with the latest tree.

The comments murph brought up are all good points. I've fixed the third (unused variable names, which was a side effect of my lazy C&P job on the code -- good catch!) but the first two are recurring problems throughout the file. I'll let Stuart or Pink decide whether we want to fix the entire file or not. I don't mind fixing it if that's what we want to do, but I think we should be as consistent as possible either way.

Comment 23

[Mike Pinkerton (not reading bugmail)]

Personally, I think |e| is ok, since it's pretty clear where it comes from and is fairly localized. If we're using the wrong variable names, that should be fixed, but could be farmed out to a separate bug. 

Comment 24

[Stuart Morgan]

Comment on attachment 279774 [details] [diff] [review]
fresh diff

sr=smorgan on the code. If the nib change was just adding a menu item, please make a new nib against the current version (if it was more complex, I can re-do the search field changes in this nib).

Comment 25

[Chris Lawson (gone)]

Attachment: updated nib

Here you go.

Comment 26

[Stuart Morgan]

Comment on attachment 283477 [details]
updated nib

sr=smorgan.
Whoever checks this in, make sure the strings from comment 16 get checked in as well.

Comment 27

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Checked in on the trunk and 18branch.