121161 - Enhancements to p3p cookie management

Status: RESOLVED FIXED

(Core :: Networking: Cookies, enhancement)

Description

[Stephen P. Morse]

Make the following enhancements to the currently-implemented p3p cookie 
management feature:

1. Currenltly the user can specify three different actions in the P3P pref 
panel.  They are accept, downgrade, and reject.  There shall now be a fourth 
action, namely flag.  Flagged cookies are accepted by the browser and treated in 
all ways like an accepted cookie.  Except the browser shall remember for the 
duration of the current session which cookies are flagged.

2. If P3P is enabled (i.e., the "enable cookies based on privacy levels" option 
is selected in the cookie preference panel), and any cookies have been 
downgraded or flagged in the current session, a cookie icon will appear on the 
status line next to the lock icon.

3. Clicking on the cookie icon on the status line will cause the cookie-manager 
dialog to open.

4. The cookie-manager dialog shall be extended to include two additional columns 
for each cookie.  These columns are "status" and "policy".

5. The status column shall indicate by a single letter whether the cookie was 
accepted, downgraded, or flagged in the current session.  Specifically,

  blank = accepted in a previous session
  a = accepted in the current session and not downgraded or flagged
  d = downgraded in the current session
  f = flagged in the current session

6. The policy column shall indicate by a numeric value the "goodness" of the 
site's compact policy, on a scale of 0 to 4.  Specifically,

   blank = don't know about compact policy because cookie was from a previous
           session
   0 = site has no compact policy
   1 = site doesn't get user consent before storing identifying info in cookies
   2 = site store's such info but allows user to opt out (implicit consent)
   3 = site doesn't store such info unless user opts in (explicit consent)
   4 = site does not store identifying information in cookies

7. When you open the cookie-manager dialog from the tasks menu or fromm the 
cookie pref panel, the status and policy columns are initially not displayed.  
User can cause them to be displayed by clicking on the the symbol at the right 
of the column headings.

8. If you open the cookie-manager dialog by clicking on the cookie icon, the 
status and policy columns shall be initially displayed.  Furthermore, the 
cookies shall be sorted by the character appearing in the status column (in the 
ordering f, d, a, blank)

Comment 1

[Stephen P. Morse]

Corrections:

5. The values appearing in the status column shall be "", "accepted", "session", 
and "flagged" instead of the single characters ' ', 'a', 'd', 'f'.  Furthermore, 
these strings shall be localized as usual.

8. When opening the cookie manager via the cookie icon, the cookies shall be 
sorted in reverse order by the value in the status column.  Although that 
sorting is language-dependent, in all cases the blank value (not accepted in 
this session) will come last.  So in English, the ordering will be:

   "session"
   "flagged"
   "accepted"
   ""

Comment 2

[Stephen P. Morse]

Attachment: Patch using temporary gif file for now

Comment 3

[Stephen P. Morse]

Above patch implements the requirements.  However we do not yet have a cookie 
icon, so temporarily that patch uses an existing icon, namely the chatzilla one 
which is CZ (read it as CookieZ if you like).

alecf and harishd, please review.  Thanks.

Comment 4

[Stephen P. Morse]

Further modification to description.  The policy information shall go into a 
field in the properties section of the selected cookie rather than into a column 
in the list of cookies.  That involves changing the following items in the 
description as follows: 

4. The cookie-manager dialog shall be extended to include a status column for 
each cookie.  Furthermore, the set of properties for the selected cookie shall 
be extended to include a policy field.

6. The policy field shall indicate the site's policy regarding collecting 
identifiable information.  The field is left blank if the site has no policy.  
Otherwise the values that can go into this field are:

   no policy on storing identifiable information
   stores identifiable information without any user consent
   stores identifiable information unless user opts out
   stores identifiable information if user opts in
   does not store identifiable information

The policy field is not present if no cookies in the cookie list have any 
policies stored for them.

7 & 8. status and policy columns -> status column

Comment 5

[Stephen P. Morse]

Attachment: revised to address the changes in comment #4

Comment 6

[Alex Bishop]

> no policy on storing identifiable information
> stores identifiable information without any user consent
> stores identifiable information unless user opts out
> stores identifiable information if user opts in
> does not store identifiable information

I'd suggest changing those to:

No policy on storing personally identifiable information
Stores personally identifiable information without your consent
Stores personally identifiable information unless you opt out
Stores personally identifiable information only if you opt in
Does not store personally identifiable information

Does "stores identifiable information without any user consent", for example,
mean that all cookies the site sets with that policy will store personally
identifiable information? If not, adding "may" to the beginning of some of the
descriptions may be helpful.

The Privacy Levels dialogue uses "collects" rather than "stores". Are these two
 equivalent (IE uses "uses" throughout)?

Comment 7

[Stephen P. Morse]

Let's not get bogged down with the wording of the policy field.  Everybody's 
going to have his favorite way of saying it, and this patch is never going to 
get checked in.

The wording here is really unimportant.  I chose to make them short so that you 
can read them in the field without having to scroll the field.  Adding in the 
word "personally" does make it more understandable but also causes the field to 
need scrolling which in turn makes the end of the string unreadable.

The policy is received from the site with no indication of which cookies it 
applies to.  So the answer to your question about whether it applies to all 
cookies is unknown.  Yes, the word "may" would be more accurate, but again makes 
the string longer and the tail end unreadable.

"Collects" and "stores" are synonymous.  So is microsoft's "uses".  If you like, 
I'll change "stores" to "uses" since it makes the string still shorter.

Comment 8

[Alec Flett]

before I procede: morse, since it looks like you've formalized this pretty well,
can you put a [living] document up on www.mozilla.org to make it easier for us
to see the "latest" version of the p3p behavior/UI? I'm finding it hard to keep
up with all the changes you've made here

also, regarding the use of the chatzilla icon, can we just spend a few minutes
and put together a really really simple icon, even if it's just a picture of a
cookie or an image of the word "cookie"?

I'll try to look at the latest patch soon.

Comment 9

[Stephen P. Morse]

To make it easier to see what the modified behavior is, I'm including an updated 
description that incorporates the changes I described in comments 1 and 4.

1. Currently the user can specify three different actions in the P3P pref 
panel.  They are accept, downgrade, and reject.  There shall now be a fourth 
action, namely flag.  Flagged cookies are accepted by the browser and treated in 
all ways like an accepted cookie.  Except the browser shall remember for the 
duration of the current session which cookies are flagged.

2. If P3P is enabled (i.e., the "enable cookies based on privacy levels" option 
is selected in the cookie preference panel), and any cookies have been 
downgraded or flagged in the current session, a cookie icon will appear on the 
status line next to the lock icon.

3. Clicking on the cookie icon on the status line will cause the cookie-manager 
dialog to open.

4. The cookie-manager dialog shall be extended to include a status column for 
each cookie.  Furthermore, the set of properties for the selected cookie shall 
be extended to include a policy field.

5. The values appearing in the status column shall be "", "accepted", "session", 
and "flagged".  These strings shall be localized as usual.

6. The policy field shall indicate the site's policy regarding collecting 
identifiable information.  The field is left blank if the site has no policy.  
Otherwise the values that can go into this field are:

   no policy on storing identifiable information
   stores identifiable information without any user consent
   stores identifiable information unless user opts out
   stores identifiable information if user opts in
   does not store identifiable information

The policy field is not present if no cookies in the cookie list have any 
policies stored for them.

7. When you open the cookie-manager dialog from the tasks menu or from the 
cookie pref panel, the status column is initially not displayed.  User can cause 
it to be displayed by clicking on the the symbol at the right of the column 
headings.

8. When opening the cookie manager via the cookie icon, the status column is 
initially displayed and the cookies shall be sorted in reverse order by the 
value in the status column.  Although that sorting is language-dependent, in all 
cases the blank value (not accepted in this session) will come last.  So in 
English, the ordering will be:

   "session"
   "flagged"
   "accepted"
   ""

Comment 10

[Stephen P. Morse]

alecf wrote:
>also, regarding the use of the chatzilla icon, can we just spend a few minutes
>and put together a really really simple icon, even if it's just a picture of a
>cookie or an image of the word "cookie"?

It's more than a few minutes.  I was about to come up with my own icon until I 
realized the amount of work involved.  For starters, UE will probably want to 
design the icon.  Then I need to create a directory to house the icon and I need 
to figure out how we get icons into cvs and into the builds.  This is a whole 
other task, and might be done by the UE team.  So it should be covered in a 
separate bug report.  I've just opened bug 121299 to cover the icon issues.

Comment 11

[Alec Flett]

sorry, I should have said _temporary_ simple icon. make it really ugly! Make UI
beg to make their own :) Using the chatzilla icon is just going to confuse
people, and result in more bugs filed against chatzilla because the chatzilla
icon appears in wierd places in the UI... not to mention that chatzilla is not
included in netscape's default build so your icon wont' even be visible if
Netscape's own nightly builds.

Comment 12

[Stephen P. Morse]

As I mentioned above, there are more issues that I'd need to figure out other 
than just having a .gif file.  That's why I opened the separate bug report on 
it.

If you like, I won't check this in until the icon bug report is checked in.  But 
I would like to get an approval on this so that it is ready for checkin, 
independent of the work on the icon.

Comment 13

[Alec Flett]

Comment on attachment 66011 [details] [diff] [review]
revised to address the changes in comment #4


>+    /* P3P status = space (none), 'a' (accepted), 'd' (downgrade), 'f' (flag) */
>+    readonly attribute char status;
>+
>+    /* Site's compact policy = 0 (no policy), 1 (no consent), 2 (implicit consent),
>+                               3 (explicit consent), 4 (no ident info), space (unknown) */
>+    readonly attribute char policy;
>+

Ack! this is just bad practice. Don't use magic values for attributes
which have a fixed set of values. Instead use typedef and const values. 
i.e.
typedef long nsCookiePolicy;
typedef long nsCookieStatus;

then, inside your interface, you say
  const nsCookiePolicy POLICY_ACCEPTED=0;
  const nsCookiePolicy POLICY_DOWNGRADED=1;
  const nsCookiePolicy POLICY_FLAGGED=2;

  const nsCookieStatus POLICY_STATUS_NONE=0;
  // etc..

  readonly attribute nsCookiePolicy status;


From JavaScript, you can access these with:
const nsICookie = Components.interfaces.nsICookie;
if (foo == nsICookie.POLICY_ACCEPTED) etc....

>+    prev_cookie->status = status;
>+    prev_cookie->policy = '0' + P3P_SitePolicy(curURL, aHttpChannel)/2;

woah.. some comments to explain what exactly you're doing? I understand the
'0'+ part 
(which we'll get rid of when we switch to const values) but why the /2? At
least put
in a comment!

>+    prev_cookie->status = status;
>+    prev_cookie->policy = '0' + P3P_SitePolicy(curURL, aHttpChannel)/2;

same here, obviously

>+
>+  /* Notify cookieStatusOverlay if we need to turn on the cookie icon */
>+  if (prev_cookie->status == P3P_Downgrade || prev_cookie->status == P3P_Flag) {
>+    nsCOMPtr<nsIObserverService> os = do_GetService("@mozilla.org/observer-service;1");
>+    if (os) {
>+        rv = os->NotifyObservers(nsnull, "cookieIcon", nsnull);
>+    }
>+  }
>+

I'm concerned you're going to get out of sync here.. you should be explicit
about what state the
icon should be in, instead of depending on the state being maintained by the
observer.

not to mention how do you know you've got the right browser window? Isn't this
going to 
toggle on the icon in all windows?


+  </RDF:Seq>
+
+  <RDF:Seq about="chrome://navigator/content/navigator.xul">
+    <RDF:li>chrome://cookie/content/cookieStatusOverlay.xul</RDF:li>
</RDF:Seq>

Why make a whole new overlay for this? you've already put an implicit
dependency 
onto cookieStatusOverlay.xul from cookieViewer.js:

> 
>+function viewCookiesFromIcon() {
>+  window.openDialog("chrome://communicator/content/wallet/CookieViewer.xul","_blank","modal=yes,chrome,resizable=yes", 1);
>+}  
>+


>+
>+    var iconHidden = true;
>+
>+    var cookieIconObserver = {
>+      observe: function(subject, topic, state) {
>+        if (!iconHidden || (topic != "cookieIcon")) {
>+          return;
>+        }

This is what I meant about getting out of sync.. get the state from the
notification function.
Besides, this observer only un-hides the icon, but never hides it again. what
if I go to another site?

And besides (and I've said this MANY times before) - all global variables
should begin with a
lower case "g" - not to mention this "global" should be a member of the
observer object, not a global.

though if you fix this code, you won't need a global variable or a member
variable at all.


>+    var status = "";
>+    if (nextCookie.status == 'a') {
>+        status = cookieBundle.getString("accepted");
>+    } else if (nextCookie.status == 'd') {
>+        status = cookieBundle.getString("downgraded");
>+    } else if (nextCookie.status == 'f') {
>+        status = cookieBundle.getString("flagged");
>+    }

you should be using switch() here.. as in:
switch (nextCookie.status) {
  case nsICookie.POLICY_STATUS_ACCEPTED:
    status = cookieBundle.getString("accepted");
    break;

   etc...

Comment 14

[harishd]

Comment on attachment 66011 [details] [diff] [review]
revised to address the changes in comment #4

>+
>+  /* Notify cookieStatusOverlay if we need to turn on the cookie icon */
>+  if (prev_cookie->status == P3P_Downgrade || prev_cookie->status == P3P_Flag) {
>+    nsCOMPtr<nsIObserverService> os = do_GetService("@mozilla.org/observer-service;1");

Could be written as nsCOMPtr<nsIObserverService>
os(do_GetService("@mozilla.org/observer-service;1"));

>+ image="chrome://chatzilla/skin/images/taskbar-irc.gif"/>

I presume this will be replaced :)

Comment 15

[Stephen P. Morse]

> I'm concerned you're going to get out of sync here.. you should be explicit
> about what state the icon should be in, instead of depending on the state
> being maintained by the observer.

There is only one state change ever.  The icon starts off not being displayed.  
If ever we encounter a cookie that gets p3p processing, the icon goes on and 
stays on for the rest of that session.  So there is no sync problem here.

> not to mention how do you know you've got the right browser window? Isn't
> this going to toggle on the icon in all windows?

I should hope that it does.

> Besides, this observer only un-hides the icon, but never hides it again. what
> if I go to another site?

If the icon is unhidden, then it remains unhidden when you go another site.  
That is the intended behavior.

>>+ image="chrome://chatzilla/skin/images/taskbar-irc.gif"/>
>
> I presume this will be replaced :)

Yes, just as soon as we get our own icon

---------------

Attaching a patch that incorporates all other points made by reviewers.

Comment 16

[Stephen P. Morse]

Attachment: modifications for comments #13 and #14

Comment 17

[Alec Flett]

Comment on attachment 66077 [details] [diff] [review]
modifications for comments #13 and #14

you reuse them the same way you'd use any other member of nsICookie:
nsICookie::STATUS_UNKNOWN

I didn't get any response on the separation of cookieStatusOverlay.xul? do we
have a reason that it needs to be seperate from cookieTasksOverlay?

I have to say that that UI sounds very strange. If I visit a site in one window
which has cookies, then all windows get a cookie icon? 

what do you do if someone opens a new window - it won't have a cookie icon..
and then the NEXT site (even if it's another window) to have cookies will make
an icon appear there? seems like at that point it would be LESS confusing to
simply have the icon on at all times. Can we get UE involved here?

at the very least, I'm going to continue to mark this needs-work until we get
some icon, no matter how ugly, that isn't borrowed from some other component.

Comment 18

[Stephen P. Morse]

> you reuse them the same way you'd use any other member of nsICookie:
> nsICookie::STATUS_UNKNOWN

Oops, that comment in the code was a note to myself to come back and clean that 
up before posting a patch.  But then I forgot to do so.

> I didn't get any response on the separation of cookieStatusOverlay.xul? do we
> have a reason that it needs to be seperate from cookieTasksOverlay?

I did remove the lines that you noted in your comment.  I didn't understand from 
your comment that you were asking that I combine the cookieStatusOverlay.xul 
file and the cookieTasksOverlay.xul file into one.  I had them as separate files 
because they overlay different parts of the chrome.  I've now combined them per 
your reuqest.

> I have to say that that UI sounds very strange. If I visit a site in one
> window which has cookies, then all windows get a cookie icon? 

The icon means that in this session there were cookies that were acted upon 
(i.e., downgraded, flagged) because of a p3p preference.  Once that occurs, the 
icon stays on.  It has nothing to do with the website that is being visited.

That's what I thought was agreed upon at the p3p meeting held last Friday.  But 
I just checked with barrowman and it looks like what he wanted was for the icon 
to go off after it is clicked on.  So I just made that modification.

Attaching revised patch that addresses all issues except for the gif file which 
is being worked on in bug 121299

Comment 19

[Stephen P. Morse]

Attachment: incorporates issues raised in comment #17

Comment 20

[Stephen P. Morse]

Attachment: incorporates new gif file

Comment 21

[Stephen P. Morse]

Attachment: the gif file taskbar-cookie.gif

Comment 22

[Alec Flett]

Comment on attachment 66168 [details] [diff] [review]
incorporates new gif file

I still don't understand why the icon has to go on in every window, I can't
imagine that was the intention.. especially given the point I raised earlier
about opening a new window - when the new window is open, it won't have the
cookie icon, but the already-opened window will. Maybe by "session" some people
thought the intention was on a per-window basis, and some thought it was on a
per-life-of-process basis?

Other than that, this patch looks fine. I'm just not ready to land this
potentially confusing UI before we have this sorted out. 

I do realize that it's going to be a lot harder make it per-window, but I think
that we would be better off with the cookie  icon to be always visible than to
have it appear and disappear in all windows.

Comment 23

[Stephen P. Morse]

Attachment: resolve a javascript error message

Comment 24

[Stephen P. Morse]

Attachment: Carry icon visibility forward into newly opened window

Comment 25

[Stephen P. Morse]

Created new patch that corrects the problem encountered when opening a new 
window.  With this new patch, the icon visibility in the newly-opened window 
will be the same as it is in all the currently opened windows.

alecf, harishd please review

Comment 26

[Samir Gehani]

The type names ``nsCookiePolicy'' and ``nsCookieStatus'' lead me to think they
are classes rather than types.  Is there some other naming convention or maybe
an ``_t'' suffix that we can use to differentiate these types from classes? 
Also, checkin window.arguments[0] for an int seems a bit ambiguous.  Changing
this to a string or character might help.  I don't care too much about the
latter so it's your call.

Comment 27

[Stephen P. Morse]

Attachment: rename items per sgehani's suggestions in comment #26

Comment 28

[Samir Gehani]

Comment on attachment 67850 [details] [diff] [review]
rename items per sgehani's suggestions in comment #26

r=sgehani

Comment 29

[Alec Flett]

Comment on attachment 67850 [details] [diff] [review]
rename items per sgehani's suggestions in comment #26

regarding "boolean isCookieIconVisible();"

functions generally imply actions, not questions... attributes imply state.

So if this is a function, you'd want to say
boolean getCookieIconVisible() or something.
However, personally I think it works best as an attribute:

readonly attribute boolean cookieIconIsVisible;

so when read it looks like
if (cookieService.cookieIconIsVisible)...


regarding the use of window.arguments:
1) since your window is now accepting arguments, you should document this at
the start of your onload handler
2) "1" is not particularily expressive w.r.t. what it's for. How about passing
in a string like "source=statusbar" or "openedFromCookieStatus" or something?

Finally, I think those strings at the end need a wording/doc review. "opts
in"/"opts out" is not going to be understood by your average joe, plus "no
policy on" should probably "no policy about" etc...

Comment 30

[Stephen P. Morse]

Attachment: address issues in comment #29

Comment 31

[Alec Flett]

Comment on attachment 68179 [details] [diff] [review]
address issues in comment #29

yay, this looks fine. sr=alecf

Comment 32

[Stephen P. Morse]

Patch checked in.  But this caused 21 ms increase in startup time.  See bug 
124024.

Comment 33

[Stephen P. Morse]

Oops, I did everything put put the "flagged" entry in the p3p pref panel.  That 
is, the patch that was checked in did all the back-end work for "flagged" but I 
forgot to put it into the ui.

Reopening and attaching another patch that adds it to the UI.

Comment 34

[Stephen P. Morse]

Attachment: add "flag" to p3p ui

Comment 35

[Stephen P. Morse]

alecf, sgehani, please review latest patch.  Thanks.

Comment 36

[Alec Flett]

Comment on attachment 69270 [details] [diff] [review]
add "flag" to p3p ui

sr=alecf

Comment 37

[Samir Gehani]

Comment on attachment 69270 [details] [diff] [review]
add "flag" to p3p ui

r=sgehani

Comment 38

[Stephen P. Morse]

Second patch checked in.