Storage Inspector Columns are not sorted correctly

RESOLVED FIXED in Firefox 55

Status

()

Firefox
Developer Tools: Storage Inspector
P2
normal
RESOLVED FIXED
4 years ago
a year ago

People

(Reporter: aryx, Assigned: miker)

Tracking

Trunk
Firefox 55
x86_64
Windows 8.1
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: [todo-mr])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

Firefox Nightly and Aurora 20140928 on Windows 8.1

The columns in the storage inspector (tested with Cookies view) currently sort alphabetically, but

a) the date columns should sort by date. At the moment, the order is e.g.
Session
4.6.2015
28.9.2015
28.9.2014
27.9.2016
27.3.2015

b) for the host list, a sorting by domain and then sorting each set by subdomain would make more sense.
Depends on: 1251561
Duplicate of this bug: 1318763
Duplicate of this bug: 1199126
Summary: Column sorting improvements (chronological by date and alphabetical by domain followed by subdomains) → Storage Inspector Columns are not sorted correctly
Assignee: nobody → mratcliffe
Status: NEW → ASSIGNED
Priority: -- → P2
Whiteboard: [todo-mr]

Jim Palmer's natural sort works with numbers and strings in the following formats:
  - strings
  - datetime
  - version number strings
  - numerics
  - IP addresses
  - filenames
  - hex
  - unicode
Created attachment 8860385 [details]
Bug 1073967 - Storage Inspector columns should use natural sort

Changlist:
  - Added Jim Palmer's well proven natural sort algorithm.
  - Added natural sort license (MIT).
  - Use natural sort everywhere inside TableWidget.js wherever we use .sort()
  - Changed browser_storage_overflow.js so that the test is faster and more maintainable. The test now also tests column sorting (ascending and descending).
  - Use natural sort everywhere inside storage.js wherever we need to slice the array. Without natural sort here we get e.g. row-1, row-10, row-100, row-2 etc.

Review commit: https://reviewboard.mozilla.org/r/132400/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/132400/
Attachment #8860385 - Flags: review?(nchevobbe)

Comment 5

a year ago
mozreview-review
Comment on attachment 8860385 [details]
Bug 1073967 - Storage Inspector columns should use natural sort

https://reviewboard.mozilla.org/r/132400/#review135288

Please fix the hardcoded `150` value in the test, or change the whole test if you think my comment of rewriting it makes sense.

::: devtools/client/storage/test/browser_storage_overflow.js:38
(Diff revision 1)
> +function checkCellValues(prefix, start, end) {
> +  let cells = [...gPanelWindow.document
> +                              .querySelectorAll("#name .table-widget-cell")];
> +
> +  if (start < end) {
> +    for (let i = start; i < end; i++) {
> +      let cell = cells[i - 1];
> +
> +      is(cell.value, `${prefix}${i}`, "Cell value is correct (ascending).");
> +    }
> +  } else {
> +    for (let i = start; i >= end; i--) {
> +      let cell = cells[150 - i];
> +
> +      is(cell.value, `${prefix}${i}`, "Cell value is correct (descending).");
> +    }
> +  }

I have the feeling we could simplify this a bit more.

Since we already retrieve all the nodes directly within the test, I think we could simplify its signature with `function checkCellValues(order)`.

We would then call :
```
// Check that the columns are sorted in a human readable way (ascending).
checkCellValues("ASC");
// Check that the columns are sorted in a human readable way (descending).
checkCellValues("DESC");
```

The total number of items is not important here, we already tested it with `checkCellLength`.

We could then only have a simple forEach to test the cells values
```
cells.forEach(function(cell, index, arr){
  let i = order === "ASC" ? index : arr.length;
  is(cell.value, `item-${i}`, `Cell value is correct (${order}).`)
})
```

What do you think of this ?

::: devtools/client/storage/test/browser_storage_overflow.js:50
(Diff revision 1)
> +
> +      is(cell.value, `${prefix}${i}`, "Cell value is correct (ascending).");
> +    }
> +  } else {
> +    for (let i = start; i >= end; i--) {
> +      let cell = cells[150 - i];

I think this should be `start` instead of 150
Attachment #8860385 - Flags: review?(nchevobbe) → review-

Comment 6

a year ago
mozreview-review-reply
Comment on attachment 8860385 [details]
Bug 1073967 - Storage Inspector columns should use natural sort

https://reviewboard.mozilla.org/r/132400/#review135288

> I have the feeling we could simplify this a bit more.
> 
> Since we already retrieve all the nodes directly within the test, I think we could simplify its signature with `function checkCellValues(order)`.
> 
> We would then call :
> ```
> // Check that the columns are sorted in a human readable way (ascending).
> checkCellValues("ASC");
> // Check that the columns are sorted in a human readable way (descending).
> checkCellValues("DESC");
> ```
> 
> The total number of items is not important here, we already tested it with `checkCellLength`.
> 
> We could then only have a simple forEach to test the cells values
> ```
> cells.forEach(function(cell, index, arr){
>   let i = order === "ASC" ? index : arr.length;
>   is(cell.value, `item-${i}`, `Cell value is correct (${order}).`)
> })
> ```
> 
> What do you think of this ?

I got the `let i` part wrong, but I think you see what I mean
Comment hidden (mozreview-request)

Comment 8

a year ago
mozreview-review
Comment on attachment 8860385 [details]
Bug 1073967 - Storage Inspector columns should use natural sort

https://reviewboard.mozilla.org/r/132400/#review135320

All good Mike, thanks !
Attachment #8860385 - Flags: review?(nchevobbe) → review+

Comment 9

a year ago
Pushed by mratcliffe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ef0ad7316ebb
Storage Inspector columns should use natural sort r=nchevobbe
https://hg.mozilla.org/mozilla-central/rev/ef0ad7316ebb
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
You need to log in before you can comment on or make changes to this bug.