Closed Bug 553791 Opened 12 years ago Closed 12 years ago

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

Categories

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

defect

Tracking

()

VERIFIED FIXED
mozilla1.9.3a5

People

(Reporter: Unfocused, Assigned: mossop)

References

Details

(Whiteboard: [rewrite])

Attachments

(1 file, 1 obsolete file)

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);
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.
Blocks: 553515
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
Assignee: nobody → dtownsend
Status: NEW → ASSIGNED
Flags: in-testsuite+
Flags: in-litmus-
Whiteboard: [rewrite] → [rewrite][fixed-in-addonsmgr]
Whiteboard: [rewrite][fixed-in-addonsmgr] → [rewrite][fixed-in-addonsmgr][needs-review]
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
Summary: Unable to tell difference between installs with an AddonInstall object and those without → Add an onExternalInstall event to notify listeners that an add-on is being installed without a managing AddonInstall
Attached patch patch rev 1Splinter Review
Implements the new event and tests it is fired by the lightweight theme manager.
Attachment #435793 - Flags: review?(robert.bugzilla)
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;
Attachment #435793 - Flags: review?(robert.bugzilla) → review+
Whiteboard: [rewrite][fixed-in-addonsmgr][needs-review] → [rewrite][fixed-in-addonsmgr]
(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.
Attached patch patch rev 2 (obsolete) — Splinter Review
Attachment #435793 - Attachment is obsolete: true
Attachment #437329 - Flags: review?(robert.bugzilla)
Comment on attachment 437329 [details] [diff] [review]
patch rev 2

Cancel that, looks like I fixed it in bug 553113 anyway. Too many patches...
Attachment #437329 - Attachment is obsolete: true
Attachment #437329 - Flags: review?(robert.bugzilla)
Attachment #435793 - Attachment is obsolete: false
Whiteboard: [rewrite][fixed-in-addonsmgr] → [rewrite][fixed-in-addonsmgr][needs-landing]
http://hg.mozilla.org/mozilla-central/rev/c90f52c3dc45
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Whiteboard: [rewrite][fixed-in-addonsmgr][needs-landing] → [rewrite]
Target Milestone: --- → mozilla1.9.3a5
Marking as verified fixed based on check-in and passing automated tests.
Status: RESOLVED → VERIFIED
Version: unspecified → Trunk
You need to log in before you can comment on or make changes to this bug.