Neil proposed a UI along these lines in the lifetime pref bug, and it got me thinking that maybe its the best way of dealing with this now. Right now, we've got some really quirky behaviour whereby if you have the downgrade to session pref set, you can't accept a cookie permanently, although you can set the permission to allow this to happen in the future. I have been thinking that separating the prompts from the lifetime prefs is the best way to go (since if we make it three buttons, the only situation we can't have is letting the "accept persistent" button be limited to X days, but if you remember the decision this isn't going to happen anyway, so either we need to change that or this. My thinking is that this is a better plan.) Neil's idea was along these lines: ( ) Accept Normally ( ) Accept for current session only ( ) Accept for [ ] days ( } Ask before accepting [ ] except for session cookies obviously this would require either a lot of monkeying with XUL, or simplifying the lifetime policy to an integer pref and migrating the previous prefs. The XUL option seems less clean, based on some preliminary hacking in the backend. This will break Firebird's prefs panel, but I can write that fix, the current Firebird UI doesn't reflect how cookies work these days anyway (neither does the seamonkey prefs for that matter)

I need to patch this into a clean tree and rebuild, no promises on whether it builds cleanly :) the part I'd want to look at is what pref combinations would get moved to which policy. If the user has "ask before accepting" set, we should use that, but if they're not then migrate to their limited lifetime prefs. (For most users, this wouldn't even be noticed since its just lifetime prefs).

eww, radiobuttons but, it provides a ton of flexibility, and if we do end up removing the lifetime to n days pref, its easy enough to cut out

Comment on attachment 140792 [details] sample of possible prefs I like your wording - assuming that the missing space between session and only is just a typo :-)

I don't see how 'Ask before accepting' has something to do with lifetime. You can combine it with the other options. Ask, but still limit to 30 days. (or for the real paranoid, to the session only)

But the "Ask" dialog already has a session checkbox...

if you're using Ask before accepting, the cookie will either be accepted permanently or for the session only. The session perm is therefore useless in conjunction with dialogs now. Either we make the "accept permanently" option/permission subject to the n days pref (which means that there's no way to allow a cookie for longer than n days without adding yet another whitelist permission) or we don't allow the combination. If we allow the combination, we make the whitelist concept fuzzier, since it overrides all the prefs EXCEPT the limit lifetime to n days option.

full patch for this, preliminary testing shows everything a-ok

Comment on attachment 140824 [details] [diff] [review] the real deal note that I still don't like the lifetime pref, but I don't think we have enough consensus to go forward with removing it yet other than cosmetic nits, this is the UI in the screenshot

ccing mscott for moa on hiding the mailnews cookie pref. Scott, I know this isn't in Thunderbird, do we still need it exposed for MailNews? Obviously we can leave the pref intact for those cases where its needed, but I haven't seen a situation where we'd want it enough to expose the option.

My first expression: Good to see that you replaced 'Enable/Disable' with 'Allow/Block'. +pref("network.cookie.lifetimePolicy", 0); // accept normally, 1-askBeforeAccepting, 2-acceptForSession,3-acceptForNDays What has 'askBeforeAccepting' to do with life time policy? Don't you agree that using 'Accept' is a bit confusing? First I have to select 'Allow ...' cookies and the next question is wether to 'Accept ...'. That's a no go if you ask me. + <textbox id="lifetimeDays" pref="true" size="4" + preftype="int" prefstring="network.cookie.lifetime.days"/> + <description>&days.label;</description> Please use: <label value="&days.label;" control="lifetimeDays"/> Because this has two advantages: 1) this aligns the text 'days' with the textbox 2) clicking on 'days' switches focus to the textbox The Cookie Manager button should be part of the first groupbox. Btw, why isn't there an option to disable cookies altogether?

s/expression/impression Oh, and I would like to keep thw mailnews cookie pref.

Either that or <label value=... accesskey=... for="lifetimeDays"/> Unfortunately I don't think you can have both for and control.

> What has 'askBeforeAccepting' to do with life time policy? the dialogs set explict acceptance now, they don't work with the limit lifetime prefs very well from 1.6 onwards, so rather than break what we've already done with whitelisting to make these prefs once again supersede the whitelist, we're making the dialogs set the expiry based on user choice (session or persistent). (they already do this, we're just removing the downgrades from that codepath) > Don't you agree that using 'Accept' is a bit confusing? First I have to select > 'Allow ...' cookies and the next question is wether to 'Accept ...'. That's a no > go if you ask me. I've been thinking about this, and I don't think its a contradiction at all. > + <textbox id="lifetimeDays" pref="true" size="4" > + preftype="int" prefstring="network.cookie.lifetime.days"/> > + <description>&days.label;</description> > > Please use: <label value="&days.label;" control="lifetimeDays"/> that's trivial, I can do that later > The Cookie Manager button should be part of the first groupbox. why? the cookie manager permissions list governs both sets of prefs, it shouldn't be associated with one or the other > Btw, why isn't there an option to disable cookies altogether? because there's no pref to disable cookies completely. If people don't want cookies, don't whitelist sites. The ability to ignore the whitelist temporarily would be good for development/research, but adding a pref for it doesn't make sense for end-users. the mailnews cookie pref still exists, but do we really need UI for it? Personally, I feel the potential harm to users outweighs any benefits that might be found. Enterprises deploying Mozilla that need it could use it still, but typical users shouldn't enable cookies in mailnews. Thunderbird doesn't have an option to undo this, once there's a --disable-cookies build option they will shift to that. That's a pretty good case for starting to drop support for it in the suite as well.

It seems the ask pref isn't very clear. Maybe you should move it, or change the workding somewhat. 'Ask for each cookie' maybe?

moving isn't feasible, since it is a unified pref now "Ask for each cookie" is what I'd come up with independently. so the label bit mentioned by HJ/Neil and the wording tweak are the two things I'm looking to change at this point, but I don't have access to the tree I'm working on this in atm. dwitte and darin, please assume that I will be making those changes when reviewing this patch. Thanks!

(In reply to comment #15) > moving isn't feasible, since it is a unified pref now Are you saying that from now on its impossible to use 'Acccept cookies for xx days' AND 'Ask before accepting'? Man that sucks!

HJ, even without this patch, "remember my decision" whitelists the site, meaning it overrides lifetime prefs. So unless you never remember the decision, it doesn't work together (except for the very first time you go to the site). Its that way in 1.6 too, although the UI doesn't reflect that. Hence this bug and this patch. The only way that we could do both would be to not let whitelisted cookies override lifetime settings, which isn't the direction we've been moving in.

But without remembering the decision, asking and then limit the lifetime does make sense. Man, this is getting too complex to be funny.

this will break Camino and FB prefs, I've already talked to pch about it for FB, but Camino needs to be updated (and I don't have a Mac or know Cocoa :)

Comment on attachment 140824 [details] [diff] [review] the real deal >Index: mozilla/extensions/cookie/nsCookiePermission.cpp >=================================================================== >+static const nsInt64 ACCEPT_NORMALLY = 0; >+static const nsInt64 ASK_BEFORE_ACCEPT = 1; >+static const nsInt64 ACCEPT_SESSION = 2; >+static const nsInt64 ACCEPT_FOR_N_DAYS = 3; why are these nsInt64's? >+ nsInt64 delta; >+ nsInt64 currentTime = NOW_IN_SECONDS; >+ delta = nsInt64(*aExpiry) - currentTime; you can shift the 3rd line onto the 1st. but i don't see how this is used in any but the final case, so why put it up here? >+ // we're not prompting, so we must be limiting the lifetime somehow >+ // if its not a session cookie, we do nothing you mean "if it is a session cookie, we do nothing"? >+ } else if (delta > mCookiesLifetimeSec) { >+ // limit lifetime to specified time >+ *aExpiry = currentTime + mCookiesLifetimeSec; >+ } indentation nit on that second line. >Index: mozilla/extensions/cookie/nsCookiePermission.h >=================================================================== >+ nsInt64 mCookiesLifetimePolicy; // the policy for the another nsInt64 here...? r=dwitte, with those fixed.

(In reply to comment #20) > (From update of attachment 140824 [details] [diff] [review]) > >Index: mozilla/extensions/cookie/nsCookiePermission.cpp > >=================================================================== > >+static const nsInt64 ACCEPT_NORMALLY = 0; > >+static const nsInt64 ASK_BEFORE_ACCEPT = 1; > >+static const nsInt64 ACCEPT_SESSION = 2; > >+static const nsInt64 ACCEPT_FOR_N_DAYS = 3; > why are these nsInt64's? because I have mCookieLifetimePolicy declared as nsInt64 (don't ask why, I don't remember where that came from). I should have fixed that, not these. I'll change this and the .h file to nsInt32 when I get home in another two hours. > >+ nsInt64 delta; > >+ nsInt64 currentTime = NOW_IN_SECONDS; > >+ delta = nsInt64(*aExpiry) - currentTime; > you can shift the 3rd line onto the 1st. but i don't see how this is used in > any but the final case, so why put it up here? not seen in the diff, but it gets used in the prompting option too (comment talks about accepting it here so there's proper logging/notifications) I'll combine the lines anyway (this is how it was originally, I just moved it elsewhere) > >+ // we're not prompting, so we must be limiting the lifetime somehow > >+ // if its not a session cookie, we do nothing > you mean "if it is a session cookie, we do nothing"? yes, fixed in my tree long ago, thought it was fixed before I uploaded the diff > >+ } else if (delta > mCookiesLifetimeSec) { > >+ // limit lifetime to specified time > >+ *aExpiry = currentTime + mCookiesLifetimeSec; > >+ } > indentation nit on that second line. > >Index: mozilla/extensions/cookie/nsCookiePermission.h > >=================================================================== > >+ nsInt64 mCookiesLifetimePolicy; // the policy for the > another nsInt64 here...? see first comment (the perils of lack of sleep, I knew there would be something silly)

>I'll change this and the .h file to nsInt32 when I get home in another two >hours. i think you want PRUint32 (there is no nsInt32). PRUint32 is unsigned 32-bit, PRInt32 is signed 32-bit.

that's right... that'll teach me to respond to stuff when I'm sleepy and at work! revised patch in 5 minutes

Comment on attachment 141130 [details] [diff] [review] with dwitte's review comments /me crosses fingers (probably) in vain

i'm trying to follow this, but am having a hard time. camino doesn't want the majority of these options (accept for N days?! talk about a UI nightmare). What prefs do i have to change to just get what i already have today? That is, enable or disable cookies entires, and an option to ask about each cookie, which can be accepted or denied (accept session-only doesn't work in camino for unknown reasons).

Mike, basically the the pref for enable/disable is the same, just the UI is renamed for clarity. For enabling dialogs, the pref network.cookie.lifetimePolicy should be set to 1 for on and 0 for off. (2 just limits lifetime to session, 3 limits lifetime to N days in conjunction with the other pref) I do need to document all of these somewhere on m.o soon, once this gets into the trunk of course. <offtopic> I hate the N days pref too, but that's a separate battle that I'm not currently winning</offtopic>

bit of a basic document on the new pref values and usage: http://www.mozilla.org/projects/netlib/cookies/cookie-prefs.html

network.cookie.lifetimePolicy / "Prompt for each cookie" How can we set it to prompt AND accept cookies for N days?

Neil: see comment 17 in this bug

(In reply to comment #17) > The only way that we could do both would be to not let whitelisted cookies > override lifetime settings, which isn't the direction we've been moving in. I don't understand why this should work this way. Whitelisted cookies can easily overwrite existing cookies. Why not? The lifetime setting was only available to remove cookies after N days, not to keep them as long as N days. Maybe I missed it but care to clarify your comment for me?

whitelisted sites already ignore the lifetime settings (either session or N days). The idea behind this is that if, for example, you wanted to allow Site X to set persistent cookies, but all others to current session or N days, then you are able to do this. The only way to allow whitelisting and N days to co-exist properly is to make the whitelisted sites subject to the N days pref, whereby you could not use the whitelisted site permission to override that pref (this is how Mozilla 1.5 and earlier behaved). Since this isn't why we implemented the behaviour in question (which already exists) we're not going to do this. Revising the UI and the pref behaviour just separates out the cases that don't work properly together at the current time (even in 1.6). Either we need to revert to 1.5 behaviour, which a lot of users I know would consider a major regression, or we move forward and clarify the pref behaviour we are going to support going forward.

(In reply to comment #29) > How can we set it to prompt AND accept cookies for N days? What if we make the 'accept session' button (its now a checkbox, but should be a button) to make the lifetime either limited to session, or to n days, depending on a hidden pref? That same pref will control what the limit to session in the pref means: session or a few days. We could change the wording from Session to Temporary or something like that. Users don't know what session means anyway :)

(In reply to comment #32) > whitelisted sites already ignore the lifetime settings (either session or N > days). Clearly a design error. The Life Time setting should always have a higher priority. Also, there's still this 300 cookies limit. How long will it take the average joe user to blow away his whitelisted cookies? Well, what do you think?

(In reply to comment #34) > The Life Time setting should always have a higher priority. That is not how the whitelist was designed. You whitelist sites that you allow to set cookies. This are sites you trust, so can set cookies how they like. For sites you don't trust, you set a default policy, which might be to allow them to set cookies, but only for a limited amount of time (current session of a few days). So, the whitelist should overrule the default policy. > Also, there's still this 300 cookies limit. How long will it take the average > joe user to blow away his whitelisted cookies? Well, what do you think? That is soooo another bug. Comment in there why you want more cookies. It's currently 300 because that is what the spec says, and there havent been complaints that is isn't enough. It limited to prevent you from DoS attacks. (unlikely, but still possible). Anyway, not this bug.

this also will have a separate patch for migrating to the new prefs (should be attached around Saturday). basic logic I intend to use is as follows: If ask before accepting is enabled, use that. If it isn't, but lifetime limiting is on, migrate to the appropriate pref. I'm debating whether to migrate this, and set a "migrationDone" type of pref, or just nuke the old prefs. Both have merits, of course, but the ugly thing is that if you go back to 1.6 and change settings, should we re-migrate? I personally don't think so, but this isn't really a big concern for me either way, but I think some migration is necessary for end-users.

Comment on attachment 142515 [details] [diff] [review] new patch, including migration for old prefs >Index: mozilla/extensions/cookie/nsCookiePermission.cpp >=================================================================== >+static const char kCookiesLifetimePolicy[] = "network.cookie.lifetimePolicy"; humm, i'm not too sure i like the lifetimePolicy name, since you're cunningly shuffling the "ask" pref in there too. then again, your UI does the same thing, and "ask" is now kinda related to lifetime since it does allow the user to set the lifetime per-cookie... i guess this is ok. now, more generally, i do feel it's less "clean" to have these different functions randomly mixed together in a general "how do you want cookies to behave?" kind of pref. and it adds migration issues. what advantage does changing the prefs themselves give, over munging some XUL to interface your desired UI to our current prefs? see the prefpanel patch in bug 221078 for an example of how to do this (and also bug 221078 comment 15, pch made the patch better when he checked in). both solutions are semi-ugly, but munging XUL doesn't introduce migration woes. that said, i'll continue reviewing... >+ PRBool migrated; >+ prefBranch->GetBoolPref(kCookiesPrefsMigrated, &migrated); >+ if (!migrated) { save the rv of the GetBoolPref call, and then do |if (NS_FAILED(rv) || !migrated)|, to catch the failure case. >+ // declare this here since it'll be used in any of the remaining cases s/any/all/ please > // check whether the user wants to be prompted >- if (mCookiesAskPermission) { >+ if (mCookiesLifetimePolicy == ASK_BEFORE_ACCEPT) { >+ // if its a session cookie and the user wants to accept these >+ // without asking just accept the cookie and return s/its/it's/, s/asking just/asking, just/ >+ // we're not prompting, so we must be limiting the lifetime somehow >+ // if it's a session cookie, we do nothing >+ if (!*aIsSession) { >+ if (delta > nsInt64(0)) { merge these two conditions into one |if|. i think you also need to set |*aResult = PR_TRUE;| somewhere inside this block, no? >Index: mozilla/extensions/cookie/nsCookiePermission.h >=================================================================== >+ : mCookiesLifetimePolicy(LL_MAXINT) i think you want to init it to 0 here. >+ PRUint32 mCookiesLifetimePolicy; // the policy for the > nsInt64 mCookiesLifetimeSec; // lifetime limit specified in seconds >+ PRPackedBool mCookiesAlwaysAcceptSession; // don't prompt for session cookies >+#ifdef MOZ_MAIL_NEWS >+ PRPackedBool mCookiesDisabledForMailNews; > #endif shift |mCookiesLifetimePolicy| down one, and make it a PRUint8, so it packs better with the other two 8-bit members. (you'll have to change the ordering in the ctor too) >Index: mozilla/extensions/cookie/resources/content/pref-cookies.xul >=================================================================== > var cookieBehavior = document.getElementById("networkCookieBehaviour"); chuckle, nice spelling inconsistency :) r=dwitte, but i want to see some solid reasoning on why migrating prefs is the way to go, instead of mangling XUL, before this lands.

Reasoning for migration vs. munging: 1) Trying to munge the prefs together just means that we kill backwards compatibility for profiles. For Firebird this wasn't an issue, being in pre-1.0 timeframe, but if a user moves to 1.7, decides to go back to 1.6 or 1.4 or whatever, their settings are changed. 2) If we're going to support four distinct options (accept as sent, accept for session, accept for N days, and ask for each cookie) then using a unified pref prevents people from using user.js and about:config to change things (unless you're planning on munging the existing prefs on each load, which is equally ugly). Either way we're processing prefs on startup to prevent unwanted combinations. Or, we're adding code to check for the case where what should now be mutually exclusive prefs are both set. All around its ugly, but migrating allows backwards compatibility, and avoids forcing embeddors to have to munge everything themselves as well. I don't see any advanatage to retaining the old-style prefs if we're going to have to munge them ourselves and force embeddors to munge them going forward. Eventually we can drop the migration code, and then we're left with a logicial pref structure.

also, I didn't mention it in the bug, but removing the mailnews pref UI has moa=sspitzer via email

>// values for mCookiesLifetimePolicy >// 0 == accept normally >// 1 == ask before accepting >// 2 == downgrade to session >// 3 == limit lifetime to N days option 1 seems out of place. option 2 and 3 describe the amount of time a cookie can be saved by the browser. but option 1 has nothing to do with time. this seems wrong to me.

The current dialogs impl actually dictates how to accept the cookie (downgrade to session or accept normally). I should change "ask before accepting" to "ask for each cookie" in the comment (its changed in the UI portion of the patch). The only part that doesn't fit into the "lifetime" part of the pref is the ability to block cookies completely. "Ask before accepting" isn't really the nature of the beast anymore. If you want to discuss this a bit further, I'll be home in 2.5 hours.

OK, after talking with mconner over IRC and thinking about this some more, I think I understand now. Reviewing...

Comment on attachment 142799 [details] [diff] [review] patch with review comments >Index: mozilla/extensions/cookie/nsCookiePermission.cpp >+// values for mCookiesLifetimePolicy >+// 0 == accept normally >+// 1 == ask before accepting >+// 2 == downgrade to session >+// 3 == limit lifetime to N days >+static const PRUint32 ACCEPT_NORMALLY = 0; >+static const PRUint32 ASK_BEFORE_ACCEPT = 1; >+static const PRUint32 ACCEPT_SESSION = 2; >+static const PRUint32 ACCEPT_FOR_N_DAYS = 3; nit: i'd use an enum for these personally, but it doesn't matter. perhaps these could be called: enum { COOKIE_LIFETIME_DEFAULT, COOKIE_LIFETIME_PROMPT, COOKIE_LIFETIME_SESSION, COOKIE_LIFETIME_N_DAYS }; >-#ifdef MOZ_PHOENIX >-static const char kCookiesLifetimeEnabled[] = "network.cookie.enableForCurrentSessionOnly"; do you have approval from Ben or one of the other FFox devs to break FFox cookie prefs? i agree they deserve pain for forking cookies, but perhaps we should make our code support either of the lifetime prefs when in migration mode. that shouldn't be too hard or too bloaty. >+ // migration code for original cookie prefs >+ // we can probably get rid of this around 1.9a timeframe this is a bad assumption. i think you will have to support folks transitioning from pre-1.7 for many years to come. >+ PRBool migrated; >+ rv = prefBranch->GetBoolPref(kCookiesPrefsMigrated, &migrated); hmm... maybe this should be a pref version number instead? that way we don't need a kCookiesPrefMigrated2 or whatever! ;-) >+ if (NS_FAILED(rv) || !migrated) { >+ PRBool warnAboutCookies = PR_FALSE; >+ prefBranch->GetBoolPref(kCookiesAskPermission, &warnAboutCookies); >+ >+ // if the user is using ask before accepting, we'll use that >+ if (warnAboutCookies) >+ prefBranch->SetIntPref(kCookiesLifetimePolicy, ASK_BEFORE_ACCEPT); /me notes that you sometimes check |rv| returned by GetBoolPref and sometimes not. might be good to be consistent. >Index: mozilla/extensions/cookie/resources/content/pref-cookies.xul >+ var _elementIDs = ["networkCookieBehavior", "networkCookieLifetime", >+ "alwaysAcceptSession", "lifetimeDays"]; nit: it seems like these element IDs should all follow the same naming convention. >+ const accept_normally = "0"; >+ const accept_session = "2"; >+ const accept_for_n_days = "3"; >+ const ask_before_accepting = "1"; yeah... in my mind, these appear in this order. since we are migrating prefs, why not change the enumeration to be in this order? it's probably important to land the localization changes before the beta freeze tonight. though i'd really like to see this patch delayed until a solution for firefox exists, i'll leave that decision up to the firefox maintainers. sr=darin (with the issues addressed)

this is solely to prevent breakage, this will need to be replaced, but blake hasn't gotten back to me

Comment on attachment 143469 [details] [diff] [review] temp patch for FF hrm - why is the UI fix "temporary"... darin explained in such a way that this seemed more logical. What is seamonkey doing here? I'm going to need better documentation in the migration code in browser .js file too - documentation that actually explains what's happening.

bug 236980 filed for the followup to review prefs for Firefox fix checked in 03/09/2004 22:47

Comment on attachment 143471 [details] [diff] [review] with better comments >Index: mozilla/browser/base/content/browser.js >+ // removed 03/09/2003 2003 ???

(In reply to comment #50) > (From update of attachment 143471 [details] [diff] [review]) > >Index: mozilla/browser/base/content/browser.js > >+ // removed 03/09/2003 Can you use some kind of different date format. this one is ambiguous (is it sept 3 or march 9)

so now that this has landed, i have to update camino? a bunch of cookie stuff no longer works :)

Steffen, it was after 3 AM, I'd been up since 7 AM, so I missed a minor detail like the correct year... :) biesi, I just used the format pch was already using in that function, I might clean that up when I write the "better" patch for bug 236980 mike, yes, that's why I cced you on this way back when :)