Closed Bug 1323129 Opened 8 years ago Closed 7 years ago

Remove support for installing multiple xpis with InstallTrigger

Categories

(Toolkit :: Add-ons Manager, defect)

Product:
Toolkit
Component:
Add-ons Manager
Version:
51 Branch
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla53
Tracking Flags:
Tracking Status
firefox53 --- fixed

People

(Reporter: aswan, Assigned: aswan)

References

Details

(Keywords: addon-compat, Whiteboard: triaged)

Attachments

(3 files)

Bug 1323129 part 1: Remove support for installing multiple xpis in InstallTrigger
58 bytes, text/x-review-board-request
rhelmer
: review+
Details
Bug 1323129 part 2: remove amIWebInstaller
59 bytes, text/x-review-board-request
rhelmer
: review+
Details
bug 1323129 part 3: clean up the old amIWebInstallListener interfaces
59 bytes, text/x-review-board-request
rhelmer
: review+
Details 
Proposal and reasoning were circulated on a few mailing lists:
https://groups.google.com/forum/#!topic/mozilla.dev.platform/om5BrXM-Gb8

If there isn't any major objection I'll aim to land this later this week.
Comment on attachment 8818162 [details]
Bug 1323129 part 1: Remove support for installing multiple xpis in InstallTrigger

https://reviewboard.mozilla.org/r/98274/#review99202

I have a few comments but this lgtm overall. It's nice how much this simplifies things!

::: toolkit/mozapps/extensions/AddonManager.jsm:455
(Diff revision 2)
> -    if (--this.installCount == 0)
> -      this.unregister();
> +    this.unregister();
>    },
>  
>    onDownloadFailed: function(install) {
> -    this.onDownloadCancelled(install);
> +    this.unregister();

Hm, why change this? It looks like `onDownloadCancelled` does this and also removes the listener... it looks like we're already removing the listener so that's not an issue now, but what if `onDownloadCancelled` changes in the future?

I guess this is all speculative but just wondering why this is better.

::: toolkit/mozapps/extensions/AddonManager.jsm:2166
(Diff revision 2)
>        if (!this.isInstallEnabled(aMimetype)) {
> -        for (let install of aInstalls)
> +        aInstall.cancel();
> -          install.cancel();
>  
>          weblistener.onWebInstallDisabled(topBrowser, aInstallingPrincipal.URI,
> -                                         aInstalls, aInstalls.length);
> +                                         [aInstall], 1);

It looks like the last arg is not used in http://searchfox.org/mozilla-central/rev/09ae31fddfbc229ef1b8f1a3ccd87ad05680e695/toolkit/mozapps/extensions/amWebInstallListener.js#281 (and it's optional in the idl)

Might be worth having that not take an array too?
Attachment #8818162 - Flags: review?(rhelmer) → review+
Whiteboard: triaged
Comment on attachment 8818162 [details]
Bug 1323129 part 1: Remove support for installing multiple xpis in InstallTrigger

https://reviewboard.mozilla.org/r/98274/#review99202

> Hm, why change this? It looks like `onDownloadCancelled` does this and also removes the listener... it looks like we're already removing the listener so that's not an issue now, but what if `onDownloadCancelled` changes in the future?
> 
> I guess this is all speculative but just wondering why this is better.

That code was all about managing N different installs with one BrowserListener object but we don't have to do that any more.  If clean-up ever becomes more complicated then yes, this will need to be rewritten but I think that that's quite unlikely and it would be good to re-do it in that case anyway.

> It looks like the last arg is not used in http://searchfox.org/mozilla-central/rev/09ae31fddfbc229ef1b8f1a3ccd87ad05680e695/toolkit/mozapps/extensions/amWebInstallListener.js#281 (and it's optional in the idl)
> 
> Might be worth having that not take an array too?

Yeah, I drew the line at touching any of the idl interfaces in this patch, but I think that would be a good next step.  I don't actually see any reason to use xpcom here, there's certainly no benefit for the code that is in-tree.  Ugh, I see some uses in the addons repo, let me dig a little further.
In addition to the obvious change for addon developers (the new restriction on the first argument to InstallTrigger.trigger), this will change some internals that could break some add-ons that use AddonManager.jsm including
- installAddonsFromWebpage getting renamed to installAddonFromWebpage
- removal of amIWebInstallListener and amIWebInstallListener2 xpcom interfaces
Keywords: addon-compat
Summary: Remove support for installing multiple xpis with IntallTrigger → Remove support for installing multiple xpis with InstallTrigger
Attachment #8822605 - Flags: feedback?(rhelmer)
Comment on attachment 8822606 [details]
bug 1323129 part 3: clean up the old amIWebInstallListener interfaces

Don't read too much into the way this is divided into multiple patches -- its a reflection of the way I wrote them and I thought it might be easier to review this way than as one gigantic patch.
There's more cleanup that can be done here, but the immediate goal is to unblock the permissions work.  The current handling of Installer and installNotifyObservers in AddonManager.jsm will get another overhaul in bug 1308295, I'll try to get that patch uploaded shortly so you can see it as well if it helps with understanding these patches.
Attachment #8822606 - Flags: feedback?(rhelmer)
Not sure if it makes the reviewing here easier or harder, but I pushed work-in-progress patches on bug 1308295 so you can see where this is all headed.
Comment on attachment 8818162 [details]
Bug 1323129 part 1: Remove support for installing multiple xpis in InstallTrigger

https://reviewboard.mozilla.org/r/98274/#review99202

> Yeah, I drew the line at touching any of the idl interfaces in this patch, but I think that would be a good next step.  I don't actually see any reason to use xpcom here, there's certainly no benefit for the code that is in-tree.  Ugh, I see some uses in the addons repo, let me dig a little further.

This is addressed in part 2
Attachment #8822605 - Flags: feedback+
Attachment #8822606 - Flags: feedback+
Attachment #8822605 - Flags: review?(rhelmer)
Attachment #8822606 - Flags: review?(rhelmer)
Comment on attachment 8822605 [details]
Bug 1323129 part 2: remove amIWebInstaller

https://reviewboard.mozilla.org/r/101470/#review102676
Attachment #8822605 - Flags: review?(rhelmer) → review+
Comment on attachment 8822606 [details]
bug 1323129 part 3: clean up the old amIWebInstallListener interfaces

https://reviewboard.mozilla.org/r/101472/#review102678
Attachment #8822606 - Flags: review?(rhelmer) → review+
Pushed by aswan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d97b2eeffc46
part 1: Remove support for installing multiple xpis in InstallTrigger r=rhelmer
https://hg.mozilla.org/integration/autoland/rev/308b6c4b0e8f
part 2: remove amIWebInstaller r=rhelmer
https://hg.mozilla.org/integration/autoland/rev/4afc16741787
part 3: clean up the old amIWebInstallListener interfaces r=rhelmer
Depends on: 1329450
Depends on: 1329073
Depends on: 1341011
Depends on: 1212515
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: