Last Comment Bug 476175 - Add search bar to Cookie Manager (Stored Cookies tab)
: Add search bar to Cookie Manager (Stored Cookies tab)
Status: RESOLVED FIXED
:
Product: SeaMonkey
Classification: Client Software
Component: Passwords & Permissions (show other bugs)
: Trunk
: All All
: -- enhancement (vote)
: seamonkey2.0b1
Assigned To: Jens Hatlak (:InvisibleSmiley)
:
Mentors:
Depends on:
Blocks: 481958 481959
  Show dependency treegraph
 
Reported: 2009-01-30 09:21 PST by Jens Hatlak (:InvisibleSmiley)
Modified: 2009-03-06 15:31 PST (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
proposed patch (10.34 KB, patch)
2009-01-30 12:33 PST, Jens Hatlak (:InvisibleSmiley)
no flags Details | Diff | Splinter Review
patch v2 (11.12 KB, patch)
2009-01-30 13:33 PST, Jens Hatlak (:InvisibleSmiley)
no flags Details | Diff | Splinter Review
patch v2a (11.13 KB, patch)
2009-01-30 13:49 PST, Jens Hatlak (:InvisibleSmiley)
iann_bugzilla: review-
Details | Diff | Splinter Review
patch v3 (11.67 KB, patch)
2009-02-08 16:46 PST, Jens Hatlak (:InvisibleSmiley)
iann_bugzilla: review+
Details | Diff | Splinter Review
patch v4, r=iann_bugzilla (11.54 KB, patch)
2009-02-24 14:21 PST, Jens Hatlak (:InvisibleSmiley)
jh: review+
neil: superreview-
Details | Diff | Splinter Review
patch v5 (10.25 KB, patch)
2009-02-28 16:47 PST, Jens Hatlak (:InvisibleSmiley)
no flags Details | Diff | Splinter Review
patch v6 (9.97 KB, patch)
2009-03-01 17:11 PST, Jens Hatlak (:InvisibleSmiley)
neil: superreview-
Details | Diff | Splinter Review
patch v7 (12.95 KB, patch)
2009-03-02 14:54 PST, Jens Hatlak (:InvisibleSmiley)
neil: superreview-
Details | Diff | Splinter Review
patch v8 (13.17 KB, patch)
2009-03-03 13:18 PST, Jens Hatlak (:InvisibleSmiley)
no flags Details | Diff | Splinter Review
patch v9 (14.74 KB, patch)
2009-03-04 14:46 PST, Jens Hatlak (:InvisibleSmiley)
neil: review+
neil: superreview+
Details | Diff | Splinter Review

Description Jens Hatlak (:InvisibleSmiley) 2009-01-30 09:21:37 PST
The list of stored cookies can grow quite large. A search bar (like FF has) would help identify which/whether cookies are stored for a web site. The search should match anywhere inside the Site.
Comment 1 Jens Hatlak (:InvisibleSmiley) 2009-01-30 12:33:00 PST
Created attachment 359810 [details] [diff] [review]
proposed patch
Comment 2 Stefan [:stefanh] 2009-01-30 13:08:46 PST
Fyi, in bug 460694, I'm making all search fields using the emptytext attribute
instead of labels. That'd be nice to have here too, I think. Can we also
implement a shortcut for focusing the field (accel+f might work)?
Comment 3 Jens Hatlak (:InvisibleSmiley) 2009-01-30 13:33:38 PST
Created attachment 359818 [details] [diff] [review]
patch v2

Replaced label and accesskey by emptytext, added shortcuts for focussing filter box (Ctrl+F) and closing the window (Ctrl+W)
Comment 4 Jens Hatlak (:InvisibleSmiley) 2009-01-30 13:49:02 PST
Created attachment 359821 [details] [diff] [review]
patch v2a

Sorry for the spam, forgot to remove the colon while moving from label to emptytext. Also renamed the entities for better compliance with the Search/Focus search box idea
Comment 5 Ian Neal 2009-02-08 15:30:39 PST
(In reply to comment #4)
> Created an attachment (id=359821) [details]
> patch v2a
> 
> Sorry for the spam, forgot to remove the colon while moving from label to
> emptytext. Also renamed the entities for better compliance with the
> Search/Focus search box idea
The problem I have with this change is that there will be hardly any space left to display the cookies (only 3 lines), so this needs to be resolved. Firefox fixed this in a number of ways - splitting exceptions from stored cookies so removing the tabs, removing some of the wording "Information about the selected Cookie", grouping of cookies on a per site basis, moving buttons all to one line, etc.

When first started "Search" is not displayed in the search box, only once you have taken focus away from the search box (e.g. selected your first cookie).

I know you have not introduced this issue but also noticed that you get a caret when clicking over each of the cookie fields (e.g. Send For).
Comment 6 Jens Hatlak (:InvisibleSmiley) 2009-02-08 16:46:28 PST
Created attachment 361196 [details] [diff] [review]
patch v3

(In reply to comment #5)
> The problem I have with this change is that there will be hardly any space left
> to display the cookies (only 3 lines), so this needs to be resolved.

Following our IRC discussion I chose to set the height explicitly, using the same value as for the image permissions dialog. There are now six lines per default (tested on WinXP after deleting localstore.rdf).

> Firefox
> fixed this in a number of ways - splitting exceptions from stored cookies so
> removing the tabs, removing some of the wording "Information about the selected
> Cookie", grouping of cookies on a per site basis, moving buttons all to one
> line, etc.

I think all of that is out of scope here, and with the current width the buttons wouldn't fit next to each other anyway (and even with a bit more width it would look squeezed (the difference: FF doesn't have a Help button in the dialog).

> When first started "Search" is not displayed in the search box, only once you
> have taken focus away from the search box (e.g. selected your first cookie).

Yes but that's in line with the Password Manager. However I changed the emptytext value to "Search Cookies" as discussed on IRC.

> I know you have not introduced this issue but also noticed that you get a caret
> when clicking over each of the cookie fields (e.g. Send For).

FF does it the same, and I think for good reason: it aids keyboard-driven selection.
Comment 7 Ian Neal 2009-02-23 15:31:51 PST
Comment on attachment 361196 [details] [diff] [review]
patch v3

>--- a/suite/common/permissions/cookieViewer.js
>+/*** =================== FILTER CODE =================== ***/
>+
>+function cookieMatchesFilter(aCookie)
>+{
>+  return aCookie.rawHost.indexOf(cookiesTreeView.filterValue) != -1 ||
>+         aCookie.name.indexOf(cookiesTreeView.filterValue) != -1 ||
>+         aCookie.value.indexOf(cookiesTreeView.filterValue) != -1;
>+}
Any reason why this function is not inlined into the filterCookies function? It only seems to be used the once and you would be able to use aFilterValue rather than cookiesTreeView.filterValue...

>+function filter()
>+{
Could you not pass this.value into the function thus avoiding the following line?
>+  var filter = document.getElementById("filter").value;
>+  if (filter == "") {
>+    clearFilter();
>+    return;
>+  }
>+
>+  // clear the display
>+  var oldCount = cookiesTreeView.rowCount;
>+  cookiesTreeView.rowCount = 0;
>+  cookiesTree.treeBoxObject.rowCountChanged(0, -oldCount);
>+
>+  // set up the filtered display
>+  cookiesTreeView.filtered = true;
>+  cookiesTreeView.filterSet = filterCookies(filter);
>+  cookiesTreeView.rowCount = cookiesTreeView.filterSet.length;
>+  cookiesTree.treeBoxObject.rowCountChanged(0, cookiesTreeView.rowCount);
>+  document.getElementById("removeAllCookies").setAttribute("disabled","true");
Missing space after the comma.
>+
>+  // if the view is not empty then select the first item
>+  if (cookiesTreeView.rowCount > 0)
>+    cookiesTreeView.selection.select(0);
>+}
>+
>+function clearFilter()
>+{
>+  // clear the display
>+  cookiesTreeView.filtered = false;
>+  cookiesTreeView.rowCount = 0;
>+  cookiesTree.treeBoxObject.rowCountChanged(0, -cookiesTreeView.filterSet.length);
>+  cookiesTreeView.filterSet = [];
>+
>+  // reload the display
This comment should be just before the loadCookies();
>+  if (lastCookieSortColumn == "rawHost") {
>+    lastCookieSortAscending = !lastCookieSortAscending; // prevents sort from being reversed
>+  }
Don't need the {} here and the comment could be moved to before the if statement so that it doesn't go past the 80 character length.
>+  loadCookies();
>+
>+  // remove the selection
>+  cookiesTreeView.selection.clearSelection();
>+}

>--- a/suite/common/permissions/cookieViewer.xul
>+          <!-- filter -->
>+          <hbox align="center">
>+            <textbox id="filter" flex="1" type="search" emptytext="&search.emptytext;"
>+                     oncommand="filter();"/>
One attribute per line when making changes if the element plus attributes does not fit in one line.
>+          </hbox>
>+          <separator class="thin"/>

r=me with those changes addressed, just need an sr now.
Comment 8 Jens Hatlak (:InvisibleSmiley) 2009-02-24 14:21:01 PST
Created attachment 363970 [details] [diff] [review]
patch v4, r=iann_bugzilla

(In reply to comment #7)
> (From update of attachment 361196 [details] [diff] [review])
> >--- a/suite/common/permissions/cookieViewer.js
> >+/*** =================== FILTER CODE =================== ***/
> >+
> >+function cookieMatchesFilter(aCookie)
> >+{
> >+  return aCookie.rawHost.indexOf(cookiesTreeView.filterValue) != -1 ||
> >+         aCookie.name.indexOf(cookiesTreeView.filterValue) != -1 ||
> >+         aCookie.value.indexOf(cookiesTreeView.filterValue) != -1;
> >+}
> Any reason why this function is not inlined into the filterCookies function? It
> only seems to be used the once and you would be able to use aFilterValue rather
> than cookiesTreeView.filterValue...

FF used it in two places but my patch doesn't so you're right.
Comment 9 neil@parkwaycc.co.uk 2009-02-25 04:47:05 PST
Comment on attachment 363970 [details] [diff] [review]
patch v4, r=iann_bugzilla

This does nasty things if you set a new cookie while the filter is active.

Also, rather than maintaining
Comment 10 neil@parkwaycc.co.uk 2009-02-25 04:50:55 PST
Sorry, hit tab instead of a, so when I hit space it submitted the comment...

Anyway, rather than maintaining the filtered flag, why not set the displayed array to be the same as the all cookies array? Also, given the existing coding style, I think I'd prefer the displayed array to be a global variable. (I'm not sure whether it would be better to rename the all cookies array, so that you could use "cookies" for the displayed cookies.)
Comment 11 Jens Hatlak (:InvisibleSmiley) 2009-02-28 16:47:11 PST
Created attachment 364723 [details] [diff] [review]
patch v5

New approach in Neil's direction:
- handles the case where cookies are added during filtered mode (calls filter() in loadCookies() which in turn is called by observer/cookieReloadDisplay)
- replaces filtered/filterSet/cookieStore by cookies/allCookies.

Notes:
- the global cookies array is either set by loadCookies() itself or indirectly by filter()
- (de)activation of the Remove All Cookies button is done in filter() in filtered mode and in loadCookies() in unfiltered mode
- DeleteCookie() and DeleteAllCookies() delete from the global cookies array only but FinalizeCookieDeletions() uses the CookieManager which has the cookie-changed observer (cookieReloadDisplay) attached which calls loadCookies() which in turn updates the global allCookies array.
Comment 12 neil@parkwaycc.co.uk 2009-03-01 09:18:42 PST
Comment on attachment 364723 [details] [diff] [review]
patch v5

>-  cookiesTreeView.rowCount = cookies.length;
>+  cookiesTreeView.rowCount = allCookies.length;
I'm not sure you need to bother with this, as an improved filter method would do it anyway.

>+  cookiesTreeView.filterValue = aFilterValue;
You write this value but never read it... why bother?

>+  if (filter == "") {
>+    clearFilter();
>+    return;
>+  }
>+
>+  // clear the display
>+  var oldCount = cookiesTreeView.rowCount;
>+  cookiesTreeView.rowCount = 0;
>+  cookiesTree.treeBoxObject.rowCountChanged(0, -oldCount);
>+
>+  // set up the filtered display
>+  cookies = filterCookies(filter);
clearFilter seems somewhat excessive, when you could use
cookies = filter ? filterCookies(filter) : allCookies;
which would also let you call filter() from loadCookies.

>+  cookiesTreeView.rowCount = cookies.length;
>+  cookiesTree.treeBoxObject.rowCountChanged(0, cookiesTreeView.rowCount);
>+  document.getElementById("removeAllCookies").setAttribute("disabled", "true");
Hmm... doesn't it make sense for the delete all cookies function to delete all the filtered cookies?

>+  // remove the selection
>+  cookiesTreeView.selection.clearSelection();
This shouldn't be necessary; rowCountChanged does that for you.
Comment 13 Jens Hatlak (:InvisibleSmiley) 2009-03-01 17:11:32 PST
Created attachment 364829 [details] [diff] [review]
patch v6

Nits addressed, even with fewer code. :-)

I had to remove the setting of gUpdatingBatch, though because otherwise the changes to the cookies array made by the Delete*Cookie* functions won't be reflected in the allCookies array which then breaks the view once the filter changes. Going through the whole observer/loadCookies process in this case makes sure that everything is set up correctly internally.

> >+  cookiesTreeView.rowCount = cookies.length;
> >+  cookiesTree.treeBoxObject.rowCountChanged(0, cookiesTreeView.rowCount);
> >+  document.getElementById("removeAllCookies").setAttribute("disabled", "true");
> Hmm... doesn't it make sense for the delete all cookies function to delete all
> the filtered cookies?

It's not so much a question of what the function should do but rather what the button should do. The button is labeled "Remove All Cookies" and that's what it does (with a confirmation dialog in between). Changing what the function does would create some ambiguity and I tried to avoid that in order to not have to change the button label depending on the filter state. Anyway if you want to delete all cookies displayed in filtered mode it's not too complicated either since deleting multiple cookies at once works so it's just a matter of selecting all cookies. I even made Ctrl+A select all entries locally but then removed it because the chance of accidentally hitting Ctrl+A + Delete (which doesn't ask for a confirmation) is just too great, especially since that same sequence has a totally different meaning in the search box above.
Comment 14 Robert Kaiser 2009-03-02 03:38:41 PST
(In reply to comment #13)
> I even made Ctrl+A select all entries locally but then
> removed it because the chance of accidentally hitting Ctrl+A + Delete (which
> doesn't ask for a confirmation) is just too great, especially since that same
> sequence has a totally different meaning in the search box above.

Sounds to me like we should really make the delete operation ask for confirmation if more than one entry is selected.
Comment 15 neil@parkwaycc.co.uk 2009-03-02 07:52:44 PST
Comment on attachment 364829 [details] [diff] [review]
patch v6

>   // sort by host column
>   CookieColumnSort('rawHost');
> 
>+  filter(document.getElementById("filter").value);
This doesn't work in this order ;-) Also, sorting filtered cookies would be interesting. One I have is to make CookieColumnSort work on unfiltered cookies, and then make it call filter to redisplay the results, although that way you would forget the selected row.

>-  gUpdatingBatch = "cookie-changed";
>+  gUpdatingBatch = "";
We really need batch updates, because otherwise deleting lots of cookies is really slow (we'd refilter for every single cookie...) so maybe DeleteCookie can manually remove the deleted cookies from the allCookies array?

>+  if (filter != "" || cookies.length == 0)
Nit: just use if (filter || !cookies.length)

>+  if (filter != "" && cookiesTreeView.rowCount > 0)
Just use if (filter && cookies.length)
[I'm not sure it makes sense to handle this differently for unfiltered views]
Comment 16 Jens Hatlak (:InvisibleSmiley) 2009-03-02 14:54:24 PST
Created attachment 365035 [details] [diff] [review]
patch v7

(In reply to comment #14)
> (In reply to comment #13)
> > I even made Ctrl+A select all entries locally but then
> > removed it because the chance of accidentally hitting Ctrl+A + Delete (which
> > doesn't ask for a confirmation) is just too great, especially since that same
> > sequence has a totally different meaning in the search box above.
> 
> Sounds to me like we should really make the delete operation ask for
> confirmation if more than one entry is selected.

I added a confirmation and the Ctrl+A shortcut now that the danger is out of the way. This should really ease deleting all displayed cookies in filtered mode without adding ambiguity to the Remove All Cookies button.

(In reply to comment #15)
> (From update of attachment 364829 [details] [diff] [review])
> >   // sort by host column
> >   CookieColumnSort('rawHost');
> > 
> >+  filter(document.getElementById("filter").value);
> This doesn't work in this order ;-)

Right, I didn't check that CookieColumnSort uses the cookies array.

> Also, sorting filtered cookies would be
> interesting. One I have is to make CookieColumnSort work on unfiltered cookies,
> and then make it call filter to redisplay the results, although that way you
> would forget the selected row.

Sorry but I don't get what you're saying there. :-/ Since CookieColumnSort() is called after filter() now and uses the cookies array, what's more to be done? (IOW: Can we leave it like that?)

> >-  gUpdatingBatch = "cookie-changed";
> >+  gUpdatingBatch = "";
> We really need batch updates, because otherwise deleting lots of cookies is
> really slow (we'd refilter for every single cookie...) so maybe DeleteCookie
> can manually remove the deleted cookies from the allCookies array?

I reverted back to the old behavior and added code to take care of allCookies; it's simple for DeleteAllCookies() but unfortunately O(m*(n/2)) or something like that for DeleteCookie() in filtered mode. FF is better off here only because it maintains a look-up table for cookies grouped by host (also used in the UI).

> >+  if (filter != "" && cookiesTreeView.rowCount > 0)
> Just use if (filter && cookies.length)
> [I'm not sure it makes sense to handle this differently for unfiltered views]

Neither am I (i.e. I'm undecided) but this way at least it's consistent with the Password Manager.
Comment 17 neil@parkwaycc.co.uk 2009-03-03 05:26:51 PST
Comment on attachment 365035 [details] [diff] [review]
patch v7

[This isn't quite right because it gets confused when you sort the cookies while they're filtered, and then clear the filter.]

>+  if (document.getElementById("filter").value) {
Nit: could use if (cookies != allCookies)

>+    // remove selected cookies from unfiltered set
>+    for (var i = 0; i < deletedCookies.length; ++i) {
>+      var item = deletedCookies[i];
>+      for (var j = 0; j < allCookies.length; ++j) {
>+        if (item == allCookies[j]) {
This should use allCookies.indexOf

>+  } else
>+    allCookies = cookies;
This looks wrong to me.

>+  } else if (e.ctrlKey && e.charCode == "a".charCodeAt(0)) {
This is wrong, because it isn't localisable. Fixing it is non-trival though. You'll have to remove it and add the "A" key in a separate bug.
Comment 18 Jens Hatlak (:InvisibleSmiley) 2009-03-03 13:18:56 PST
Created attachment 365277 [details] [diff] [review]
patch v8

(In reply to comment #17)
> (From update of attachment 365035 [details] [diff] [review])
> [This isn't quite right because it gets confused when you sort the cookies
> while they're filtered, and then clear the filter.]

True. With my new approach I tested the following conditions:
- opening the dialog
- changing sort, then filtering once, twice, ...
- filtering, then changing sort, then going back to unfiltered view
- watching the view (filtered and unfiltered) while adding a cookie
- watching the view after deleting a cookie.

In the initial case the view should be sorted by Site, afterwards the sorting should be kept in all cases. AFAICS this applies to my implementation.

> >+  if (document.getElementById("filter").value) {
> Nit: could use if (cookies != allCookies)

No, DeleteSelectedItemFromTree() modifies the cookies array, moving all cookies marked for deletion to the deletedCookies array. Using your check I would lose the latter and I see no need to query the selection myself.

> >+  } else
> >+    allCookies = cookies;
> This looks wrong to me.

Why? The cookies array has been modified and the allCookies array needs to be updated, no?

> >+  } else if (e.ctrlKey && e.charCode == "a".charCodeAt(0)) {
> This is wrong, because it isn't localisable. Fixing it is non-trival though.
> You'll have to remove it and add the "A" key in a separate bug.

Removed. I hope you don't mind me keeping the 46 -> KeyEvent.DOM_VK_DELETE change.
Comment 19 neil@parkwaycc.co.uk 2009-03-03 15:48:39 PST
(In reply to comment #18)
>(In reply to comment #17)
>>(From update of attachment 365035 [details] [diff] [review])
>>>+  if (document.getElementById("filter").value) {
>>Nit: could use if (cookies != allCookies)
>No, DeleteSelectedItemFromTree() modifies the cookies array, moving all cookies
>marked for deletion to the deletedCookies array. Using your check I would lose
>the latter and I see no need to query the selection myself.
...
>>>+  } else
>>>+    allCookies = cookies;
>>This looks wrong to me.
>Why? The cookies array has been modified and the allCookies array needs to be
>updated, no?
When filter() does cookies = allCookies; [when there is no filter] this makes both variables refer to the same array, so changing one "changes" the other. In particular, cookies == allCookies is always true when there is no filter, and allCookies = cookies; has no effect because they are already the same.

>Removed. I hope you don't mind me keeping the 46 -> KeyEvent.DOM_VK_DELETE
>change.
Yes, .keyCode should always be compared with a KeyEvent code.
Comment 20 neil@parkwaycc.co.uk 2009-03-04 04:47:25 PST
Comment on attachment 365277 [details] [diff] [review]
patch v8

>-      if (lastCookieSortColumn == "rawHost") {
>-        lastCookieSortAscending = !lastCookieSortAscending; // prevents sort from being reversed
>-      }
You know, I think you've just fixed an unrelated bug - in the old code, it looks like deleting a cookie resets the sort order to rawHost.

>+  // sort by last column (initially Site column)
>+  if (lastCookieSortColumn) {
>+    lastCookieSortAscending = !lastCookieSortAscending; // prevents sort from being reversed
>+    lastCookieSortAscending = SortTree(cookiesTree, cookiesTreeView, cookies,
>+                                       lastCookieSortColumn, lastCookieSortColumn,
>+                                       lastCookieSortAscending);
>+  } else
>+    CookieColumnSort('rawHost');
So, some thoughts on this code...
1. Hardly worth setting lastCookieSortAscending = !lastCookieSortAscending; when we're going to set it again. Might as well inline that in to the call to SortTree.
2. In fact, since we know what the result of SortTree is going to be, we don't need to set lastCookieSortAscending at all.
3. Or, instead of 2), you could just call CookieColumnSort all the time. This works especially well if lastCookieSortColumn and lastCookieSortAscending are initialised to their "default" values (rawHost and true).
4. Or, you could set the lastCookieSort variables and also set the sortDirection attribute in the XUL, so you can always call SortTree instead.
5. Although in fact you don't need to sort unless you're clearing a filter.
Comment 21 Jens Hatlak (:InvisibleSmiley) 2009-03-04 14:46:17 PST
Created attachment 365546 [details] [diff] [review]
patch v9

(In reply to comment #19)
>>>>+    allCookies = cookies;
>>>This looks wrong to me.
>>Why? The cookies array has been modified and the allCookies array needs to be
>>updated, no?
>When filter() does cookies = allCookies; [when there is no filter] this makes
>both variables refer to the same array

True; removed that part.

(In reply to comment #20)
> (From update of attachment 365277 [details] [diff] [review])
> >-      if (lastCookieSortColumn == "rawHost") {
> >-        lastCookieSortAscending = !lastCookieSortAscending; // prevents sort from being reversed
> >-      }
> You know, I think you've just fixed an unrelated bug

I know, I was just curious to see whether anyone else noticed. ;-)

> in the old code, it
> looks like deleting a cookie resets the sort order to rawHost.

No, deletion doesn't trigger the observer because that's specifically disabled in FinalizeCookieDeletions() by setting gUpdatingBatch to "cookie-changed". Adding a cookie triggers the problem in the old code, though.

> >+  // sort by last column (initially Site column)
> >+  if (lastCookieSortColumn) {
> >+    lastCookieSortAscending = !lastCookieSortAscending; // prevents sort from being reversed
> >+    lastCookieSortAscending = SortTree(cookiesTree, cookiesTreeView, cookies,
> >+                                       lastCookieSortColumn, lastCookieSortColumn,
> >+                                       lastCookieSortAscending);
> >+  } else
> >+    CookieColumnSort('rawHost');
> So, some thoughts on this code...
> 1. Hardly worth setting lastCookieSortAscending = !lastCookieSortAscending;
> when we're going to set it again. Might as well inline that in to the call to
> SortTree.

Right, done.

> 2. In fact, since we know what the result of SortTree is going to be, we don't
> need to set lastCookieSortAscending at all.

Right, done.

> 3. Or, instead of 2), you could just call CookieColumnSort all the time. This
> works especially well if lastCookieSortColumn and lastCookieSortAscending are
> initialised to their "default" values (rawHost and true).

CookieColumnSort has too much overhead for my taste. All parts except the first one (which I replicated) are irrelevant for this case (neither sort column nor sort order changed).

> 4. Or, you could set the lastCookieSort variables and also set the
> sortDirection attribute in the XUL, so you can always call SortTree instead.

Right, done.

> 5. Although in fact you don't need to sort unless you're clearing a filter.

Wrong, filterCookies() creates a new array every time the filter changes, thus a resort needs to be done in each case filter() is called, not only when going back to unfiltered mode.
Comment 22 neil@parkwaycc.co.uk 2009-03-04 16:12:04 PST
(In reply to comment #21)
> (In reply to comment #20)
> > 5. Although in fact you don't need to sort unless you're clearing a filter.
> Wrong, filterCookies() creates a new array every time the filter changes, thus
> a resort needs to be done in each case filter() is called, not only when going
> back to unfiltered mode.
I might have misremembered, but I sorta(!) thought the allCookies array was always sorted, so the result of filtering it was always going to be sorted too.
Comment 23 neil@parkwaycc.co.uk 2009-03-04 16:14:58 PST
Comment on attachment 365546 [details] [diff] [review]
patch v9

>+var lastCookieSortColumn = "rawHost";
>+var lastCookieSortAscending = true;
...
>               <treecol id="domainCol" label="&treehead.cookiedomain.label;" flex="5"
>-                       onclick="CookieColumnSort('rawHost', true);" persist="width hidden"/>
>+                       onclick="CookieColumnSort('rawHost', true);" persist="width hidden"
>+                       sortDirection="descending"/>
Should be "ascending", no?
Comment 24 neil@parkwaycc.co.uk 2009-03-05 04:43:34 PST
(In reply to comment #23)
>>+                       sortDirection="descending"/>
>Should be "ascending", no?
Oh wow, the cookie viewer has had this wrong all along?!
Comment 25 Jens Hatlak (:InvisibleSmiley) 2009-03-05 10:14:20 PST
I'll file follow-up bugs for adding the (localizable) Ctrl+A shortcut and correcting the relationship between sort marker and sort direction. Although we're nowhere near being consistent; e.g. the Password Manager has no sort markers at all while the Bookmark Manager seems to use tri-state sort markers (or there's another bug there).
Comment 26 Stefan [:stefanh] 2009-03-05 12:37:40 PST
http://hg.mozilla.org/comm-central/rev/a5720b15e053

Note You need to log in before you can comment on or make changes to this bug.