1689060 - Show experiment usage in permission list in Addon-Manager.

Status: RESOLVED FIXED

(Thunderbird :: Add-Ons: Extensions API, enhancement)

Description

[John Bieling (:TbSync)]

Similar to bug 1621213 we should show the experiment usage also in the permission list. Any other real WebExtension permission (which was not prompted for in case of experiment) should not be displayed.

Comment 1

[John Bieling (:TbSync)]

Attachment: bug1689060_show_experiment_usage_in_permission_list.patch

I was not able to write a patch for M-C, because they do not have the Experiment locale and Extension.formatPermissionStrings does the %S -> appName replacement only for selected messages, not for our "experiment".

Comment 2

[John Bieling (:TbSync)]

Attachment: permissionlist.PNG

Example of how the permission list looks like with this patch applied for an add-on using Experiments. Without this patch it would list only standard WebExtension permissions (if any).

Comment 3

[Geoff Lankow (:darktrojan)]

Comment on attachment 9199469 [details] [diff] [review]
bug1689060_show_experiment_usage_in_permission_list.patch

My only concern is that the two blocks after this need to happen ASAP, but this code waits for a CE to exist. It's probably fine, but I'd be happier if this was moved down beyond the setting of window.browserBundle.

Comment 4

[John Bieling (:TbSync)]

Attachment: bug1689060_show_experiment_usage_in_permission_list_v2.patch

Moved the code to the bottom as suggested.

Comment 5

[John Bieling (:TbSync)]

Comment on attachment 9199798 [details] [diff] [review]
bug1689060_show_experiment_usage_in_permission_list_v2.patch

[Approval Request Comment]
This fixes the permission list for MailExtensions using Experiments.

Regression caused by (bug #):
User impact if declined:
Testing completed (on c-c, etc.):
Risk to taking this patch (and alternatives if risky):
I guess none.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/28471cef02ac
show MailExtension Experiment usage in permission list. r=darktrojan

Comment 7

[John Bieling (:TbSync)]

mac test fails, manifest is missing, follow up patch is in the making.

Comment 8

[John Bieling (:TbSync)]

Attachment: bug1689060_show_experiment_usage_in_permission_list_follow_up_mac_fix.patch

Comment 9

[Magnus Melin [:mkmelin]]

Comment on attachment 9200111 [details] [diff] [review]
bug1689060_show_experiment_usage_in_permission_list_follow_up_mac_fix.patch

Review of attachment 9200111 [details] [diff] [review]:
-----------------------------------------------------------------

::: mail/base/content/aboutAddonsExtra.js
@@ +84,5 @@
>  
>    // Override parts of the addon-permission-list customElement to be able
>    // to show the usage of Experiments in the permission list.
>    await customElements.whenDefined("addon-permissions-list");
> +  AddonPermissionsList.prototype.setAddon = async function(addon) {

It does look a bit risky to change this function to be async. The rest of the code (in m-c) isn't really prepared for that (doesn't await it)

Comment 10

[John Bieling (:TbSync)]

Than please back out the patch for now.

Comment 11

[Magnus Melin [:mkmelin]]

Backout: https://hg.mozilla.org/comm-central/rev/8a19f3ab9e40

Comment 12

[John Bieling (:TbSync)]

Attachment: bug1689060_show_experiment_usage_in_permission_list_v3.patch

I had a look at the relevant m-c code and the method setAddon() is indeed called without being awaited for. But setAddon() itself is calling this.render() which is an async funtion without awaiting it as well.

https://searchfox.org/mozilla-central/source/toolkit/mozapps/extensions/content/aboutaddons.js#2529-2532

IIRC, in this case, it does not matter, if setAddon() is sync or async.

Comment 13

[Magnus Melin [:mkmelin]]

Comment on attachment 9202058 [details] [diff] [review]
bug1689060_show_experiment_usage_in_permission_list_v3.patch

Review of attachment 9202058 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, r=mkmelin

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/b2b077dcd1e3
Show experiment usage in permission list. r=mkmelin

Comment 15

[John Bieling (:TbSync)]

Comment on attachment 9202058 [details] [diff] [review]
bug1689060_show_experiment_usage_in_permission_list_v3.patch

[Approval Request Comment]
Regression caused by (bug #):
User impact if declined:
Wrong permission listed in add-on manager

Testing completed (on c-c, etc.):
Risk to taking this patch (and alternatives if risky):
I do not see any

Comment 16

[Wayne Mery (:wsmwk)]

Comment on attachment 9202058 [details] [diff] [review]
bug1689060_show_experiment_usage_in_permission_list_v3.patch

[Triage Comment]
Approved for beta

Comment 17

[Rob Lemley [:rjl]]

Thunderbird 86.0b3:
https://hg.mozilla.org/releases/comm-beta/rev/bd5b133fac7b

Comment 18

[Rob Lemley [:rjl]]

Thunderbird 86.0b3:
https://hg.mozilla.org/releases/comm-beta/rev/840f898185ac

Comment 19

[John Bieling (:TbSync)]

Thank you for fixing this, Rob!

Comment 20

[Magnus Melin [:mkmelin]]

Plan to uplift to 78?

Comment 21

[John Bieling (:TbSync)]

Yes, my next uplift session including try runs to be able to request uplift is scheduled for next week.

Comment 22

[John Bieling (:TbSync)]

Attachment: bug1689060_show_experiment_usage_in_permission_list_ESR78.patch

Wrong patch.

Comment 23

[John Bieling (:TbSync)]

Attachment: bug1689060_show_experiment_usage_in_permission_list_ESR78_v2.patch

This patch is for ESR78. It had to be adapted a bit, as the template does not exist and the entire XUL section needs to be created by hand, but the used technique to add the experiment permission display did not change.

Comment 24

[John Bieling (:TbSync)]

Comment on attachment 9207028 [details] [diff] [review]
bug1689060_show_experiment_usage_in_permission_list_ESR78_v2.patch

[Approval Request Comment]
Regression caused by (bug #):
User impact if declined:
Cannot see correct permissions.

Testing completed (on c-c, etc.):

• tested locally successfully
• try run: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=0ff537038e67c2f9ef865909cbb018a17e70ed33

Risk to taking this patch (and alternatives if risky):
Low

Comment 26

[Wayne Mery (:wsmwk)]

Comment on attachment 9207028 [details] [diff] [review]
bug1689060_show_experiment_usage_in_permission_list_ESR78_v2.patch

[Triage Comment]
Approved for esr78

Comment 27

[Rob Lemley [:rjl]]

Thunderbird 78.9.0:
https://hg.mozilla.org/releases/comm-esr78/rev/55e3d9d0bc76