Remove "tree" from the permission preferences

RESOLVED FIXED in Firefox 63

Status

()

enhancement
P3
normal
RESOLVED FIXED
a year ago
10 months ago

People

(Reporter: Paolo, Assigned: Paolo)

Tracking

(Blocks 2 bugs)

Trunk
Firefox 63
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox61 wontfix, firefox63 fixed)

Details

Attachments

(2 attachments, 4 obsolete attachments)

(Assignee)

Description

a year ago
<tree id="permissionsTree"> can be replaced with a listbox.
This tree uses `getCellText` which makes it hard to refactor to use Fluent. If we could migrate it away from <tree> then migrationt o Fluent should be easy.

For reference, the sitePermissions patch in bug 1457021 uses the same logic, but doesn't use <tree> so I was able to nicely refactor it together with post-l10n sorting of the list. Since both UIs are very similar, it would be great to unify them further.
Blocks: 1457021
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 4

11 months ago
Comment on attachment 8977089 [details]
Bug 1446362 - Part 1 - Rename functions in "permissions.js" to match "sitePermissions.js".

This aligns the code of the permissions dialog with the one added in bug 1373206.
Attachment #8977089 - Flags: feedback?(jhofmann)
(Assignee)

Comment 5

11 months ago
Comment on attachment 8977090 [details]
Bug 1446362 - Part 2 - Remove "tree" from the permission preferences.

We may be able to merge the two dialogs in the future. For the moment, I'm planning to keep them separate, because the other dialog is already converted to Fluent while this isn't yet. Let me know if you'd rather unify them sooner.
Attachment #8977090 - Flags: feedback?(jhofmann)
Comment on attachment 8977089 [details]
Bug 1446362 - Part 1 - Rename functions in "permissions.js" to match "sitePermissions.js".

Sorry for taking so long to get to this.

This is definitely feedback+

I'd also give this r+ since looking at the patch I'm assuming you're just reordering and renaming functions in the file, though this patch is really exploring the limits of reviewboard move highlighting, so feel free to point out  any other notable changes whenever you feel like it's ready for review.

Thanks!
Attachment #8977089 - Flags: feedback?(jhofmann) → feedback+

Comment 7

11 months ago
mozreview-review
Comment on attachment 8977090 [details]
Bug 1446362 - Part 2 - Remove "tree" from the permission preferences.

https://reviewboard.mozilla.org/r/245164/#review253336

This looks good to me.

I agree that it would be nice to eventually merge the two permission dialogs, but there's unfortunately quite a bit of UX differences (which is the reason why we decided to create a second one back in the day). It might make sense to create options for these behaviors in a unified dialog (e.g. showSearch, allowAdditions, allowPermissionChange).

::: browser/components/preferences/permissions.js:66
(Diff revision 1)
>  
> -    const l10n = permissionExceptionsL10n[this._type];
>      let permissionsText = document.getElementById("permissionsText");
> -    document.l10n.setAttributes(permissionsText, l10n.description);
>  
> +    const l10n = permissionExceptionsL10n[this._type];

nit: let l10n

::: browser/components/preferences/permissions.js:97
(Diff revision 1)
>  
> -    Services.obs.notifyObservers(null, NOTIFICATION_FLUSH_PERMISSIONS, this._type);
> +    Services.obs.notifyObservers(null, "flush-pending-permissions", this._type);
>      Services.obs.addObserver(this, "perm-changed");
>  
>      this._loadPermissions();
> +    await this.buildPermissionsList();

Making this function async seems like it was done on accident when sitePermissions.js was converted to fluent, at least I can't see any "await" in buildPermissionsList...

::: browser/components/preferences/permissions.js:366
(Diff revision 1)
>      }
>  
>      window.close();
>    },
>  
> -  _lastPermissionSortColumn: "",
> +  async buildPermissionsList(sortCol) {

Can probably remove the "async"
Attachment #8977090 - Flags: feedback?(jhofmann) → feedback+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 13

11 months ago
mozreview-review
Comment on attachment 8983472 [details]
Bug 1446362 - Part 4 - Replace "var" with "let" and remove the parameter prefix.

https://reviewboard.mozilla.org/r/249334/#review255504


Code analysis found 1 defect in this patch:
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: browser/components/preferences/permissions.js:458
(Diff revision 1)
>                                                          this._lastPermissionSortAscending);
> -    this._lastPermissionSortColumn = aColumn;
> +    this._lastPermissionSortColumn = column;
>      let sortDirection = this._lastPermissionSortAscending ? "descending" : "ascending";
>      let cols = document.querySelectorAll("treecol");
>      cols.forEach(c => c.removeAttribute("sortDirection"));
> -    let column = document.querySelector(`treecol[data-field-name=${aColumn}]`);
> +    let column = document.querySelector(`treecol[data-field-name=${column}]`);

Error: Parsing error: identifier 'column' has already been declared [eslint]
(Assignee)

Comment 14

11 months ago
Jared, I just noticed that Johann is away. Could you review parts 1-4 here, on which Johann has already given feedback? They are just code cleanup to align "permissions.js" to "sitePermissions.js" so that the files match in part 5. I'd like to land them sooner so they have less chance to bitrot, in fact after review I'll probably move them to a separate bug.
Flags: needinfo?(jaws)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee: nobody → paolo.mozmail
Status: NEW → ASSIGNED
Flags: needinfo?(jaws)

Comment 20

11 months ago
mozreview-review
Comment on attachment 8977089 [details]
Bug 1446362 - Part 1 - Rename functions in "permissions.js" to match "sitePermissions.js".

https://reviewboard.mozilla.org/r/245162/#review256210
Attachment #8977089 - Flags: review+

Comment 21

11 months ago
mozreview-review
Comment on attachment 8983470 [details]
Bug 1446362 - Part 2 - Reorder "permissions.js" to match "sitePermissions.js".

https://reviewboard.mozilla.org/r/249330/#review256212
Attachment #8983470 - Flags: review+
Attachment #8977089 - Flags: review?(jhofmann)
Attachment #8983470 - Flags: review?(jhofmann)

Comment 22

11 months ago
mozreview-review
Comment on attachment 8983471 [details]
Bug 1446362 - Part 3 - Remove the unused setOrigin function.

https://reviewboard.mozilla.org/r/249332/#review256214
Attachment #8983471 - Flags: review+

Comment 23

11 months ago
mozreview-review
Comment on attachment 8983472 [details]
Bug 1446362 - Part 4 - Replace "var" with "let" and remove the parameter prefix.

https://reviewboard.mozilla.org/r/249334/#review256216
Attachment #8983472 - Flags: review+
Attachment #8983471 - Flags: review?(jhofmann)
Attachment #8983472 - Flags: review?(jhofmann)
(Assignee)

Updated

11 months ago
Depends on: 1467713

Comment 24

10 months ago
mozreview-review
Comment on attachment 8983687 [details]
Bug 1446362 - Part 1 - Refactor tests to reduce some duplication.

https://reviewboard.mozilla.org/r/249508/#review257220
Attachment #8983687 - Flags: review?(jhofmann) → review+

Comment 25

10 months ago
mozreview-review
Comment on attachment 8977090 [details]
Bug 1446362 - Part 2 - Remove "tree" from the permission preferences.

https://reviewboard.mozilla.org/r/245164/#review257646

Seems good, thanks!

::: browser/components/preferences/permissions.js:429
(Diff revision 4)
> +                 b.querySelector(".website-capability-value").getAttribute("value");
> +        };
> +        break;
> +    }
> +
> +    let comp = new Services.intl.Collator(undefined, {

It seems a bit unnecessary to hoist the comp variable here, but I get that this is the same in sitePermissions.jsm, so it's probably better to leave it.
Attachment #8977090 - Flags: review?(jhofmann) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

10 months ago
Attachment #8977089 - Attachment is obsolete: true
(Assignee)

Updated

10 months ago
Attachment #8983470 - Attachment is obsolete: true
(Assignee)

Updated

10 months ago
Attachment #8983471 - Attachment is obsolete: true
(Assignee)

Updated

10 months ago
Attachment #8983472 - Attachment is obsolete: true
(Assignee)

Comment 28

10 months ago
Updated to fix a leftover observer registration and other tests. New tryserver build:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c65c355e70357475a7e78abd3e671022ca08c436

Comment 29

10 months ago
Pushed by paolo.mozmail@amadzone.org:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a190aea35898
Part 1 - Refactor tests to reduce some duplication. r=johannh
https://hg.mozilla.org/integration/mozilla-inbound/rev/770ab6059408
Part 2 - Remove "tree" from the permission preferences. r=johannh

Comment 30

10 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/a190aea35898
https://hg.mozilla.org/mozilla-central/rev/770ab6059408
Status: ASSIGNED → RESOLVED
Last Resolved: 10 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 63
Just saw this landing on the central repo. What are the disadvantages of Tree?
Flags: needinfo?(paolo.mozmail)
(In reply to Jens Hausdorf from comment #31)
> Just saw this landing on the central repo. What are the disadvantages of
> Tree?

Short term: removing it unblocks changing the localization system in about:preferences to Fluent. Longer term: we are migrating away from XUL/XBL widgets in the chrome and to alternatives built with web standards. See Bug 1446341 for more details about tree specifically.
Flags: needinfo?(paolo.mozmail)
You need to log in before you can comment on or make changes to this bug.