230624 - Rework cookie dialogs to allow setting session-only cookies/set session perm

Status: RESOLVED FIXED

(Core :: Networking: Cookies, enhancement, P1)

Description

[Mike Connor [:mconnor]]

I should have filed this bug a long time ago.

I'm going to split this off from the domain selection bug since I want to focus
on this part for 1.7a.  Modifying the domain to be set can come after I hork
cookiepromptservice :)

Comment 1

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

Make sure you don't make the dialog too crowded. imho, the dialog is meant to
ask about storing one cookie, not as a interface to manage all you cookie
permissions.
The checkbox is only there to stop sites that set a cookie for every image, and
to prevent poor users for having to say "no" a thousand times.
(same goes for the domain selection actually)

Comment 2

[Mike Connor [:mconnor]]

Attachment: patch v1

Comment 3

[Mike Connor [:mconnor]]

Comment on attachment 138855 [details] [diff] [review]
patch v1

in light of that other bug, I'd like some feedback ASAP :)

Comment 4

[Mike Connor [:mconnor]]

hmm, this patch breaks Deny, but Allow/Session both work... I'll have to look at
this tomorrow.  mvl, if you can point out why that'd be nice :)

Comment 5

[Mike Connor [:mconnor]]

Attachment: patch v2

This one has actually been tested for all six cases.  Ironically, the XUL part
was harder than the backend bits.  Fun.

Comment 6

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

Comment on attachment 138856 [details] [diff] [review]
patch v2

>Index: mozilla/extensions/cookie/nsCookiePromptService.cpp
> nsCookiePromptService::CookieDialog(nsIDOMWindow *aParent,
>                                     nsICookie *aCookie,
>                                     const nsACString &aHostname,
>                                     PRInt32 aCookiesFromHost,
>                                     PRBool aChangingCookie,
>                                     PRBool *aRememberDecision,
>-                                    PRBool *aAccept)
>+                                    PRInt32 *aAccept)
> {

You should also update the .idl to this changed method. (I'm suprised it
compiles...)

>Index: mozilla/extensions/cookie/nsCookiePermission.cpp
> NS_IMETHODIMP 
> nsCookiePermission::CanSetCookie(nsIURI     *aURI,
>                                  nsIChannel *aChannel,
>                                  nsICookie2 *aCookie,
>                                  PRBool     *aIsSession,
>                                  PRInt64    *aExpiry,
>-                                 PRBool     *aResult)
>+                                 PRInt32    *aResult)
> {

Same here.

>@@ -386,17 +386,30 @@ nsCookiePermission::CanSetCookie(nsIURI 

>+        switch (*aResult) {
>+          case 2:
>+          case 1:
>+          case 0:
>+          default:
>+        }

Why not 0,1,2 ? :)

>Index: mozilla/extensions/cookie/resources/content/cookieAcceptDialog.xul
>@@ -81,13 +85,17 @@
>-      <hbox id="okCancelButtonsRight"/>
>+      <hbox>
>+        <button dlgtype="accept"/>
>+	<button dlgtype="extra1"/>
>+	<button dlgtype="cancel"/>
>+      </hbox>

I think this will break on the mac. There, 'Ok' and 'Cancel' are switched
compared to windows. Look at the cvs log for this file to find the bug, and the
pain it was to get it working. Please don't undo that :)

>Index: mozilla/extensions/cookie/resources/content/cookieAcceptDialog.js

> function cookieAccept()
> {
>+}
>+
>+function cookieAcceptSession()
>+{
> }

Maybe you can fold those in one function, by passing a parameter?

>Index: mozilla/extensions/cookie/resources/locale/en-US/cookieAcceptDialog.dtd

>+<!ENTITY     button.session.label           "Session">
>+<!ENTITY     button.session.accesskey       "S">

If i read the code right, this button will downgrade the cookie. But doesn't
ACCESS_SESSION mean that only session cookies will be accepted? a bit
confusing... (or maybe i am confused about what access_session means :) )


Can you provide a screenshot on how the dialog wil look?

Comment 7

[Mike Connor [:mconnor]]

> You should also update the .idl to this changed method. (I'm suprised it
> compiles...)  
> Same here.

ok

>@@ -386,17 +386,30 @@ nsCookiePermission::CanSetCookie(nsIURI 

>+        switch (*aResult) {
>>+          case 2:
>>+          case 1:
>>+          case 0:
>>+          default:
>>+        }

> Why not 0,1,2 ? :)

follows the convention elsewhere (session, white, black), not that it really matters

>Index: mozilla/extensions/cookie/resources/content/cookieAcceptDialog.xul
>@@ -81,13 +85,17 @@
>>-      <hbox id="okCancelButtonsRight"/>
>>+      <hbox>
>>+        <button dlgtype="accept"/>
>>+      <button dlgtype="extra1"/>
>>+      <button dlgtype="cancel"/>
>>+      </hbox>

> I think this will break on the mac. There, 'Ok' and 'Cancel' are switched
> compared to windows. Look at the cvs log for this file to find the bug, and 
> the pain it was to get it working. Please don't undo that :)

this would be somethng to find out.  Does the Mac have Deny/Allow currently?

>Index: mozilla/extensions/cookie/resources/content/cookieAcceptDialog.js

> function cookieAccept()
> {
>+}
>+
>+function cookieAcceptSession()
>+{
> }

> Maybe you can fold those in one function, by passing a parameter?

no, because these get assigned using doSetOKCancel

I'll post a screenshot after work, stick a Session button between Allow/Deny and
that's pretty much the UI change

Comment 8

[Mike Connor [:mconnor]]

Attachment: patch v3

this even addresses the Mac isse.  Unfortunately, Mac platformOverlay turns
ABCD into CBDA, which is why Cancel is collapsed and button2/3 are exposed.

other than that, this addressed all the other comments, except that the bit
about the params doesn't work if we want to register using doSetOKCancel.

Comment 9

[Mike Connor [:mconnor]]

Comment on attachment 138922 [details] [diff] [review]
patch v3

still feeling brave? :)

Comment 10

[Mike Connor [:mconnor]]

Attachment: what it looks like (linux)

on Mac, flip allow and deny

Comment 11

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

> this even addresses the Mac isse.  Unfortunately, Mac platformOverlay turns
> ABCD into CBDA, which is why Cancel is collapsed and button2/3 are exposed.

I don't have a mac, but i think that is what you want. windows should be
"accept, session, deny", mac should be "session, deny, accept". cancel and ok
are always the last two buttons. So just trust the old, tested code please :)
Maybe you should find someone with a mac and ask.

(I will look at the rest later, no time now)

Comment 12

[Mike Connor [:mconnor]]

sure, OK/Cancel do this, but I don't think Session/Deny/Accept is better than
Deny/Session/Accept and just because we use take over the Ok/Cancel buttons.

need to read the HIG I guess :)

Comment 13

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

random thought: how about another checkbox instead of a button? 
[ ] Downgrade this cookie to session only
or something. It will fix the button problem. Too much buttons are bad. The
session can be made persist, so it will be checked for the next cookie, just
like the remember checkbox.

Comment 14

[Mike Connor [:mconnor]]

that will add height to the dialog, where the current impl doesn't, but I think 
its a better idea.  The problem comes from wanting to implement the "set 
cookies for *.domain.com" RFE, and the dialog gets ugly from there.

Maybe the answer is:

[ ] Remember my decision for [Dropdown with appropriate options]
[ ] Downgrade this cookie to session-only

hmm, that seems sane.  Scary.

any other random thoughts before I rewrite that bit?

Comment 15

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

if you want random thoughts:
session only will be only used by advanced users. (hmm, the entire dialog
actually...)
A checkbox in the "details" part, next to the expires: info?
Expires: 2005-02-05 18:05      [ ] Downgrade
with a tooltip on downgrade explaining it "Downgrade this cookie to only this
session".

But your idea sound more sane, especially for the domain part :)

Comment 16

[Mike Connor [:mconnor]]

the entire dialog will get used by power users/gun-toting privacy nuts anyway, I
just want sanity :)

should have a patch up later

Comment 17

[Mike Connor [:mconnor]]

Attachment: patch v4 with reworked dialog

this actually looks semi-decent, I'll attach a screenshot here in a second

Comment 18

[Mike Connor [:mconnor]]

Comment on attachment 139002 [details] [diff] [review]
patch v4 with reworked dialog

I do actually like this method better, good random thought!

Comment 19

[Mike Connor [:mconnor]]

Attachment: with the checkbox

Comment 20

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

Comment on attachment 139002 [details] [diff] [review]
patch v4 with reworked dialog

>Index: mozilla/extensions/cookie/nsICookiePromptService.idl
>   /* Open a dialog that asks for permission to accept a cookie
>-   * Returns true when the permission is given.
>-   * return values are not modified when something fails.
>+   * Returns the follow results
>+   * 0 == deny cookie
>+   * 1 == accept cookie
>+   * 2 == accept cookie for current session
>+   *
>+   * Remembering the decision sets the matching permission
>    * 
>    * @param parent
>    * @param cookie
>    * @param hostname          the host that wants to set the cookie, 
>    *                           not the domain: part of the cookie
>    * @param cookiesFromHost   the number of cookies there are already for this host
>    * @param aChangingCookie   are we changing this cookie?
>    * @param checkValue        is the decision remembered?
>    

Doesn't javadoc use @returns for the return values? You can use that here.
And if you are changing it anyway, please be so good to update aChangingCookie
-> changingCookie and checkValue->rememberDecision. In the description for that
last one you can merge you added text above about matching permissions.

>-  boolean cookieDialog(in nsIDOMWindow parent,
>+  long cookieDialog(in nsIDOMWindow parent,
>                        in nsICookie cookie,
>                        in ACString hostname,
>                        in long cookiesFromHost,
>                        in boolean changingCookie,
>                        out boolean rememberDecision);

See, different names :)
Also, please align the parameters.

>Index: mozilla/extensions/cookie/resources/content/cookieAcceptDialog.xul

>         <hbox id="checkboxContainer">
>           <checkbox id="persistDomainAcceptance"
>-                    label="&dialog.remember.label;" accesskey="&dialog.remember.accesskey;"
>+                    label="&dialog.remember.label;" 
>+                    accesskey="&dialog.remember.accesskey;"
>+                    persist="checked"/>
>+        </hbox>
>+        <hbox id="acceptSessionContainer">
>+          <checkbox id="acceptSession"
>+                    label="&dialog.acceptSession.label;" 
>+                    accesskey="&dialog.acceptSession.accesskey;"
>                     persist="checked"/>

I think i like it better with those two swapped. First, set the permission,
then apply the above the all sites.

>Index: mozilla/extensions/cookie/resources/locale/en-US/cookieAcceptDialog.dtd
>+<!ENTITY     dialog.acceptSession.label     "Accept cookie for the current session only">

You already know it is about a cookie. Is is needed in this sentence again?

>Index: mozilla/netwerk/cookie/public/nsICookiePermission.idl
>-  boolean canSetCookie(in nsIURI     aURI,
>+  long canSetCookie(in nsIURI     aURI,
>                        in nsIChannel aChannel,
>                        in nsICookie2 aCookie,
>                        inout boolean aIsSession,
>                        inout PRInt64 aExpiry);

Nit: align those parameter.

Other then those nits, r=mvl

btw: do you know this will break camino? They implement their own cookie prompt
service. http://lxr.mozilla.org/mozilla/search?string=nsICookiePromptService
You should try to contact someone with a mac to fix this, and not leave camino
buster for too long :)

Comment 21

[Mike Connor [:mconnor]]

Attachment: with review comments

Comment 22

[Darin Fisher]

Comment on attachment 139172 [details] [diff] [review]
with review comments

please document change to nsICookiePermission.	comments say that it returns
true, but the type has been changed to boolean.

i think you should define const values on nsICookiePromptService for the
cookieDialog return values.  magic numbers are to be avoided ;-)

actually, i don't understand why you need to modify nsICookiePermission.  you
don't change any of the callsites, so why change the interface?  shouldn't the
implementation just treat all non-zero return values from cookieDialog as TRUE?

Comment 23

[Darin Fisher]

>please document change to nsICookiePermission.	comments say that it returns
>true, but the type has been changed to boolean.

i meant: "... but the type has been changed from boolean to long"

Comment 24

[Mike Connor [:mconnor]]

Attachment: new patch

I should have caught the magic numbers myself!	I just tested leaving aResult
as PRBool, works fine, no issues.

Comment 25

[Mike Connor [:mconnor]]

Comment on attachment 139174 [details] [diff] [review]
new patch

all good points, addressed here, tested and working

Comment 26

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

*** Bug 107813 has been marked as a duplicate of this bug. ***

Comment 27

[Darin Fisher]

Comment on attachment 139174 [details] [diff] [review]
new patch

sr=darin

Comment 28

[Mike Connor [:mconnor]]

checked in 01/28/2004 19:34 by bz

Comment 29

[Mike Pinkerton (not reading bugmail)]

what does it mean to accept session only but have "remember this decision"
checked? how does that go into the cookie sites black/white list? does it?

Comment 30

[Mike Connor [:mconnor]]

that's kind of ugly, actually.  There is a separate permission, that dwitte
didn't define a public value for (separate bug), but its value for
testPermission is 8.  Basically it works like accept, but downgrades all cookies
accepted from that site to session cookies.  Take a look at a current Mozilla
nightly to see how the menus/cookie manager show the permission.  bug 225857 is
where I implemented everything except the dialogs.

Comment 31

[Mike Pinkerton (not reading bugmail)]

this is what i see with this implemented in camino:

if i go to www.aol.com and choose the "session only" option with "remember" for
the first cookie, I get an error that says "redirection limit for this URL
exceeded probably because cookies are blocked" and in the cookie site list,
www.aol.com is marked as Deny, even after a restart.

is this expected?

Comment 32

[[e-mail]]

No, that's not expected.

mconnor, you broke camino :)

Comment 33

[Mike Connor [:mconnor]]

mmm, spiffy.  Unless the Camino cookie viewer is updated too, it'll probably 
say deny, due to the fun logic we had in cookieviewer where if it didn't match 
the allow perm, it would say it was blocked.  (Firebird still says this if you 
set the perm)

what does aol.com show as in cookperm.txt?  allow for session should be 0i (0T 
is allow, 0F is block).  If its actually showing as 0F there's something not 
being set properly.  I'll be around on IRC in like three hours or tomorrow 
morning.

Comment 34

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

So, I'm having a lot of trouble understanding the UI design here.  Why is
"Accept for this session only" a checkbox?  Once I check that box, am I supposed
to click "Allow" or "Deny"?  What happens for each?  Why isn't it just a button?
 Never mind the issue in comment 29...

Comment 35

[Mike Connor [:mconnor]]

part of why mvl and I eventually agreed on the checkbox was because a) it let
people with established browsing habits not have to adapt to a third accesskey,
since there were a number of people complaining about accepting with the
"remember this decision" checkbox overriding prefs in 1.6 (since the whitelist
perm overrides lifetime prefs).  The other part that convinced me was that it
clearly explained what checking it does (at least to the people I ran it by)
where a "session" button would be unclear.

re: comment 29, if you check the box, it accepts the cookie for the current
session only (read as: it downgrades the cookie).  If you also have "remember
this decision" checked,  it then always accepts cookies for the current session
only (downgrades).

Comment 36

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

I agree with dbaron. I didn't think of the combination "[v] accept for this
session only" and "reject". Those clearly conflict.
So, turning it into a button would be good, but the wording is problematic
there. does a button "Downgrade" make sense? "Session" doesn't imo.
Getting used to a new accesskey isn't that much of a problem, as it is a new
feature.
I'm not sure where the button should be on the mac. Will using the platform
overlay work?

Comment 37

[Mike Connor [:mconnor]]

using the unconvential overlay pattern, I can make it work, yes

reopening, but we still need to figure out a wording though

Comment 38

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

"Accept Temporarily"?

Comment 39

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

or perhaps "Accept Until Quit"?

Comment 40

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Actually, never mind "Accept Until Quit".  I was trying to be more precise, but
it could be interpreted in many other ways.

Comment 41

[Mike Pinkerton (not reading bugmail)]

i'm still trying to figure out what i'm missing to get this working in camino. i
stepped through the code and it's setting everything correctly as far as i can
tell. Am i missing setting some pref so necko knows about session-based cookies?

it just seems to be aol.com. Ideas?

Comment 42

[Mike Pinkerton (not reading bugmail)]

if i comment out |*aIsSession = PR_TRUE| in nsCookiePermission::CanSetCookie,
then it works again. Still groping around...

Comment 43

[Mike Connor [:mconnor]]

Attachment: Patch with Allow/Allow for Session/Block options

The overlay on OS X will turn this into Allow for Session/Allow/Block, but if
that's the convention, I can live with it.

Comment 44

[[e-mail]]

Comment on attachment 142543 [details] [diff] [review]
Patch with Allow/Allow for Session/Block options

>   document.getElementById("ok").label = dialog.getAttribute("acceptLabel");
>   document.getElementById("ok").accessKey = dialog.getAttribute("acceptKey");
>+  document.getElementById("Button2").label = dialog.getAttribute("extra1Label");
>+  document.getElementById("Button2").accessKey = dialog.getAttribute("extra1Key");  
>   document.getElementById("cancel").label = dialog.getAttribute("cancelLabel");
>   document.getElementById("cancel").accessKey = dialog.getAttribute("cancelKey");
You're obviously anticipating a bug for <dialog> to do this for you ;-)

Comment 45

[Mike Connor [:mconnor]]

Comment on attachment 142543 [details] [diff] [review]
Patch with Allow/Allow for Session/Block options

jst, since dbaron is in France and all, can you get to this one?

Comment 46

[Johnny Stenback (:jst)]

Comment on attachment 142543 [details] [diff] [review]
Patch with Allow/Allow for Session/Block options

sr=jst

Comment 47

[Mike Connor [:mconnor]]

Fixed, again!