302865 - In cookies exceptions list, should also have option for session cookies only, not just allow/deny

Status: VERIFIED FIXED

(Camino Graveyard :: Preferences, defect)

Description

[Mike Nguyen]

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8b4) Gecko/20050730 Camino/0.9a2+
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8b4) Gecko/20050730 Camino/0.9a2+

I have some sites set to allow session cookies only in my hostperm.1 file (e.g.
"host cookie 8 ...") and Camino will respect that, but there's no way to set
that from the GUI. It seems like a small change to add a third option, "Session
cookies only"

Reproducible: Always

Steps to Reproduce:

Actual Results:  
You can only select allow/deny in Preferences->Privacy->Edit Exceptions List...;
sites that have an "host cookie 8..." in hostperm.1 show up as deny, even though
Camino behaves correctly.

Expected Results:  
Should have an option for "Session cookies only" also, not just "Allow" and "Deny"

Comment 1

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

Dupe of 174070, or different?

Comment 2

[Mike Nguyen]

Bug 174070 is for a global setting that you can set in user.js; this bug is for
per-site settings in hostperm.1.

Comment 3

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

Since Pinkerton gave it a target, confirming ;)

Comment 4

[Chris Lawson (gone)]

Here's a proposal:

Retitle the "Allow/Deny" column to "Policy" and add a third option, titled "Session only".

Thoughts?

cl

Comment 5

[Mike Nguyen]

Sounds reasonable enough to me.

Comment 6

[Chris Lawson (gone)]

Taking. I'm about 90% done with this one already. The only issue I have left is how to ensure that the "Policy" column can be re-sized, and give it a slightly larger default width (so the phrase "Session Only" shows up in its entirety).

cl

Comment 7

[Chris Lawson (gone)]

Attachment: The patch so far

Here's what I've got so far. It's fully functional, but has the visual anomalies I mentioned above. If anyone has any ideas on that, I'm open to suggestions.

cl

Comment 8

[Chris Lawson (gone)]

Attachment: nib file with second column retitled to "Policy"

Fixes the visual problems mentioned earlier, too. This is ready for some review lovin'.

cl

Comment 9

[Chris Lawson (gone)]

Comment on attachment 202760 [details] [diff] [review]
The patch so far

Asking for r=pink.

Comment 10

[Chris Lawson (gone)]

Attachment: the real nib shady

The *real* NIB file with the fixes.

cl

Comment 11

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

Comment on attachment 202767 [details]
the real nib shady

This nib is obsolete now that the one from bug 314557 has landed....

Comment 12

[Chris Lawson (gone)]

(In reply to comment #11)
> (From update of attachment 202767 [details] [edit])
> This nib is obsolete now that the one from bug 314557 has landed....

Right-o. I'll post a new one later tonight, hopefully.

cl

Comment 13

[Chris Lawson (gone)]

Attachment: updated for latest nib changes

This should be ready to go now, barring any other changes to the nib or PrivacyPane.mm.

cl

Comment 14

[Chris Lawson (gone)]

CCing Simon, retargeting for 1.0 since we now have a working patch.

cl

Comment 15

[Chris Lawson (gone)]

Comment on attachment 203176 [details]
updated for latest nib changes

Obsolete due to check-in of bug 269084.

Comment 16

[Chris Lawson (gone)]

Comment on attachment 202760 [details] [diff] [review]
The patch so far

Obsolete due to check-in of bug 269084.

Comment 17

[Samuel Sidler (old account; do not CC)]

Chris, are you working on an updated patch?

Comment 18

[Chris Lawson (gone)]

Woops, forgot to post the new one. Will get that up tonight.

cl

Comment 19

[Chris Lawson (gone)]

Attachment: patch to current source for PrivacyPane.mm

Updated source code.

Comment 20

[Chris Lawson (gone)]

Attachment: Updated nib file, zipped up.

The nib file.

Comment 21

[Simon Fraser [no longer active]]

So if we allow the user to specify session-only in the Exceptions List, why don't we:
a) for all cookies (which is what many users want), and
b) for each cookie in the "this page wants to set a cookie" sheet?

Comment 22

[Chris Lawson (gone)]

(In reply to comment #21)
> So if we allow the user to specify session-only in the Exceptions List, why
> don't we:
> a) for all cookies (which is what many users want), and

Bug 174070 (which is assigned to Simon, hah!)

> b) for each cookie in the "this page wants to set a cookie" sheet?

Bug 173521, which was WONTFIXed, although I think there's quite a bit of support for it (despite Mike's decision).

cl

Comment 23

[Simon Fraser [no longer active]]

We should do all or none.

Comment 24

[Chris Lawson (gone)]

(In reply to comment #23)
> We should do all or none.

I agree, and my vote would be for "all".

cl

Comment 25

[Chris Lawson (gone)]

Attachment: Use identical wording to request sheet

This matches the wording used in the request sheet (as updated by bug 323842), using "Allow for Session" rather than "Session Only" (which smorgan pointed out was a bad idea).

Comment 26

[Chris Lawson (gone)]

Attachment: updated nib file

Changes "Allow/Deny" column title to "Policy", adds a bit of width so "Allow for Session" isn't truncated.

cl

Comment 27

[Chris Lawson (gone)]

(In reply to comment #23)
> We should do all or none.
 
Because of Core bug 249596, which was just WONTFIXed, we cannot do exactly what was requested in bug 174070 without a lot of convoluted logic and a great deal of user confusion, so my vote has just changed to "all but that".

cl

Comment 28

[Chris Lawson (gone)]

Attachment: fixed per stuart's comments, also fixes broken search on policy column

Accidentally posted this in the other session cookie bug. Oops.

cl

Comment 29

[Stuart Morgan]

Comment on attachment 209319 [details] [diff] [review]
fixed per stuart's comments, also fixes broken search on policy column

Looks good--and yes pink, I even tested aol.com ;)

r=me

Comment 30

[Stuart Morgan]

Comment on attachment 208828 [details]
updated nib file

Nib looks fine (although marking things as patches makes them text/plain, so don't do that).

Comment 31

[Stuart Morgan]

Oh, and whoever checks this in needs to remember to add an entry in the Privacy pane's Localizable.strings file for "Allow for Session"

Comment 32

[Simon Fraser [no longer active]]

Comment on attachment 209319 [details] [diff] [review]
fixed per stuart's comments, also fixes broken search on policy column

>Index: PrivacyPane.mm
>===================================================================

> // popup indices
> const int kAllowIndex = 0;
>-const int kDenyIndex = 1;
>+const int kSessionOnlyIndex = 1;
>+const int kDenyIndex = 2;

Use an enum (implicit ordering)?

>+// nsIPermissonManager, for some reason, doesn't have a constant for Allow for Session
>+const int kAllowForSessionAction = 8;

You should use nsICookiePermission::ACCESS_SESSION rather than this.


> // callbacks for sorting the permission list
>+PR_STATIC_CALLBACK(int) indexForCapability(int aCapability)
>+{
>+  if (aCapability == nsIPermissionManager::DENY_ACTION)
>+    return kDenyIndex;
>+  if (aCapability == kAllowForSessionAction)
>+    return kSessionOnlyIndex;
>+  else
>+    return kAllowIndex;

Use a switch() statement.


>+  // set up policy popups
>   NSPopUpButtonCell *popupButtonCell = [mPermissionColumn dataCell];
>   [popupButtonCell setEditable:YES];
>-  [popupButtonCell addItemsWithTitles:[NSArray arrayWithObjects:[self getLocalizedString:@"Allow"], [self getLocalizedString:@"Deny"], nil]];
>+  [popupButtonCell addItemsWithTitles:[NSArray arrayWithObjects:[self getLocalizedString:@"Allow"], [self getLocalizedString:@"Allow for Session"], [self getLocalizedString:@"Deny"], nil]];

Wrap these lines like:

    [popupButtonCell addItemsWithTitles:[NSArray arrayWithObjects:[self getLocalizedString:@"Allow"],
                                                                  [self getLocalizedString:@"Allow for Session"],
                                                                  [self getLocalizedString:@"Deny"],
                                                                  nil]];

sr=me with those changes.

Comment 33

[Chris Lawson (gone)]

Attachment: Applies Simon's suggestions

Suggestions fixed, along with two more fixes pointed out on IRC.

Pink said this could go in on trunk whenever someone gets around to it, along with the related bug 323842.

cl

Comment 34

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

When the checker-in-er person adds the new Localizable.string for this bug (comment 31), can he also change the string in bug 328802, please.

Comment 35

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

Also please get the string change for bug 329011 with these other strings changes.

Comment 36

[Simon Fraser [no longer active]]

Comment on attachment 209425 [details] [diff] [review]
Applies Simon's suggestions

>Index: PrivacyPane.h
>===================================================================

>+// network.cookie.cookieBehavior settings
>+// these are the defaults, overriden by whitelist/blacklist
>+typedef enum ECookieBehaviorSettings
>+{
>+  eAcceptAllCookies,
>+  eAcceptCookiesFromOriginatingServer,
>+  eDenyAllCookies
>+};
>+
>+// for the "Policy" column in the Exceptions List
>+typedef enum ECookiePolicyPopupIndex
>+{
>+  eAllowIndex,
>+  eSessionOnlyIndex,
>+  eDenyIndex
>+};

You are not actually defining new types here; you probably want

typedef enum Foo {
...
} Foo;


>Index: PrivacyPane.mm
>===================================================================

> // callbacks for sorting the permission list
>+PR_STATIC_CALLBACK(int) indexForCapability(int aCapability)
>+{
>+  switch (aCapability)
>+  {
>+    case nsIPermissionManager::DENY_ACTION:
>+      return eDenyIndex;
>+	case nsICookiePermission::ACCESS_SESSION:
>+      return eSessionOnlyIndex;
>+    default: // match nsIPermissionManager::ALLOW_ACTION and other values
>+      return eAllowIndex;
>+  }

You could make this self-documenting:

    switch (aCapability)
    {
        case nsIPermissionManager::DENY_ACTION:
            return eDenyIndex;

        case nsICookiePermission::ACCESS_SESSION:
            return eSessionOnlyIndex;

        case nsIPermissionManager::ALLOW_ACTION:
        default:
            return eAllowIndex;
    }

We'd better be sure that the values of the enums in nsIPermissionManager and
nsICookiePermission don't conflict!

Comment 37

[Chris Lawson (gone)]

Attachment: latest update per Simon's most recent suggestions

Transferred flags. This should be ready for checkin now.

cl

Comment 38

[Mark Mentovai]

Strings change is in on the trunk and 1.8 branch.

Comment 39

[Mark Mentovai]

Checked in, trunk and 1.8.

Comment 40

[Mark Mentovai]

Patch needs detabbing :(

Comment 41

[[e-mail]]

Verified on 1.8 branch and trunk