<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.

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.

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.

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!

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"

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]

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.

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

Comment on attachment 8983470 [details] Bug 1446362 - Part 2 - Reorder "permissions.js" to match "sitePermissions.js". https://reviewboard.mozilla.org/r/249330/#review256212

Comment on attachment 8983471 [details] Bug 1446362 - Part 3 - Remove the unused setOrigin function. https://reviewboard.mozilla.org/r/249332/#review256214

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

Comment on attachment 8983687 [details] Bug 1446362 - Part 1 - Refactor tests to reduce some duplication. https://reviewboard.mozilla.org/r/249508/#review257220

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.

Updated to fix a leftover observer registration and other tests. New tryserver build: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c65c355e70357475a7e78abd3e671022ca08c436

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

https://hg.mozilla.org/mozilla-central/rev/a190aea35898 https://hg.mozilla.org/mozilla-central/rev/770ab6059408

Just saw this landing on the central repo. What are the disadvantages of Tree?

(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.