613385 - No notifications are sent out for webpage attempts to install add-ons when xpinstall.enabled=false

Status: VERIFIED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Dave Townsend [:mossop]]

Bug 561878 stopped sending out any info that the UI could work with to display anything, need to remove that and see what was actually broken because right now it seems to me that the code should just work right without it.

Comment 1

[Dave Townsend [:mossop]]

Attachment: patch rev 1

I believe that when I fixed bug 561878 I was relying too much on the outcome of the automated tests and the frontend wasn't fully wired up at that point. In fact the test harness wasn't doing the right thing and treated disabling installation the same as blocked installation. Were we not past API freeze it'd probably make sense to send different notifications for the two rather than relying on the frontend to check the pref to distinguish them but here we are.

This removes the bad fix, adds a better test that verifies the actual notification in the UI and fixes the xpinstall test harness to work right.

Comment 3

[Robert Strong (they/them - no direct email)]

Talked with Dave briefly about this and think it would be best to make the api change since not doing it now will make it so consumers have to change it in the future.

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+1]]

As given on bug 576876, this should also handle the update case.

Comment 5

[Dave Townsend [:mossop]]

Attachment: patch rev 2

This is the alternate patch with the API change. I'm going to email the drivers to confirm that this would be ok. It basically just adds a new method to amIWebInstallListener to notify of the disabled case and our default implementation sends that through the observer service as addon-install-disabled.

Comment 6

[Robert Strong (they/them - no direct email)]

Comment on attachment 492790 [details] [diff] [review]
patch rev 2

>diff --git a/toolkit/mozapps/extensions/amIWebInstallListener.idl b/toolkit/mozapps/extensions/amIWebInstallListener.idl
>--- a/toolkit/mozapps/extensions/amIWebInstallListener.idl
>+++ b/toolkit/mozapps/extensions/amIWebInstallListener.idl
>@@ -60,20 +60,36 @@ interface amIWebInstallInfo : nsISupport
> };
> 
> /**
>  * The registered amIWebInstallListener is used to notify about new installs
>  * triggered by websites. The default implementation displays a confirmation
>  * dialog when add-ons are ready to install and uses the observer service to
>  * notify when installations are blocked.
>  */
>-[scriptable, uuid(ea806f3a-1b27-4d3d-9aee-88dec4c29fda)]
>+[scriptable, uuid(a5503979-89c8-441e-9e4a-321df379c172)]
> interface amIWebInstallListener : nsISupports
> {
>   /**
>+   * Called when installation by websites is currently disabled.
"currently" doesn't add value to the comment

The browser changes look fine but I'm not a peer of this directory under browser so I can't r+ that part :(

Comment 7

[Dave Townsend [:mossop]]

Comment on attachment 492790 [details] [diff] [review]
patch rev 2

Gavin, the browser changes are just basically moving code around, can I get a quick r+ on it?

Comment 8

[Dave Townsend [:mossop]]

Landed: http://hg.mozilla.org/mozilla-central/rev/9ff362a5b8db

Comment 9

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Marking as verified fixed with builds on all platforms like Mozilla/5.0 (X11;
Linux i686; rv:2.0b8pre) Gecko/20101125 Firefox/4.0b8pre ID:20101125030318