Closed Bug 142179 Opened 22 years ago Closed 6 years ago

Cookie Manager: sort "Cookie Sites" should sort by Domain (TLD -> left, as labels, not strings, etc.)

Categories

(Firefox :: Settings UI, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED WORKSFORME

People

(Reporter: John.L.Villalovos, Unassigned)

References

(Depends on 1 open bug)

Details

Attachments

(2 files, 4 obsolete files)

Sort order in "Cookie Sites" needs to be done by Top Level Domain (TLD) on down
in the Cookie Manager.

Currently the cookie manager sorts sites alphabetically from left to right.

So "my.yahoo.com" is not located anywhere near "www.yahoo.com".

The sort needs to be done so it sorts by TLD first, then domain, sub-domain, etc...

The current behavior makes it very difficult to tell all the sites in a domain
that may be setting cookies.
Attached patch An attempt to fix the problem (obsolete) — Splinter Review
This is a patch I wrote.  I did NOT test this because I am not sure how to
compile all of Mozilla :(

I did test the ReverseHostName function and it appeared to work in my client
side Javascript code.

My idea in this patch is to reverse the host name for the RawHost field.  Since
it appears that things get sorted by RawHost but the Host field gets displayed.
 So hopefully this will work.

Thanks,
John Villalovos
Attached patch Second attempt to fix problem (obsolete) — Splinter Review
I learned how to test my fix :)  After testing discovered bugs.  I have added a
"reverseHost" data field to the Cookie and Permissions structures.

I now sort the columns on these structures.  It seems to work pretty well :)

Feedback appreciated.
*** Bug 131503 has been marked as a duplicate of this bug. ***
Updating platform, OS, and severity.

Would like to see this functionality.
Severity: minor → enhancement
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Windows 2000 → All
Hardware: PC → All
Filed bug 143166 which requests this behavior for image blocking sites as well.
Well a side affect of my patch is that the Image Manager is also sorted this
way.  This is because the Image manager is in reality the Cookie manager.
Blocks: 107957
Blocks: 143472
No longer blocks: 107957
Blocks: 143166
I rebuilt my local version of mozilla with the latest patch. Seems to work as
advertised (only tested cookies since I don't block images). Can you get it
reviewed and checked in?

I didn't look at the patches for Passwords/Forms. I encrypt that info and the
builds I make never seem to be able to deal with that.

Anyhow, I really like this.
Attached patch Fix part 3 (obsolete) — Splinter Review
Change from this fix and the previous fix is that now the ReverseHostName
function in nsWalletTreeUtils.js any space or ":" characters and any thing
following the space or ":" characters will be trimmed off.

This way when the Password Manager (SignonViewer.js) calls this function
entries will show up next to each other even if they have stuff like:

my.site.com:80
my.site.com:80 (Text from site)
my.site.com (Text from site)

I've tested this and it appears to work for me.
Target Milestone: --- → mozilla1.0.1
Status: NEW → ASSIGNED
Target Milestone: mozilla1.0.1 → mozilla1.1alpha
Priority: -- → P3
Target Milestone: mozilla1.1alpha → mozilla1.2alpha
Aren't ports supposed to be stripped off and not stored?
I'm thinking of bug 142803 here.
Comment on attachment 83080 [details] [diff] [review]
Fix part 3

>+    var workname = hostname.replace(/(:.*$)|(\s+.*$)/g, '')
I know I said this will become unnecessary, but you forgot the ; and you could
have used var workname = hostname.replace(/[:\s].*/, "");

>+    var hostsplit = workname.split( "." );
>+    var newhostname = "";
>+    for ( var i = hostsplit.length - 1; i >= 0; i-- ) {
>+        if ( hostsplit[i] != "" ) {
>+            newhostname = newhostname + hostsplit[i] + ".";
>+        }
>+    }
>+    return newhostname;
Use the reverse and join functions:
return hostname.split(".").reverse().join(".");
Attachment #83080 - Flags: needs-work+
Attached patch Patch to fix this bug. Works :) (obsolete) — Splinter Review
Thanks for the comments.  I'll leave the thing that strips off the port number
for now because I'm not sure when they will fix the other bug.

I did take your comments about the javascript and put those in to this new
patch.	Thanks for the comments because I know very little about javascript.
Attachment #83080 - Attachment is obsolete: true
I had left a .orig file lying around in the previous patch :(
Attachment #91675 - Attachment is obsolete: true
*** Bug 160632 has been marked as a duplicate of this bug. ***
Comment on attachment 91677 [details] [diff] [review]
Got rid of .orig file

r=morse
shouldn't patches labelled first-attempt and second-attempt be marked as
obsolete?
Attachment #91677 - Flags: review+
Attachment #82367 - Attachment is obsolete: true
Attachment #82369 - Attachment is obsolete: true
I made previous patches obsolete
Target Milestone: mozilla1.2alpha → mozilla1.3alpha
-> suresh
Assignee: morse → suresh
Status: ASSIGNED → NEW
Blocks: 194878
Tried to improve searchability of summary b/c of latest dupe...

BTW, yes, please keep the Image Manager bug separate, b/c they do have different
QA and are in different compoennts. (You can make the Image Manager bug depend
on this bug if you want).
QA Contact: tever → cookieqa
Summary: Sort order in "Cookie Sites" needs to be done by Top Level Domain on down → Cookie Manager: sort "Cookie Sites" should sort by Domain (TLD -> left, as labels, not strings, etc.)
*** Bug 202044 has been marked as a duplicate of this bug. ***
mvl: what do you think of this one?

the patch here looks ok, although we can remove the bit about truncating port,
because ports in cookies were ditched long ago. ;)

actually i don't like the regexps, because wallet is slow enough already without
having extra help. your tree-in-C++ idea sounded a bit enthusiastic but maybe we
could push the sorting to C++? so we could throw a sort order to the enum (or
array, whatever, since we'll need to re-roll it b/c of frozen interfaces) and
have it call up nsVoidArray's sort. maybe this needs benchmarking at some point
to see if it actually buys anything, although from previous tests we know
sorting a voidarray is uberfast - we have no idea about JS though.
The current patch shouldn't be adding to much extra time to the display.  Pretty
much an order N increase because the values get reformatted before being sorted.
 But yes it is slow with a lot of cookies so if the builtin Javascript sort
procedure could be faster that would be nice :)

And yes the part about ports looks like it could be removed since I no longer
see ports being stored in the cookie information.  But the underlying code here
was also being used by the patch for the Password manager and Image manager. 
And hopefully in the future by the Popup manager.  So I don't know if any of
those store ports off the top of my head.

But it would be nice if this was actually checked into the code sometime.  I'm
guessing that this would provoke some positive responses from people if it was
changed.
heh, apologies for not elaborating further. I agree we should support this, it's
a good rfe... my question is how, exactly. wallet needs a really good whacking
at some point, and we may have a better way to fix this... I just want to see
things get better before they get worse, and slowing down something that's
already slow is probably not a good thing. ;) (of course, benchmarking would
allow us to decide more easily)

and yeah, the permissions backend does have the capability to store ports, but
that feature is semi-broken so we're considering either a) fixing it or b)
removing it. until then we may want to keep the port-handling string fu you've got.

see bug 202131 about speeding up the sorting.
It would be a nice enhancement to have a general purpose class to do this type
of sorting. Many other interfaces could benefit from it, like history.
What's the status on this one? I find that this is one of the feature I miss
most about Mozilla. It seems to have been open for about 18 months now - is this
going in any time soon?
By the way - whoever implements this, remember to *not* reverse the order of
components for plain IP addresses (those need to sort by first byte first, as
they do today). Only host*names* should be reversed.
> IP addresses (those need to sort by first byte first, as they do today)

I suspect current behavior is to simply sort IP addresses as if they were a
string. This would mean you get the "incorrect" order:

1.5.6.7
10.5.6.7
2.5.6.7

Or something similar (10 would probably come first, actually). This is a very
minor issue and if it's much additional work, can probably be skipped for now.

Does this just need a superreview? I just manually cleaned out my cookies, and
had they been sorted better, I would have saved quite a bit of time.
Unfortunately the patch no longer works; furthermore it doesn't deal with IP
addressed cookies as was pointed out in comment 24.
> I suspect current behavior is to simply sort IP addresses as if 
> they were a string. 

> 1.5.6.7
> 10.5.6.7

This is not as bad as it seems - at least all the 1.5.*.*'s will be contiguous,
even if the last two octets are "incorrectly sorted" (from a numeric point of view).

The whole point of special sorting of cookies is to get cookies from the same
domain near each other so that we don't have to scrounge around for them.

(Of course, if the cookies from a site come as a mixture of hostname and
IP-address-based cookies, then we have problems :-).
Just my 2 cents: I don't care about ip addresses because I don't know what sites
they are anyway. Once you go to IP6, then it becomes even more difficult. The
average user doesn't even know anything about dot notation, they probably think
it's just a special name. 

But here something? How about organizing the cookies and the site lists in a
tree structure instead of a flat list? I'd be able to turn enable/disable
cookies based on the domain.

a) when the TLD changes, suggest inserting a half-line blank
space, so that all the cookies under the same TLD can be
easily seen / grasped as a visual unit.


b) bug # 236133 is related, it requests cookies also be able
to be sorted by date of creation, by date of last access,
by number of updates, etc.
-> me, YACMB

why does the IP addressed sites sort order really matter?  I mean, sure it'd be
nice, and if there's a rational way of doing it, then cool, but I don't think
the sort that happens with this method is worse than what we currently have.
Assignee: skasinathan → mconnor
The reason the IP addressed sites being sorted is to identify cookies from
similiar sites. The way it is now, cookies from different hosts under the same
domain are all over the place. I believe the idea is, even under IP address,
having them together is good.

Although, I also believe some people were making a case for other parts of the
browser using the same sort code, so something like a password manager having IP
sites organized correctly might make a big difference over say, cookies.
*** Bug 240028 has been marked as a duplicate of this bug. ***
Get domain sorting done first. For IP address support, you will need to support
IPv6 and IPv4, so you can't sort them correctly w/o code that identifies the
literals, so just focus on one thing at a time. 

From glancing at the patch, it seems that this is implemented w/in cookie
manager. I was thinking the other day that a domain sorting function might be
useful elsewhere as well, in history, bookmarks, email, etc.
Has there really been no activity since April?  Is there some other bug I should
be reading instead?

This is a real problem for me, since I manage cookies aggressively.  Not being
able to see: 1) cookies by domain and 2) most recent cookies is a real hindrance.

I'll test out the latest patch file.

Given that this was opened over 2 years ago, perhaps you could implement
something now, and figure out a more general solution later?
its an enhancement, and its for Seamonkey.  My primary focus, right now, is with
Firefox and finishing off 1.0.  If someone else wants to take a stab at it,
great, otherwise I'll get to it at some point.
I've created an patch that adds the cookie sorting behavior for Firefox.

Bugzilla keeps telling me I have not specified the file to attach.  I don't
understand this: I've made sure to accept cookies, turned off Privoxy, and used
the 'Browse' button to specify the file to be attached.

So I'll have to add it inline here.  Sorry about that.

-----------------------------------
This is my first patch, so I'll describe the process in case I did it wrong.

First, I grabbed the Firefox source tarball for 0.9.3, and unpacked it.

Then, I dropped down into ./mozilla and ran "gmake -f client.mk checkout".  This
is supposed to fetch me the latest source.

Back up one dir, and I ran "patch -p0 < cookiesorter.patch"

Back down to ./mozilla, and ran "gmake -f client.mk build".  The resulting
dist/bin/firefox now has the cookie sorting behavior.

Here is the patch file:
--------------------------------------------------------------
diff -r -c
./mozilla/browser/components/cookieviewer/content/CookieExceptions.xul
./mozilla.new/browser/components/cookieviewer/content/CookieExceptions.xul
*** ./mozilla/browser/components/cookieviewer/content/CookieExceptions.xul	Mon
Nov  3 22:02:34 2003
--- ./mozilla.new/browser/components/cookieviewer/content/CookieExceptions.xul
Mon Sep 27 18:59:06 2004
***************
*** 61,67 ****
         onselect="PermissionSelected();">
     <treecols>
       <treecol id="siteCol" label="&treehead.sitename.label;" flex="3"
!                   onclick="PermissionColumnSort('rawHost', true);"
persist="width"/>
       <splitter class="tree-splitter"/>
       <treecol id="statusCol" label="&treehead.status.label;" flex="1"
                    onclick="PermissionColumnSort('capability', true);"
persist="width"/>
--- 61,67 ----
         onselect="PermissionSelected();">
     <treecols>
       <treecol id="siteCol" label="&treehead.sitename.label;" flex="3"
!                   onclick="PermissionColumnSort('reverseHost', true);"
persist="width"/>
       <splitter class="tree-splitter"/>
       <treecol id="statusCol" label="&treehead.status.label;" flex="1"
                    onclick="PermissionColumnSort('capability', true);"
persist="width"/>
diff -r -c ./mozilla/browser/components/cookieviewer/content/CookieViewer.js
./mozilla.new/browser/components/cookieviewer/content/CookieViewer.js
*** ./mozilla/browser/components/cookieviewer/content/CookieViewer.js	Sun Apr 25
14:11:57 2004
--- ./mozilla.new/browser/components/cookieviewer/content/CookieViewer.js	Mon
Sep 27 18:58:58 2004
***************
*** 113,118 ****
--- 113,119 ----
    this.isDomain = isDomain;
    this.host = host;
    this.rawHost = rawHost;
+   this.reverseHost = ReverseHostName( host );
    this.path = path;
    this.isSecure = isSecure;
    this.expires = expires;
***************
*** 146,152 ****
    cookiesTree.treeBoxObject.view = cookiesTreeView;
  
   // sort by host column
!   CookieColumnSort('rawHost');
  
    // disable "remove all cookies" button if there are no cookies
    document.getElementById("removeAllCookies").disabled = cookies.length == 0;
--- 147,153 ----
    cookiesTree.treeBoxObject.view = cookiesTreeView;
  
   // sort by host column
!   CookieColumnSort('reverseHost');
  
    // disable "remove all cookies" button if there are no cookies
    document.getElementById("removeAllCookies").disabled = cookies.length == 0;
diff -r -c ./mozilla/browser/components/cookieviewer/content/CookieViewer.xul
./mozilla.new/browser/components/cookieviewer/content/CookieViewer.xul
*** ./mozilla/browser/components/cookieviewer/content/CookieViewer.xul	Sun Apr
25 14:11:57 2004
--- ./mozilla.new/browser/components/cookieviewer/content/CookieViewer.xul	Mon
Sep 27 18:59:06 2004
***************
*** 49,55 ****
                  hidecolumnpicker="true">
          <treecols>
            <treecol id="domainCol" label="&treehead.cookiedomain.label;" flex="2"
!                        onclick="CookieColumnSort('rawHost', true);"
persist="width"/>
            <splitter class="tree-splitter"/>
            <treecol id="nameCol" label="&treehead.cookiename.label;" flex="1"
                         onclick="CookieColumnSort('name', true);" persist="width"/>
--- 49,55 ----
                  hidecolumnpicker="true">
          <treecols>
            <treecol id="domainCol" label="&treehead.cookiedomain.label;" flex="2"
!                        onclick="CookieColumnSort('reverseHost', true);"
persist="width"/>
            <splitter class="tree-splitter"/>
            <treecol id="nameCol" label="&treehead.cookiename.label;" flex="1"
                         onclick="CookieColumnSort('name', true);" persist="width"/>
diff -r -c ./mozilla/browser/components/cookieviewer/content/treeUtils.js
./mozilla.new/browser/components/cookieviewer/content/treeUtils.js
*** ./mozilla/browser/components/cookieviewer/content/treeUtils.js	Tue Feb 24
20:52:16 2004
--- ./mozilla.new/browser/components/cookieviewer/content/treeUtils.js	Mon Sep
27 18:58:58 2004
***************
*** 144,146 ****
--- 144,161 ----
    return ascending;
  }
  
+ function ReverseHostName( hostname ) {
+   // This function will take a host name like bugzilla.mozilla.org and
+   // reformat it to org.mozilla.bugzilla.  This way when the sorting is done
+   // hosts from the same domain will be next to each other.
+   if (hostname == "") {
+     return hostname;
+   }
+   // Delete any ':' or ' ' (space) and anything that follows it.  This is due
+   // to the fact that sometimes they store the port number with the host name
+   // in the cookie.  I think they are going to stop doing that but at the
+   // moment they still are.
+   var workname = hostname.replace(/[:\s].*/, "");
+   // Reverse the name and return it
+   return workname.split(".").reverse().join(".");
+ }
diff -r -c ./mozilla/browser/components/prefwindow/content/permissions.js
./mozilla.new/browser/components/prefwindow/content/permissions.js
*** ./mozilla/browser/components/prefwindow/content/permissions.js	Sat Aug 23
19:01:13 2003
--- ./mozilla.new/browser/components/prefwindow/content/permissions.js	Tue Sep
28 11:09:38 2004
***************
*** 82,87 ****
--- 82,88 ----
    this.number = number;
    this.host = host;
    this.rawHost = rawHost;
+   this.reverseHost = ReverseHostName( host );
    this.type = type;
    this.capability = capability;
    this.perm = perm;
***************
*** 158,164 ****
  
    // sort and display the table
    permissionsTree.treeBoxObject.view = permissionsTreeView;
!   PermissionColumnSort('rawHost', false);
  
    // disable "remove all" button if there are none
    document.getElementById("removeAllPermissions").disabled =
permissions.length == 0;
--- 159,165 ----
  
    // sort and display the table
    permissionsTree.treeBoxObject.view = permissionsTreeView;
!   PermissionColumnSort('reverseHost', false);
  
    // disable "remove all" button if there are none
    document.getElementById("removeAllPermissions").disabled =
permissions.length == 0;
If possible, I'd like to have one sorting function that solves the DNS + IPv4
problem in a central location. While hacking every module locally sounds not to
time-consuming, don't forget we eventually will have to re-hack for IPv6.

I've created bug 276474 for such a purpose. I've also got about 50% of the
javascript needed for such a function working. I've moved the bugs that depend
on this bug to the new bug.
No longer blocks: 143166, 143472, 194878
Depends on: 276474
Blocks: 222559
*** Bug 291184 has been marked as a duplicate of this bug. ***
*** Bug 295952 has been marked as a duplicate of this bug. ***
Flags: blocking-aviary1.1?
This isn't the right bug, and would have a vastly different fix.
Flags: blocking-aviary1.1? → blocking-aviary1.1-
Not going to be working on any Seamonkey UI bugs for the foreseeable future. 
You can filter on "danlikesgoats" to delete this spam.
Assignee: mconnor → nobody
Priority: P3 → --
Target Milestone: mozilla1.3alpha → ---
Depends on: 331510
Blocks: 100573
Depends on: 282220
Seriously, this has been reported in 2002, there is a patch available since 2004, the list of duplicates keeps adding up, and no-one has taken the time to fix this ever since?
The patch does not apply to any code that has been around recently.
I've had this in my tree for ages; I no longer remember what parts of it I wrote (though I'm pretty sure I took at least parts from something else in this bug).

This just fixes the cookie exceptions window and not the cookie manager window.
Comment on attachment 460158 [details] [diff] [review]
patch for Firefox cookie *exceptions* window

I think dwitte can review this...
Attachment #460158 - Flags: review?(dwitte)
Comment on attachment 460158 [details] [diff] [review]
patch for Firefox cookie *exceptions* window

Sure. A screenshot would help, so I can make sure it looks sensible, but I trust that it does.

This won't exactly work for IP addresses, but there's no harm in grouping them strangely in the sort order, I suppose.

r=dwitte
Attachment #460158 - Flags: review?(dwitte) → review+
If the new "Data Manager" Kairo introduceded in Seamonkey lands on Firefox (I understand it's being ported), wouldn't this bug be considered fixed?
It's called the permissions manager in Firefox and is accessible in the current nightlies through about:permissions

And currently the cookie manager is not obsoleted, in fact if you click "Manage Cookies…" for a domain in the permissions manager it simply opens the cookie manager with the search box filled out.
(In reply to [Baboo] from comment #52)
> It's called the permissions manager in Firefox and is accessible in the
> current nightlies through about:permissions

Thanks, you made my day. I've been looking for this feature for months!

> 
> And currently the cookie manager is not obsoleted, in fact if you click
> "Manage Cookies…" for a domain in the permissions manager it simply opens
> the cookie manager with the search box filled out.
Component: Networking: Cookies → Preferences
Product: Core → Firefox
With the new tab-based preferences it's even worse since it takes into account the http/https prefix now. If there is a website with both http and https cookie exceptions you don't find them near to each other, but you'll see

http://bar.foo
http://foo.bar
https:/bar.foo
https:/foo.bar

So either put http/https into a separate column to allow website ordering or disregard the http/https prefix when doing the sorting, so that we get

http://bar.foo
https:/bar.foo
http://foo.bar
https:/foo.bar
Why is there a differentiation between http and https after all?
Good news, this works in the new data manager!
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → WORKSFORME
You need to log in before you can comment on or make changes to this bug.