Closed Bug 1385221 Opened 7 years ago Closed 7 years ago

Implement permission sorting in sitePermissions.xul dialog.

Categories

(Firefox :: Settings UI, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 57
Tracking Status
firefox57 --- fixed

People

(Reporter: prathiksha, Assigned: prathiksha, Mentored)

References

Details

Attachments

(1 file)

Assignee: nobody → prathikshaprasadsuman
Status: NEW → ASSIGNED
Attachment #8896788 - Flags: review?(jhofmann)
Comment on attachment 8896788 [details]
Bug 1385221 - Implement Permission sorting in the Permissions dialog.

https://reviewboard.mozilla.org/r/168074/#review173978

You can't sort a map. You should just amend your buildPermissionsList function to use a sorted list of permissions instead of this._permissions directly.

You will also need to maintain the sort direction and the currently sorted column for displaying it in the UI (the downward arrow).

We probably also want to sort not only by capability, but also by origin.

You can get a good idea of how some of this is implemented by looking at the sorting code in site data: https://searchfox.org/mozilla-central/rev/e5b13e6224dbe3182050cf442608c4cb6a8c5c55/browser/components/preferences/siteDataSettings.js#84

Let me know if you need help and feel free to post WIP patches for feedback :)
Attachment #8896788 - Flags: review?(jhofmann)
Comment on attachment 8896788 [details]
Bug 1385221 - Implement Permission sorting in the Permissions dialog.

https://reviewboard.mozilla.org/r/168074/#review174844

Nice, this is much closer to the goal. We can still optimize it a bit.

::: browser/components/preferences/sitePermissions.js:269
(Diff revision 2)
>      for (let item of oldItems) {
>        item.remove();
>      }
>  
> +    // Sort permissions.
> +    let sites = this._constructSortArray();

You don't need a _constructSortArray function, you can just call Array.from(this._permissions.values()).

I'd like to suggest using .values() instead of entries() to avoid the whole complexity that comes with having an array [origin, {origin, capabilityString}] instead of simply {origin, capabilityString}.

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Map/values

::: browser/components/preferences/sitePermissions.js:270
(Diff revision 2)
>        item.remove();
>      }
>  
> +    // Sort permissions.
> +    let sites = this._constructSortArray();
> +    let sortedSites = this._sortSites(sites, sortCol);

Nit: this should probably be called sortedPermissions and the function should probably be called _sortPermissions.

::: browser/components/preferences/sitePermissions.js:271
(Diff revision 2)
>      }
>  
> +    // Sort permissions.
> +    let sites = this._constructSortArray();
> +    let sortedSites = this._sortSites(sites, sortCol);
> +    this._permissions = new Map(sortedSites);

There's no reason to override this._permissions. You can just iterate over sortedSites (which should probably be called sortedPermissions) in line 275.

::: browser/components/preferences/sitePermissions.js:297
(Diff revision 2)
> +    }
> +
> +    let sortFunc = null;
> +    switch (column.id) {
> +      case "siteCol":
> +        sortFunc = (a, b) => {

Since both values are strings you can just use .localeCompare instead of building your own comparison manually, I think.

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String/localeCompare

(See the sort example on that page).

::: browser/components/preferences/sitePermissions.js:355
(Diff revision 2)
> +  onClickTreeCol(event) {
> +    this._buildPermissionsList(event.target);
> +  },
> +
> +  onCommandSearch() {
> +    let sortColumn = document.querySelector("treecol[data-isCurrentSortCol=true]");

You're getting this sortColumn so many times that I'd like to suggest using a default argument to _buildPermissionsList instead.

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Functions/Default_parameters

::: browser/components/preferences/sitePermissions.xul:45
(Diff revision 2)
>                   hidecolumnpicker="true" flex="1"
>                   onkeypress="gSitePermissionsManager.onPermissionKeyPress(event);"
>                   onselect="gSitePermissionsManager.onPermissionSelect();">
>        <listheader>
>          <treecol id="siteCol" label="&treehead.sitename2.label;" flex="3"
> -                 data-field-name="origin" persist="width" width="50"/>
> +                 data-field-name="origin" persist="width" width="50"

Since you're not going to use data-field-name for sorting (which is ok for me), you can probably remove it.
Attachment #8896788 - Flags: review?(jhofmann) → review-
Comment on attachment 8896788 [details]
Bug 1385221 - Implement Permission sorting in the Permissions dialog.

https://reviewboard.mozilla.org/r/168074/#review174958

This needs a (couple of) test(s). :)

::: browser/components/preferences/sitePermissions.js:267
(Diff revision 3)
>      for (let item of oldItems) {
>        item.remove();
>      }
>  
> +    // Sort permissions.
> +    let sites = Array.from(this._permissions.values());

Nit: you could move this line into this._sortPermissions since it's not really useful to pass this as a parameter (it should also not be called sites)

::: browser/components/preferences/sitePermissions.js:295
(Diff revision 3)
> +    let sortFunc = null;
> +    switch (column.id) {
> +      case "siteCol":
> +        sortFunc = (a, b) => {
> +          let aOrigin = a.origin.toLowerCase();
> +          let bOrigin = b.origin.toLowerCase();

You don't have to lower case the origins.

::: browser/components/preferences/sitePermissions.js:301
(Diff revision 3)
> +          return aOrigin.localeCompare(bOrigin);
> +        };
> +        break;
> +
> +      case "statusCol":
> +        sortFunc = (a, b) => {

Sorry if that wasn't clear, you can use localeCompare for statusCol as well I think.

::: browser/components/preferences/sitePermissions.js:331
(Diff revision 3)
> +    column.setAttribute("data-last-sortDirection", sortDirection);
> +
> +    return sites;
> +  },
> +
> +  onClickTreeCol(event) {

I think you can get rid of this function, it only calls another function now.

::: browser/components/preferences/sitePermissions.js:335
(Diff revision 3)
> +
> +  onClickTreeCol(event) {
> +    this._buildPermissionsList(event.target);
> +  },
> +
> +  onCommandSearch() {

See above, I think we can remove this.
Attachment #8896788 - Flags: review?(jhofmann) → review-
Comment on attachment 8896788 [details]
Bug 1385221 - Implement Permission sorting in the Permissions dialog.

https://reviewboard.mozilla.org/r/168074/#review181258

Sorry for the long wait :(

This looks good to me, thank you!

There's one more thought I had when trying the patch. When sorting by origin, we could consider ignoring the URI scheme, to e.g. get http://google.com and https://google.com next to each other. I don't want that discussion to block this bug, so I'd suggest just deferring it for later (maybe wait if people are making bugs about it).

::: browser/components/preferences/sitePermissions.js:282
(Diff revision 8)
> +
>      this._setRemoveButtonState();
>    },
> +
> +  _sortPermissions(column) {
> +    let permissionsArray = Array.from(this._permissions.values());

Nit: it should be clear that this is an array, you can just call it permissions.

::: browser/components/preferences/sitePermissions.xul:49
(Diff revision 8)
>          <treecol id="siteCol" label="&treehead.sitename2.label;" flex="3"
> -                 data-field-name="origin" persist="width" width="50"/>
> +                 persist="width" width="50"
> +                 onclick="gSitePermissionsManager.buildPermissionsList(event.target)"/>
>          <splitter class="tree-splitter"/>
>          <treecol id="statusCol" label="&treehead.status.label;" flex="1"
> -                 data-field-name="capability" persist="width" width="50"/>
> +                 persist="width" width="50" data-isCurrentSortCol="true"

IMO we should sort by origin initially, instead of status. What do you think?
Attachment #8896788 - Flags: review?(jhofmann) → review+
Comment on attachment 8896788 [details]
Bug 1385221 - Implement Permission sorting in the Permissions dialog.

https://reviewboard.mozilla.org/r/168074/#review181288

::: browser/components/preferences/sitePermissions.js:319
(Diff revision 8)
> +    let cols = this._list.querySelectorAll("treecol");
> +    cols.forEach(c => {
> +      c.removeAttribute("data-isCurrentSortCol");
> +      c.removeAttribute("sortDirection");
> +    });
> +    column.setAttribute("data-isCurrentSortCol", true);

Nit: you can make "true" a string since it's going to be converted to a string anyway.
Comment on attachment 8896788 [details]
Bug 1385221 - Implement Permission sorting in the Permissions dialog.

https://reviewboard.mozilla.org/r/168074/#review181258

> IMO we should sort by origin initially, instead of status. What do you think?

I agree with you but the specs spefically asks us to sort by status initially (https://mozilla.invisionapp.com/share/4ZB63CHD9#/screens/237173273). So based on what we discussed on IRC, we will be going with the spec.
Do you want me to land it? :)
Flags: needinfo?(prathikshaprasadsuman)
(In reply to Johann Hofmann [:johannh] from comment #17)
> Do you want me to land it? :)

If you think the new revision is good, yes. :) All tests are passing.
Flags: needinfo?(prathikshaprasadsuman)
Pushed by jhofmann@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d032ead9b6db
Implement Permission sorting in the Permissions dialog. r=johannh
https://hg.mozilla.org/mozilla-central/rev/d032ead9b6db
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
You need to log in before you can comment on or make changes to this bug.