1496632 - Permissions UI for WebExtensions

Status: RESOLVED FIXED

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

Description

[Geoff Lankow (:darktrojan)]

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.

Comment 1

[Geoff Lankow (:darktrojan)]

Attachment: 1496632-part1-addon-install-notifications-1.diff

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

Comment 2

[Richard Marti (:Paenglab)]

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.

Comment 3

[Geoff Lankow (:darktrojan)]

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.

Comment 4

[Richard Marti (:Paenglab)]

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

[Jorg K (CEST = GMT+2)]

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.

Comment 6

[Geoff Lankow (:darktrojan)]

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

Comment 7

[Geoff Lankow (:darktrojan)]

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

Comment 8

[Geoff Lankow (:darktrojan)]

Attachment: 1496632-part1-addon-install-notifications-2.diff

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.

Comment 9

[Geoff Lankow (:darktrojan)]

Attachment: 1496632-part2-webextension-notifications-1.diff

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.

Comment 10

[Geoff Lankow (:darktrojan)]

Attachment: 1496632-part3-sideloaded-1.diff

In this part I replace the detection of sideloaded extensions and the UI that goes with it.

Comment 11

[Geoff Lankow (:darktrojan)]

Attachment: 1496632-part4a-global-notifications-1.diff

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.

Comment 12

[Geoff Lankow (:darktrojan)]

Attachment: 1496632-part4b-diff-1.diff

This is a diff between Firefox's PopupNotifications.jsm and my new GlobalPopupNotifications.jsm.

Comment 13

[Geoff Lankow (:darktrojan)]

Attachment: 1496632-part5-about-newaddon-1.diff

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

Comment 14

[Geoff Lankow (:darktrojan)]

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

Comment 15

[Geoff Lankow (:darktrojan)]

Attachment: 1496632-part6-test-1.diff

Comment 16

[Geoff Lankow (:darktrojan)]

Attachment: 1496632-part1-addon-install-notifications-3.diff

Comment 17

[Geoff Lankow (:darktrojan)]

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.

Comment 18

[Richard Marti (:Paenglab)]

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.

Comment 19

[Geoff Lankow (:darktrojan)]

Attachment: 1496632-part4a-global-notifications-2.diff

Comment 20

[Geoff Lankow (:darktrojan)]

Attachment: 1496632-part4b-diff-2.diff

Comment 21

[Jorg K (CEST = GMT+2)]

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.

Comment 22

[Magnus Melin [:mkmelin]]

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

Comment 23

[Jorg K (CEST = GMT+2)]

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

[Magnus Melin [:mkmelin]]

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

Comment 25

[Magnus Melin [:mkmelin]]

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 26

[Magnus Melin [:mkmelin]]

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?

Comment 27

[Jorg K (CEST = GMT+2)]

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

Comment 28

[Geoff Lankow (:darktrojan)]

Attachment: 1496632-part1-addon-install-notifications-4.diff

Comment 29

[Geoff Lankow (:darktrojan)]

Attachment: 1496632-part2-webextension-notifications-2.diff

Comment 30

[Geoff Lankow (:darktrojan)]

Attachment: 1496632-part3-sideloaded-2.diff

Comment 31

[Geoff Lankow (:darktrojan)]

Attachment: 1496632-part4a-global-notifications-3.diff

Comment 32

[Geoff Lankow (:darktrojan)]

Attachment: 1496632-part6-test-2.diff

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

Comment 33

[Geoff Lankow (:darktrojan)]

Attachment: 1496632-part7-legacy-warning-1.diff

Comment 34

[Jorg K (CEST = GMT+2)]

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!

Comment 35

[Pulsebot]

Pushed by [e-mail]:
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