The backend already exists, and since the whole statusbar cookie thing is slipping to 1.7a, we should add the UI for session-only whitelisting for 1.6. Patch coming up in a little while.

Comment on attachment 135628 [details] [diff] [review] patch >+ var canStr, cannotStr, canSessionStr; Uninitialized variable. >+ // 8 is the equivalent of nsIPermissionManager.ALLOW_SESSION_ONLY, not a public value >+ <button id="btnSession" label="&allowSiteSession.label;" disabled="true" >+ oncommand="setCookiePermissions(8);"/> What an ugly comment. dwitte, I really, really, would like to see this in nsICookiePermission.

addresses the uninitialized error with cookieViewer. Converts a couple if/elseif/else to switch statements. Actually hides the session button in the image viewer. Fixes some other misc things too.

> + const ALLOW_SESSION_ONLY = 8; nsICookiePermission.idl would be the best place to make this public. Too bad the file doesn't exist :)

Comment on attachment 137076 [details] [diff] [review] patch v2 > setRadioButton("UseCookiesDefault", uri, nsIPermissionManager.UNKNOWN_ACTION, "cookie"); > setRadioButton("AllowCookies", uri, nsIPermissionManager.ALLOW_ACTION, "cookie"); >+ setRadioButton("AllowSessionCookies", uri, ALLOW_SESSION_ONLY, "cookie"); > setRadioButton("BlockCookies", uri, nsIPermissionManager.DENY_ACTION, "cookie"); > setRadioButton("UseImagesDefault", uri, nsIPermissionManager.UNKNOWN_ACTION, "image"); > setRadioButton("AllowImages", uri, nsIPermissionManager.ALLOW_ACTION, "image"); > setRadioButton("BlockImages", uri, nsIPermissionManager.DENY_ACTION, "image"); Ugly or what? >+ element = document.getElementById("btnSession"); >+ element.hidden = "true"; This dialog still thinks it's a popup manager too...

Comment on attachment 137076 [details] [diff] [review] patch v2 so this ALLOW_SESSION_ONLY.. nothing in this bug explains why this isn't an interface constant. I see people complain but no actual reasons here. Why can't it go into nsIPermissionManager or nsICookiePermission, and if they don't exist, why can't we create them?

Well, in the original bug where I added this constant, I didn't make it public on nsICookiePermission because it didn't exactly belong there... it's an interface that the cookieservice talks to, and the cookieservice doesn't need (nor should have) any knowledge of these "internal" constants. However, we need it somewhere, so I'm fine with adding it to nsICookiePermission, provided it's commented to the effect of what I wrote above. And provided that its presence (as an implementation detail) won't affect the possible freezing of the nsICookiePermission interface (at some point in the (possibly far) future). If it does affect that interfaces' freezability, we could make a new interface just to hold that constant. But let's not let this debate hold up this patch for 1.6...

Comment on attachment 137076 [details] [diff] [review] patch v2 ok, ok. please put a comment in the code itself where you're defining ALLOW_SESSION_ONLY describing where the constant should eventually go. also, since its a bitfield, it might be worth defining as 1<<4 or something. sr=alecf

as much as I want this to be in 1.6 final, it missed the localization deadline going to attach a new patch before then with the requested comment from alecf and we can land this once the trunk reopens

this is the one for checkin

landed on trunk for 1.7a

Will this bug also handle an additional button in the cookie confirmation dialog? At the moment there are only "allow" and "deny" available.

so... does this mean that the site can set any cookies but they will be downgraded to session cookies; or that only session cookies from that site will be accepted?

#12: no, the dialogs haven't been hooked up yet, but I have another bug that deals with enhancing the UI for that dialog, and I hope to get to it before 1.7a #13: any cookies set will be downgraded to session cookies with the session cookie permission