1073967 - Storage Inspector Columns are not sorted correctly

Status: RESOLVED FIXED

(DevTools :: Storage Inspector, defect, P2)

Description

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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.

Comment 3

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

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

Comment 4

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: 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/

Comment 5

[Nicolas Chevobbe [: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

Comment 6

[Nicolas Chevobbe [:nchevobbe]]

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 7

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Comment on attachment 8860385 [details]
Bug 1073967 - Storage Inspector columns should use natural sort

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/132400/diff/1-2/

Comment 8

[Nicolas Chevobbe [:nchevobbe]]

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 !

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ef0ad7316ebb
Storage Inspector columns should use natural sort r=nchevobbe

Comment 10

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/ef0ad7316ebb