Closed Bug 1073967 Opened 5 years ago Closed 3 years ago

Storage Inspector Columns are not sorted correctly

Categories

(DevTools :: Storage Inspector, defect, P2)

x86_64
Windows 8.1
defect

Tracking

(firefox55 fixed)

RESOLVED FIXED
Firefox 55
Tracking Status
firefox55 --- fixed

People

(Reporter: aryx, Assigned: miker)

References

Details

(Whiteboard: [todo-mr])

Attachments

(1 file)

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.
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
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 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 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 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+
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
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.