Permissions UI for WebExtensions

RESOLVED FIXED in Thunderbird 64.0

Status

enhancement
RESOLVED FIXED
9 months ago
4 months ago

People

(Reporter: darktrojan, Assigned: darktrojan)

Tracking

unspecified
Thunderbird 64.0
Dependency tree / graph

Thunderbird Tracking Flags

(thunderbird64+ fixed)

Details

Attachments

(7 attachments, 10 obsolete attachments)

18.76 KB, patch
Details | Diff | Splinter Review
48.06 KB, patch
darktrojan
: review+
Details | Diff | Splinter Review
21.81 KB, patch
darktrojan
: review+
Details | Diff | Splinter Review
5.69 KB, patch
darktrojan
: review+
Details | Diff | Splinter Review
58.93 KB, patch
darktrojan
: review+
Details | Diff | Splinter Review
8.42 KB, patch
darktrojan
: review+
Details | Diff | Splinter Review
3.45 KB, patch
jorgk
: review+
Details | Diff | Splinter Review
Assignee

Description

9 months ago
Since the old add-on installation UI has been removed, there's no better time to replace it with something that can also handle WebExtensions permissions. Without it, extensions are granted any permission asked for with no user interaction.

We'll need to set the pref extensions.webextPermissionPrompts to true, listen for some observer notifications, and display some UI. Basically just copy browser/modules/ExtensionsUI.jsm but with UI of our choice. I'm not a big fan of doorhanger/arrowpanel notifications in Thunderbird and the Firefox UI doesn't work out of the box, so we could do something a little different here.
Assignee

Comment 1

8 months ago
Part 1.

Don't look too closely at the code, I'm more interested in what you think of the UI it creates.

This part doesn't include any of the WebExt stuff, lists of permissions etc., I have another patch for that. I should also be using this new UI to replace the sideloaded add-on UI and remove about:newaddon (again).
Assignee: nobody → geoff
Status: NEW → ASSIGNED
Attachment #9015170 - Flags: feedback?(richard.marti)
Attachment #9015170 - Flags: feedback?(philipp)
Attachment #9015170 - Flags: feedback?(mkmelin+mozilla)
Attachment #9015170 - Flags: feedback?(jorgk)
Comment on attachment 9015170 [details] [diff] [review]
1496632-part1-addon-install-notifications-1.diff

This looks good, thank you.

Only one thing, would it be possible that the panel has its anchor in the Add-on tab instead of a separate icon on the end of the tab bar? It could be either the center of the tab or the tab icon itself.
Attachment #9015170 - Flags: feedback?(richard.marti) → feedback+
Assignee

Comment 3

8 months ago
Possible? Maybe, but with some problems. Firstly, what if the add-ons tab isn't open? Secondly, this is the Firefox notification code, and shows and hides things as it sees fit, and also it's very current-tab-centric. If the user switches tab (which might happen if the add-ons tab was open but not current) the notification vanishes. I'm cheating somewhat just by having it somewhere outside the tabs.
Okay, but the Add-on tab should be always open as it's only possible to install extensions inside this tab. The main tab and all other tabs don't let install extensions, or not?

Comment 5

8 months ago
Comment on attachment 9015170 [details] [diff] [review]
1496632-part1-addon-install-notifications-1.diff

Since this notification is displayed when installing manually, I suggest to use the strings from bug 1494215:
Allow the installation of the following add-on in #1:;
Allow the installation of the following #2 add-ons in #1:

BTW, that removes the notification and some of the code just added in bug 1494215, right? A bonus is that you can now drag two add-ons and you'll get two notifications.
Attachment #9015170 - Flags: feedback?(jorgk) → feedback+
Assignee

Comment 6

8 months ago
(In reply to Richard Marti (:Paenglab) from comment #4)
> Okay, but the Add-on tab should be always open as it's only possible to
> install extensions inside this tab. The main tab and all other tabs don't
> let install extensions, or not?

It's possible to install extensions from other tabs.
Assignee

Comment 7

8 months ago
(In reply to Jorg K (GMT+2) from comment #5)
> Comment on attachment 9015170 [details] [diff] [review]
> 1496632-addon-install-notifications-1.diff
> 
> Since this notification is displayed when installing manually, I suggest to
> use the strings from bug 1494215:
> Allow the installation of the following add-on in #1:;
> Allow the installation of the following #2 add-ons in #1:
> 
> BTW, that removes the notification and some of the code just added in bug
> 1494215, right? A bonus is that you can now drag two add-ons and you'll get
> two notifications.

These are the same strings Firefox uses. Actually, with the WebExt changes added the prompt just says "Add Foo?" and a list of permissions.

But that reminds me, I can now remove the old strings from messenger.properties.
Assignee

Updated

8 months ago
Attachment #9015170 - Attachment description: 1496632-addon-install-notifications-1.diff → 1496632-part1-addon-install-notifications-1.diff
Assignee

Comment 8

8 months ago
Okay, here goes. A trilogy in five parts (with more to come).

It's intended that these patches would be applied together, but I have split them up for ease of understanding. It should be possible to apply them one-at-a-time to test changes, except for this first part, you'll need to import resource:///modules/ExtensionsUI.jsm yourself before it will work.

In this part, I introduce pop-up notifications, and ExtensionsUI.jsm, which uses them to confirm or deny add-on installation. This replaces the previous UI, which has been removed.
Attachment #9015170 - Attachment is obsolete: true
Attachment #9015170 - Flags: feedback?(philipp)
Attachment #9015170 - Flags: feedback?(mkmelin+mozilla)
Assignee

Comment 9

8 months ago
In this part I add the new permissions notifications that come with WebExtensions. This is mostly copied from browser/modules/ExtensionsUI.jsm, but a diff of the two files is pretty useless.
Assignee

Comment 10

8 months ago
Posted patch 1496632-part3-sideloaded-1.diff (obsolete) β€” β€” Splinter Review
In this part I replace the detection of sideloaded extensions and the UI that goes with it.
Assignee

Comment 11

8 months ago
Here we take Firefox's pop-up notification UI and bend it to our will. We want our notifications to stick around even if the user switches tabs. I've kept most of the function signatures the same, even if they work differently, to hopefully avoid confusion later on.
Assignee

Comment 12

8 months ago
Posted patch 1496632-part4b-diff-1.diff (obsolete) β€” β€” Splinter Review
This is a diff between Firefox's PopupNotifications.jsm and my new GlobalPopupNotifications.jsm.
Assignee

Comment 13

8 months ago
This patch removes about:newaddon, which isn't used after part 3.
Assignee

Comment 14

8 months ago
Still to come: tests to make sure all this doesn't break, and some UI tweaks.
Assignee

Comment 15

8 months ago
Posted patch 1496632-part6-test-1.diff (obsolete) β€” β€” Splinter Review
Assignee

Updated

8 months ago
Attachment #9015832 - Attachment is obsolete: true
Assignee

Comment 17

8 months ago
Comment on attachment 9016168 [details] [diff] [review]
1496632-part1-addon-install-notifications-3.diff

I'm not sure who'd be best to review this, but I think it's one of you two. I'll let you decide which. :)

Please review all the parts (except 4b which isn't actually a patch). I'm not going to set r? on them all as you don't need the bugspam. And please read the bug from comment 8 onwards before you begin.
Attachment #9016168 - Flags: review?(richard.marti)
Attachment #9016168 - Flags: review?(jorgk)
Comment on attachment 9016168 [details] [diff] [review]
1496632-part1-addon-install-notifications-3.diff

It's too much JS for my experience for a knowledge. But some feedback:
- Part 1 and 4 don't apply on mail/base/modules/moz.build. No big problem.
- When trying to add unsupported extensions the popup shows twize that the extension is corrupt.
- When adding two extensions together, only one is added.
Attachment #9016168 - Flags: review?(richard.marti)
Assignee

Comment 19

8 months ago
Attachment #9015836 - Attachment is obsolete: true
Assignee

Comment 20

8 months ago
Posted patch 1496632-part4b-diff-2.diff (obsolete) β€” β€” Splinter Review
Attachment #9015838 - Attachment is obsolete: true

Comment 21

8 months ago
Sorry about the delay here. Magnus, would you like to take the review, most likely more your kind of stuff. Looks like we want that landed on TB 64, so there are only a few days left.
Flags: needinfo?(mkmelin+mozilla)
Attachment #9015834 - Flags: review+
Attachment #9015835 - Flags: review+
Comment on attachment 9016168 [details] [diff] [review]
1496632-part1-addon-install-notifications-3.diff

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

::: mail/base/modules/ExtensionsUI.jsm
@@ +75,5 @@
> +
> +    let showNextConfirmation = () => {
> +      // Make sure the browser is still alive.
> +      // if (!gBrowser.browsers.includes(browser))
> +      //   return;

remove this?

@@ +83,5 @@
> +        this.showInstallConfirmation(browser, pending.shift());
> +    };
> +
> +    // If all installs have already been cancelled in some way then just show
> +    // the next confirmation

nit: period.

@@ +267,5 @@
> +        let needsDownload = function needsDownload(install) {
> +          return install.state != AddonManager.STATE_DOWNLOADED;
> +        };
> +        // If all installs have already been downloaded then there is no need to
> +        // show the download progress

.

@@ +269,5 @@
> +        };
> +        // If all installs have already been downloaded then there is no need to
> +        // show the download progress
> +        if (!installInfo.installs.some(needsDownload))
> +          return;

the are a few if's without braces. maybe make them all have braces for consistency.

@@ +356,5 @@
> +        break;
> +      }
> +      case "addon-install-confirmation": {
> +        let showNotification = () => {
> +          let height = undefined;

i don't think you need to initialize it.

@@ +372,5 @@
> +        let progressNotification = getNotification("addon-progress", browser);
> +        if (progressNotification) {
> +          let downloadDuration = Date.now() - progressNotification._startTime;
> +          let securityDelay = Services.prefs.getIntPref("security.dialog_enable_delay");
> +          if (securityDelay - downloadDuration > 0) {

I'd make it 

if ((securityDelay - downloadDuration) > 0) {

for clarity
Attachment #9016168 - Flags: review?(jorgk) → review+

Comment 23

8 months ago
(In reply to Magnus Melin [:mkmelin] from comment #22)
> > +          if (securityDelay - downloadDuration > 0) {
> I'd make it 
> if ((securityDelay - downloadDuration) > 0) {
> for clarity

securityDelay  > downloadDuration ?
Comment on attachment 9017367 [details] [diff] [review]
1496632-part4a-global-notifications-2.diff

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

::: mail/base/modules/GlobalPopupNotifications.jsm
@@ +42,5 @@
> +  // binding's content.
> +  let notificationEl;
> +  let parent = aElement;
> +  while (parent && (parent = aElement.ownerDocument.getBindingParent(parent)))
> +    notificationEl = parent;

please add { } for all loops

@@ +72,5 @@
> +}
> +
> +Notification.prototype = {
> +
> +  id: null,

remove the empty line above id

@@ +172,5 @@
> +    throw "Invalid tabbrowser";
> +  if (iconBox && ChromeUtils.getClassName(iconBox) != "XULElement")
> +    throw "Invalid iconBox";
> +  if (ChromeUtils.getClassName(panel) != "XULPopupElement")
> +    throw "Invalid panel";

make all these throw new Error(".....") instead of throwing raw strings

@@ +236,5 @@
> +    this.tabbrowser.tabContainer.addEventListener("TabSelect", this, true);
> +}
> +
> +PopupNotifications.prototype = {
> +

empty line

@@ +428,5 @@
> +      throw "PopupNotifications_show: invalid ID";
> +    if (mainAction && isInvalidAction(mainAction))
> +      throw "PopupNotifications_show: invalid mainAction";
> +    if (secondaryActions && secondaryActions.some(isInvalidAction))
> +      throw "PopupNotifications_show: invalid secondaryActions";

here too throw Errors not strings

@@ +663,5 @@
> +      let popupnotification = doc.getElementById(popupnotificationID);
> +      if (popupnotification)
> +        gNotificationParents.set(popupnotification, popupnotification.parentNode);
> +      else
> +        popupnotification = doc.createXULElement("popupnotification");

add braces for consistency

@@ +1120,5 @@
> +          event.keyCode == event.DOM_VK_RETURN))
> +      return;
> +
> +    if (this._currentNotifications.length == 0)
> +      return;

these too could use some braces

@@ +1396,5 @@
> +    this._remove(notification);
> +    this._update();
> +  },
> +
> +  _onMenuCommand: function PopupNotifications_onMenuCommand(event) {

remove the naming, and make it

 _onMenuCommand() {

@@ +1424,5 @@
> +    this._remove(target.notification);
> +    this._update();
> +  },
> +
> +  _notify: function PopupNotifications_notify(topic) {

here too
Attachment #9017367 - Flags: review+
Comment on attachment 9017368 [details] [diff] [review]
1496632-part4b-diff-2.diff

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

What's this modified toolkit file doing here?
Comment on attachment 9016167 [details] [diff] [review]
1496632-part6-test-1.diff

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

::: mail/test/mozmill/content-tabs/test-install-xpi.js
@@ +73,3 @@
>  
> +  mc.waitForElement(notification);
> +  mc.sleep(500);

is the sleep needed or can we wait for some element to be ready?
Attachment #9016167 - Flags: review+

Comment 27

8 months ago
(In reply to Magnus Melin [:mkmelin] from comment #25)
> What's this modified toolkit file doing here?
See comment #12: This is a diff between Firefox's PopupNotifications.jsm and my new GlobalPopupNotifications.jsm.
Flags: needinfo?(mkmelin+mozilla)
Assignee

Comment 28

8 months ago
Attachment #9016168 - Attachment is obsolete: true
Attachment #9018536 - Flags: review+
Assignee

Comment 29

8 months ago
Attachment #9015834 - Attachment is obsolete: true
Attachment #9018537 - Flags: review+
Assignee

Comment 30

8 months ago
Attachment #9015835 - Attachment is obsolete: true
Attachment #9018538 - Flags: review+
Assignee

Comment 31

8 months ago
Attachment #9017367 - Attachment is obsolete: true
Attachment #9017368 - Attachment is obsolete: true
Attachment #9018540 - Flags: review+
Assignee

Comment 32

8 months ago
(In reply to Magnus Melin [:mkmelin] from comment #26)
> Comment on attachment 9016167 [details] [diff] [review]
> 1496632-part6-test-1.diff
> 
> Review of attachment 9016167 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mail/test/mozmill/content-tabs/test-install-xpi.js
> @@ +73,3 @@
> >  
> > +  mc.waitForElement(notification);
> > +  mc.sleep(500);
> 
> is the sleep needed or can we wait for some element to be ready?

The sleep is needed. The UI needs some time to sort itself out.
Attachment #9016167 - Attachment is obsolete: true
Assignee

Comment 33

8 months ago
Attachment #9018542 - Flags: review?(jorgk)
Assignee

Updated

8 months ago
Attachment #9018541 - Flags: review+

Comment 34

8 months ago
Comment on attachment 9018542 [details] [diff] [review]
1496632-part7-legacy-warning-1.diff

Thanks. The "Learn More..." (with proper ellipsis) is still there somewhere? From the context I can't see how the two different messages are use:
The add-on could not be installed because it is not compatible with %1$S %2$S.
This add-on could not be installed because it is not compatible with %1$S %2$S.

I don't think it's necessary to distinguish between "The" and "This" for website and local install, "The" would work in both cases. Your call.

Ship it!
Attachment #9018542 - Flags: review?(jorgk) → review+

Comment 35

8 months ago
Pushed by geoff@darktrojan.net:
https://hg.mozilla.org/comm-central/rev/ba7329940979
Move existing add-on installation UI to notification panels; r=mkmelin
https://hg.mozilla.org/comm-central/rev/e16ec20c7395
Add WebExtension permission prompts to notification UI; r=mkmelin
https://hg.mozilla.org/comm-central/rev/7d6de3006fc3
Add prompt for sideloaded extensions to notification UI; r=mkmelin
https://hg.mozilla.org/comm-central/rev/1c197e89daef
Fork PopupNotifications.jsm and remove dependencies on the tabbed browser; r=mkmelin
https://hg.mozilla.org/comm-central/rev/92444675efe9
Remove no-longer-used about:newaddon, backout of 7a59c0ecec8e (bug 1473178); r=backout
https://hg.mozilla.org/comm-central/rev/0399b6a574c6
Update XPI installation test for new UI; r=mkmelin
https://hg.mozilla.org/comm-central/rev/52e953facdcf
Copy warning about legacy extension incompatibility; r=jorgk DONTBUILD
Status: ASSIGNED → RESOLVED
Closed: 8 months ago
Resolution: --- → FIXED
Assignee

Updated

8 months ago
Target Milestone: --- → Thunderbird 64.0
You need to log in before you can comment on or make changes to this bug.