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 :)

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 on attachment 138855 [details] [diff] [review] patch v1 in light of that other bug, I'd like some feedback ASAP :)

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 :)

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

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?

> 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

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 on attachment 138922 [details] [diff] [review] patch v3 still feeling brave? :)

on Mac, flip allow and deny

> 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)

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 :)

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.

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?

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 :)

the entire dialog will get used by power users/gun-toting privacy nuts anyway, I just want sanity :) should have a patch up later

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

Comment on attachment 139002 [details] [diff] [review] patch v4 with reworked dialog I do actually like this method better, good random thought!

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 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?

>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"

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

Comment on attachment 139174 [details] [diff] [review] new patch all good points, addressed here, tested and working

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

Comment on attachment 139174 [details] [diff] [review] new patch sr=darin

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

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?

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.

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?

No, that's not expected. mconnor, you broke camino :)

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.

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...

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).

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?

using the unconvential overlay pattern, I can make it work, yes reopening, but we still need to figure out a wording though

"Accept Temporarily"?

or perhaps "Accept Until Quit"?

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

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?

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

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 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 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 on attachment 142543 [details] [diff] [review] Patch with Allow/Allow for Session/Block options sr=jst

Fixed, again!