225857 - Implement session cookie UI

Status: RESOLVED FIXED

(Core :: Networking: Cookies, defect)

Description

[Mike Connor [:mconnor]]

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 1

[Mike Connor [:mconnor]]

Attachment: patch

Comment 2

[[e-mail]]

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.

Comment 3

[Mike Connor [:mconnor]]

Attachment: patch v2

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.

Comment 4

[Michiel van Leeuwen (email: mvl+moz@)]

> +    const ALLOW_SESSION_ONLY = 8;

nsICookiePermission.idl would be the best place to make this public. Too bad the
file doesn't exist :)

Comment 5

[[e-mail]]

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 6

[Alec Flett]

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?

Comment 7

[[e-mail]]

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 8

[Alec Flett]

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

Comment 9

[Mike Connor [:mconnor]]

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

Comment 10

[Mike Connor [:mconnor]]

Attachment: patch with comment explaining ALLOW_SESSION_ONLY const

this is the one for checkin

Comment 11

[Mike Connor [:mconnor]]

landed on trunk for 1.7a

Comment 12

[Sven Grull]

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

Comment 13

[Christian :Biesinger (don't email me, ping me on IRC)]

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?

Comment 14

[Mike Connor [:mconnor]]

#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