1414406 - Remove the inline options feature for add-ons and remove the setting-* XBL bindings for mobile and desktop

Status: RESOLVED FIXED

(Toolkit :: Add-ons Manager, task, P5)

Description

[Dave Townsend [:mossop]]

Now that legacy add-ons have gone away we can remove the inline options types from the add-ons manager on desktop and android.

Comment 1

[Brian Grinstead [:bgrins]]

What bindings do you expect can be removed as part of this?

Comment 2

[Nick Alexander :nalexander [he/him]]

(In reply to Brian Grinstead [:bgrins] from comment #1)
> What bindings do you expect can be removed as part of this?

This stemmed out of looking at the Android checkbox-* stuff, I think.  It led us to think settings-* could be removed: http://searchfox.org/mozilla-central/source/mobile/android/chrome/content/bindings/settings.xml.  I think there's an identical Desktop equivalent.  This is all in about:addons on Android, and probably on Desktop too.

Comment 3

[Dave Townsend [:mossop]]

The desktop equivalent is here: http://searchfox.org/mozilla-central/source/toolkit/mozapps/extensions/content/setting.xml.

Comment 4

[Brian Grinstead [:bgrins]]

OK, updating the description to match. Looks like there are 13 bindings to be removed in those files.

Comment 5

[Andy McKay]

Matching priority to the meta bug, please adjust if you want it to be higher.

Comment 6

[Dave Townsend [:mossop]]

Attachment: Bug 1414406: Switch plugins to use inline browser options.

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

Comment 7

[Dave Townsend [:mossop]]

Attachment: Bug 1414406: Remove the inline options feature for add-ons.

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

Comment 8

[Andrew Swan [:aswan]]

Comment on attachment 8927513 [details]
Bug 1414406: Switch plugins to use inline browser options.

https://reviewboard.mozilla.org/r/198820/#review204220

My XUL knowledge isn't deep enough to know if there are subtle pitfalls here but this all looks straightforward and unit tests should cover the user-facing things we care about.

Comment 9

[Andrew Swan [:aswan]]

Comment on attachment 8927514 [details]
Bug 1414406: Remove the inline options feature for add-ons.

https://reviewboard.mozilla.org/r/198822/#review204224

Woohoo!

::: python/mozboot/mozboot/osx.py:353
(Diff revision 1)
>              'wget',
>          ]
>          self._ensure_homebrew_packages(packages)
>  
>          casks = [
> -            'java',
> +            'java8',

Did you mean to include this?

::: toolkit/mozapps/extensions/content/extensions.js
(Diff revision 1)
>  
>      cmd_showItemPreferences: {
>        isEnabled(aAddon) {
> -        if (!aAddon ||
> +        if (!aAddon || (!aAddon.isActive && aAddon.type !== "plugin")) {
> -            (!aAddon.isActive && aAddon.type !== "plugin") ||
> -            !aAddon.optionsURL) {

Just to be clear, there's a change here from checking for `optionsURL` to `optionsType` right?  I think that doesn't matter for webextensions since we set those properties at the same time (ie an addon either has neither or has both) but just wanted to make sure I'm not misunderstanding your intention here.

::: toolkit/mozapps/extensions/test/browser/browser_details.js:1013
(Diff revision 1)
>  
>      is_element_hidden(get("detail-size"), "Size should be hidden");
>  
>      is_element_hidden(get("detail-downloads"), "Downloads should be hidden");
>  
> -    is_element_visible(get("detail-prefs-btn"), "Preferences button should be visible");
> +    is_element_hidden(get("detail-prefs-btn"), "Preferences button should be visible");

nit: change the message.  I guess this whole portion of this test is probably going to be on the chopping block soon though...

::: toolkit/mozapps/extensions/test/xpcshell/test_manifest.js:497
(Diff revision 1)
>      do_check_false(a16.userDisabled);
>      do_check_false(a16.appDisabled);
>      do_check_true(a16.isCompatible);
>      do_check_true(a16.providesUpdatesSecurely);
>  
> -    do_check_neq(a17, null);
> +    do_check_eq(a17, null);

This also doesn't have a long future ahead of it, but while we're here can we add a comment either here or above where the manifest is written indicating that that extension uses an obsolete and hence illegal value for optionsType so we refuse to load it (and same for the similar ones below)

Comment 10

[Dave Townsend [:mossop]]

Comment on attachment 8927514 [details]
Bug 1414406: Remove the inline options feature for add-ons.

https://reviewboard.mozilla.org/r/198822/#review204224

> Did you mean to include this?

Nope. Removed.

> Just to be clear, there's a change here from checking for `optionsURL` to `optionsType` right?  I think that doesn't matter for webextensions since we set those properties at the same time (ie an addon either has neither or has both) but just wanted to make sure I'm not misunderstanding your intention here.

Previously specifying optionsURL but not optionsType meant we defaulted to a now obsolete type. So now it defaults to an optionsType of null in that situation so optionsURL is no longer a good way to test if an add-on has options we can open.

Comment 11

[Dave Townsend [:mossop]]

Comment on attachment 8927514 [details]
Bug 1414406: Remove the inline options feature for add-ons.

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

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f438ce3fc88e
Switch plugins to use inline browser options. r=aswan
https://hg.mozilla.org/integration/autoland/rev/f501182ed44b
Remove the inline options feature for add-ons. r=aswan

Comment 13

[Natalia Csoregi [:nataliaCs]]

Backed out for ESLINT failures /toolkit/mozapps/extensions/content/extensions.js

Backout: https://hg.mozilla.org/integration/autoland/rev/aa1ee6561a971514fcec11a02a5091588152c879

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=f501182ed44bf1cd4ab61d17502d39bad24bd6f9&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=144410381&repo=autoland&lineNumber=235

[task 2017-11-13T21:22:28.289Z] Error processing command. Ignoring because optional. (optional:packages.txt:comm/build/virtualenv_packages.txt)
235
[task 2017-11-13T21:29:53.947Z] TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/toolkit/mozapps/extensions/content/extensions.js:1193:16 | Unnecessary 'else' after 'return'. (no-else-return)
236
[task 2017-11-13T21:29:53.947Z] TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/toolkit/mozapps/extensions/content/extensions.js:3606:14 | 'stripTextNodes' is defined but never used. (no-unused-vars)
237
[taskcluster 2017-11-13 21:29:54.632Z] === Task Finished ===

Comment 14

[Dave Townsend [:mossop]]

Comment on attachment 8927513 [details]
Bug 1414406: Switch plugins to use inline browser options.

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

Comment 15

[Dave Townsend [:mossop]]

Comment on attachment 8927514 [details]
Bug 1414406: Remove the inline options feature for add-ons.

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

Comment 16

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

This also causes Talos g2 failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&group_state=expanded&fromchange=cfbdb1dc35cd8071cb97bcdf0deeaabaa1e33bea&filter-searchStr=T-e10s%28g2%29&tochange=aa1ee6561a971514fcec11a02a5091588152c879&selectedJob=144410359

Comment 17

[Dave Townsend [:mossop]]

Attachment: Bug 1414406: Remove inline options from talos tabswitch test add-on.

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

Comment 18

[Dave Townsend [:mossop]]

Comment on attachment 8927514 [details]
Bug 1414406: Remove the inline options feature for add-ons.

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

Comment 19

[Dave Townsend [:mossop]]

Comment on attachment 8928193 [details]
Bug 1414406: Remove inline options from talos tabswitch test add-on.

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

Comment 20

[Dave Townsend [:mossop]]

Comment on attachment 8927514 [details]
Bug 1414406: Remove the inline options feature for add-ons.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/198822/diff/4-5/

Comment 21

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8928193 [details]
Bug 1414406: Remove inline options from talos tabswitch test add-on.

https://reviewboard.mozilla.org/r/199418/#review204606

Thanks!

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a44297e0db42
Switch plugins to use inline browser options. r=aswan
https://hg.mozilla.org/integration/autoland/rev/36cb0ea65892
Remove inline options from talos tabswitch test add-on. r=mconley
https://hg.mozilla.org/integration/autoland/rev/ac82533933fb
Remove the inline options feature for add-ons. r=aswan

Comment 23

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/a44297e0db42
https://hg.mozilla.org/mozilla-central/rev/36cb0ea65892
https://hg.mozilla.org/mozilla-central/rev/ac82533933fb

Comment 24

[Naomi Smith [:spinda] (non-bmo mail: [e-mail])]

FYI, this broke Min Vid, which still uses OPTIONS_TYPE_INLINE:

https://github.com/meandavejustice/min-vid/blob/0d8d1a3d6fcff1601cb89d9c2d6a29a65b3dd889/install.rdf#L23

Comment 25

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Thanks for the heads up. Filed on github as https://github.com/meandavejustice/min-vid/issues/1127.

Comment 26

[Madalin Cotetiu]

Is manual testing required on this bug? If yes, please provide some STR and the proper extension(if required) or set the “qe-verify -“ flag.

Thanks!

Comment 27

[Dave Townsend [:mossop]]

(In reply to Madalin Cotetiu from comment #26)
> Is manual testing required on this bug? If yes, please provide some STR and
> the proper extension(if required) or set the “qe-verify -“ flag.
> 
> Thanks!

No manual testing is required here.

Comment 28

[Aris]

There was no reason to remove inline options in Thunderbird.
Now devs have to rewrite their add-ons, just because Dave Townsend decided there is no need for inline options anymore.
Thanks again for wasting the time of add-on developers!