1836482 - Convert AddonManager.jsm to an ES module

Status: RESOLVED FIXED

(Toolkit :: Add-ons Manager, task)

Description

[Mark Banner (:standard8)]

AddonManager.jsm should be able to be converted to an ES module reasonably easy, however this is currently one test where it fails, because the test is trying to unload it and then re-import it:

https://searchfox.org/mozilla-central/rev/87ba454e5c68ff77dff9acb9d7b0b51d6df12d11/toolkit/mozapps/extensions/test/xpcshell/test_childprocess.js#10

ES modules cannot be unloaded. This will need someone from the team to decide if the test is still useful, and the appropriate solution.

Comment 1

[Rob Wu [:robwu]]

Is this the only test failure?

To fix this failure, add head = to https://searchfox.org/mozilla-central/rev/71ff2d99628938eb74e983898c0fa2b1be6d5ef9/toolkit/mozapps/extensions/test/xpcshell/xpcshell.ini#32 (so that head_addons.js is not loaded). Then unloading isn't needed any more.

An alternative is to append a query string as a cache buster, like what I suggested before at https://searchfox.org/mozilla-central/rev/71ff2d99628938eb74e983898c0fa2b1be6d5ef9/services/settings/test/unit/test_remote_settings_release_prefs.js#9-11

Comment 2

[Rob Wu [:robwu]]

Attachment: Bug 1836482 - Remove Cu.unload from test_childprocess.js

To prepare for migration of AddonManager.jsm to AddonManager.sys.mjs, we
need to remove the use of Cu.unload from test_childprocess.js, because
ESM cannot be unloaded.

To avoid the need to unload, set head = in the test manifest to avoid
loading head_addons.js (specified in [DEFAULT] in xpcshell.ini).

head_addons.js was responsible for offering the createAppInfo & gAppInfo
test helpers, which was imported from AddonTestUtils.sys.mjs. We cannot
import AddonTestUtils because that module also imports AddonManager. The
underlying implementation is imported instead (AppInfo.sys.mjs).

Comment 3

[Rob Wu [:robwu]]

Attachment: Bug 1836482 - Convert AddonManager.jsm to ESM

Ran the following:

./mach esmify --convert toolkit/mozapps/extensions/AddonManager.jsm

and manually fixed the AsyncShutdown import by renaming it and declaring
a new var, because AddonManagerPrivate.overrideAsyncShutdown relies on
it being non-const.

Comment 4

[Mathew Hodson]

(In reply to Rob Wu [:robwu] from comment #3)

and manually fixed the AsyncShutdown import by renaming it and declaring
a new var, because AddonManagerPrivate.overrideAsyncShutdown relies on
it being non-const.

You could also import it using ChromeUtils like this. That's what was done in ProductAddonChecker.jsm with ServiceRequest.

ChromeUtils.defineESModuleGetters(lazy, {
  // This is overridden by xpcshell tests.
  AsyncShutdown: "resource://gre/modules/AsyncShutdown.sys.mjs",
});

Comment 5

[Rob Wu [:robwu]]

Attachment: Bug 1836482 - Replace AddonManager.jsm imports with AddonManager.sys.mjs

This patch was generated as follows:

Run:
./mach esmify --imports . --prefix=toolkit/mozapps/extensions/AddonManager
In the output there are linter/prettifier errors due to unused
XPCOMUtils or separate importESModule calls. These have been fixed
manually and verified with ./mach lint --outgoing.

The esmify script also inserts many unwanted newlines around imports
that are broken on two lines due to length. Due to the number of these,
I fixed them programatically.

1. Create patch from the changes so far.
2. From the patch, delete all lines that consist of "+" (i.e. added blank line).
3. Reset the working dir and apply the revised patch.
4. Verify that the diff between step 1 and 3 looks reasonable.
5. Verify that this patch as a whole looks reasonable.

Commands:

git diff > rename.diff
:%g/^+$/d
git commit -va -m WIP-rename
git revert HEAD
git apply --recount rename.diff
git diff HEAD^  # and verify that the removed lines are ok.
git commit -va  # one last review to verify correctness of whole patch.
git rebase -i HEAD~3  # drop the WIP + reverted commit, pick only the last.

git apply has the --recount option to force it to ignore mismatches
in line counts, which happens because we deleted added lines (^+$)
without fixing up the line counts in the file headers.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/88b2e42976d2
Remove Cu.unload from test_childprocess.js r=Standard8
https://hg.mozilla.org/integration/autoland/rev/909b53b0ab30
Convert AddonManager.jsm to ESM r=Standard8

Comment 7

[Norisz Fay [:noriszfay]]

https://hg.mozilla.org/mozilla-central/rev/88b2e42976d2
https://hg.mozilla.org/mozilla-central/rev/909b53b0ab30

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8a4b7b46101d
Replace AddonManager.jsm imports with AddonManager.sys.mjs r=Standard8,webcompat-reviewers,twisniewski

Comment 9

[Iulian Moraru]

https://hg.mozilla.org/mozilla-central/rev/8a4b7b46101d