182540 - display all stored cookies

Status: RESOLVED FIXED

(Camino Graveyard :: Preferences, defect)

Description

[Derek Dineen]

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X; en-US; rv:1.0.1) Gecko/20021104 Chimera/0.6
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X; en-US; rv:1.0.1) Gecko/20021104 Chimera/0.6

In "Preferences-Security-Edit Cookie Sites" a complete list of stored cookies is
not shown. Only after one of the shown cookie sites is "Removed" will the entire
list be accessible. Also, it would be nice to be able to change the Allow/Deny
status of each site from this window.

Reproducible: Always

Steps to Reproduce:
1.Navigator menu
2.Preferences
3.Privacy
4.Edit Cookie Sites

Actual Results:  
Only one window's worth of stored cookie sites is available. A site must be
"Removed" before the complete list of cookie sites is availble.

Expected Results:  
Show the complete list of cookie sites immediately. Also allow the changing of a
site to and from Allow/Deny from this window.
Thanks for taking the time. I really enjoy using Chimera.

Comment 1

[Simon Fraser [no longer active]]

Right. This isn't really a bug; what you want is an RFE for UI to 'show all
cookies'.

Comment 2

[Mike Pinkerton (not reading bugmail)]

the current UI makes this much more understandable. updating summary.

Comment 3

[cannedbrain+mozilla]

Is there any way to store cookies inside the cookie list without having to turn
on the confirmation box? It's just too much and time consuming. It'd be easier
to edit which one(s) to deny/accept later. Sorry if this isn't an appropriate
place to file request. I'm not sure what the original poster mean (if it's a bug
or a request.) Seems like he found a bug the list didn't display a complete list
that he had to remove 1 of his cookies to show them all.

Comment 4

[gs]

Attachment: Tar containing Privacy Pane with cookie management

3 files (PrivacyPane .nib .h .mm) which simplify the privacy preference pane
and implement cookie management / permission management on a tabbed sheet.

Comment 5

[Jasper]

Wow Stephen what is that .nib file? You completely stripped the privacy pane
(removed the password section), put the security pane in the privacy pnae (why?)
and stripped the web features pane (removed the pop up and window settings
features).

Why in heavens name did you do that.

Comment 6

[gs]

> Wow Stephen what is that .nib file? You completely stripped the privacy pane
> (removed the password section), put the security pane in the privacy pnae (why?)
> and stripped the web features pane (removed the pop up and window settings
> features).

Eh?

In the privacy pane, I only renamed the 'Edit Sites' button to 'Edit Cookies',
and removed the 'Clear Stored Cookies' button which now exists as the 'Remove
All' button in the cookie management tab.

Something obviously went wrong uploading the .nib file. :}

Comment 7

[gs]

Probably some breakage inserting the new nib?

Comment 8

[gs]

Attachment: Tar containing Privacy Pane with Cookie Management

Ah - two privacy pane nib files exist in the tree, a very old version and the
current version. Guess which one I tarred?

Anyway, hopefully should be corrected now. Apologies.

Comment 9

[Jasper]

Aah that's better, thank you very much :)
Looks like a very promissing improvement.

Comment 10

[gs]

Attachment: Privacy pane with cookie and permission management sheet

Code improvements. Seems stable.

Comment 11

[Jasper]

How about also implementing the "Only from site you navigate to" option? If I
remeber correctly somebody (Mike?) said that it perhaps was something that could
be implemented? Seems to me that it would perfectly fit in with other "blocking
unwanted things" options (popups, resize).

Comment 12

[gs]

I've had a go at implementing 'accept cookies only from websites you navigate
to', but I'm wary to upload the patch as it requires a small change in the file
'Classes/Application/PreferenceManager.mm' which otherwise only allows the
preference options for cookies on (0) or cookies off (2) (and not cookies from
originating server (1)).

I wonder if there's a deeper reason why this is?

Comment 13

[Jasper]

Well uhm it just appears that the devs didn't implement that feautre yet and so
never put the code in place. I see no reason why you couldn't upload the patch.

Comment 14

[gs]

OK, a cosmetic issue to do with the Cocoa GUI instead of some Mozilla level issues.

Comment 15

[gs]

Attachment: Privacy pane with cookie/permission management and 3rd party cookies

- Implemented 'Accept cookies only from sites you navigate to' cookie option
- Implemented multiple-selections in cookie and site permission tables (note to
previous coder - just reverse enumerate through arrays to solve index shifting
problem when deleting objects!)
- Implemented ability to remove all site permissions

A change is needed in the file /Classes/Application/PrivacyManager.mm in the
'syncMozillaPrefs' method. Just comment out the line which changes the int
'acceptCookies' from 1 to 2.

Be warned - Camino still picks up 3rd party cookies with 'accept cookies only
from sites...' option enabled. I suggest this is a Mozilla issue.

Comment 16

[Jasper]

Perhaps a fix for that can be found in Bug 149115.

Comment 17

[Mike Pinkerton (not reading bugmail)]

the reason why i didn't include that option was because i figured most people
wouldn't really understand it. it was a conscious choice. we've already got too
many options, does the average user really care about that? what's a good
representative sample? not us, that's for sure.

Comment 18

[Jasper]

I think it's rather a question of asking our selves (the people who know these
things) wheter it's usefull for any user to have such an option. And if it's
usefull to enable such a feature by default.

As far as I understand this feature makes sure that 3d party cookies, which
apereantly are most of the time unwanted (not asked for and not handy), are not
excepted. If this is really true I think this is an option that is the same as
the pop-up blocker. It enables the user to block unwanteed activity with a
preset that aperantly is always correct.

Comment 19

[Jasper]

Devel,

Would it be posible to enable users to use the columns to sort the cookie
entries? See Bug 179054 for code that enables this, which you could perhaps use.

Comment 20

[gs]

Sure. Looking at the patch, it just replaces the depreciated SupportsArray with
COMArray which supports sorting. Should be no problem.

Comment 21

[David Haas (gone, not reading mail)]

Attachment: updated patch

I've taken the liberty of updating this patch to incorporate the sorting fix
found in bug 149115 as mentioned in comment 16.

The patch is probably a little messier than it needs to be due to the re-naming
of some member variables & shifting of methods around in the file - but I liked
how Devel (?) had cleaned things up, so I ran with it.

It also patches PreferenceManager.mm as requested in comment 12 - but I don't
know if this is required/desired/actually even works as is.

I made a tiny change on the nib - so it'll be coming in the next attachment.

Comment 22

[David Haas (gone, not reading mail)]

Attachment: Update NIB file

here's the updated nib.

Comment 23

[Jasper]

Update please? Is this ready for review or landing?

Comment 24

[David Haas (gone, not reading mail)]

Attachment: update

updated so that it still compiles.

Comment 25

[S Woodside]

I don't see anything at all in that window when I pull it up (Edit Sites...)
how do I test it?

Comment 26

[David Haas (gone, not reading mail)]

comment 25: check the "ask about each cookie" preference.  browse for a while. 
it'll fill up fast.

Comment 27

[S Woodside]

NB: camino project file uses PreferencePanes/Privacy/English.lproj/Privacy.nib
(not PreferencePanes/Privacy/Privacy.nib )

Comment 28

[S Woodside]

OK I got it working now :-) Last time I think I replaced the wrong nib file.

Overall I think it's very good. I think however that there should be a "remove all cookies" button 
right on the Privacy panel.

code comments:
- what's up with this crazy name "OrgMozillaChimeraPreferencePrivacy" ?

+  IBOutlet NSTableView* mPermissionsTable;
+  nsIPermissionManager* mPermissionManager;		// STRONG (should be nsCOMPtr)
+  nsCOMArray<nsIPermission>* mCachedPermissions;// parallel list for speed, STRONG
+
+  IBOutlet NSTableView* mCookiesTable;
+  nsICookieManager* mCookieManager;
+  nsCOMArray<nsICookie>* mCachedCookies;

I would be much happier if we could gradually create a greater separation between Cocoa and C++ 
code. Which basically is the same as separating Camino and Mozilla/Gecko code. In this case, to 
move mPermissionManager, mCachedPermissions, mCookieManager, mCachedCookies, into a 
seperate file and export a Cocoa API from that class. Which would introduce a layer of indirection. 
(why? partly because it "feels right" also because it means that I know what API (cocoa or moz) I'm 
using, and finally because SOME coders ONLY know cocoa and some only know moz.)

+
+  // build parallel cookie list
+  nsCOMPtr<nsISimpleEnumerator> cookieEnum;
+	if ( mCookieManager )
+		mCookieManager->GetEnumerator(getter_AddRefs(cookieEnum));
+
+    mCachedCookies = new nsCOMArray<nsICookie>;
+	if ( mCachedCookies && cookieEnum ) {
+		PRBool hasMoreElements = PR_FALSE;
+		cookieEnum->HasMoreElements(&hasMoreElements);
+		while ( hasMoreElements )
+		{
+			nsCOMPtr<nsICookie> cookie;
+			cookieEnum->GetNext(getter_AddRefs(cookie));
+			mCachedCookies->AppendObject(cookie);
+			cookieEnum->HasMoreElements(&hasMoreElements);
+		}
+	}
+
+
+	// bring up sheet
 	[NSApp beginSheet:mCookieSitePanel
-        modalForWindow:[mCookiesEnabled window]   // any old window accessor
+        modalForWindow:[mAskAboutCookies window]   // any old window accessor
         modalDelegate:self
-        didEndSelector:@selector(editCookieSitesSheetDidEnd:returnCode:contextInfo:)
+        didEndSelector:NULL
         contextInfo:NULL];

fix the tabwidth in that section

+    NSEnumerator *e = [rows reverseObjectEnumerator];

just curious, why use a reverse enumerator?

+-(IBAction) removeAllCookies: (id)aSender
+{
+  if ( mCookieManager ) {
+      // remove all cookies from cookie manager
+      mCookieManager->RemoveAll();
+      // create new cookie cache

fix tabwidth to 2 spaces

Comment 29

[gs]

> just curious, why use a reverse enumerator?

Just a simple solution to the problem of shifting indexes when removing multiple
objects from arrays (by their index). Try the same thing enumerating forwards,
and you'll understand...

Comment 30

[S Woodside]

fix the tabs and I'll r+

I still think we need a "remove all cookies" button though.

Comment 31

[Jasper]

So what's up with this patch, let's not forget it.

Comment 32

[S Woodside]

Attachment: patch2

dave's patch, just fixed the whitespace problem.

Comment 33

[Mike Nguyen]

This was the main reason I used Dave's unofficial builds...

Comment 34

[S Woodside]

Comment on attachment 138585 [details] [diff] [review]
patch2

r+ as per my previous comment

Comment 35

[S Woodside]

Comment on attachment 138585 [details] [diff] [review]
patch2

needs another review

Comment 36

[Ludovic Hirlimann [:Usul]]

Comment on attachment 138585 [details] [diff] [review]
patch2

I don't see any cookies in the list.
I've read Dave's comment and checked the reveleant box "ask about each cookie".
And I should be able to see cookies from site that created cookies before I
switched the option on.
Also when the edit window is opened and if i close it without using the done
button (cliking the red button on the upper left and side of the window), this
will freeze camino and I need to kill it.

Comment 37

[Jasper]

Come on guys let's finish this bug it's so far,  before we start to forget about it.

Comment 38

[Stuart Morgan]

I hear from Jasper that this patch needs a new developer, so I thought I'd take
a crack at it; I've gotten it compiling against the current tree, but I haven't
tested it much yet.  I have some UI issues with the design of the last nib,
though, so I'm planning on making some rearrangements.

A few questions about underlying behavior though, that make a big difference in
the design:

-If the user has built up an accept/deny list, then turns off "ask about each
cookie", is their existing list still enforced?

-If the user selects both "Only from visited sites" and "Ask about each", what
is the underlying behavior?

I suppose I could play around and find out, but I thought I'd see if anyone
knows off-hand.

Comment 39

[Stuart Morgan]

One other question, as long as I'm rearranging this pane. Right now, "Store
passwords in the keychain" and "Auto-fill passwords" are two separate check
boxes. Are there really people who want their passwords stored in the keychain,
but want to have to open the key chain and retrieve them all manually? It seems
like this would be a good place to simplify preferences by making that just one
check box.

Comment 40

[Mike Pinkerton (not reading bugmail)]

(In reply to comment #38)
> -If the user has built up an accept/deny list, then turns off "ask about each
> cookie", is their existing list still enforced?

yes, the list is always enforced even if 'ask about each' is turned off.

Comment 41

[Stuart Morgan]

Ok, so the option labels in the last submitted nib are actually misleading... in
fact, I can't think of *any* good way to name a radio-button version of this.

When I modify this patch, I'll use a Firefox-style scheme (another indented
checkbox) instead.

Comment 42

[Stuart Morgan]

This is pretty much ready to go, but I'd like to test it more first. In the mean
time, I'll post some screenshots to get feedback on whether all the changes I
made are desireable.  The biggies:

 - Use check boxes, instead of radio buttons, to avoid a misleading "Any
website" or "All websites" label.

 - Split Edit Sites and Edit Cookies into two buttons. I didn't think they were
similar enough to belong in the same sheet, especially since people seemed to be
confused about why the default list (site editing) wasn't listing any cookies.
Since neither table is clearly better as a default, I thought it better to make
two buttons.

 - Split Passwords into a separate tab. This reduces the visual clutter of the
panels, and is more consistent with all the other pref panes, where things are
only listed together if they are very tightly related.

 - Removed the description of cookies. While marginally more useful than
Firefox's "Cookies are delicious delicacies", the question is whether it would
really make the pane usable for someone without any knowledge of what cookies
are. I think the answer is no; unlike other prefs, where a description can say
in a sentence or two what the pref is, why you might want it, and why you might
*not* want it ("automatically pass to helpers" is a perfect example of this),
cookies are a far more complex issue. Like Javascript, I think it's too complex
to explain in a pref panel. If you know already, great, but if not, you probably
shouldn't mess with the options.

 - Removed the auto-fill, as explained in my comment 39 (giving control of both
prefs to one checkbox), and made the pref visually consistent with other prefs
like cache, history, and auto-helpers.

I know this is beyond the original scope of the bug, but as long as canges were
being made it seemed worthwhile to improve the entire pane. If some of these
changes are not wanted, I have no problem taking them back out. I'll post an
updated patch once there is some consensus on the changes.

Comment 43

[Stuart Morgan]

Attachment: Screenshot of my proposed Cookies pane

Comment 44

[Stuart Morgan]

Attachment: Screenshot of my proposed Passwords pane

Comment 45

[Jasper]

So far I think you made some very good UI decisions. I'd like to know what the
developers thinks of you rprogress so far. I like how you simplified things.

Comment 46

[Jasper]

I do think that a description of what a cookie is would be valuable any how.

We now have:
Cookies are small pieces of information that some website ask to store on (and
later retrieve from) your computer.

Omniweb has:
"Cookies" allow web sites to store inofrmation on you computer.

I think we should have something like this:
Cookies are small pieces of information for web sites stored on your computer.

Comment 47

[Stuart Morgan]

> I do think that a description of what a cookie is would be valuable any how.

Why? That's the question I asked myself, and didn't have an answer for. I'm not
necessarily against having a description, but I think we should have a reason to
put one (generally, anything that doesn't help an interface hurts it).

The only reason I could think of to put a description is to tell the user what
the prefs are for, but I can't think of any short description that will do that.

Comment 48

[Jasper]

Well I think that especially in the case of cookies it's necesarry to have a
good description since it's such a difficult thing to explain or new users.
Please try to come up with something. 

Comment 49

[Stuart Morgan]

Dear Lord.

After reading bug 233339 several times in order to fix cookie pref breakage,
I've learned that it's impossible to totally disable cookies. Period. The prefs
are actually "accept all by default", "accept originating page by default", and
"accept none *by default*". However, any whitelist that you have created
supercedes *all* of those prefs.

That means that I need to totally redesign the panel yet again. Among other
things, the site permissions need more prominence/explaining. Also, the ability
to actually add sites to the whitelist right in the panel would be nice, but
that may be a separate RFE later on, depending on how complex that is.

New screenshots in a few days, I guess.

Comment 50

[Stuart Morgan]

Ok, the new patch is ready. I've toned down most of my interface changes based
on some discussions elsewhere, and so that this patch won't have to wait on
decisions about the look.

Comment 51

[Stuart Morgan]

Attachment: Th new Privacy NIB

Updated NIB, without tabs, and with more complete pref explanations.

Comment 52

[Stuart Morgan]

Attachment: Revised patch

Updated patch.	Just to summarize all the changes from the current Privacy
pane:
- Cookie viewing/deleting
- Pref for accepting cookies only from originating servers
New since the last patch as well:
- Ability to change the allow/deny pref right in the site viewer (since I not
infrequently hit the wrong button when asked about cookies, and have "remember
this decision" turned on)
- Preservation of selection (partially) when changing the sort order in either
viewer
- Fixed display of session cookie expire time
- Split edit cookie and edit sites since they are very different, and putting
them together was causing confusion.

Comment 53

[Mike Pinkerton (not reading bugmail)]

landed with some changes:
- wording in nib
- use of cString building host, should be UTF8String
- const warnings on comparitors

looks really good, i'm impressed. thanks for the effort!