1494215 - Re-add xpinstallConfirm after its m-c removal in bug 1473933

Status: RESOLVED FIXED

(Thunderbird :: Add-Ons: General, enhancement)

Description

[Richard Marti (:Paenglab)]

Bug 1473933 removes the xpinstallConfirm.xul which we are using to ask the user if the installation of a extension is okay.

Maybe instead of re-adding the files and somehow patch AddonManager.jsm, we could look to use the popup FX uses. In FX this popup appears in the URL bar.

Not tested, but probably until this is fixed, extensions can't be installed because we can't accept the installation.

Comment 1

[Richard Marti (:Paenglab)]

Geoff, you already re-added the newAddon.xul, could you look into this? My JS experience is too low to fix this.

Comment 2

[Tim Nguyen :ntim]

You could also port whatever installation panel Firefox is using.

Comment 3

[Tim Nguyen :ntim]

See:
https://searchfox.org/mozilla-central/search?q=addon-install-confirmation&case=false&regexp=false&path=browser%2F

https://searchfox.org/mozilla-central/source/browser/base/content/browser-addons.js

Comment 4

[Richard Marti (:Paenglab)]

This is what I meant with the popup.

Comment 5

[Jorg K (CEST = GMT+2)]

Causes test failures:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&selectedJob=201674639&revision=aaa4fd29eb4d6298ffd1d8e7828085b2706cbbbd

TEST-UNEXPECTED-FAIL | /builds/worker/workspace/build/tests/mozmill/content-tabs/test-install-xpi.js | test-install-xpi.js::test_install_xpi_offer
TEST-UNEXPECTED-FAIL | /builds/worker/workspace/build/tests/mozmill/content-tabs/test-install-xpi.js | test-install-xpi.js::test_xpinstall_actually_install

Comment 6

[Jorg K (CEST = GMT+2)]

I've talked to Richard and Aceman on IRC. So instead of resurrecting the old xpinstallConfirm.xul/js/css/dtd/properties, we should port the stuff from comment #3. That will take a while, so let's disable the failing tests for now.

Comment 7

[Jorg K (CEST = GMT+2)]

Attachment: 1494215-disable-tests.patch

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/d9356caafc10
temporarily disable parts of MozMill test content-tabs/test-install-xpi.js. rs=bustage-fix

Comment 9

[Geoff Lankow (:darktrojan)]

I'll take this. I'm not sure that the Firefox UI is going to fit well in Thunderbird, but I'll set that up first and then decide.

Comment 10

[Geoff Lankow (:darktrojan)]

Attachment: 1494215-xpi-install-1.diff

A good permanent fix is going to take some time to create. In the meantime I've moved the crucial parts to a notification, so that users can install add-ons again. (Not being able to do that is bad, even on Daily.)

I'll deal with the strings once I have some feedback.

Comment 11

[Magnus Melin [:mkmelin]]

Comment on attachment 9014621 [details] [diff] [review]
1494215-xpi-install-1.diff

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

Where am I supposed so see this? If I try to go to add-ons manager, select e.g. DKIM verifier, click Add to Thunderbird, it still just tries to download the xpi.

Comment 12

[Jorg K (CEST = GMT+2)]

Drag an add-on onto the manager.

Comment 13

[Jorg K (CEST = GMT+2)]

Comment on attachment 9014621 [details] [diff] [review]
1494215-xpi-install-1.diff

Perfect, I love it. Get it ready and landed :-)

Comment 14

[Geoff Lankow (:darktrojan)]

Attachment: 1494215-xpi-install-2.diff

Comment 15

[Jorg K (CEST = GMT+2)]

Comment on attachment 9014976 [details] [diff] [review]
1494215-xpi-install-2.diff

Very nice. This the notification also shows up when you install manually via the menu or drag&drop, I've changed the string to:

Allow the installation of the following add-on in #1:;
Allow the installation of the following #2 add-ons in #1:

I couldn't work out how to trigger the plural message. Even when selecting two add-ons via the menu, I get two notifications, one on top of the other. Via drag&drop it only offers to install one. That's minor and most likely also doesn't work in FF.

Comment 16

[Geoff Lankow (:darktrojan)]

Yes, that's interesting. Possibly a hangover from when you could install a XPI with several XPIs inside it (which might still be a thing, not sure) because I seem to recall that was the only way to have more than one extension listed in the old UI.

Anyway, it doesn't matter much. Ship it!

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/9528e42be304
Show notification to allow add-on installation after xpinstallConfirm removal in bug 1473933. r=jorgk

Comment 18

[Jorg K (CEST = GMT+2)]

(In reply to Geoff Lankow (:darktrojan) from comment #16)
> Ship it!
You bet!