1698158 - Add Services to the globals provided to the WebExtensions API modules global

Status: RESOLVED FIXED

(WebExtensions :: General, task, P1)

Description

[Luca Greco [:rpl] [:luca] [:lgreco]]

Services.jsm is already loaded by the time we do load the WebExtensions API modules.
We do already import it in ExtensionCommon.jsm here, before we do create the global where the WebExtensions API modules are being executed.

As mentioned in Bug 1697404 comment 1 there are many extensions API modules (both the one integrated natively in Firefox and the experimental API part of some privileged extensions builtin into Firefox).

Given how common it is and the fact that the jsm is actually already loaded, we could provide Services by default in that global and as a side-effect reduce the chances that a privileged extension which embeds multiple experimental API could break itself by importing it in different conflicting ways (e.g. See Bug 1697404 comments 4).

Comment 1

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: Bug 1698158 - part 1 - Provide Services by default in webextensions api modules. r?mixedpuppy!

Comment 2

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: Bug 1698158 - part 2 - Remove unnecessary jsm imports in existing WebExtensions API modules. r?mixedpuppy!

Depends on D108226

Comment 3

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: Bug 1698158 - part 3.1 - Remove unnecessary imports from builtin addons experimental API. r?mixedpuppy!

Depends on D108227

Comment 4

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: Bug 1698158 - part 3.2 - Remove unnecessary imports from testharness addons experimental API. r?mixedpuppy!

Depends on D108228

Comment 5

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: Bug 1698158 - part 3.3 - Remove unnecessary imports from talos testharness addons experimental API. r?mixedpuppy!

Depends on D108229

Comment 6

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: Bug 1698158 - part 3.4 - Remove unnecessary imports from sync tps testharness experimental API. r?mixedpuppy!

Depends on D108230

Comment 7

[Luca Greco [:rpl] [:luca] [:lgreco]]

Pushed to try here (don't include talos, android and services tps test harness, but let's see how it goes with the webextensions tests and the experimental APIs likely used by all tests harnesses, e.g. quitter may be one of them):

https://treeherder.mozilla.org/jobs?repo=try&revision=d5b811b5f455db0503b926442daa2112294ea261

Comment 8

[Luca Greco [:rpl] [:luca] [:lgreco]]

There is one issue that isn't yet addressed by these patches yet:

eslint doesn't know which globals are already defined in a webextensions API script global and so it is going to complain and the developer working on the API will likely assume that eslint is right and just import it explicitly again.

For the natively integrated APIs we can more easily list the globals that we know exist into the eslint configs in tree, but that may not help when a new builtin addon is added in tree and also when it is being developed outside of mozilla-central tree.

One strategy that we may use to improve that could be to define a plugin that does configure eslint with the list of globals exposed (we don't change that list that often and so keeping it updated shouldn't be that big of an issue), but it would still be something that the developer do need to add to their eslint config to be an effective way of avoiding that those globals would be re-imported manually.

Comment 9

[Mark Banner (:standard8)]

(In reply to Luca Greco [:rpl] [:luca] [:lgreco] from comment #8)

One strategy that we may use to improve that could be to define a plugin that does configure eslint with the list of globals exposed (we don't change that list that often and so keeping it updated shouldn't be that big of an issue), but it would still be something that the developer do need to add to their eslint config to be an effective way of avoiding that those globals would be re-imported manually.

You could set up an environment as we do for similar items here:

https://searchfox.org/mozilla-central/source/tools/lint/eslint/eslint-plugin-mozilla/lib/environments

We regularly publish new updates of eslint-plugin-mozilla to npm, so other repos could import that.

Comment 10

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: Bug 1698158 - part 4 - Add some notes about the globals available in the WebExtensions API scripts in in-tree docs. r?mixedpuppy!

Depends on D108232

Comment 11

[Luca Greco [:rpl] [:luca] [:lgreco]]

(In reply to Mark Banner (:standard8) from comment #9)

You could set up an environment as we do for similar items here:

https://searchfox.org/mozilla-central/source/tools/lint/eslint/eslint-plugin-mozilla/lib/environments

We regularly publish new updates of eslint-plugin-mozilla to npm, so other repos could import that.

That is exactly what I was thinking of, thanks a lot for pointing it out where it does live.
I'll file a separate issue to take a look into that.

Comment 12

[Luca Greco [:rpl] [:luca] [:lgreco]]

Link to one more push to try: https://treeherder.mozilla.org/jobs?repo=try&revision=00476ee9aa0e10e2ded14f7f2688a2e82213ec8f

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7793d8d8a817
part 1 - Provide Services by default in webextensions api modules. r=mixedpuppy
https://hg.mozilla.org/integration/autoland/rev/72d6d9258332
part 2 - Remove unnecessary jsm imports in existing WebExtensions API modules. r=mixedpuppy
https://hg.mozilla.org/integration/autoland/rev/05ef06f4180d
part 3.1 - Remove unnecessary imports from builtin addons experimental API. r=mixedpuppy,webcompat-reviewers,denschub
https://hg.mozilla.org/integration/autoland/rev/8a98d844e248
part 3.2 - Remove unnecessary imports from testharness addons experimental API. r=mixedpuppy
https://hg.mozilla.org/integration/autoland/rev/f65a4978d75e
part 3.3 - Remove unnecessary imports from talos testharness addons experimental API. r=mixedpuppy,perftest-reviewers,sparky
https://hg.mozilla.org/integration/autoland/rev/dbdead49c1c8
part 3.4 - Remove unnecessary imports from sync tps testharness experimental API. r=mixedpuppy
https://hg.mozilla.org/integration/autoland/rev/8ea36600327c
part 4 - Add some notes about the globals available in the WebExtensions API scripts in in-tree docs. r=mixedpuppy

Comment 14

[Alexandru Michis [:malexandru]]

https://hg.mozilla.org/mozilla-central/rev/7793d8d8a817
https://hg.mozilla.org/mozilla-central/rev/72d6d9258332
https://hg.mozilla.org/mozilla-central/rev/05ef06f4180d
https://hg.mozilla.org/mozilla-central/rev/8a98d844e248
https://hg.mozilla.org/mozilla-central/rev/f65a4978d75e
https://hg.mozilla.org/mozilla-central/rev/dbdead49c1c8
https://hg.mozilla.org/mozilla-central/rev/8ea36600327c