Last Comment Bug 233339 - rewrite CanSetCookie to reflect what dialogs now do
: rewrite CanSetCookie to reflect what dialogs now do
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Networking: Cookies (show other bugs)
: Trunk
: All All
: P2 normal (vote)
: mozilla1.7alpha
Assigned To: Mike Connor [:mconnor]
:
: Patrick McManus [:mcmanus]
Mentors:
Depends on:
Blocks: 64336 114131 216743 222561
  Show dependency treegraph
 
Reported: 2004-02-07 00:43 PST by Mike Connor [:mconnor]
Modified: 2004-03-10 15:39 PST (History)
12 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
first crack at this (13.29 KB, patch)
2004-02-07 00:51 PST, Mike Connor [:mconnor]
no flags Details | Diff | Splinter Review
sample of possible prefs (30.77 KB, image/png)
2004-02-07 01:11 PST, Mike Connor [:mconnor]
neil: review+
Details
the real deal (27.76 KB, patch)
2004-02-07 14:14 PST, Mike Connor [:mconnor]
dwitte: review+
Details | Diff | Splinter Review
with dwitte's review comments (27.76 KB, patch)
2004-02-10 23:01 PST, Mike Connor [:mconnor]
no flags Details | Diff | Splinter Review
with dwitte's comments and the wording tweak (27.77 KB, patch)
2004-02-15 14:48 PST, Mike Connor [:mconnor]
no flags Details | Diff | Splinter Review
new patch, including migration for old prefs (29.37 KB, patch)
2004-02-28 01:32 PST, Mike Connor [:mconnor]
dwitte: review+
Details | Diff | Splinter Review
patch with review comments (29.52 KB, patch)
2004-03-02 18:02 PST, Mike Connor [:mconnor]
darin.moz: superreview+
Details | Diff | Splinter Review
temp patch for FF (7.50 KB, patch)
2004-03-09 23:29 PST, Mike Connor [:mconnor]
no flags Details | Diff | Splinter Review
with better comments (7.72 KB, patch)
2004-03-10 00:06 PST, Mike Connor [:mconnor]
no flags Details | Diff | Splinter Review

Description Mike Connor [:mconnor] 2004-02-07 00:43:04 PST
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)
Comment 1 Mike Connor [:mconnor] 2004-02-07 00:51:40 PST
Created attachment 140791 [details] [diff] [review]
first crack at this

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).
Comment 2 Mike Connor [:mconnor] 2004-02-07 01:11:44 PST
Created attachment 140792 [details]
sample of possible 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 3 neil@parkwaycc.co.uk 2004-02-07 05:00:35 PST
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 :-)
Comment 4 Michiel van Leeuwen (email: mvl+moz@) 2004-02-07 07:12:24 PST
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)
Comment 5 neil@parkwaycc.co.uk 2004-02-07 07:27:33 PST
But the "Ask" dialog already has a session checkbox...
Comment 6 Mike Connor [:mconnor] 2004-02-07 08:00:04 PST
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.
Comment 7 Mike Connor [:mconnor] 2004-02-07 14:14:10 PST
Created attachment 140824 [details] [diff] [review]
the real deal

full patch for this, preliminary testing shows everything a-ok
Comment 8 Mike Connor [:mconnor] 2004-02-07 14:17:30 PST
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
Comment 9 Mike Connor [:mconnor] 2004-02-07 14:20:40 PST
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.
Comment 10 HJ 2004-02-07 16:40:14 PST
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?
Comment 11 HJ 2004-02-07 16:45:03 PST
s/expression/impression

Oh, and I would like to keep thw mailnews cookie pref.
Comment 12 neil@parkwaycc.co.uk 2004-02-07 16:53:58 PST
Either that or <label value=... accesskey=... for="lifetimeDays"/>
Unfortunately I don't think you can have both for and control.
Comment 13 Mike Connor [:mconnor] 2004-02-07 20:15:41 PST
> 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.
Comment 14 Michiel van Leeuwen (email: mvl+moz@) 2004-02-08 02:32:12 PST
It seems the ask pref isn't very clear. Maybe you should move it, or change the
workding somewhat. 'Ask for each cookie' maybe?
Comment 15 Mike Connor [:mconnor] 2004-02-08 08:23:08 PST
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!
Comment 16 HJ 2004-02-08 12:09:17 PST
(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!
Comment 17 Mike Connor [:mconnor] 2004-02-08 12:22:32 PST
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.
Comment 18 Michiel van Leeuwen (email: mvl+moz@) 2004-02-08 13:14:39 PST
But without remembering the decision, asking and then limit the lifetime does
make sense. Man, this is getting too complex to be funny.
Comment 19 Mike Connor [:mconnor] 2004-02-09 13:23:59 PST
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 20 dwitte@gmail.com 2004-02-10 19:43:45 PST
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.
Comment 21 Mike Connor [:mconnor] 2004-02-10 20:14:09 PST
(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)

Comment 22 dwitte@gmail.com 2004-02-10 20:17:49 PST
>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.
Comment 23 Mike Connor [:mconnor] 2004-02-10 22:32:51 PST
that's right... that'll teach me to respond to stuff when I'm sleepy and at
work!  revised patch in 5 minutes
Comment 24 Mike Connor [:mconnor] 2004-02-10 23:01:32 PST
Created attachment 141130 [details] [diff] [review]
with dwitte's review comments
Comment 25 Mike Connor [:mconnor] 2004-02-10 23:04:50 PST
Comment on attachment 141130 [details] [diff] [review]
with dwitte's review comments

/me crosses fingers (probably) in vain
Comment 26 Mike Pinkerton (not reading bugmail) 2004-02-11 17:51:14 PST
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).
Comment 27 Mike Connor [:mconnor] 2004-02-11 18:20:15 PST
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>
Comment 28 Mike Connor [:mconnor] 2004-02-14 18:53:37 PST
bit of a basic document on the new pref values and usage:
http://www.mozilla.org/projects/netlib/cookies/cookie-prefs.html
Comment 29 Neil Pryde 2004-02-14 20:33:54 PST
network.cookie.lifetimePolicy  /  "Prompt for each cookie"

How can we set it to prompt AND accept cookies for N days?
Comment 30 Mike Connor [:mconnor] 2004-02-14 23:26:11 PST
Neil: see comment 17 in this bug
Comment 31 Neil Pryde 2004-02-15 00:15:39 PST
(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?
Comment 32 Mike Connor [:mconnor] 2004-02-15 00:37:37 PST
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.
Comment 33 Michiel van Leeuwen (email: mvl+moz@) 2004-02-15 03:22:24 PST
(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 :)
Comment 34 HJ 2004-02-15 04:23:17 PST
(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?
Comment 35 Michiel van Leeuwen (email: mvl+moz@) 2004-02-15 04:34:33 PST
(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.
Comment 36 Mike Connor [:mconnor] 2004-02-15 14:48:24 PST
Created attachment 141486 [details] [diff] [review]
with dwitte's comments and the wording tweak
Comment 37 Mike Connor [:mconnor] 2004-02-19 14:12:17 PST
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 38 Mike Connor [:mconnor] 2004-02-28 01:32:06 PST
Created attachment 142515 [details] [diff] [review]
new patch, including migration for old prefs
Comment 39 dwitte@gmail.com 2004-03-02 00:47:41 PST
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.
Comment 40 Mike Connor [:mconnor] 2004-03-02 05:29:09 PST
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.
Comment 41 Mike Connor [:mconnor] 2004-03-02 18:02:09 PST
Created attachment 142799 [details] [diff] [review]
patch with review comments

also, I didn't mention it in the bug, but removing the mailnews pref UI has
moa=sspitzer via email
Comment 42 Darin Fisher 2004-03-09 11:38:34 PST
>// 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.
Comment 43 Mike Connor [:mconnor] 2004-03-09 13:13:13 PST
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.
Comment 44 Darin Fisher 2004-03-09 17:56:10 PST
OK, after talking with mconner over IRC and thinking about this some more, I
think I understand now.  Reviewing...
Comment 45 Darin Fisher 2004-03-09 18:28:30 PST
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)
Comment 46 Mike Connor [:mconnor] 2004-03-09 23:29:35 PST
Created attachment 143469 [details] [diff] [review]
temp patch for FF

this is solely to prevent breakage, this will need to be replaced, but blake
hasn't gotten back to me
Comment 47 Ben Goodger (use ben at mozilla dot org for email) 2004-03-09 23:37:12 PST
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.
Comment 48 Mike Connor [:mconnor] 2004-03-10 00:06:02 PST
Created attachment 143471 [details] [diff] [review]
with better comments
Comment 49 Mike Connor [:mconnor] 2004-03-10 00:25:54 PST
bug 236980 filed for the followup to review prefs for Firefox

fix checked in 03/09/2004 22:47
Comment 50 Steffen Wilberg 2004-03-10 13:00:10 PST
Comment on attachment 143471 [details] [diff] [review]
with better comments

>Index: mozilla/browser/base/content/browser.js
>+  // removed 03/09/2003
2003 ???
Comment 51 Christian :Biesinger (don't email me, ping me on IRC) 2004-03-10 13:19:34 PST
(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)
Comment 52 Mike Pinkerton (not reading bugmail) 2004-03-10 14:02:40 PST
so now that this has landed, i have to update camino? a bunch of cookie stuff no
longer works :)
Comment 53 Mike Connor [:mconnor] 2004-03-10 15:39:25 PST
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 :)

Note You need to log in before you can comment on or make changes to this bug.