More enhancements to p3p cookie manager

VERIFIED FIXED in mozilla1.0

Status

()

VERIFIED FIXED
17 years ago
17 years ago

People

(Reporter: morse, Assigned: morse)

Tracking

Trunk
mozilla1.0
x86
Windows NT
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments, 17 obsolete attachments)

1.53 KB, image/jpeg
Details
23.41 KB, image/jpeg
Details
11.89 KB, patch
samir_bugzilla
: review+
Details | Diff | Splinter Review
3.66 KB, patch
jag+mozilla
: superreview+
Details | Diff | Splinter Review
3.16 KB, patch
Details | Diff | Splinter Review
(Assignee)

Description

17 years ago
For background, see prevous bug reports on this issue:

   bug 98882: implement p3p cookie management
   bug 121161: enhance p3p cookie management by adding a p3p warning icon.
 
The following additional requirements have now been received:

1. Have p3p cookie management on by default
2. Throw up a dialog when the user clicks the p3p warning icon so that he
   doesn't go straight to the cookie manager.  Dialog gives him the opportunity
   to disable the p3p warning icon from reappearing in the future

In addition, item 2 above was expanded as follows:

  When the user clicks the p3p warning icon, the user will be presented with
  a dialog that will:
  + Briefly explain what a cookie is
  + Briefly explain why the warning icon was triggered
  + Explain that the user can view cookies using the Cookie Manager

  The user can take the following actions from the dialog:
  + Go to the Cookie Manager to view, manage and delete cookies
  + Cancel the dialog
  + Turn off future warnings so that the p3p warning icon will no longer be
    presented to the user as the user encounters cookies with inadequate
    compact policies
(Assignee)

Comment 1

17 years ago
There is currently a consideration for two different forms of the new dialog.  
One would have three buttons (view cookies, stop showing warning icon in future, 
cancel) whereas the other would have two buttons and a checkbox (checkbox to 
stop showing warning icon in future, buttons of view cookies and cancel).

Attaching a patch that can go either way.  As posted it does the 
two-button-plus-checkbox form, but if you change the commented out code in 
cookieTasksOverlay.xul you can get the other behavior.

The actual wording in the dialog box is not yet finalized.  But that can be 
changed easily in the future and should not be considered as part of the review 
for this patch.
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla1.0
(Assignee)

Comment 2

17 years ago
Created attachment 72633 [details] [diff] [review]
patch that can do either three-button form or two-button form
(Assignee)

Updated

17 years ago
Keywords: nsbeta1

Comment 3

17 years ago
reposting my email comments to capture discussion here:
 

We have two scenarios for use - one case which has users becoming annoyed with
the icon, another where the user investigates and finds some sort of value.  If
we design only for the first case, then we are guaranteed that the feature will
fail.  I don't think by guaranteeing failure, we are  meeting the requirement
handed to us.  If we design for both cases, then we allow the user to decide if
P3P is useful, and it's not the browser who's in the way of that decision.   
One thing to keep in mind is that we do not want to ruin the experience for P3P
users (present or future) with a nagging dialog.  Therefore, the flow that
encompasses primarily two users: the P3P user and the Normal user. 


BEGIN FLOW -------------------


[Start] User notices icon
        [decision: a or b]
      a) clicks icon
              • user sees explanation
                [decision i or ii]
                    i) receives no value
                       [decision 1 or 2]
                        1) ignores message (cancels or hits ok)
                             [loops to Start] (adds value to the curious or
undecided user)
                        2) flips switch to turn off notification
                            [end] (adds value for Average User)
                   ii) receives value
                        • continues to cookie management
                          [loop to Start-->shortcut to Cookie Mgmt] (adds value
for P3P user)
    
      b) ignores icon - continue browsing task
         [loops to Start]


END FLOW ------------------


Dialog Design:


----------------------------------------------------------------------
A cookie is a small bit of information stored on your computer by some
web sites. You have visited a web site which uses a cookie on your
computer in a way that requires you to be notified. This Icon notifies you
when such a site has been encountered

-- Insert Image of Status Bar with Icon Showing --

The Cookie Manager's Privacy Levels settings determine what kinds of
cookie use require you to be notified.


[ ] Do not show future notifications        [[Ok]]   [View Cookie Manager]
      [ ] Do not ask me this again
                   
--------------------------------------------------------------------------
                                        


Behavior:  [Do not ask me this again] is dependent on [Do not show future
notifications], so that the latter becomes disabled if the former is selected.
This allows three things:  1) pose the question again later when they have time
to consider it (hit [OK]) 2) Decide the feature is useless and not be notified
ever again [Do not show future notifications], 3) go straight to cookie manager
because the feature brings value [Do not ask me this again]
  
(Assignee)

Comment 4

17 years ago
Just want to inject a touch of reality into all this.  There is a
pre-canned dialog that I can easily bring up.  That's what the patch I
included does.  It's very simple to implement that way.  The pre-canned
dialog allow for a text message, an optional checkbox, and up to three
buttons.  And closing the dialog with the X or escape is equivalent to
hitting the rightmost button.  Alternately, I can design a new dialog
and put whatever we want in it.  Not extremely difficult, but an order
of magnitude more difficult than using the pre-canned ones.

The dialog Marlon proposed above has an image in it (not supported by the
pre-canned dialog) and a second checkbox in it (pre-canned dialog allows
for only one checkbox).  Do we really want to do all the extra work to
get these additional items in the dialog?  Do we really want to have a
picture of the icon inside the dialog?

Comment 5

17 years ago
Some comments on the text in Marlon's comment of 2002-03-05 16:31:

- lowercase "icon" in the third sentence

- the sentence after the icon should read like this:

    "Use the Cookie Manager to control cookies and privacy settings."

  - mainly because the words "Privacy Levels" in this context have no meaning.
Also, this proposed phrasing tells the user what the user can do about the
situation rather than describing, in arcane language, how the software works.

Another option for the first checkbox label is this:

  "Don't show the cookie notification icon again" 

I prefer this because it's explicit--in Marlon's version, is the "notification"
the icon or this dialog?

It looks like "Don't ask me this again" means is that you want to go straight to
the Cookie Manager when you click the icon (assuming you haven't turned off the
icon permanently). If this is in fact desirable, I'd prefer more explicit
phasing, like "Don't show this dialog box again." "Don't ask me" suggests that
the dialog is asking a question, which it's not.

I'm dubious about the placement of OK and Show Cookie Manager buttons. Marlon's
argument suggests that users will frequently want to click the cookie icon and
delve into the details of the Cookie Manager and p3p dialogs. I'm not so sure
this is the case. Some users may want to adjust the settings once or twice, and
then when they've got it right continue with their work, noting the warning icon
if they've chosen to have it displayed when there's a problem. If this
interpretation is correct, then the the additional checkbox may just be
complicating things unnecessarily. But I could be wrong.

I think we could live without showing the icon, if things get tight re
implementation. It's a nice touch but the user has already clicked it in order
to see this dialog, so they should have some idea what the "cookie notification
icon" is.

If we delete the second checkbox, then you would always see this dialog when you
click the cookie icon. The only way to get rid of it completely would be to
select "Don't show the cookie notification icon again" and click OK (which would
be the rightmost button). Show Cookie Manager would get clicked only by those
people who really wanted to dive into the Cookie Manager to check status or
change settings. This would make sense if this is something we expect to happen
infrequently. If we expect people to be checking the Cookie Manager frequently,
then Marlon's approach may make better sense. The text changes I've suggested
here would work for either approach.

Comment 6

17 years ago
from seans comments:

>I think we could live without showing the icon, if things get tight re
>implementation. It's a nice touch but the user has already clicked it in order
>to see this dialog, so they should have some idea what the "cookie notification
>icon" is.


I agree, though it would be helpful in explicitly pointing out the new and
totally unfamiliar feature, since practically nobody is going to know what is P3P


>Some users may want to adjust the settings once or twice, and
>then when they've got it right continue with their work, noting the warning
>icon if they've chosen to have it displayed when there's a problem. If this
>interpretation is correct, then the the additional checkbox may just be
>complicating things unnecessarily. But I could be wrong.


Two checkboxes seems complicated and it may be too hard to implement, but it
offers the most flexibility, meeting the 3 requirements i stated previously. If
we had to simplify it even further i would then suggest we drop [Do not show
notifications] checkbox and keep [Launch Cookie Manager Directly].  Then we
would have to add an easy to hit [Do not show notifications] checkbox in the
cookie manager itself.  This would be the only alternative solution that would
still meet the requirements outlined in the flow model.



>If we expect people to be checking the Cookie Manager frequently,
>then Marlon's approach may make better sense. The text changes I've suggested
>here would work for either approach.

that's the primary scenario i believe we should be designing for.




OPTION 1
----------------------------------------------------------------------
A cookie is a small bit of information stored on your computer by some
web sites. You have visited a web site which uses a cookie on your
computer in a way that requires you to be notified. This icon notifies you
when such a site has been encountered

-- Insert Image of Status Bar with Icon Showing --

Use the Cookie Manager to control cookies and privacy settings.



[ ] Do not show notifications   [View Cookie Manager]   [[Ok]]  [Cancel]
      [ ] Launch Cookie Manager next time
                   
--------------------------------------------------------------------------





OR



OPTION 2 (Simplified)
----------------------------------------------------------------------
A cookie is a small bit of information stored on your computer by some
web sites. You have visited a web site which uses a cookie on your
computer in a way that requires you to be notified. This icon notifies you
when such a site has been encountered

-- Insert Image of Status Bar with Icon Showing --

Use the Cookie Manager to control cookies and privacy settings.

Click [Don't Display Icon] if you do not want to be notified again.

Click [Continue] to view the Cookie Manager and alter P3P notification settings.



                         [Don't Display Icon]    [[Continue]]   [Cancel]
                   
--------------------------------------------------------------------------
Behavior: Takes user directly to Cookie Manager next time they click on the
icon, without showing this again.  The Cookie Manager would now have to offer
[Don't Display Icon] Pref.

Comment 7

17 years ago
I'm a little confused. 
Why do we think typical users will be using this frequently enough for it to be
the primary scenario?  I had assumed the main use would be to see the new icon,
click on it, and then make the dialog go away forever.  For instance, I only
know of one person who cares about cookies enough to block them.  Are there
really a significant number of people who are going to be messing with cookies
like this?  

The "requires you to be notified" sounds scary, as if something has happened
that was so bad that you must be served legal notice (it is my sad duty to
inform you...)  Also, the notification is not about encountering the site, but
the use of the cookie. Better wording escapes me at the moment though.

Finally, I think we do need the image. Even though users just clicked on it,
they are unlikely to know our term for it.
(Assignee)

Comment 8

17 years ago
I have the implementation of the general dialog (not a pre-canned one) working.  
I'll be posting the patch shortly.  But in the meantime I am posting the 
screenshot so we can all see what it looks like and decide if that is what is 
really wanted.
(Assignee)

Comment 9

17 years ago
Created attachment 73095 [details]
screen shot using a full dialog (not pre-canned one)

Comment 10

17 years ago
thanks steve.  I think in the email discussion we decided that adding a help
button would not be much benefit here.  If we can't explain what's going on in a
few lines of text, users wont bother to click help.

i really have a bigger preference for option 2 which i supplied in my last
comment.  your implementation is more like option 1 than option 2.  I am not
sure what, if any, we decided on however. since Dbarrowman's last comments i
have put the subject on hold.   Moving forward (if we are indeed doing so)
Option 1 does not seem to necessarily need a [cancel] button does it?  If [[Ok]]
was the default action, and there were no [cancel] or [help], then i think the
design would be safe. 

I still prefer option 2 however for it's simplicity and getting you to cookie
manager to view the policy.



from peter's comments:

>I'm a little confused. 
>Why do we think typical users will be using this frequently enough for it to be
>the primary scenario?  I had assumed the main use would be to see the new icon,
>click on it, and then make the dialog go away forever.

  
that's assuming the wrong thing.  first of all, the dialog isn't going away, the
icon notification is.  that of course means the dialog goes away too, but not
completely. For P3P user (hypothetical user of course) they should be taken to
cookie management to view the policy, the reason they are being notified in the
first place.  Why should we notify someone if we're just telling them to click
here and make it stop?  That's a silly approach.  No matter how futile it may
seem, this is a 'feature'. Our product is full of features, not full of random
behaviors that we ask our users to correct.
(Assignee)

Comment 11

17 years ago
Marlon, I hear you.  Once I get the dialog completely working (I'm very close 
right now), it will be relatively easy for me to rearrange it from option 1 to 
option 2.  But before I do so I'll post the patch as of that time, just in case 
we decide we want to go back to option 1.

Regarding the cancel button for option 1, yes it is needed.  The difference in 
sematics between the OK and the Cancel buttons is that in the former case the 
changes made to the checkboxes are remembered whereas in the latter case they 
are not.

I'll remove the help button.
(Assignee)

Comment 12

17 years ago
Created attachment 73108 [details] [diff] [review]
Patch for option 1 (for achival purposes)
(Assignee)

Comment 13

17 years ago
Created attachment 73124 [details]
screenshot for option 2
(Assignee)

Comment 14

17 years ago
Created attachment 73126 [details]
the real screenshot for option 2 (preceding one labelled option 2 was really option 1)
Attachment #73124 - Attachment is obsolete: true
(Assignee)

Comment 15

17 years ago
Created attachment 73134 [details] [diff] [review]
Patch for option 2
(Assignee)

Comment 16

17 years ago
The patches and screen shots for options 1 and 2 are now all attached.  As soon 
as we get some agreement, I can send these out for review and get them checked 
in.

My own personal preference is for option 1, because option 2 does not offer the 
user any means of disabling the intermediate dialog.

Comment 17

17 years ago
Suggestions re Option 1
-----------------------

Delete second and third sentences and replace with this (to address Peter's
concerns):

  You have visited a web site that has set one of your cookies and 
  triggered the cookie notification icon:

The original idea for the icon was "Image of Status Bar with Icon Showing". This
would be preferable to showing just the cookie icon, since it gives some visual
context.

Change checkbox labels as follows:

  [X] Do not show cookie icon again

  [X] Launch Cookie Manager directly next time

Question re interface of option 1: Do we really need the first checkbox? That
is, is it really a good idea to make it so easy to turn off the icon? As Marlon
points out, "Why should we notify someone if we're just telling them to click
here and make it stop?"  Like the security lock, the cookie icon is just a
visual warning, it doesn't interfere with the user's work. I don't see any
problem forcing users to drill down to the Privacy Levels dialog to turn this off.  


Comments re Option 2
--------------------

I also prefer Option 1, for the same reason that Steve gives--it lets people
turn off the dialog. Mentioning p3p in the UI is confusing--most users won't
know what it means. The checkbox seems a better solution than having to explain
the buttons in this way. Finally, I'm dubious about the suggestion that the
Cookie Manager, rather than the Privacy Levels dialog, should show the "don't
display icon" option. 

The Privacy Levels dialog controls cookie acceptance policy, so it seems like
the natural place from which to control the icon tied to that policy. All you
can do in the Cookie Manager is identify (in the Status column) which cookies
have been flagged or accepted for the session only. Also, as mentioned above I'm
not so sure it's a good idea to make the icon so easy to turn off. You should
have to think about it, at least to the extent of viewing the Privacy Levels
dialog and the policy settings currently in force. In any case, I suspect the
expert users who may care about cookie notifications will be more concerned
about cookie status than turning off the icon.


Cookie icon option
-------------------

Assuming the icon display option does in fact belong at the bottom of the
Privacy Levels dialog, I suggest this wording:

[X} Show cookie icon when policy requires flag or session-only acceptance


Steve Morse and Dave Barrowman: Does it make sense that flag and session-only
are the only conditions that should trigger the icon? Also, what exactly will
the default Privacy Levels setting be? If flag/session is correct, only Medium
and Low involve settings that would trigger the icon.

Comment 18

17 years ago
I believe cookies that were rejected because of the compact policy should also
trigger the icon, even though they will not show up in the manager.

Comment 19

17 years ago
Dave, as a policy thing I agree with what you said.  However, given our current
implementation I wonder whether it's a good idea.  With this on by default,
we're likely to be hitting that situation and then users will walk into this
looking for what happened and won't find any way to discover the cause of the
icon coming up. The one case that seems to matter would be where you wanted to
go to a site that had denied cookies and you were trying to figure out how to
enable them.  Is there any easily comprehended way to undo the denial? 
(Assignee)

Comment 20

17 years ago
At a meeting today the following was agreed upon:

1. No pref to disable the cookie icon nor to bypass any dialog that comes up 
when the icon is pressed.

2. Cookie icon is to appear when cookie is rejected due to p3p, not just if it 
is downgraded or flagged.

3. Cookie Mananger dialog is to have a button that brings up the p3p dialog

4. When cookie icon is clicked on, the dialog that comes up should look as 
follows:

   Cookie Warning

   -----------------------------
   A cookie is a small bit of information stored on your computer by some 
   web sites. A web site you have visited has set a cookie and triggered the 
   cookie notification icon, as required by your privacy settings:

            [picture of status line containing icon goes here]

   Use the Cookie Manager to manage your cookies and privacy settings.

   [Help]        [Turn Off Privacy Settings]   [View Cookie Manager]   [[Close]]

There is an issue as to whether the help button is to go on the left side or the 
right side.  This issue is deeper than just this dialog but rather permeates 
most dialogs in the browser.
(Assignee)

Comment 21

17 years ago
Created attachment 73348 [details] [diff] [review]
Patch to implement requirements in comment #20
(Assignee)

Comment 22

17 years ago
Created attachment 73349 [details]
jpg image to go along with requirments in comment #20
(Assignee)

Comment 23

17 years ago
Created attachment 73351 [details]
screen shot of dialog implemented by preceding patch (for comment #20)
(Assignee)

Comment 24

17 years ago
Oops, my button labeling wasn't what was in the spec.  So I'll attach a revised 
screenshot with the corrected button labels.
(Assignee)

Comment 25

17 years ago
Created attachment 73355 [details]
screenshot for comment #20 but with corrected button labels
Attachment #73351 - Attachment is obsolete: true
(Assignee)

Comment 26

17 years ago
Created attachment 73356 [details] [diff] [review]
Patch for comment #20 but with corrected button labels
Attachment #73348 - Attachment is obsolete: true
(Assignee)

Comment 27

17 years ago
I left out one detail from the agreed-upon behavior in comment #20.  Namely, if 
the cookie-acceptance pref is not set to p3p, then the cookie manager dialog is 
not to have a p3p button (item 3 in comment 20).  Attaching revised patch that 
accomplishes this additional detail.

After implementing it, I noticed that there is a strange quirk here.  
Specifically, assume that the user goes to the cookie pref-panel and changes the 
cookie acceptance pref from either p3p to something else, or from something else 
to p3p.  Now, from that pref panel he clicks on the cookie-manager button.  The 
pref has not yet been changed because the user has not yet clicked OK in the 
pref panel.  So the state (hidden or visible) of the p3p button in the cookie 
manager window that comes up will not be consistent with the user's new, but as 
yet uncommited, choice for the pref.  I could add some more code to force the 
cookie manager to display the button based on the pending choice.  But maybe the 
user experience is alright as it is, based on the actual pref setting and not 
the pending one.  I'll wait for some input from UE before changing this 
behavior.
(Assignee)

Comment 28

17 years ago
Created attachment 73372 [details] [diff] [review]
patch for requirements in comment #20 plus added requirement in comment #27
Attachment #73356 - Attachment is obsolete: true

Comment 29

17 years ago
nsbeta1+
Keywords: nsbeta1 → nsbeta1+

Comment 30

17 years ago
We probably need an access key to provide the same functionality as clicking the
icon.  Steve, is this hard?
(Assignee)

Comment 31

17 years ago
Do we have access keys for any of the other icons on the status line?  Namely 
the lock icon and the toilet-bowl plunger icon?

Also, how would a user ever discover such an access key?

Comment 32

17 years ago
Those are both in the menus (File:Work Offline & View:Page Info), so they are
keyboard accessbible.  Hopefully, a menu item should be sufficient for this.

Comment 33

17 years ago
I think the menu item implemented for this feature will be for showing the full
P3P viewer so not sure it maps to the same thing. We currently don't have a
keyboard equivalent for Manage Stored Cookies (which I think is what the icon
displays if you click it). I suppose we can add one but because it's in a third
level submenu, I'm not sure it qualifies. Usually we assign keyboard equivs for
frequently accessed items. We run out of the equivs really fast too. Any
suggestions? 
(Assignee)

Comment 34

17 years ago
Created attachment 73603 [details]
Same as comment #20 screenshot but with five buttons.

Comment 35

17 years ago
Agree with Trudelle - I don't think an accelerator is necessary for manage
stored cookies. Keyboard users can use Alt or F10 to access the menu bar on
Windows/Linux. OS X users have "Universal Access" preferences where they can
choose how to access their main menu bar.

Comment 36

17 years ago
But clicking the icon is slightly different than going to the cookie manager,
since we hit the dialog first, plus it turns off the icon.  I don't know if
there is a graceful solution - maybe we just ensure that any viewing of the
cookie manager turns off the icon and live with the fact that the dialog is
circumvented.

Comment 37

17 years ago
It is different, but users who do this frequently enough to want a command key
will presumably set things up to go straight to the cookie mgr (assuming we are
offering that option).  If so, then perhaps it would be appropriate to give it a
keystroke, if one can be found (perhaps one with a modifier?), or at least raise
it to a second level menu.  IMO, nothing in the product should be buried so deep
in menus.

Comment 38

17 years ago
In the five-button version, the final line of text needs to refect the new UI.
How about this:

  Use the Cookie Manager to manage your cookies and view their privacy status.

Also, if the name of the fifth button and the title of the dialog it opens are
going to remain "Privacy Levels," the end of the second sentence in this dialog
should probably be "privacy levels" rather than "privacy settings."

Is it worth changing the name of the Privacy Levels dialog (and button in this
one) to Privacy Settings? There are more than levels involved. Maybe this should
be a separate bug.

Also, Privacy Settings does not seem like the right name for this dialog, which
can bring you to those settings but does not actually contain them. How about
Privacy Warning or Cookie Warning?

Comment 39

17 years ago
Why do we need five buttons? I thought we agreed that the three action buttons
plus help were sufficient. Did we determine or did IA specify that we need to
get to Privacy Levels from the dialog? I remember that we were going to provide
access to that from the Cookie Manager as being sufficient. 

Also on the keyboard equiv issue I don't think we need one. We can address the
placement of Manage Cookies in the menu spec and see if we can elevate it. 

On the issue of weirdness leaving prefs and them not being activated I don't
think the pref should take effect when the user goes to the Cookie Manager.
Since the Prefs window stays open, the user may simply be looking to see what's
in the Cookie Manager or alternatively reading about privacy issues. I agree
it's weird to go to the cookie manager and not see what you've selected in Prefs
appear, but the current behavior of Prefs is not to update until the user clicks
OK. 
(Assignee)

Comment 40

17 years ago
Lori wrote:

> Why do we need five buttons?

Nobody said that we did.  I included it for comparison sake so Dave could show 
it to IA and let them decide which one they wanted.

> On the issue of weirdness leaving prefs and them not being activated I don't
> think the pref should take effect when the user goes to the Cookie Manager.

Nobody said that either.  What I said was that the cookie manager should behave 
as if the pref was changed when it makes it decision as to whether or not to 
display the go-to-p3p button.  The pref is not actually changed and won't be 
unless the user clicks on ok from the pref panel.

Comment 41

17 years ago
Thanks for the clarifications. 

> On the issue of weirdness leaving prefs and them not being activated I don't
> think the pref should take effect when the user goes to the Cookie Manager.

>Nobody said that either.  What I said was that the cookie manager should behave 
>as if the pref was changed when it makes it decision as to whether or not to 
>display the go-to-p3p button.  The pref is not actually changed and won't be 
>unless the user clicks on ok from the pref panel.

O.k. I misunderstood what you wrote earlier. In that case, I still don't think
the Cookie Manager should change because that will create a confusing situation
for users. What if they decide that they don't want the setting but viewed the
cookie manager that assumed the pref was set. When they return, it won't be
there. Only show the button when the pref is set. That will be the clearest and
most consistent behavior. 

(Assignee)

Comment 42

17 years ago
Lori, I'm now inclined to favor the 5-button dialog (and not add a p3p button to 
the cookie manager dialog) just because it avoids this potentially confusing 
problem.
(Assignee)

Comment 43

17 years ago
Based on private e-mails, it looks like the consensus is on the five-button 
approach.  Sean has been updating the wording as well.  Attaching a screenshot 
that I think reflects Sean's latest wording update.  Also attaching a patch that 
implements all this.
(Assignee)

Comment 44

17 years ago
Created attachment 74099 [details]
Five-button screenshot with Sean's latest wording
Attachment #73603 - Attachment is obsolete: true
(Assignee)

Comment 45

17 years ago
Created attachment 74101 [details] [diff] [review]
patch for five-button version
(Assignee)

Comment 46

17 years ago
cc'ing jag and samir for reviews of the five-button patch.

Comment 47

17 years ago
In the latest screenshot, I don't see any words that indicate why there is an
image in the icon at all.  Even after you add an arrow pointing to the cookie
icon, I don't think this works right.
(Assignee)

Comment 48

17 years ago
I agree with selmer's comment 47.  I suggest either adding the phrase "shown 
below"

   ... and triggered the cookie notification icon shown below, as required ...

or remove the image completely.

Sean, what are your thoughts on the wording?

Comment 49

17 years ago
I agree that the point of the icon is currently kind of lost. How about

... and triggered the cookie notification icon shown here, as required ...

The consensus earlier was that we need the icon. I'm not so sure now--after all,
they have to click it to see this dialog at all. But if the icon needs to stay,
something like the above wording would be better.

Comment 50

17 years ago
I like the addition suggested by Sean in #49.
(Assignee)

Comment 51

17 years ago
Which addition of Sean's?  He presented two alternatives:

1. Change of wording

2. Not being sure that we need the icon.
(Assignee)

Comment 52

17 years ago
Created attachment 74241 [details] [diff] [review]
patch for five-button version including sean's wording change in comment 49
(Assignee)

Comment 53

17 years ago
Created attachment 74242 [details]
screen shot for sean's wording change from comment 49
Attachment #74099 - Attachment is obsolete: true
Attachment #74101 - Attachment is obsolete: true
(Assignee)

Comment 54

17 years ago
Created attachment 74244 [details]
the real screen shot with sean's changes from comment 49
Attachment #74242 - Attachment is obsolete: true
(Assignee)

Comment 55

17 years ago
Excuse the spams, but for some reason I'm having a problem getting the correct 
screen shot into the bug report.  Stand by -- I'll upload it shortly
(Assignee)

Updated

17 years ago
Attachment #74244 - Attachment is obsolete: true
(Assignee)

Comment 56

17 years ago
Created attachment 74257 [details]
and now the real screen shot that includes wording change for comment 49

Comment 57

17 years ago
help target for this dialog is "cookie_notify"; placeholder section checked in,
text to follow.
(Assignee)

Comment 58

17 years ago
Created attachment 74274 [details] [diff] [review]
patch with help button hooked up
Attachment #72633 - Attachment is obsolete: true
Attachment #73095 - Attachment is obsolete: true
Attachment #73108 - Attachment is obsolete: true
Attachment #73126 - Attachment is obsolete: true
Attachment #73134 - Attachment is obsolete: true
Attachment #73355 - Attachment is obsolete: true
Attachment #73372 - Attachment is obsolete: true
Attachment #74241 - Attachment is obsolete: true

Comment 59

17 years ago
I don't like the help button. ask for dialog support for the what's this 
widget (windows) and the mac help button.

the notification text is useless, it doesn't appear to hint at what the cookie 
is or why it would violate my settings.

turn off privacy settings gives no hint about duration or scope.

view cookie manager doesn't make sense to me, I might want to view the site's 
cookie's or cookie policy but i don't think i'd want to load some generic 
application.

close? how about continue?

view privacy settings? that's the second button beginning with view. yuck.

-- ok, that was my first reaction to this bug, i'll read the comments and add 
consider them...

About marlon's flow chart, the dialog appears to be static, which means the 
user is going to get sick of it containing no useful information. you could 
have a 5 word hovertip over the cookie icon "site violates your cookie rules"

--
ok, i've read the entire bug report, and i don't see any substance in it or the 
one picture i looked at.

let me see if i understand this.  I see this cookie on and off in my status 
area.  I click on it, I get the same dialog each time, with the same text.

marlon asks what value i get from this, and why I won't disable this.
no value, will disable. can't permanently remove.

What might be useful:

I see the icon, i hover over it, I get a 5-15 word summary of the current 
site's status.  Which indicates whether there were cookies dropped, accepted 
perhaps even how many.

If i click on it, I get a quick list of the cookies set by the site, and a 
collapsed item of rejected cookies.  If I expand that list, I see the cookies 
that my browser rejected for me, and something lets me find out why the cookie 
was rejected.  If I select a cookie which is not in the rejected set, I can 
drag it to the rejected set, delete it, or change its value (* - netscape6 
should not let me do this, someone might get upset that a major browser would 
allow users flexibility or power).

+---------------------------------------------------------+
| cookies from p3pconfused.timeless:8080              |?|x|
+---------------------------------------------------------+
| policy file http://p3pconfused.timeless:8080/p3ppolicy  |
| +-------------------------------v---------------------+ |
| | Cookie                        | Value/Reason        | |
| +-------------------------------+-------------------v-+ |
| |   username                    | timeless          |^| |
| |   password                    | ..................| | |
| | v rejected                    |                   | | |
| | | phone number                | too personal      |v| |
| +-------------------------------^-------------------^-+ |
| [ Ignore site ]          [ Privacy settings ] [ Close ] |
+---------------------------------------------------------+

I still have no idea what turn off privacy settings means wrt duration + scope, 
or why someone would want to do it.

The only thing that might be useful to someone is a way to get rid of the 
cookie entirely, and the way I'd expect to do that is through the preferences 
software panel. But of course, someome already explained that we won't let 
people remove it.

I've looked at the two prereq bugs and still don't get this.

The dialog you're developing is a tutorial dialog, which is really not 
something we need.

Comment 60

17 years ago
sr=hyatt

Comment 61

17 years ago
Comment on attachment 74274 [details] [diff] [review]
patch with help button hooked up

r=sgehani
Attachment #74274 - Flags: review+

Comment 62

17 years ago
Comment on attachment 74274 [details] [diff] [review]
patch with help button hooked up

a=asa (on behalf of drivers) for checkin to the 1.0 trunk
Attachment #74274 - Flags: approval+
(Assignee)

Comment 63

17 years ago
checked in.
Status: ASSIGNED → RESOLVED
Last Resolved: 17 years ago
Resolution: --- → FIXED
(Assignee)

Comment 64

17 years ago
I just backed out one small part of the patch -- namely the default setting of 
cookie behavior pref being p3p instead of accept-all.  Reason I did this is 
because p3p will not be part of the mozilla release, and that means that the 
pref will start off in an invalid state.

We could add some code to detect this, and change it from p3p to accept if p3p 
is not present.  Or we could simply initialize the pref to p3p in the netscape 
tree and leave it as accept in the mozilla tree.

The part of the patch that I backed out was the change in all.js
(Assignee)

Comment 65

17 years ago
Bug 131381 fixed the problem mentioned in comment 64 (it also fixed another 
problem related to having p3p turned on by default).  So now the pref can be 
turned on by default.

I just checked back in the thing I backed out in comment 64.

Comment 66

17 years ago
pref change backed out.  reopening...  :-)

-pref("network.cookie.cookieBehavior",       0); // 0-Accept,
1-dontAcceptForeign, 2-dontUse
+pref("network.cookie.cookieBehavior",       3); // 0-Accept,
1-dontAcceptForeign, 2-dontUse, 3-p3p

Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Comment 67

17 years ago
Comment on attachment 74274 [details] [diff] [review]
patch with help button hooked up

removing approval.
Attachment #74274 - Flags: approval+
(Assignee)

Comment 68

17 years ago
The default of setting the pref to p3p was backed out (comment 66) because it 
caused a significant increase in window-open time (from 480ms to 540ms).

So I did some investigating to find out where the time was being spent.  My 
measurements ruled out things like creating the cookie icon, installing the 
observers that I needed to manage the cookie icon, etc.  All of these were 
taking 0 ms (based on the precision of my instrumentation).  Then I measured the 
time it was taking to install the p3p dll.  120ms.  That was the culprit.

So let me explain why I was installing the dll, and what I can do instead.

When the cookie-behavior pref is set to p3p, the cookie module initialization 
code (cookie_SetBehaviorPref in nscookies.cpp) makes a test to see if the p3p 
dll is present.  If it is not present, we can't leave the pref set to p3p.  For 
one thing, the cookie pref panel doesn't even display that choice when the dll 
is missing, in which case we would have a radio button for cookie behavior with 
no choice chosen.

The test that the cookie module was doing to determine if the p3p dll is present 
is:

   nsCOMPtr<nsICookieConsent> p3p(do_GetService(NS_COOKIECONSENT_CONTRACTID));
   if (!p3p) {

That was not only testing if the dll was present, was also installing it.  
That's where all the time was being taken.

So I have a patch that simply tests but doesn't install.  And this test can be 
done very efficiently in a different place.  Therefore the patch consists of 
three things:

1. Set cookie behavior to p3p by default
2. Make test for existence of p3p dll
3. Remove previous test that install p3p dll
(Assignee)

Comment 69

17 years ago
Created attachment 76060 [details] [diff] [review]
test for presense of p3p dll without installing it
(Assignee)

Comment 70

17 years ago
Created attachment 76063 [details] [diff] [review]
same patch but without whitespace differerences -- easier to review

Comment 71

17 years ago
Have you tested this patch and compared whether it pretty much does not regress
performance from before your very first patch that did regress performance?  If
not, can you work with mcafee or the perf group to ensure the numbers are OK by
applying this patch to a perf tbox.  Thanks.

With the above sanity check r=sgehani.  
(Assignee)

Comment 72

17 years ago
Samir, I'm one step ahead of you.  I just spoke with mcafee about 15 minutes ago 
and he is indeed going to run the patch on a performance tbox.

Comment 73

17 years ago
Nice.  I'm sure reporting back that we don't degreade performance will help your
super-reviewer.
(Assignee)

Comment 74

17 years ago
Yes, exactly.  In fact, note that I hadn't asked for reviews yet and didn't plan 
on doing so until I got the green light from mcafee.  You were just too fast for 
me, contemplating my review request before I even made it. ;-)

Comment 75

17 years ago
Steve, isn't instantiation of the p3p.dll a one-time thing?  I'm not sure why
window open would pay this penalty for every window (as implied by comment #68.)
 Perhaps you meant startup?  Do we pay a page load penalty for having P3P turned on?
(Assignee)

Comment 76

17 years ago
Steve, you are correct, it is only at startup.  I was going to say "startup" 
instead of "window open" time in my comment 68, but I believe that the 
measurement on tinderbox was the time it takes to open the first window for that 
particular test.  So I assumed that this was just part of the total startup 
time.

If we could do the total startup in just 540ms, we would be in terrif shape. ;-)

Comment 77

17 years ago
Both Ts and Txul tests of this patch look ok on facedown.
I had a little uptick on my return-to-baseline test, but ?
I think this patch behaved ok so let's try it.

Comment 78

17 years ago
Comment on attachment 76060 [details] [diff] [review]
test for presense of p3p dll without installing it

sr=jag
Attachment #76060 - Flags: superreview+

Comment 79

17 years ago
Comment on attachment 76063 [details] [diff] [review]
same patch but without whitespace differerences -- easier to review

a=asa (on behalf of drivers) for checkin to the 1.0 trunk
Attachment #76063 - Flags: approval+
(Assignee)

Comment 80

17 years ago
Checked in this new patch (that tests for but doesn't install the p3p module) 
and all looks good.  On the first tinderbox cycle after the checkin, the Txul 
time actually went down by 4ms (surely just an aberation).  But it certainly 
didn't go up by 80ms, so we are home free.

Closing this out, hopefully for the last time.
Status: REOPENED → RESOLVED
Last Resolved: 17 years ago17 years ago
Resolution: --- → FIXED
There was an extra newline preventing the new window time increase from
registering on comet, which is the performance test tinderbox that builds
--with-extensions=all.  Bug 133624 was filed on the Txul slowdown and is holding
the tree closed.
(Assignee)

Comment 82

17 years ago
Let me retract what I said in comment 76.

> I believe that the measurement on tinderbox was the time it takes to open
> the first window for that particular test.  So I assumed that this was just
> part of the total startup time.

I now realize that I was mistaken.  The Txul measurment on tinderbox is not for 
startup.  Rather it is the average of opening a window ten times.

So now I don't understand why this number increases when p3p is turned on.  
The timing that I had zeroed in on was the initial loading of the p3p module 
and my patch moved that to a later time.  That did improve the timing tests that 
I performed, but it should have had no effect on the Txul time.  And that indeed 
turned out to be the case, after an unnoticed flaw in my latest checkin was 
corrected.

So there is a significant p3p penalty for every window open.  It only occurs if 
the p3p module is present and if the p3p pref is turned on.  It will take more 
investigation to find out just why that is happening.
(Assignee)

Comment 83

17 years ago
Opening bug 135046 to deal with the window-opening performance hit that was 
introduced here.

Comment 84

17 years ago
verified 4/25/02 builds, winNT4, linux rh6, mac osX
Status: RESOLVED → VERIFIED

Comment 86

17 years ago
Shouldn't this project get marked as complete on
http://komodo.mozilla.org/planning/branches.cgi ?
You need to log in before you can comment on or make changes to this bug.