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"

Dupe of 174070, or different?

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

Since Pinkerton gave it a target, confirming ;)

Here's a proposal: Retitle the "Allow/Deny" column to "Policy" and add a third option, titled "Session only". Thoughts? cl

Sounds reasonable enough to me.

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

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

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

Comment on attachment 202760 [details] [diff] [review] The patch so far Asking for r=pink.

The *real* NIB file with the fixes. cl

Comment on attachment 202767 [details] the real nib shady This nib is obsolete now that the one from bug 314557 has landed....

(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

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

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

Comment on attachment 203176 [details] updated for latest nib changes Obsolete due to check-in of bug 269084.

Comment on attachment 202760 [details] [diff] [review] The patch so far Obsolete due to check-in of bug 269084.

Chris, are you working on an updated patch?

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

Updated source code.

The nib file.

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?

(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

We should do all or none.

(In reply to comment #23) > We should do all or none. I agree, and my vote would be for "all". cl

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).

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

(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

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

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 on attachment 208828 [details] updated nib file Nib looks fine (although marking things as patches makes them text/plain, so don't do that).

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 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.

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

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.

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

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!

Transferred flags. This should be ready for checkin now. cl

Strings change is in on the trunk and 1.8 branch.

Checked in, trunk and 1.8.

Patch needs detabbing :(

Verified on 1.8 branch and trunk