Blair McBride [:Unfocused] (UNAVAILABLE) Reporter
	Description • 14 years ago

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

	Blair McBride [:Unfocused] (UNAVAILABLE) Reporter
	Comment 1 • 14 years ago

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.

	Dave Townsend [:mossop] Assignee
	Comment 2 • 14 years ago

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

	Dave Townsend [:mossop] Assignee
	Comment 3 • 14 years ago

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

	Dave Townsend [:mossop] Assignee
	Comment 4 • 14 years ago

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

	Robert Strong (they/them - no direct email)
	Comment 5 • 14 years ago

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;

	Dave Townsend [:mossop] Assignee
	Comment 6 • 14 years ago

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

	Dave Townsend [:mossop] Assignee
	Comment 8 • 14 years ago

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

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

	Dave Townsend [:mossop] Assignee
	Comment 9 • 14 years ago

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

	Henrik Skupin [:whimboo][⌚️UTC+2]
	Comment 10 • 14 years ago

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