Closed
Bug 1323129
Opened 8 years ago
Closed 8 years ago
Remove support for installing multiple xpis with InstallTrigger
Categories
(Toolkit :: Add-ons Manager, defect)
Tracking
()
RESOLVED
FIXED
mozilla53
Tracking | Status | |
---|---|---|
firefox53 | --- | fixed |
People
(Reporter: aswan, Assigned: aswan)
References
Details
(Keywords: addon-compat, Whiteboard: triaged)
Attachments
(3 files)
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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 3•8 years ago
|
||
mozreview-review |
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+
Assignee | ||
Updated•8 years ago
|
Whiteboard: triaged
Assignee | ||
Comment 4•8 years ago
|
||
mozreview-review-reply |
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.
Assignee | ||
Comment 5•8 years ago
|
||
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
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•8 years ago
|
Attachment #8822605 -
Flags: feedback?(rhelmer)
Assignee | ||
Comment 9•8 years ago
|
||
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)
Assignee | ||
Comment 10•8 years ago
|
||
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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 14•8 years ago
|
||
mozreview-review-reply |
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
Updated•8 years ago
|
Attachment #8822605 -
Flags: feedback+
Updated•8 years ago
|
Attachment #8822606 -
Flags: feedback+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•8 years ago
|
Attachment #8822605 -
Flags: review?(rhelmer)
Attachment #8822606 -
Flags: review?(rhelmer)
Comment 18•8 years ago
|
||
mozreview-review |
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 19•8 years ago
|
||
mozreview-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+
Comment 20•8 years ago
|
||
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
Comment 21•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/d97b2eeffc46
https://hg.mozilla.org/mozilla-central/rev/308b6c4b0e8f
https://hg.mozilla.org/mozilla-central/rev/4afc16741787
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox53:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
You need to log in
before you can comment on or make changes to this bug.
Description
•