234183 - Cookie Manager cleanup for 1.7b

Status: RESOLVED FIXED

(Core :: Networking: Cookies, defect)

Description

[Mike Connor [:mconnor]]

A bunch of this is overlapping and I'll just end up bitrotting myself if I try
to submit a collection of patches for these bugs.

Comment 1

[Mike Connor [:mconnor]]

Attachment: first patch

I fully expect this isn't that good :)

Comment 2

[Mike Connor [:mconnor]]

Attachment: cleaner patch

Comment 3

[Mike Connor [:mconnor]]

Comment on attachment 141485 [details] [diff] [review]
cleaner patch

whee

Comment 4

[Mike Connor [:mconnor]]

brief summary of the changes in this patch:

- properly persist hidden state of the columns in the stored cookies tab (fixes
bug 125924 in the process, although all the columns should persist this)  Also
required was fixing the duplicate ids of "statusCol" in CookieViewer.xul
- fix a strict warning in CookieViewer.js (bug 233910)
- enable sort arrows in cookie/image managers by properly setting sortDirection
(bug 65793)
- add an "Expires" column to the stored cookies tab in Cookie Manager

Comment 5

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

Comment on attachment 141485 [details] [diff] [review]
cleaner patch

>Index: mozilla/extensions/wallet/cookieviewer/resources/content/nsWalletTreeUtils.js
>@@ -113,12 +113,16 @@ function SortTree(tree, view, table, col
>   // do the sort or re-sort
>   var compareFunc = function compare(first, second) {
>+    // we need this to let the expires column sort properly
>+    if (column == "expires")
>+      return first.expiresSortValue.localeCompare(second.expiresSortValue);
>+    
>     return first[column].toLowerCase().localeCompare(second[column].toLowerCase());
>   }

You are mixing two approaches here. You could either update the xul file to
call SortTree with expiresSortCalue as columnname, in which case you don't need
the added if(). This is why you added a string value, because it needs to be
lowercased.
The other way is hardcode a check to see if you are sorting the expires column,
and use a numeric compare function.
Choose one.

Comment 6

[Mike Connor [:mconnor]]

Attachment: using numeric sort

yeah, that works better, I don't think I really understood how it worked
before.

There's still another bug here, namely that when we restore the selections, we
select the same rows as before, but that's not the same cookies.  Is that
intended behaviour?  (This exists on current trunk and in Firefox)

Comment 7

[Mike Connor [:mconnor]]

Comment on attachment 141909 [details] [diff] [review]
using numeric sort

<dwitte> yeah, you know you want to

Comment 8

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

Comment on attachment 141909 [details] [diff] [review]
using numeric sort

>Index: mozilla/extensions/wallet/cookieviewer/resources/content/CookieViewer.js

> function CookieColumnSort(column) {
>   lastCookieSortAscending =
>-    SortTree(cookiesTree, cookiesTreeView, cookies,
>-                 column, lastCookieSortColumn, lastCookieSortAscending);
>+      SortTree(cookiesTree, cookiesTreeView, cookies,
>+               column, lastCookieSortColumn, lastCookieSortAscending);

>+  if (sortedCol.getAttribute("sortDirection") == "ascending")
>+    sortedCol.setAttribute("sortDirection", "descending");
>+  else
>+    sortedCol.setAttribute("sortDirection", "ascending");

You know the direction you really used to sort by, you can deduce it from
|lastCookieSortAscending|. So why not use that, instead of switching the order
and hoping that is correct? 

> function PermissionColumnSort(column, updateSelection) {

>+  if (sortedCol.getAttribute("sortDirection") == "ascending")
>+    sortedCol.setAttribute("sortDirection", "descending");
>+  else
>+    sortedCol.setAttribute("sortDirection", "ascending");

Same thing here.

>Index: mozilla/extensions/wallet/cookieviewer/resources/content/nsWalletTreeUtils.js
>@@ -113,12 +113,22 @@ function SortTree(tree, view, table, col

>   var compareFunc = function compare(first, second) {
>+    // we need this to let the expires column sort properly
>+    if (column == "expires") {
>+      if (first.expiresSortValue > second.expiresSortValue)
>+        return 1;
>+      else if  (first.expiresSortValue < second.expiresSortValue)
>+        return -1;
>+      else
>+        return 0;
>+    }
>+    
>     return first[column].toLowerCase().localeCompare(second[column].toLowerCase());
>   }

First, you can move the column check out of the compareFunc. do something like:
|if (column == "expires") { compareFunc = ..... } else { compareFunc = .... }|
It saves the check for column on every compare.
But i'm not sure i like the special casing of the expires column. Maybe an
extra parameter indicating the type (numeric or alphanumeric)?

Comment 9

[Mike Connor [:mconnor]]

WRT to using the previous sort direction, using lastCookieSortAscending causes 
a problem.  We only want to sort descending if the current column is sorted as 
ascending.  If I used that to determine if it was sorted ascending, and used it 
in that manner, then the first sort of a different column would default to 
descending sort, which would be unexpected.  Clicking on a new column should 
default to sorting ascending, not the reverse of whatever order the previous 
column was sorted by.  This would mean a very inconsistent UI, since the first 
click on, say, the expires column would have different results depending on 
what you previously were using.

I wasn't feeling ambitious enough to rewrite sorting to allow sort vs. display 
parameters.  I do need to do that to do sorting by domain, but with a week 
before freeze I don't have time for that currently.  I'd like to go with this 
now and rework tree sorting to allow both sort and display values in 1.8a.

I'll split out the compareFunc stuff into separate vars.  I was going to do 
that at one point, I don't remember why I didn't.

Comment 10

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

(In reply to comment #9)
> WRT to using the previous sort direction, using lastCookieSortAscending causes 
> a problem.  We only want to sort descending if the current column is sorted as 
> ascending.

But isn't lastCookieSortAscending what actually determines the orded to sort?
(or am i misreading the patch?)
What i mean is that you should use the same code to show the sort direction and
do the actual sort. It looks that it uses different logic now.

Comment 11

[Mike Connor [:mconnor]]

hmm, you might be right at that, I need to test this when I get home in a few 
hours.

No sleep till beta, I suppose :)

Comment 12

[Mike Connor [:mconnor]]

Attachment: with mvl's comments

...must...stop... commenting on bugs at work

mvl: yeah, you're right, I just wasn't awake enough at work to understand the
sortTree code that takes care of that.	I also added a comment indicating that
special handling for the expires column is a hack that I'll get rid of later
(which is hopefully in 1.8a)

Comment 13

[Mike Connor [:mconnor]]

Comment on attachment 142790 [details] [diff] [review]
with mvl's comments

magic 8-ball sez: move request back to mvl

Comment 14

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

Comment on attachment 142790 [details] [diff] [review]
with mvl's comments

>Index: mozilla/extensions/wallet/cookieviewer/resources/content/CookieViewer.js
>@@ -198,13 +205,14 @@ function Cookie(number,name,value,isDoma
>-  this.expires = expires;
>+  this.expires = GetExpiresString(expires);
>+  this.expiresSortValue = expires;
>@@ -330,13 +338,13 @@ function CookieSelected() {
>-    {id: "ifl_expires", value: GetExpiresString(cookies[idx].expires)},
>+    {id: "ifl_expires", value: cookies[idx].expires},

Now, you don't need the 2 seperate fields, right? you can just use
GetExpiresString like it was, and use expires as a numeric value when actually
sorting.
Or is this a small pref improvement? only convert once. In that case, it's ok
:)

>Index: mozilla/extensions/wallet/cookieviewer/resources/content/nsWalletTreeUtils.js
>-  var compareFunc = function compare(first, second) {
>-    return first[column].toLowerCase().localeCompare(second[column].toLowerCase());
>+  // this is a temporary hack for 1.7, we should implement
>+  // display and sort variables here for trees in general

eww, don't mention version numbers. Stuff like that tends to stay for a loing
time, and it will get back to you. On the other hand, it increases the pressure
to you to fix it :)

>Index: mozilla/extensions/wallet/cookieviewer/resources/locale/en-US/CookieViewer.dtd
>+<!ENTITY     treehead.cookieexpires.label   "Expires"> 

I'm no native english speaker, but wouldn't "Expiry" be better?

anyway, r=mvl

Comment 15

[Mike Connor [:mconnor]]

it is a small perf improvement, yes, and it sets the stage for the whole
display/sort values I want to go ahead with.  Also, pressure is necessary :)
Expiry would be inconsistent with the Expires: string we already have.

Comment 16

[Mike Connor [:mconnor]]

Comment on attachment 142790 [details] [diff] [review]
with mvl's comments

jag, I really want to get this in before beta freeze, if you can't get to it,
I'd really appreciate it

Comment 17

[jag (Peter Annema)]

Comment on attachment 142790 [details] [diff] [review]
with mvl's comments

Heh, I got a little confused by the sorting code. Then I remembered that
|foo["bar"]| is the same as |foo.bar|.

Does compareFunc have to return -1 and 1, or any negative and positive value?
In the latter case you could reduce your "comparison" to a simply subtraction.

sr=jag.

Comment 18

[Mike Connor [:mconnor]]

checked in 03/09/2004 15:58