Closed Bug 1414406 Opened 7 years ago Closed 6 years ago

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

Categories

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

task

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox57 --- wontfix
firefox59 --- fixed

People

(Reporter: mossop, Assigned: mossop)

References

Details

(Whiteboard: [xbl-remove-unused])

Attachments

(3 files)

Now that legacy add-ons have gone away we can remove the inline options types from the add-ons manager on desktop and android.
What bindings do you expect can be removed as part of this?
Flags: needinfo?(dtownsend)
(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.
The desktop equivalent is here: http://searchfox.org/mozilla-central/source/toolkit/mozapps/extensions/content/setting.xml.
Flags: needinfo?(dtownsend)
OK, updating the description to match. Looks like there are 13 bindings to be removed in those files.
Summary: Remove the inline options feature for add-ons → Remove the inline options feature for add-ons and remove the setting-* XBL bindings for mobile and desktop
Matching priority to the meta bug, please adjust if you want it to be higher.
Priority: -- → P5
Attachment #8927513 - Flags: review?(aswan)
Attachment #8927514 - Flags: review?(aswan)
Whiteboard: [xbl-remove-unused]
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.
Attachment #8927513 - Flags: review?(aswan) → review+
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)
Attachment #8927514 - Flags: review?(aswan) → review+
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.
Pushed by dtownsend@mozilla.com:
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
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 ===
Flags: needinfo?(dtownsend)
Comment on attachment 8928193 [details]
Bug 1414406: Remove inline options from talos tabswitch test add-on.

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

Thanks!
Attachment #8928193 - Flags: review?(mconley) → review+
Pushed by dtownsend@mozilla.com:
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
Flags: needinfo?(dtownsend)
Thanks for the heads up. Filed on github as https://github.com/meandavejustice/min-vid/issues/1127.
Flags: needinfo?(dtownsend)
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!
Flags: needinfo?(dtownsend)
(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.
Flags: needinfo?(dtownsend) → qe-verify-
Depends on: 1434321
No longer depends on: 1434321
Depends on: 1418314
Depends on: CVE-2018-5165
Depends on: 1454322
Type: enhancement → task
You need to log in before you can comment on or make changes to this bug.