Closed Bug 225857 Opened 17 years ago Closed 16 years ago

Implement session cookie UI

Categories

(Core :: Networking: Cookies, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla1.7alpha

People

(Reporter: mconnor, Assigned: mconnor)

References

Details

Attachments

(1 file, 2 obsolete files)

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.
Attached patch patch (obsolete) — Splinter Review
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-
Attached patch patch v2 (obsolete) — Splinter 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
Blocks: 216743
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
Blocks: 100573
You need to log in before you can comment on or make changes to this bug.