Closed Bug 613385 Opened 11 years ago Closed 11 years ago

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


(Toolkit :: Add-ons Manager, defect)

Not set



Tracking Status
blocking2.0 --- betaN+


(Reporter: mossop, Assigned: mossop)




(1 file, 1 obsolete file)

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.
blocking2.0: --- → betaN+
Attached patch patch rev 1 (obsolete) — Splinter Review
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.
Assignee: nobody → dtownsend
Attachment #491972 - Flags: review?(robert.bugzilla)
Whiteboard: [has patch][needs review rs]
Duplicate of this bug: 576876
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.
Whiteboard: [has patch][needs review rs]
As given on bug 576876, this should also handle the update case.
Attached patch patch rev 2Splinter Review
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.
Attachment #492790 - Flags: review?(robert.bugzilla)
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 :(
Attachment #492790 - Flags: review?(robert.bugzilla) → review+
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?
Attachment #492790 - Flags: review?(
Attachment #492790 - Flags: review?( → review+
Attachment #491972 - Attachment is obsolete: true
Attachment #491972 - Flags: review?(robert.bugzilla)
Closed: 11 years ago
Flags: in-testsuite+
Flags: in-litmus-
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b8
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
Depends on: 1063418
You need to log in before you can comment on or make changes to this bug.