Closed Bug 1446362 Opened 6 years ago Closed 6 years ago

Remove "tree" from the permission preferences

Categories

(Firefox :: Settings UI, enhancement, P3)

Product:
Firefox
Component:
Settings UI
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 63
Tracking Flags:
Tracking Status
firefox61 --- wontfix
firefox63 --- fixed

People

(Reporter: Paolo, Assigned: Paolo)

References

(Blocks 2 open bugs)

Details

Attachments

(2 files, 4 obsolete files)

Bug 1446362 - Part 2 - Remove "tree" from the permission preferences.
59 bytes, text/x-review-board-request
johannh
: review+
Details
Bug 1446362 - Part 1 - Refactor tests to reduce some duplication.
59 bytes, text/x-review-board-request
johannh
: review+
Details 
<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 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)
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 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 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.
Flags: needinfo?(jaws)
Assignee: nobody → paolo.mozmail
Status: NEW → ASSIGNED
Flags: needinfo?(jaws)
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 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 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 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)
Depends on: 1467713
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 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+
Attachment #8977089 - Attachment is obsolete: true
Attachment #8983470 - Attachment is obsolete: true
Attachment #8983471 - Attachment is obsolete: true
Attachment #8983472 - Attachment is obsolete: true
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
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox63: --- → fixed
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.

Attachment

General

Creator:
Created:
Updated:
Size: