Closed Bug 1323391 Opened 8 years ago Closed 7 years ago

Sort sites listed in Settings of Site Data

Categories

(Firefox :: Settings UI, defect)

Product:
Firefox
Component:
Settings UI
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 53
Tracking Flags:
Tracking Status
firefox53 --- fixed

People

(Reporter: Fischer, Assigned: Fischer)

References

Details

(Whiteboard: [storage-v1])

Attachments

(1 file)

Bug 1323391 - Sort sites listed in Settings of Site Data,
58 bytes, text/x-review-board-request
Gijs
: review+
Details 
Should be able to sort sites listed in Settings of Site Data by usage
No longer depends on: 1312374
Assignee: nobody → fliu
(In reply to Fischer [:Fischer] from comment #1)
> Created attachment 8820119 [details]
> Bug 1323391 - Sort sites listed in Settings of Site Data by usage,
> 
> Review commit: https://reviewboard.mozilla.org/r/99640/diff/#index_header
> See other reviews: https://reviewboard.mozilla.org/r/99640/

Hi Gijs,

This patch adds the feature of sorting sites listed in the Settings subdialog by the storage usage column [1].

One test case is added as well.
Here is the try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=877d93d99e4d18983f780dd2233738197a62ff67

[1] https://mozilla.invisionapp.com/share/4Y87EJO39#/screens/179637924
Comment on attachment 8820119 [details]
Bug 1323391 - Sort sites listed in Settings of Site Data,

https://reviewboard.mozilla.org/r/99640/#review100192

I noted some trivial issues below. But more generally, we should suport sorting on all columns, not just the size one, and I am not convinced that sorting on size is the right default. Alphabetical by hostname seems like it would be easier for users to understand. Either way, we should implement sort for all of these. Perhaps you can reuse/abstract some of the sorting code as used in onPermissionsSort in permissions.js.

::: browser/components/preferences/siteDataSettings.js:81
(Diff revision 1)
> +      this._sortSites(this._sites, "decending");
> +      usageCol.setAttribute("sortDirection", "decending");

Same spelling mistakes here.

::: browser/components/preferences/siteDataSettings.xul:34
(Diff revision 1)
>      <richlistbox id="sitesList" orient="vertical" flex="1">
>        <listheader>
>          <treecol flex="4" width="50" label="&hostCol.label;"/>
>          <treecol flex="2" width="50" label="&statusCol.label;"/>
> -        <treecol flex="1" width="50" label="&usageCol.label;"/>
> +        <treecol flex="1" width="50" label="&usageCol.label;" accesskey="&usageCol.accesskey;"
> +                 id="usageCol" sortDirection="decending"/>

"descending".

Also, I don't think you want an access key here. Or, if you do, we should have one for each column.

I'm also not convinced that *defaulting* the dialog to sort by usage is the right decision.
Attachment #8820119 - Flags: review?(gijskruitbosch+bugs)
(In reply to :Gijs Kruitbosch from comment #3)
> Comment on attachment 8820119 [details]
> Bug 1323391 - Sort sites listed in Settings of Site Data by usage,
> 
> https://reviewboard.mozilla.org/r/99640/#review100192
> 
> I noted some trivial issues below. But more generally, we should suport
> sorting on all columns, not just the size one, and I am not convinced that
> sorting on size is the right default. Alphabetical by hostname seems like it
> would be easier for users to understand. Either way, we should implement
> sort for all of these. Perhaps you can reuse/abstract some of the sorting
> code as used in onPermissionsSort in permissions.js.
> 
Check with the UX team. Will follow other tables, like cookies table, to sort on all columns and default by hostname. The specs will be updated too.

> ::: browser/components/preferences/siteDataSettings.js:81
> (Diff revision 1)
> > +      this._sortSites(this._sites, "decending");
> > +      usageCol.setAttribute("sortDirection", "decending");
> 
> Same spelling mistakes here. 
Sorry for this typo and thanks for spotting it.
Just found why the UI works correctly and I didn't find it in the first place is because the CSS rule[1] only checks "ascending":
```
... > xul|*.treecol-sortdirection[sortDirection] {
  ...
}

... > xul|*.treecol-sortdirection[sortDirection="ascending"] {
  ...
}
```

[1] https://dxr.mozilla.org/mozilla-central/source/toolkit/themes/shared/in-content/common.inc.css#789

> ::: browser/components/preferences/siteDataSettings.xul:34
> (Diff revision 1)
> > +        <treecol flex="1" width="50" label="&usageCol.label;" accesskey="&usageCol.accesskey;"
> > +                 id="usageCol" sortDirection="decending"/>
> 
> "descending".
> 
> Also, I don't think you want an access key here. Or, if you do, we should
> have one for each column.
> 
OK, will update.
(In reply to Fischer [:Fischer] from comment #0)
> Should be able to sort sites listed in Settings of Site Data by usage
Update the description:
---------------------------------------------------------------
Should be able to sort sites listed in Settings of Site Data by
  - host (default in ascending order)
  - permission status
  - usage
Summary: Sort sites listed in Settings of Site Data by usage → Sort sites listed in Settings of Site Data
Hi Gijs,

Update patch to enable to sort sites by host(default), permission status, or usage.
TRY: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c4653ff96c25b920e61db4129ee7fc560b51b872

Thanks
Comment on attachment 8820119 [details]
Bug 1323391 - Sort sites listed in Settings of Site Data,

https://reviewboard.mozilla.org/r/99640/#review102446

This is basically ready besides the issues below. I would have given r+ except I really don't follow the data-last-sortDirection thing. Can you explain / fix? :-)

::: browser/components/preferences/in-content/tests/browser_advanced_siteData.js:294
(Diff revision 2)
> +    let actual = siteItems[i].getAttribute("data-origin");
> +    is(actual, expected, "Should reverse sites to the descending order by usage");
> +  }
> +
> +  mockSiteDataManager.unregister();
> +  gBrowser.removeCurrentTab();

Nit: yield BTU.removeTab(gBrowser.selectedTab);

::: browser/components/preferences/siteDataSettings.js:36
(Diff revision 2)
> +    }
> +
>      this._list = document.getElementById("sitesList");
>      SiteDataManager.getSites().then(sites => {
>        this._sites = sites;
> -      this._sortSites(this._sites, "decending");
> +      let sortCol = this._list.querySelector("#hostCol");

Nit: document.getElementById("hostCol");

Note that this would cause the sort to always reset when the dialog is reopened. Normally speaking, we persist the sort direction (using the XUL persist mechanism) and reuse it once the dialog is reopened. Is there a particular reason not do this here?

::: browser/components/preferences/siteDataSettings.js:52
(Diff revision 2)
>     * @param sites {Array}
> -   * @param order {String} indicate to sort in the "decending" or "ascending" order
> +   * @param col {XULElement} the <treecol> being sorted on
>     */
> -  _sortSites(sites, order) {
> -    sites.sort((a, b) => {
> -      if (order === "ascending") {
> +  _sortSites(sites, col) {
> +    let isCurrentSortCol = col.getAttribute("data-isCurrentSortCol")
> +    let sortDirection = col.getAttribute("data-last-sortDirection") || "ascending";

Why do we need the extra `data-last-sortDirection` attribute? Why isn't just the `sortDirection` attribute enough?

::: browser/components/preferences/siteDataSettings.js:76
(Diff revision 2)
> +    sites.sort(sortFunc);
> +    if (sortDirection === "descending") {
> +      sites.reverse();
> -      }
> +    }

Instead of reversing, make it part of the sort:

```js
if (sortDirection == "descending") {
  sites.sort((a, b) => sortFunc(b, a));
} else {
  sites.sort(sortFunc);
}
```
Attachment #8820119 - Flags: review?(gijskruitbosch+bugs)
(In reply to :Gijs from comment #8)
> :::
> browser/components/preferences/in-content/tests/browser_advanced_siteData.js:
> > +  gBrowser.removeCurrentTab();
> 
> Nit: yield BTU.removeTab(gBrowser.selectedTab);
Updated, thanks.

> ::: browser/components/preferences/siteDataSettings.js:36
> > +      let sortCol = this._list.querySelector("#hostCol");
> 
> Nit: document.getElementById("hostCol");
Updated, thanks.

> Note that this would cause the sort to always reset when the dialog is
> reopened. Normally speaking, we persist the sort direction (using the XUL
> persist mechanism) and reuse it once the dialog is reopened. Is there a
> particular reason not do this here?
The cookies list in Privacy and the saved logins list in Security all reset to sort on the host column on loading. This is to be consistent with them.

> ::: browser/components/preferences/siteDataSettings.js:52
> > -  _sortSites(sites, order) {
> > -    sites.sort((a, b) => {
> > -      if (order === "ascending") {
> > +  _sortSites(sites, col) {
> > +    let isCurrentSortCol = col.getAttribute("data-isCurrentSortCol")
> > +    let sortDirection = col.getAttribute("data-last-sortDirection") || "ascending";
> 
> Why do we need the extra `data-last-sortDirection` attribute? Why isn't just
> the `sortDirection` attribute enough?
This is to remember each column's sort direction. Fox example,
1. User sorts on usage in descending order to spot the most usage used.
2. User switches to status column to take a look
3. User switches back to usage column and can spot the most usage used instantly (because sort direction is remembered).

> ::: browser/components/preferences/siteDataSettings.js:76
> (Diff revision 2)
> > +    sites.sort(sortFunc);
> > +    if (sortDirection === "descending") {
> > +      sites.reverse();
> > -      }
> > +    }
> 
> Instead of reversing, make it part of the sort:
> 
> ```js
> if (sortDirection == "descending") {
>   sites.sort((a, b) => sortFunc(b, a));
> } else {
>   sites.sort(sortFunc);
> }
> ```
Updated, thanks.
(In reply to Fischer [:Fischer] from comment #10)
> > ::: browser/components/preferences/siteDataSettings.js:52
> > > -  _sortSites(sites, order) {
> > > -    sites.sort((a, b) => {
> > > -      if (order === "ascending") {
> > > +  _sortSites(sites, col) {
> > > +    let isCurrentSortCol = col.getAttribute("data-isCurrentSortCol")
> > > +    let sortDirection = col.getAttribute("data-last-sortDirection") || "ascending";
> > 
> > Why do we need the extra `data-last-sortDirection` attribute? Why isn't just
> > the `sortDirection` attribute enough?
> This is to remember each column's sort direction. Fox example,
> 1. User sorts on usage in descending order to spot the most usage used.
> 2. User switches to status column to take a look
> 3. User switches back to usage column and can spot the most usage used
> instantly (because sort direction is remembered).

OK, but then, can't we simply not remove the sortDirection attribute all the time?
Flags: needinfo?(fliu)
(In reply to :Gijs from comment #11)
> OK, but then, can't we simply not remove the sortDirection attribute all the
> time?

... or does that have other, undesirable, side-effects, like maybe styling changes or something?
(In reply to :Gijs from comment #12)
> (In reply to :Gijs from comment #11)
> > OK, but then, can't we simply not remove the sortDirection attribute all the
> > time?
> 
> ... or does that have other, undesirable, side-effects, like maybe styling
> changes or something?
Yes, you are right. The little "v" sorting icon would appear based on the sortDirection attribute.
Flags: needinfo?(fliu)
Comment on attachment 8820119 [details]
Bug 1323391 - Sort sites listed in Settings of Site Data,

https://reviewboard.mozilla.org/r/99640/#review102816

::: browser/components/preferences/in-content/tests/browser_advanced_siteData.js:235
(Diff revisions 2 - 3)
>      is(actual, expected, "Should sort sites in the ascending order by host");
>    }
>  
>    // Test reversing to the descending order by host
>    hostCol.click();
> -  mockSites.reverse();
> +  mockSites.sort((a, b) => sortByHost(b ,a));

I actually thought we could leave it as .reverse() in the test, as you already have a sorted array, so you don't need to do the sort again... does the test pass if you use .reverse()? If so, I guess I don't really mind either way... 

I ask because keeping the implementation of the test in sync with the implementation of the feature is odd in cases like this, because it shouldn't make any difference to the test how the sorting is implemented - it should only verify that it works. :-)
Attachment #8820119 - Flags: review?(gijskruitbosch+bugs) → review+
(In reply to Fischer [:Fischer] from comment #13)
> (In reply to :Gijs from comment #12)
> > (In reply to :Gijs from comment #11)
> > > OK, but then, can't we simply not remove the sortDirection attribute all the
> > > time?
> > 
> > ... or does that have other, undesirable, side-effects, like maybe styling
> > changes or something?
> Yes, you are right. The little "v" sorting icon would appear based on the
> sortDirection attribute.

Ah, OK then. Let's leave it like this.
(In reply to :Gijs from comment #14)
> > -  mockSites.reverse();
> > +  mockSites.sort((a, b) => sortByHost(b ,a));
> 
> I actually thought we could leave it as .reverse() in the test, as you
> already have a sorted array, so you don't need to do the sort again... does
> the test pass if you use .reverse()? If so, I guess I don't really mind
> either way... 
> 
> I ask because keeping the implementation of the test in sync with the
> implementation of the feature is odd in cases like this, because it
> shouldn't make any difference to the test how the sorting is implemented -
> it should only verify that it works. :-)
> 
When, for example, a.usage equals to b.usage, then the order of a and b by sorting ascending then ".reverse()" would be different from by sorting descending. On the UI, the both results fulfill the demand of sorting on some column, however, there could be a not-main-focus difference for the above example.

Maybe, we could define more details on the sorting rules for this kind of case. But I am just not sure if this would make things too complex since for the most daily usage simply sorting on one column as commanded is enough. Like just want to find and clear sites using the most usage.
(In reply to Fischer [:Fischer] from comment #16)
> Maybe, we could define more details on the sorting rules for this kind of
> case. But I am just not sure if this would make things too complex since for
> the most daily usage simply sorting on one column as commanded is enough.
> Like just want to find and clear sites using the most usage.

Then the test isn't verifying what it wants to verify - it's too strict. It should just check that after being sorted, the items in the DOM list are sorted correctly (ie every item is greater-or-equal (or smaller-or-equal) than the next). Then the test will pass independently from how the sorting algorithm is implemented.
Comment on attachment 8820119 [details]
Bug 1323391 - Sort sites listed in Settings of Site Data,

Hi, Updated the test to make it independent from how the sorting algorithm is implemented, thanks.
Attachment #8820119 - Flags: review+ → review?(gijskruitbosch+bugs)
Comment on attachment 8820119 [details]
Bug 1323391 - Sort sites listed in Settings of Site Data,

https://reviewboard.mozilla.org/r/99640/#review102886

::: browser/components/preferences/in-content/tests/browser_advanced_siteData.js:37
(Diff revisions 3 - 4)
>    }
>  };
>  
>  const mockSiteDataManager = {
> -  sites: [
> +  sites: new Map([
> +    [ 

Nit: trailing whitespace here and below.
Attachment #8820119 - Flags: review?(gijskruitbosch+bugs) → review+
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/2a4988f51726
Sort sites listed in Settings of Site Data, r=Gijs
Keywords: checkin-needed
sorry had to back out this for eslint failure, i.e., https://treeherder.mozilla.org/logviewer.html#?job_id=66754872&repo=autoland&lineNumber=8459
Flags: needinfo?(fliu)
Backout by ihsiao@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1d31a5d94ce9
Backed out changeset 2a4988f51726 for eslint failure
(In reply to Iris Hsiao [:ihsiao] from comment #25)
> sorry had to back out this for eslint failure, i.e.,
> https://treeherder.mozilla.org/logviewer.
> html#?job_id=66754872&repo=autoland&lineNumber=8459
It looks like the eslint rules updated on the latest central. Just fix the eslint issue.
Flags: needinfo?(fliu)
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/278daccd7e54
Sort sites listed in Settings of Site Data, r=Gijs
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/278daccd7e54
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox53: --- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: