Closed Bug 225857 Opened 17 years ago Closed 16 years ago
Implement session cookie UI
16.74 KB, patch
|Details | Diff | Splinter Review|
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.
Attachment #135628 - Flags: review?(neil.parkwaycc.co.uk)
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.
Attachment #135628 - Flags: review?(neil.parkwaycc.co.uk) → review-
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.
Attachment #135628 - Attachment is obsolete: true
Attachment #137076 - Flags: review?(neil.parkwaycc.co.uk)
> + 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...
Attachment #137076 - Flags: review?(neil.parkwaycc.co.uk) → review+
Attachment #137076 - Flags: superreview?(alecf)
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
Attachment #137076 - Flags: superreview?(alecf) → superreview+
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
Target Milestone: --- → mozilla1.7alpha
this is the one for checkin
Attachment #137076 - Attachment is obsolete: true
landed on trunk for 1.7a
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
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
You need to log in before you can comment on or make changes to this bug.