1323128 - Remove support for multi-package xpis

Status: RESOLVED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Andrew Swan [:aswan]]

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 1

[Andrew Swan [:aswan]]

Attachment: Bug 1323128 Remove support for multi-package xpis

Review commit: https://reviewboard.mozilla.org/r/98270/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/98270/

Comment 2

[Robert Helmer [:rhelmer]]

Comment on attachment 8818161 [details]
Bug 1323128 Remove support for multi-package xpis

https://reviewboard.mozilla.org/r/98270/#review98992

I see the string `linkedInstalls` in at least two tests:
`test_install.js`
`test_install_strictcompat.js`

I expect a try run will dredge these up though.

I don't think there's a need to re-review if you just kill some old test code to get try to pass.

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:227
(Diff revision 1)
>  // Map new string type identifiers to old style nsIUpdateItem types
>  const TYPES = {
>    extension: 2,
>    theme: 4,
>    locale: 8,
> -  multipackage: 32,
> +//  multipackage: 32,

Why commented out and not removed?

Comment 3

[Robert Helmer [:rhelmer]]

Comment on attachment 8818161 [details]
Bug 1323128 Remove support for multi-package xpis

https://reviewboard.mozilla.org/r/98270/#review98992

Ah on further inspection, these just test that `linkedInstalls` is explicitly `false` so don't cause any failures. Still, should remove them to avoid confusion for future archaeologists.

Comment 4

[Andrew Swan [:aswan]]

Comment on attachment 8818161 [details]
Bug 1323128 Remove support for multi-package xpis

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/98270/diff/1-2/

Comment 5

[Andrew Swan [:aswan]]

Comment on attachment 8818161 [details]
Bug 1323128 Remove support for multi-package xpis

https://reviewboard.mozilla.org/r/98270/#review98992

Thanks for catching that, removed.

> Why commented out and not removed?

Ah, I figured I should leave something here indicating that this used to be used for something else so nobody should come along and re-use that value.  But you're right, leaving it commented out isn't great, what do you think?  And while we're at it, why do we only use powers of 2 here?  Its not like we can combine different values together in a bitfield...

Comment 6

[Andrew Swan [:aswan]]

Comment on attachment 8818161 [details]
Bug 1323128 Remove support for multi-package xpis

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/98270/diff/2-3/

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b133e1edcf02
Remove support for multi-package xpis r=rhelmer

Comment 8

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/b133e1edcf02

Comment 9

[rctgamer3]

There are still a dozen themes that used multi-package XPIs to bundle an add-on with their theme, such as Silvermel and Charamel, which bundle an add-on that allows for customization of the theme.
I didn't find any notice about the removal or any advance warning that support for this would be removed. 

Wouldn't it be better to delay the removal until 57? Themes (in their current form) are going to break with Firefox 57 anyway so why remove it early and break updates to these themes? I tried installing a multi-package XPI in 53 and all it did was tell me that the add-on was corrupt. No doorhanger notification about them not being supported, nothing.
If I were a user of a multi-XPI theme or add-on I would at least want to be notified about one of my add-ons no longer being supported.