553791 - Add an onExternalInstall event to notify listeners that an add-on is being installed without a managing AddonInstall

Status: VERIFIED FIXED

(Toolkit :: Add-ons Manager, defect, P1)

Description

[Blair McBride [:Unfocused] (UNAVAILABLE)]

It's not possible to tell the difference between installs that use an AddonInstall object, and those that do not (ie, only send onInstalling and onInstalled); other than checking against all known AddonInstall objects.

This may warrent a new event. Or possibly re-using onNewInstall as:
onNewInstall(null, Addon);

Comment 1

[Blair McBride [:Unfocused] (UNAVAILABLE)]

To clarify: This makes the onInstalling event mean two different things (progressing through an install vs starting a new install), with no non-hacky way of telling them apart.

This is blocking fully supporting the search-engine and lightweight themes providers.

Comment 2

[Dave Townsend [:mossop]]

I've added an additional parameter to the onInstalling event which caries either the AddonInstall responsible for the install or null if there is none. It comes before the requiresRestart parameter so slightly breaks the API but I think it's the better choice and we're still early enough to do that.

http://hg.mozilla.org/projects/addonsmgr/rev/8ea6126d6d2e

Comment 3

[Dave Townsend [:mossop]]

Per discussion this isn't the way to go here. Backed out at http://hg.mozilla.org/projects/addonsmgr/rev/584c6ffa8895 and landed the new onExternalInstall event at http://hg.mozilla.org/projects/addonsmgr/rev/46c152650157

Comment 4

[Dave Townsend [:mossop]]

Attachment: patch rev 1

Implements the new event and tests it is fired by the lightweight theme manager.

Comment 5

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

Comment on attachment 435793 [details] [diff] [review]
patch rev 1

>diff --git a/toolkit/components/search/nsSearchService.js b/toolkit/components/search/nsSearchService.js
>--- a/toolkit/components/search/nsSearchService.js
>+++ b/toolkit/components/search/nsSearchService.js
>@@ -2242,6 +2242,8 @@ Engine.prototype = {
>         AddonManagerPrivate.callAddonListeners("onUninstalled", wrapper);
>       }
>       else {
>+        AddonManagerPrivate.callInstallListeners("onExternalInstall", null,
>+                                                 wrapper, null, false);
>         AddonManagerPrivate.callAddonListeners("onInstalling", wrapper, false);
>         AddonManagerPrivate.callAddonListeners("onInstalled", wrapper);
>       }
>@@ -2789,6 +2791,10 @@ SearchService.prototype = {
>     }
> 
>     let wrapper = new SearchAddon(this, aEngine);
>+    // TODO make this pass the existing engine, but that gets mangled by the
>+    // search service
Bug filed?

>+    AddonManagerPrivate.callInstallListeners("onExternalInstall", null, wrapper,
>+                                             null, false);
>     AddonManagerPrivate.callAddonListeners("onInstalling", wrapper, false);
>     AddonManagerPrivate.callAddonListeners("onInstalled", wrapper);
> 
>diff --git a/toolkit/mozapps/extensions/LightweightThemeManager.jsm b/toolkit/mozapps/extensions/LightweightThemeManager.jsm
>--- a/toolkit/mozapps/extensions/LightweightThemeManager.jsm
>+++ b/toolkit/mozapps/extensions/LightweightThemeManager.jsm
>@@ -130,8 +130,11 @@ var LightweightThemeManager = {
> 
>     if (aData) {
>       let theme = this.getUsedTheme(aData.id);
>+      // TODO detect if it is an install and act accordingly
Bug filed?

>       if (!theme) {
>         var wrapper = new AddonWrapper(aData);
>+        AddonManagerPrivate.callInstallListeners("onExternalInstall", null,
>+                                                 wrapper, null, false);
>         AddonManagerPrivate.callAddonListeners("onInstalling", wrapper, false);
>       }
>       let current = this.currentTheme;

Comment 6

[Dave Townsend [:mossop]]

(In reply to comment #5)
> (From update of attachment 435793 [details] [diff] [review])
> >diff --git a/toolkit/components/search/nsSearchService.js b/toolkit/components/search/nsSearchService.js
> >--- a/toolkit/components/search/nsSearchService.js
> >+++ b/toolkit/components/search/nsSearchService.js
> >@@ -2242,6 +2242,8 @@ Engine.prototype = {
> >         AddonManagerPrivate.callAddonListeners("onUninstalled", wrapper);
> >       }
> >       else {
> >+        AddonManagerPrivate.callInstallListeners("onExternalInstall", null,
> >+                                                 wrapper, null, false);
> >         AddonManagerPrivate.callAddonListeners("onInstalling", wrapper, false);
> >         AddonManagerPrivate.callAddonListeners("onInstalled", wrapper);
> >       }
> >@@ -2789,6 +2791,10 @@ SearchService.prototype = {
> >     }
> > 
> >     let wrapper = new SearchAddon(this, aEngine);
> >+    // TODO make this pass the existing engine, but that gets mangled by the
> >+    // search service
> Bug filed?

The changes for the search service shouldn't really be in this patch, it'll get covered eventually in bug 552747.

> 
> >+    AddonManagerPrivate.callInstallListeners("onExternalInstall", null, wrapper,
> >+                                             null, false);
> >     AddonManagerPrivate.callAddonListeners("onInstalling", wrapper, false);
> >     AddonManagerPrivate.callAddonListeners("onInstalled", wrapper);
> > 
> >diff --git a/toolkit/mozapps/extensions/LightweightThemeManager.jsm b/toolkit/mozapps/extensions/LightweightThemeManager.jsm
> >--- a/toolkit/mozapps/extensions/LightweightThemeManager.jsm
> >+++ b/toolkit/mozapps/extensions/LightweightThemeManager.jsm
> >@@ -130,8 +130,11 @@ var LightweightThemeManager = {
> > 
> >     if (aData) {
> >       let theme = this.getUsedTheme(aData.id);
> >+      // TODO detect if it is an install and act accordingly
> Bug filed?

Should have really read "detect if it is an update and act accordingly" and I've just done it so if you want to re-skim over that bit, that is the only change.

Comment 7

[Dave Townsend [:mossop]]

Attachment: patch rev 2

Comment 8

[Dave Townsend [:mossop]]

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

Cancel that, looks like I fixed it in bug 553113 anyway. Too many patches...

Comment 9

[Dave Townsend [:mossop]]

http://hg.mozilla.org/mozilla-central/rev/c90f52c3dc45

Comment 10

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

Marking as verified fixed based on check-in and passing automated tests.