23508 - Cookie confirmation dialog should show all data

Status: VERIFIED FIXED

(Core :: Networking: Cookies, enhancement, P3)

Description

[Henrik Gemal]

When you go to a site that send a cookie and you have enabled "warn me about
cookies" you'll get a Confirm box that warns you about the cookie.

The Confirm box should include the cookie name, machine set, path, date, etc.
Attached is a image with the three cookie dialogs from Netscape, Mozilla and IE.
I really like some of the stuff from the IE dialog.

Comment 1

[Henrik Gemal]

Attachment: Cookie Dialog from Netscape, Mozilla and IE

Comment 2

[Eli Goldberg]

I agree. Most people have no idea what a 'cookie' means, and IE's dialog teaches
its meaning to the user via their dialog.

[changing severity to 'enhancement']

Comment 3

[shuang (gone)]

reassign it to german for final UI clean up. Thanks for the screen shots.

Comment 4

[Stephen P. Morse]

This issue was discussed at length about two years ago when the new cookie nag
box was designed.  It was decided at that time (jar was involved in the decision
so I am copying him on this report) that there was too much "noise" in this box
for the novice user.  That is why I removed all that and simplified the nag
box.

Therefore closing this out as invalid.

Comment 5

[Henrik Gemal]

Are you serious about this?
Then what about a pref to decide whether the user wants the basic or the
advanced cookie dialog?

Or what about just a "Show more details" dialog in the current cookie dialog?

Over 60% of the internet users use IE and I havn't heard a single complain about
the cookie dialog! Most people always just leave the default setting ("accept
all cookies") on.

So when you decide to change this setting to "ask me every time a cookie is
being set" then you're normally a pro!

Comment 6

[Jim Roskind]

This feature is very different from the old "show me each time a cookie is set."
The old feature was an endless nag, that no one could IMO endure (novice, or
expert!).

This dialog is meant to be usable by a relative novice.  The design is such that
you can typically say "No" only once to each cookie-setting-site, and not be
bothered very often at all. If the feature becomes an "expert only feature,"
then it will be interesting, but will have missed the mass market it was trying
to help.  To reach that audience, the basic dialog must be very simple.

If folks insist on a more complex dialog, I'd argue for having a pref hidden
deep within some advanced list of options (which is among the suggestions just
made above).  IMO, such a feature is nice... but is indeed another feature...
while our list is and can grow beyond bounds.

If someone wants to champion this "extra info via pref" feature from beyond the
walls of Netscape... because they feel it would be cool... then I would be glad
to see such things arrive in the tree. The critical point is that it must be
possible to have a relatively simple dialog that won't scare or confuse folks,
and it is certainly the case that the Netscape branded variant of Mozilla will
be interested in supporting that variant as a default (it is currently the only
sub-option for this feature).

Note that the default setting for cookie acceptance will continue to be "accept
all cookies."  However, if folks are concerned about cookies, then this option
should be a nice compromise (far from decline all).

Comment 7

[Ben Goodger (use ben at mozilla dot org for email)]

Maybe I'm not imaginative enough, but why would you want to see the contents of
the cookie anyway? 99 out of 100 times its uncomprehensible gibberish. As you
say, most people have this set to "Accept all cookies" and Mozilla provides a
dialog already that displays the value and other information about cookies
already stored on the computer, as well as giving the user the option to remove
those cookies.

I seriously doubt that anyone except the most masochistic geek would be able to
glean enough information from the cookie data attached to the IE dialog to form
a decision on whether or not they want to reject or accept the cookie.

As Jim says, this is not saying that someone cannot develop such a feature and
contribute it. Its just that we will not be using our own resources to develop
it when we believe the current system to be satisfactory.

Comment 8

[Henrik Gemal]

Ok, then what about just marking the bug as RFE?

Comment 9

[Henrik Gemal]

Would it be possible to expand the dialog to take advantage of XUL?

Comment 10

[Eli Goldberg]

gemal, you can re-open the bug as an RFE --- but, someone ultimately has to be
convinced to do the actual work to make it happen. If you code, go for it.

I'm re-opening and resolving this as WONTFIX --- there's nothing invalid about
this request besides the fact that three people have said that they don't
personally care for it.

Comment 11

[Eli Goldberg]

[altering resolution to WONTFIX]

Comment 12

[Eli Goldberg]

[verified]

Comment 13

[Matthew T (active 1999-2002)]

Are you allowed to do that, Eli? Verify your own resolutions??

IMO, this should be reopened and assigned to [e-mail], with HELP
WANTED. I, for one, regularly accept or deny individual cookies based on their
content.

If necessary, the cookie data could be shown by clicking a (persistent)
disclosure triangle in the dialog, so the dialog in its default form wouldn't
scare novice users.

Comment 14

[Matthew T (active 1999-2002)]

Reopening as requested by Eli.

Comment 15

[Matthew T (active 1999-2002)]

ObComment [this annoyance is currently under discussion elsewhere]

Comment 16

[leger]

Assigning all open "[e-mail]" bugs to "[e-mail]" to weed 
thru.

Comment 17

[Dan Erikson]

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

Comment 18

[leger]

Moving UE/UI component bugs over to new User Interface: Design Feedback 
component.  UE/UI component will be deleted.

Comment 19

[leger]

Assigning to [e-mail], help wanted to work on.

Comment 20

[Stephen P. Morse]

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

Comment 21

[bugzilla]

If there was a question before, there definately *IS* a demand for this:



8. Cookies - Netscape no longer tells me the contents of a cookie! Horrors! I 

want to be able to pick and choose the cookies I save based on their content, eg. 

ad cookies vs username/password cookies. Netscape 4.x had this wonderful feature. 

IE5 has a similar feature, but it doesn't show as much content or in as intuitive 

a manner. Do I have to go to the cookie manager to remove the cookies? Why can't 

I do this when the cookies come in?



(this was from a user's initial review of Netscape PR1 on macfixit:  http://

www.macfixit.com/ultimate/Forum21/HTML/000010.html)

Comment 22

[Kent Howard]

Attachment: Patch to show cookie data in cookie confirmation dialog.

Comment 23

[Stephen P. Morse]

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

Comment 24

[Matthew T (active 1999-2002)]

It would be nicer if the information was in a table, but this is better than 
nothing. Could someone review and check in? Jim?

Comment 25

[Stephen P. Morse]

From the above comments (Gemal and Roskind), it was agreed that if this feature 
went in it would have to be controlled by a pref.  However the submitted patch 
puts in the added information unconditionally, without testing any pref.

Comment 26

[Jim Roskind]

Reassigning to get help from non-netscape contributors.
If there is a complete patch created, please re-assign to the module owner, or
get permission, and land the fix.
It sure seems like the folks that want this would be happy with an "invisible
pref" that was set by manually editing the prefs.js. This would avoid the UI/UE
issues arround the pref, and make the UI presentation of the data less critical
to usability for an average user.

Comment 27

[Matthew T (active 1999-2002)]

Better than an invisible pref would be a disclosure triangle in the dialog. This 
is a preliminary design:

+--------------------------------------------------------+
|[] Cookie request ::::::::::::::::::::::::::::::::::::::|
+--------------------------------------------------------+
| Mozilla has been asked to store a cookie   (( Allow )) |
| while opening the address                  (  Deny   ) |
| [http://webmail.com.foo/$INBOX          ].             |
| Do you want to allow this cookie?          (  Stop   ) |
|                                                        |
| [/] Remember this decision                             |
|                                                        |
| |> About this cookie                                   |
+--------------------------------------------------------+

+--------------------------------------------------------+
|[] Cookie request ::::::::::::::::::::::::::::::::::::::|
+--------------------------------------------------------+
| Mozilla has been asked to store a cookie   (( Allow )) |
| while opening the address                  (  Deny   ) |
| [http://webmail.com.foo/$INBOX          ].             |
| Do you want to allow this cookie?          (  Stop   ) |
|                                                        |
| [/] Remember this decision                             |
| __                                                     |
| \/ About this cookie                                   |
|                                                        |
|           Server: webmail.com.foo                      |
|             Name: user-id                              |
|            Value: morse                                |
| Could be read by: any server in the domain .com.foo    |
|     Would expire: 2001-12-31                           |
|                                                        |
+--------------------------------------------------------+

If you opened/closed the `About this cookie' part for one cookie, that state 
would be remembered for future cookie dialogs.

Comment 28

[Jim Roskind]

My geek side loves the suggestion.  I agree, for folks that want to see the 
info, the above is the nicest way to expose it mentioned so far.

My fear of complications for an inocent user is fretting about any of this.

Again, if we could get outside contributors to land all this stuff, that would 
be great.  Because we're so crunched, Netscape folks won't have time to mess 
with UI smoothing etc at this point, so we'd still have to have a pref and let 
marketing and Netscape UI make a in/out call for Nav 6 after it landed in 
mozilla.

My *guess* is that we'll branch for Nav 6 in late September, so if external 
contributors are hoping it will hitch along into Navigator, it needs to land 
RSN.

Comment 29

[Jesse Ruderman]

mpt: I think the "could be read by" (or "for") field should be moved up into 
the always-visible portion of the dialog, and the "url requested" field should 
be moved down.  The domain for the cookie is shorter and more useful in 
deciding whether to accept a cookie (for example, moderately paranoid users 
don't want foo.dhs.org to set *.dhs.org cookies, but won't mind foo.dhs.org 
cookies).

"While opening the address" (or "URL requested") is vague btw... what address 
am I opening when I surf to altavista and get an engage or doubleclick cookie?  
Any ideas on how to fix that?

Comment 30

[Jacob Steenhagen]

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

Comment 31

[Marcia Knous [:marcia]]

Netscape nav triage team: based on Steve Morse's pre-triage recommendation, this 
is not a beta stopper.

Comment 32

[Francesco Chemolli]

Another thing that could be designed would be the ability to choose whether
accepting/rejecting any cookie from a site/domain, or to just accept/deny ONE
cookie (as identified by name) from that site/domain.
From the UI point of view, it would be just a matter of adding another button.
From the infrastructure POV, I'm not sure (I'm not a coder).

Comment 33

[Robin Lionheart]

The accept cookie dialog is not very useful right now, because it doesn't give
the user enough information to make an informed decision. If nothing else, it
should at least mention when the cookie expires.

Right now the dialog makes no distinction between a cookie that lasts for one
browser session and a cookie that tracks you until 2038. It would also be nice
if the cookie options had an option to automatically accept only cookies that
expire during this browser session.

Comment 34

[Jesse Ruderman]

An option to automatically accept only session cookies is bug 64336.

Comment 35

[Mark LTC]

Not having name & value information for a cookie is a show stopper for me.
I always review cookies to decide wether I want to allow/deny, the "remember
this decision" is great for the doubleclicks of this world, but in some cases
you want better granularity of control.

E.g: site example.com wants to set a cookie for user/password, and then
later on wants to set another for some arbitrary value which you don't wish
to store. In this case I want to permanently remember the user/password cookie
and permanently deny the arbitrary value. Doing it from the cookie alert box
is how it should ahppen, with an option in the cookie dialog box to turn on
expert cookie control to allow site/time/name granularity control on each
cookie. You may refer to it as "masochistic geek" behaviour but internet
privacy is a very high priority for a lot of people. Personally I'm going
back to Communicator until it's resolved.

Comment 36

[kellan]

Lack of this feature is one of the primary reasons I don't use mozilla. (and
wanting to submit an RFE for it was my sole motivation for creating this
bugzilla account)  I am a web developer, so not only do I not use mozilla for my
surfing, the lack of good cookie information is a major impediment to the use of
mozilla when developing websites.  This leads to my websites not being mozilla
compatible, which can only be a bad thing.  There have been some good UI
suggestions in this thread, that seem to address the "novice users will be
overwhelmed" concerns.  Also the current design creates no path for users to
move from novice to expert which would be a tragedy.  Thanks.

Comment 37

[Henrik Gemal]

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

Comment 38

[Anthony DeRobertis]

My report has been marked a duplicate of this bug.

Comment 39

[Anthony DeRobertis]

Attachment: iCab accept cookie dialog

Comment 40

[Boris Zbarsky [:bzbarsky]]

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

Comment 41

[Christopher Aillon (sabbatical, not receiving bugmail)]

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

Comment 42

[Stephen P. Morse]

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

Comment 43

[Dave Miller [:justdave]]

The lack of cookie information is one of the reasons I continue to use iCab when
surfing sites I'm not familiar with.  Just knowing if a site is setting cookies
or not doesn't me absolutely no good whatsoever.  I need to know what kind of
cookies they're setting before I can make an informed decision.  There are ways
to show this information without overwhelming a novice.  (See lots of good
comments above)

Comment 44

[Doron Rosenberg (IBM)]

once <expander/> gets into the tree, will take a look at this.

Comment 45

[Joosep-Georg Järvemaa]

So, folks... I turned cookie warning on and what a dissapointment - i see only a
 nothing-containing dialog the cookie... Is it too much to add a button
("Details >>") which allows to see complete information about the cookie? And
the details are show every next warning until i click "<< No details" some other
time...

Comment 46

[Norbert Pabis]

I support all those people that demand more information about cookie in
confirmation dialog.
Do it as you want:
- hidden pref
- UI pref
- "more details" button

But people who create websites really need it to debug ther work.
If you care so much about novice users hide this feature , but please *enable*
it anyway.

Comment 47

[Diego Biurrun]

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

Comment 48

[Matthias Versen [:Matti]]

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

Comment 49

[Phil Miller]

Attachment: Opera 6's Cookie Dialog

Note all of the wonderful information it contains. Also not the options that
Opera gives for EACH INDIVIDUAL COOKIE, IN ADDITION TO BY SITE/SERVER!

Comment 50

[Chris Campbell]

CookiePal's UI is better, IMO.  See bug 102198 for screenshot.

Comment 51

[Simon Spiegel]

iCab's (see attachment) is still better and offers the most possibilities.

Comment 52

[Joosep-Georg Järvemaa]

It must be possible to accept/refuse all cookies for this domain or from this 
server.

PS. iCab's one doesn't show server which wants to set cookie.

Comment 53

[Matthew T (active 1999-2002)]

Attachment: Mini-spec update (obsoletes comment 27)

Comment 54

[a_geek]

Well, it sort of worked, with 0.9.8 and 0.9.9 under Linux, but breaks again
with build #2002022613, also under Linux.

Can we please have some non-JoJo-behaviour of such bugs, and please up the
severity to major (See also comment #43 and possibly other comments relating
to privacy)?

Thanks...

Comment 55

[Matthias Versen [:Matti]]

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

Comment 56

[Robin Lionheart]

That makes 11 duplicates of this bug. It's getting close to making the Most Frequently Reported Bugs list at http://bugzilla.mozilla.org/duplicates.cgi.

Comment 57

[a_geek]

hello,

it appears to be broken again with build id 2002040306 under linux :(

please up the severity to major. thanks


best.
--toni++

Comment 58

[Ian Pottinger]

adding self to cc list

Comment 59

[Quinn Yost (mythdraug)]

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

Comment 60

[a_geek]

Hello,

want to confirm that the bug is still there (build # 2202042021, ie, post-1.0rc1)

Please also see comment #57.

Why is this bug unowned? Sounds like if nobody wants to work on it :(


Thanks!

Comment 61

[timeless]

now it's owned. please hurry up and fix it. if you have no intention, please 
reassign it to nobody.  really, no one is working on this. Doron said he would 
after he lands expander.

Comment 62

[Christian :Biesinger (don't email me, ping me on IRC)]

a_geek, note that this is not a bug, but an enhancement.

also note that it depends on a bug that hasn't been fixed yet, so this one can't
be fixed either.

but feel free to fix bug 85045 and this one yourself. otherwise please shut up.
thanks.

Comment 63

[a_geek]

Wrt. comment #62, I'd like to say that a bug, err, "enhancment" with 49 votes
and many dupes (and counting) is not an enhancement, but a real bug.
Without this bug fixed you often can't make an informed decision
whether you want to accept or refuse a cookie.

Please also note that it worked at some point in the past, so breaking
it again definitely is a bug, and that I consider the dependency on a
GUI element to expand and collapse the details (bug 85045) is invalid,
or at least _much_ less important than just to show the data. _That_
would be an "enhancement".

And no, I'm not going to shut up.

Comment 64

[John Levon]

a_geek, you don't need to shut up, this bug is asssigned to you.
We're all waiting for a patch.

Comment 65

[a_geek]

Hello,
below are a few ideas on what and how to do, as well as a few questions. Enjoy ;)

- At first, the cookie dialog should just show the complete cookie data.

- For enhancements to the UI, likely forcing a rewrite, I have different
 ideas on how to do it: For one, I guess most users have their preferences
 about the cookie dialog and don't intend to switch from one cookie to
 the next. Hence there should be some controls in the appropriate
 preferences dialog where the user can select which fields to view by
 default. Next, some cookies are really big and thus ugly to handle. I'd
 like to see a cookie dialog box on the screen that's at most as wide as the
 screen, but to accomodate huge cookies, they should show the contents in
 a "window" that can be panned right and left, using a scroll bar if
 necessary. For the other details, it would imho be "well enough" if the
 cookie dialog shows one item at a time, eg. expiry or path. There could
 be one control like a drop-down menu where the user can select the
 attribute of the cookie he wants to see, with the value display flipping
 accordingly (would save screen real estate, and the neccessary controls
 are already there, at least in other parts of the UI). If someone would
 strive for the "default attributes" idea outlined above, then these
 could be just statically displayed w/o needing such a fancy control.

([e-mail] suggested that the cookie dialog should present the cookie
data in a fashion that allows cut&paste as well: "multiline wrapped,
scrollable, copyable.")

Comment 66

[Sander]

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

Comment 67

[Sander]

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

Comment 68

[cw]

Apologies for the recent duplicate, guess I just didn't try all the possible
search terms for this.
As there hasn't been an update for a while does anyone know of the status of this?

My comments:
The idea that fits well with me is the one where the normal dialogue is
presented with a "More info" button that shows the content of the cookie (maybe
an enhancement could be to have pref.js options to control excatly what that
button reveals, default would be everything)

Anyhow, something along the lines of the way IE does it is a good start, extra
functionality can come later.

Comment 69

[Matthias Versen [:Matti]]

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

Comment 70

[Alex Bishop]

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

Comment 71

[Stephen P. Morse]

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

Comment 72

[Alan W. Presley]

So, is this still a debate on whether or not to enhance or how to program the enhancement?

Comment 73

[Michiel van Leeuwen (email: mvl+moz@)]

re-assinging to self, as I have a patch.

Comment 74

[Michiel van Leeuwen (email: mvl+moz@)]

Attachment: patch v1

A first attempt for a patch.

It doesn't follow mpt's spec's. What I did was just immitate the current dialog
box, and add a disclosure button. That button shows the content of the cookie.
Further improvements can be discussed in others bug's I think.

All comments are welcome, especially about the c++ part. I'm not sure if my way
of opening the dialog is the correct way. As far as I know, I can not use the
prompt service, because that doesn't allow custom xul.

Comment 75

[Stephen P. Morse]

Could you please attach a screen shot of your dialog.  Also make sure you get
module owner's permission before checking in such a patch.

Comment 76

[Michiel van Leeuwen (email: mvl+moz@)]

Attachment: screenshot of the dialogs

The screenshots. Only modern.

Comment 77

[Michiel van Leeuwen (email: mvl+moz@)]

Attachment: screenshots of the dialogs

Screenshots of the dialogs as in the patch.

Comment 78

[Christian :Biesinger (don't email me, ping me on IRC)]

the "More Info" button should change its label to "Less Info" when clicked (and
on click hide the additional info)

Comment 79

[Henrik Gemal]

the icab dialog
http://bugzilla.mozilla.org/attachment.cgi?id=34985&action=view
is really cool. perhaps we could make it look more like that than ie

Comment 80

[Simon Spiegel]

See comment #52, we need an option to accept/allow all cookies from a specific
server or domain.

Comment 81

[Alan W. Presley]

screen shot looks EXACTLY like what is needed!  Good Job!
Will there be a way to set a preference so that all future cookie notification stays in "More Info" mode?

Comment 82

[Alex Bishop]

When the cookie details are shown, I think the checkbox should move to the
bottom of the dialogue along with the buttons. It looks a bit lost staying at
the top.

I think the label of the 'More Info' button is a bit vague. If I hadn't already
known the purpose of the button I would have assumed that it was a help button.
I suggest renaming it to 'Cookie Details' or something.

With the current dialogue, the wording makes it clear that checking the checkbox
will accept/reject all cookies from the site in question. However, when the
details are shown, some users may get confused and think that the box applies
only to that specific cookie. IE gets round this by labelling its checkbox
'Apply my decision to all cookies from this Web site'. Similiar labelling could
be used for the Mozilla dialogue (possibly without the word 'Web' as it's not
used in the question at the top of the dialogue).

Comment 83

[Stephen P. Morse]

What is the difference between attachment id=95538 and attachment id=95539?

Are the fields describing the cookie editable or are the read only?  If the
former, why?

Comment 84

[Christian Reis]

On layour:
- The labels should be right-aligned, not left-aligned.
- When clicking on More Info, this design has the buttons moving to the bottom
of the window, which will force the user to move the mouse if he wants to hide
details. This will also make it less convenient for him to click on Accept or
Decline. For this reason, I recommend having a layout where the buttons stay
where they are, and the contents added to the bottom of the window (not unlike
mpt's design). The checkbox should stay at the same position, additionally.
- To acheive this, it may be that the accept/decline buttons will be better off
vertically-aligned to the right, and the ( More Info ) button left-aligned at
the bottom. This would have the effect of presenting a similar layout to mpt's
suggestion, sans the disclosure triangle.

About wording:
- Instead of "Remember this decision", I suggest "Use this choice for all
cookies from this site". 

Comment 85

[Michiel van Leeuwen (email: mvl+moz@)]

Thanks for al the feedback so far.

The biggset problem with the placing of the buttons is that when using a
<dialog> you can't place anything under the buttons. So when the buttons are
moved, I need to use <window>, and create the buttons myself.

About the vertical placing of the buttons: that is possbile, but used nowhere
else in mozilla, as far as I know. Is it a good idea then?

The 'More Info' Button currently works as a toggle, and it remebers it's state.
The textboxes are read-only. The labels in other places in mozilla are also
aligned left. It seems inconsistent to change that. 

The 2 screenshots are excatly the same. I just made a mistake while uploading.

And who is the module owner? There is no 'cookie' component on
http://www.mozilla.org/owners.html

Comment 86

[Stephen P. Morse]

> And who is the module owner?

I am.

Comment 87

[Michiel van Leeuwen (email: mvl+moz@)]

Attachment: New screenshot

I changed some wording and the location of the buttons.

The patch for that needs some work. I will upload that tomorrow.

Comment 88

[Christian Reis]

Comments on the latest screenshot:

- I suggest using "Contents" instead of "Information" as it is a bit more
descriptive.
- I think the question "Do you want to allow it?" isn't necessary, since
Accept/Decline already matches the intention in "wants". This would make the
dialog less wide and easier to scan.
- "Cookie info" could become "Show details" and "Hide details" (depending on
state), and I would suggest aligning it to the left so the layout would become:

    /|\  The site .foo.bar wants to modify an existing cookie.  
   / * \ [ ] Use this choice for all cookies from this site.
     
  ( Show details )                  (  Accept  ) (( Decline ))

  +-- About this cookie -------------------------------------+
  |                                                          |

I'd also like to say thanks for working on this; this has been a top request for
*very* long.

Comment 89

[Alex Bishop]

I think 'Allow' and 'Deny' would be better wording than 'Accept' and 'Decline'.
'Accept' and 'Decline' work well when a site is setting a cookie but they don't
make as much sense when a site is asking to modify a cookie.

Comment 90

[Stephen P. Morse]

I was initially against this bug because showing too much information in the 
cookie nag box is confusing to users.  It was not be accident but by deliberate 
design that the nag box in mozilla shows less information than the nag box in 
4.x.

However I like the work that was done here and I think that the added 
information would be useful (to the .1% of users who want it) provided it has no 
negative impact on the users who don't want it.  That means that the current 
nag box should not be changed other than having the one added button to get more 
information.

That said, I would be willing to approve this patch provided the appearance and 
wording in the current nag box is retained.  If you want to change the wording, 
then open a separate bug on that.

That means that the text would remain as "The site X wants to set a cookie.  Do 
you want to allow it?", that the checkbox text remains as "Remember this 
decision", and that the buttons remain as "Yes" and "No", with an added button 
to get the cookie information. 

Comment 91

[Jeremy M. Dolan]

I agree with morse, except for two nits:

More then 1 in 1000 users want this. Closer to 1 in 10. Remember. You don't get
the box in the first place unless you've entered prefs, expanded the privacy
group, and modified your default cookie acceptance policy. 1 in 10 of THOSE
users want this. I'm assuming this is why there's no help button in the dialog.

That said, you're still right. It's very important it has "no negative impact on
the users who don't want it".

This is the second nit: "and that the buttons remain as "Yes" and "No"". While
other wording changes would need to be debated by UI people, can't we all at
least agree Yes/No are bad dialog button options?

Comment 92

[Stephen P. Morse]

That particular issue (yes/no) has been debated previously regarding this dialog 
and the agreement was yes and no (it's the answer to the question posed).  I 
have no objection if you want to open that debate again, but doing so here would 
jeopardize the fine work that Leeuwan has done.  I recomment we get his work 
checked in now using the original wording, and then debate changing the wording 
in a separate bug report.  (This bug report is about adding new information, not 
about changing the labelling of buttons.)

Comment 93

[Michiel van Leeuwen (email: mvl+moz@)]

I can provide a patch with the old wording. But is agree with comment 82 that
the checkbox question is a bit vague when the cookie details are displayed. We
should change that line.
And where do you think the buttons should be placed? Like in the last screenshot?

Comment 94

[Stephen P. Morse]

I have no strong feelings about the placement of the buttons in the expanded
view.  My only concern is that the normal view not be changed (except for the
addition of one extra button).  That includes the wording on the checkbox.  If
there is disagreement on that wording, that should be the topic of a separate
bug report.

Comment 95

[Christian Reis]

Agree with Morse that wording should be a separate bug to avoid conflicts of the
religious order on this overburdened bug. 

I do advise for the button layout to be as I suggested in the last mockup, and
the use of `Contents' as label for the cookie data item.

Filed bug 163350 (with Alex's suggested button wording).

Comment 96

[Stephen P. Morse]

> I do advise for the button layout to be as I suggested in the last mockup,

As I said before, I have no strong feelings either way about the button placement

> and the use of `Contents' as label for the cookie data item.

That would make the wording in this dialog be inconsistent with the wording in
the cookie-manager dialog.  If you want to change the wording, it needs to be
changed in both places.  Again, open a separate bug report if that's what you want.

Comment 97

[Matthew T (active 1999-2002)]

> About the vertical placing of the buttons: that is possbile, but used nowhere
> else in mozilla, as far as I know. Is it a good idea then?

It's compulsory. There are two simple requirements you need to meet.
(A) The buttons must not change position between states, otherwise you get the
    where's-the-off-switch problem Kiko described.
(B) The distance between a button and the edge of the window must not change
    between states, otherwise the window edge cannot be used as a visual guide
    to find the button (like it can in every other alert or dialog).
That's why the buttons need to be at the top right, as I showed in attachment
63889 [details], not the bottom right. Another example of this arrangement is the Search
Messages window in mail/news.

Other problems with attachment 95538 [details]:
2.  Without the individual cookie info shown, `Remember this decision' is merely
    highly misleading. With the individual cookie info shown, however, the
    checkbox is Just Plain Wrong and must be changed. The checkbox does *not*
    remember this decision, i.e. whether the cookie shown should be accepted;
    rather, it determines whether *any* cookie should be accepted or declined
    for the same host. That can be shown more truthfully, more obviously, with
    less clutter, and with fewer clicks required, if using a pair of buttons
    rather than a checkbox (as I showed in attachment 63889 [details]).
3.  The individual cookie info should be indented the same amount as the initial
    question, rather than wrapping under the /!\ icon.
4.  The field labels should be right-aligned, not left-aligned.
5.  In any row of adjacent pushbuttons, either all of them should open/close
    windows, or none of them should. So you should not have `More Info', which
    does not open/close a window, right next to `Accept' and `Decline' which do.

Comment 98

[Michiel van Leeuwen (email: mvl+moz@)]

Attachment: Screenshot v3

This is my newest screenshot. It has the labels aligned to the right. Is has
the information-box aligned with the text above. The text on the 'more info'
button also changes on pressing it.
It has the same wording as the current box, execpt for the checkbox-question.
That would be to vague when using the current text. And the new wording won't
have effect on existing users.
I suggest to move the discussion about moving the buttons to the right and
vertically align them to the wording bug.

Comment 99

[Michiel van Leeuwen (email: mvl+moz@)]

Attachment: patch v3

The patch to get the previous screenshot.

Comment 100

[Robert Accettura [:raccettura]]

I like this one.  While silent here, I have been keeping a close eye on this bug.  
The last version seems to fit the bill just right.  

Comment 101

[Christian Reis]

> (B) The distance between a button and the edge of the window must not change
>    between states, otherwise the window edge cannot be used as a visual guide
>    to find the button (like it can in every other alert or dialog).

In the layout Michiel has implemented, it is highly unlikely that the distance
to the top and right edges of the window will change. Compounded to the fact
that dialogs in Mozilla usually have horizontal layout for the buttons, I'd say
let this stay for now. We've been ages discussing this bug and we finally have a
reasonable UI for it, IMO.

We can avoid further religious warfare, by postponing this change till we can do
some user testing; this can be done together with rewording for bug 163350.

Which brings me to ask. Michiel, what happens when the cookie contents are very
long? Is the dialog fixed width?

I've filed bug 163380 on the change from Information to Contents (and lets not
let that fall into limbo please). (Michiel, again, reassign to nobody if you
don't feel like doing them)

I'm removed dependency on the disclosure element; someone can file another bug
to block this when the time comes and disclosure comes into being. I've added
the bugspill I filed as blocked bugs. I'm also assigning to the patch provider.
Michiel, once again, thanks for the work. 

Since this seems to have some consensus going, can we move on to getting reviews
on this patch? Morse?

Comment 102

[Tim]

Comment on attachment 95771 [details] [diff] [review]
patch v3

>+++ mozilla/extensions/cookie/resources/content/cookieAcceptDialog.xul	18 Aug 2002 19:54:55 -0000
[snip]
>+
>+<!DOCTYPE window SYSTEM "chrome://cookie/locale/cookieAcceptDialog.dtd">

Change this to:
<!DOCTYPE dialog SYSTEM "chrome://cookie/locale/cookieAcceptDialog.dtd">
(I've already got a huge patch in another bug to fix other occurrences of this;
I don't need any more =)

>+
>+<dialog
>+    id="cookieperm-window"
>+    buttons=""

Comment 103

[timeless]

comments on the patch up to comment 100 (iow there will be a bit of duplication
from the later comments, sorry)

i agree w/ mpt's spec (attachment 63889 [details]), but while you're ignoring it please
change 'site' to 'domain':
+<!ENTITY     button.rememberdecision.label  "Use this choice for all cookies
from this site">

why this matters:
1. load http://www.washingtonpost.com
2. accept a bunch of cookies from the domain www.washingtonpost.com
3. you'll one other cookie (perhaps) from domain questionmarket.com

the 'site' is 'http://www.washingtonpost.com', but your choice *must* affect the
cookie domain (www.washingtonpost.com), not the site. if it affects the site
then it would mean that you automatically accepted the third party cookie
(questionmarket.com).

Also note that IE doesn't let you hide the extra information. The logic is
pretty simple:
a. you asked for it
b. your request for more information is not a persistent value
c. your request was about a single cookie (the next dialog you get will require
you to ask for more info too)

anyway, since you persist the state between dialogs (!b), ie's approach doesn't
work.

I suppose i might as well do a partial code review.
random nits (note that the nits generally apply to the whole patch, not just the
random instances i mention):
1. your use of whitespace should be consistent with the file
a.
-  if (!dialog) {
-    *checkValue = 0;
-    return PR_FALSE;
+  if(cookie_s) {
+    nsCOMPtr<nsIWindowWatcher>
wwatch(do_GetService("@mozilla.org/embedcomp/window-watcher;1"));
+    if (!wwatch) {
a' you just added two different styles |if(| and |if (| to replace a single
style |if (|

b.
+    res = block->SetInt(1, *checkValue);
+
+    res = block->SetString(2, NS_ConvertASCIItoUCS2(cookie_s->name).get() );
b' |foo(...)| v. |foo(... )|

c.
+    gDateService = Components.classes[nsScriptableDateFormat_CONTRACTID]
+      .getService(nsIScriptableDateFormat);
c' the second line should line up with something useful, most of us align it
with .classes

d.
+  document.getElementById('ifl_isSecure').setAttribute("value",
+    params.GetInt(6) ?
d' like c', i'd align it with "value"

2. directly calling QueryInterface is generally frowned upon:
+      active->QueryInterface(NS_GET_IID(nsIDOMWindowInternal),
getter_AddRefs(activeParent));

2' something like this is prefered: (this code isn't tested)
+    nsCOMPtr<nsIDOMWindow> active;
+    wwatch->GetActiveWindow(getter_AddRefs(active));
+    nsCOMPtr<nsIDOMWindowInternal> activeParent(active);

3. it's nice if the original code line is meaningful (something talking about
the cookie acceptance dialog or at least cookie management)
+ * The Original Code is Mozilla.org code.
3~ if you were to write mozilla.org, you would spell it 'mozilla.org', not
'Mozilla.org' @see http://www.mozilla.org/README-style.html

4. you need verify stuff exists and is non null before using it
+  params = window.arguments[0].QueryInterface(nsIDialogParamBlock);
a. "arguments" in window
b. window.arguments.length
c. QueryInterface will throw an exception, so instead of null checking a/b you
should probably wrap it in a try block.

5. please use meaningful constants instead of magic numbers:
+  var messageText = params.GetString(0);
+  params.SetInt(0, 0); // say that ok was pressed

6. (this is a personal nit, you or morse may disagree in which case it's
discardable, well all nits are discardable, but you might have trouble getting
approval for your code if some of them aren't addressed)
you're explicitly comparing against true/false:
+  if (document.getElementById('infobox').hidden == false) {
6' i'd simply write:
+  if (!document.getElementById('infobox').hidden) {

7. variables used with magic numbers should have meaningful names:
+function GetExpiresString(expires) {
+  if (expires) {
+    var date = new Date(1000*expires);
perhaps |secondsUntilExpiring|
[for readers: Date() accepts GMTmilliseconds - well maybe that's UTCmilliseconds]

8. doctype should match the root element
+<!DOCTYPE window SYSTEM "chrome://cookie/locale/cookieAcceptDialog.dtd">
+
+<dialog
8' <!DOCTYPE dialog SYSTEM ...>

9. please put the id on the same line as the node:
+<dialog
+    id="cookieperm-window"
not doing so makes grep/lxr output useless.

10. i think you're encouraged to include a type attribute:
+  <stylesheet src="chrome://global/skin/dialog.css"/>
+  <script src="cookieAcceptDialog.js"/>
(correct types for the lines above would be text/css and
application/x-javascript respectively)

11. use <?xml-stylesheet ?> instead of <style> in xul files
for example:
/mailnews/extensions/smime/resources/content/msgReadSecurityInfo.xul, line 39 --
<?xml-stylesheet href="chrome://messenger/skin/smime/msgReadSecurityInfo.css"
type="text/css"?>

12. <dialog><hbox/></dialog> is silly
<dialog orient="horizontal"/> should work.

13. localization/skinability warning. inline styles in packages are generally
frowned upon.
+      <vbox flex="1" style="max-width: 45em;">

14. err.
+          <checkbox id="checkbox" oncommand="onCheckboxClick(this);"
label="&button.rememberdecision.label;"/>
a. a generic checkbox handler is useless
b. you do not need to pass this to oncommand
c. passing this may be expensive
a'b'c' perhaps onToggleAlwaysAcceptFromDomain()
d. please don't use generic names for ids.
d' id="persistDomainAcceptance" might be better

15. cache strings
+   
document.getElementById('disclosurebutton').setAttribute("label",cookieBundle.getString('showdetails'));
every second time you click the button it will reretrieve this string. that's silly.

16. don't confuse localizers or reviewers by inserting inconsistent whitespace
+no=no
+AtEndOfSession = at end of session
16' please remove the spaces around '=' unless they are meaningful, in which
case you should provide a localizer note explaining why they are there.

17. don't change your case convention for identifiers 
+no=no
+AtEndOfSession = at end of session
+
+showdetails=Show details
17' atEndOfSession showDetails

nb. you may not need a .properties file, it's possible to rely on xul/dtd instead.

Comment 104

[Alex Bishop]

The latest screenshot uses 'Show details' and 'Hide details'. Please change those
to 'Show Details' and 'Hide Details' (note the capital 'D's) to be consistent
with the case on the other buttons in Mozilla.

Comment 105

[Stephen P. Morse]

Please do not make any changes to the wording.  This bug is about adding more
information about the cooke and not about changing existing wording.  Any
requests for such changes should go into separate bug reports.

That includes the checkbox wording of "remember this decision".  And it includes
the changing of "site" to "domain".  That latter issue, by the way, was
evaluated quite heavily by the tech pubs department, and they had very sound
reasons for going with "sites" even though it wasn't technical correct.

Also please restore the alignment of the labels to be left justified so that
they will be consistent with the cookie-manager dialog.  If you want to change
that, then you will need to change them both at the same time, and again that
needs to be a separate bug report.

I think it's strange that you would intentionally waste real-estate by having
the "about this cookie" box be aligned with the text above it rather than coming
to the left margin.  But if that's what you really want, I won't object.

Finally, in regards to the question about what happens if the text is too wide
to fit in the field, I would hope that the field is scrollable in the same
manner that the corresponding fields in the cookie-manager dialog are.  Is that
indeed the case.  (Of course, if the box came up to the left margin, you would
have more information in the field before it would need to be scrolled.  But
that's your call.)

Comment 106

[Michiel van Leeuwen (email: mvl+moz@)]

Attachment: Patch v4

This patch uses the original wording everywhere. The only extra text is for the
details-button, which is labeled |Show Details| or |Hide Details| depending on
the state.
The alignment of the labels is back to the left.
Most of the remarks of timeless are fixed. Thanks. I only had a few problems:

> 12. <dialog><hbox/></dialog> is silly
> <dialog orient="horizontal"/> should work.
somehow that doesn't work. Don't know why. 

> nb. you may not need a .properties file, it's possible to rely on xul/dtd 
> instead.
How do I do that? Could not find any info on that.

Comment 107

[Stephen P. Morse]

>> nb. you may not need a .properties file, it's possible to rely on xul/dtd 
>> instead.
> How do I do that? Could not find any info on that.

I've cleverly done that in the past by creating a dummy attribute in a xul tag,
having that attribute get its value from the dtd file, and then having the js
file read the value of that attribute.

But at times I've had super-reviewers reject that by saying it was a "hack" and
insisting that I use a separate .properties file.  So you're dammed if you do
and dammed if you don't. :-(

Please post a screenshot showing what your latest patch looks like.

Comment 108

[timeless]

:-( mozilla crashed
1. i agree with morse that you shouldn't indent the details, ie doesn't either
and it fixes my complaint about a horizontal dialog :)

2. re constants, if you stick a constant into an idl file then in c++ you can do
|<interface>::<constant>| and in js you can do
|Components.interfaces.<interface>.<constant>|.

if you don't do that then please prefix 'const ' to your constant definitions in
the js file.

3. check return values:
+    res = wwatch->OpenWindow(active,
"chrome://cookie/content/cookieAcceptDialog.xul", "_blank",
+                             "centerscreen,chrome,modal,titlebar", block,
+                             getter_AddRefs(dialogwin));
+
+    /* get back output parameters */

openWindow can fail, hence the result which you should check (which per jst nit
should be called |rv|, but we'll ignore that unless morse/jst demand it) before
proceding.

keep up the good work :-).

Comment 109

[Michiel van Leeuwen (email: mvl+moz@)]

Attachment: patch v5

While creating the requested screenshot I noticed that the expires-date wasn't
displayed. Now it is. 
And while I was doing that, I also corrected the latest remarks.

Comment 110

[Michiel van Leeuwen (email: mvl+moz@)]

Attachment: screenshot v5

The accompanying screenshot

Comment 111

[Stephen P. Morse]

That looks fine to me, providing you change the spelling of "remeber" to
"remember".  ;-)

as module owner, a=morse

timeless seems well along on doing the review, so I'll leave the r= to him.

Then all you'll need in order to check in is an sr.  Let me know if you don't
have checkin privileges, in which case I'll check it in for you.

Comment 112

[Henrik Gemal]

in the cookie manager the Expires is shown in locale date, like:
13 august 2012 15:03:46
for me on my english win2k localized to Danish.

is this also thought into the patch?

Comment 113

[timeless]

hg: this patch uses the same code as the cookieviewer or whatever, so it should
do the right thing.

mvl:
+  if (!this_cookie) {
+    PR_FREEIF(path_from_header);
+    PR_FREEIF(host_from_header);
+    PR_FREEIF(name_from_header);
+    PR_FREEIF(cookie_from_header);
+    nsCRT::free(setCookieHeaderInternal);
+    return;
+  }
PR_FREEIF is really PR_DELETEIF, since you're about to return, you shouldn't use
it (it's a waste of a cycle to assign 0 to the pointers) -- as usual if morse
wants to trump this request you may ignore it (to be consistent with the file)
-- instead please use if (foo) PR_Free(foo);

+    nsCOMPtr<nsIDialogParamBlock>
block(do_CreateInstance("@mozilla.org/embedcomp/dialogparam;1"));
please add a check for |if (!block)|
+    block->SetString(eMessageText, szMessage);

if you're using both of these, please make their strings consistent:
+<!ENTITY     button.showdetails.label       "Show details">
+<!ENTITY     button.hidedetails.label       "Hide details">
+showDetails=Show Details
+hideDetails=Hide Details

i've decided to waste time in the spellchecker, can you change "Int's" and
"Strings" to "integers" (or "numbers") and "strings"? or at least make the
case/word choice consistent in these lines:
+// Continue at a lower _number_, as > 7 gives problems (max _int number_ is 8),
+// And you the next items are _Int's_ and not _Strings_.
+// _Int's_ and _Strings_ are separate, thay can have the same index

(from email) "thay"
perhaps:
+// Numbers and strings are handled separately so they can share the same indices.

persistance should be persistence (thanks n7p1)

morse, can we change:
+  PRBool isDomain;   /* is it a domain instead of an absolute host? */
to
+  PRBool isDomain;   /* it i a domain instead of a[n absolute] host */
? everytime i see that question mark i ask myself if the coder is
confused/concerned and then i stop and answer, no it's just a comment...

I think [n absolute] should be dropped:

absolute in dns has a specific meaning <q
src="http://docsrv.caldera.com/NET_tcpip/dnsC.domname.html">
Each name can be relative or absolute (fully qualified). A fully qualified
domain name ends at the root domain, indicated by a trailing dot (example:
volga.mynet.com.). A domain name minus the dot may be seen as a relative domain
name (example: volga.mynet). Again, parallels exist with the UNIX filesystem, in
which the file /usr/bin/hack may be indicated as /usr/bin/hack (its absolute
name) or simply hack (its relative name if the directory is known to be
/usr/bin).</q>

I presume that this flag is talking about "discovercard.com" vs.
".discovercard.com" and not "discovercard.com" vs "discovercard.com." (note that
i actually do load both sites, and I am always disappointed to see that i get
the wrong site when i add the trailing dot.

I think i'm done (don't forget to fix the spelling error morse caught).

Comment 114

[Michiel van Leeuwen (email: mvl+moz@)]

Attachment: patch v6

Spelling changed. I should not try to write decent English late in the night
anymore. Just go to bed. :-)

I will leve the PR_FREEIF to morse. I hope he will check this in for me, as I
don't have permissions. So maybe he can do it at the same time.

(No screenshot this time. That didn't change)

Comment 115

[Stephen P. Morse]

timeless wrote:

> PR_FREEIF is really PR_DELETEIF, since you're about to return, you shouldn't use
> it (it's a waste of a cycle to assign 0 to the pointers) -- as usual if morse
> wants to trump this request you may ignore it (to be consistent with the file)

Leeuwen wrote:

> I will leve the PR_FREEIF to morse.

I have no strong feelings one way or the other regarding changing those
occurences of PR_FREEIF to PR_DELETEIF.  If you'd like to do so, please provide
a revised patch containing it.  That will make my eventual check-in easier.

Have you requested a super-review of these changes?  If not, you should do so. 
Timeless, are you prepared to add your r= to this patch?

Comment 116

[Michiel van Leeuwen (email: mvl+moz@)]

I checked wat PR_DELETEIF does, and according to lxr, it doesn't exist :-)
So I wan't change it.... http://lxr.mozilla.org/seamonkey/ident?i=PR_DELETEIF

Who can I ask for super-review?

Comment 117

[Alex Bishop]

> Who can I ask for super-review?

See the list at http://www.mozilla.org/hacking/reviewers.html.

Comment 118

[timeless]

yeah i'll mark an r=
the change is from 
PR_FREEIF(foo) to if (foo) PR_Free(foo);
what i meant is that the meaning of PR_FREEIF is actually PR_DELETEIF
PR_FREEIF(foo) => if (foo) PR_DELETE(foo)
PR_DELETE(foo) => { PR_Free(foo); foo=NULL; }
anyway, i'm sick of explaining this. bug 163696 filed.

i did try to explain what to do in comment 113:
-- instead please use if (foo) PR_Free(foo);

Comment 119

[Quinn Yost (mythdraug)]

I had the following errors trying to compile with v6.   (Win2k)

nsCookies.h(83) error C2146: syntax error: missing ';' before identifier 'expires'
nsCookies.h(83) error C2501: 'time_t' : missing storage-class or type specifiers
nsCookies.h(83) error C2501: 'expires' : missing storage-class or type specifiers
nsCookies.h(84) error C2146: syntax error: missing ';' before identifier
'lastAccessed'
nsCookies.h(84) error C2501: 'time_t' : missing storage-class or type specifiers
nsCookies.h(84) error C2501: 'lastAccessed' : missing storage-class or type
specifiers

Comment 120

[Michiel van Leeuwen (email: mvl+moz@)]

Attachment: patch v6b

Missed #include <time.h>
That broke the build. Thanks for testing.

Comment 121

[Quinn Yost (mythdraug)]

A couple JavaScript strict warnings:
  cookieAcceptDialog.js line 72:   cookieBundle undeclared
  cookieAcceptDialog.js line 90:   params undeclared

And a JavaScript error:
  cookieAcceptDialog.js line 143:  eCheckboxState not defined

  I'm being prompted for all cookies, regardless of telling the confirmation to
"Remember this decision".

Comment 122

[Michiel van Leeuwen (email: mvl+moz@)]

Attachment: patch v6c

Fixed the JS errors. The not remembering error was alsa a JS error. I should
learn to turn on strict js errors.

Comment 123

[Tim]

Comment on attachment 96194 [details] [diff] [review]
patch v6c

>Index: mozilla/extensions/cookie/nsPermissions.cpp

>+    block->SetString(eCookieName, NS_ConvertASCIItoUCS2(cookie_s->name).get());
>+    block->SetString(eCookieValue, NS_ConvertASCIItoUCS2(cookie_s->cookie).get());
>+    block->SetString(eCookieHost, NS_ConvertASCIItoUCS2(cookie_s->host).get());
>+    block->SetString(eCookiePath, NS_ConvertASCIItoUCS2(cookie_s->path).get());
Those pointers (from get() ) are dangling.  SetString does not copy, so you
either need to hold a reference to the strings until after the dialog closes or
copy the PRUnichar*s (the former sounds cheaper =).

Comment 124

[Michiel van Leeuwen (email: mvl+moz@)]

Attachment: patch v7

New Patch. Pointer issue should be fixed now.

Comment 125

[Christian :Biesinger (don't email me, ping me on IRC)]

+yes=yes
+no=no
+atEndOfSession=at end of session

You dislike uppercase?

+// XXX Can I share these with nsIPermissions.cpp (or .h) somehow?

if it's an .idl file, you can. else, you can't. :)

that's just what I spotted right now with my being tired...

Comment 126

[Stephen P. Morse]

Christian,

"yes", "no", and "at end of session" are currently lower-cased in the
cookie-manager dialog.  So if you want to change that behavior, then open a new
bug on that issue.  For this bug, we need to be consistent with the current
implementation.

Comment 127

[Diego Biurrun]

Isn't this patch ready for review yet, I have been running it without problems
for some time.  Great work!

Comment 128

[Michiel van Leeuwen (email: mvl+moz@)]

Yes, I think so. timeless or morse, can one of you mark the last patch r=?

Comment 129

[Stephen P. Morse]

timeless, since you've already reviewed this, please add your r=.

Comment 130

[Henry E. Thorpe]

I applied the patch to the 1.1 branch, and compiled it on FreeBSD 4.5/i386, on
which it is working fine (it's my normal browser now for a day).

I struggled through getting a Win32 version compiled, last night.  Patches also
seem to work on Win32 (I see the cookie dialog with the [Show Details] button,
details are correct, show details sticky), although I have a long way to go
before I have a usable Win32 browser. (let's here it for FreeBSD ports)

The only problem with the patch is a failure on the
extensions/cookie/nsPermissions.cpp file, which I had to patch by hand.

Nice work, Michiel!

Comment 131

[Michiel van Leeuwen (email: mvl+moz@)]

Attachment: patch v8

A patch with the constants moved to a (new) .idl file, as requested.

Comment 132

[Michiel van Leeuwen (email: mvl+moz@)]

Attachment: path v8.2

I screwed up, and forgot the mac build system. Sorry for the spam.

Comment 133

[Michiel van Leeuwen (email: mvl+moz@)]

Attachment: Yet Another Patch

change |(true)| to |PR_TRUE|

Comment 134

[timeless]

Comment on attachment 98024 [details] [diff] [review]
Yet Another Patch

r=timeless

(note to people examining the makefile with concerns about consistency, the
prececding lines used tabs tssk tssk).

great work

Comment 135

[Alec Flett]

Comment on attachment 98024 [details] [diff] [review]
Yet Another Patch

the comment /* copy */ isn't enough - I think you need to indicate the
ownership of the strings like name_from_header, etc

other than that, this looks ok. sr=alecf with the comment fix.

Comment 136

[Michiel van Leeuwen (email: mvl+moz@)]

Attachment: Yet Another Patch v2

added requested comments.

  // put the cookie information into the temporary struct.
  // just copy pointers to the char * strings (cookie_from_header etc).
  // we still own the strings, and we must free them if the cookie is
  // rejected.

Comment 137

[Stephen P. Morse]

Comment on attachment 98630 [details] [diff] [review]
Yet Another Patch v2

bringing reviews forward from previous patch

Comment 138

[Stephen P. Morse]

Fix checked in.

Comment 139

[Stephen P. Morse]

See bug 170237 for error introduced with this checkin.

Comment 140

[Michael Lefevre]

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

Comment 141

[Stephen P. Morse]

This feature is being disabled because it caused problems for embedders.  See
bug 179798.  Reopening.

Comment 142

[[e-mail]]

Marking resolved, per checkin for bug 179798... fixes the embedding problem.
Note that this patch was never backed out, so nothing more needs to be done.

Comment 143

[benc]

VERIFIED/FIXED:
I've run UI functional tests on several milestones right now. There are minor
issues, but they all seem to have open bugs documenting them.