1716642 - Replace calls to GlobalManager.extensionMap.get() with WebExtensionPolicy.getByID()

Status: RESOLVED FIXED

(WebExtensions :: General, task, P5)

Description

[Mark Banner (:standard8)]

We want to replace calls to GlobalManager.extensionMap.get() with WebExtensionPolicy.getByID(). This will allow us to remove imports of GlobalManager which is not explicitly exported.

I'm happy to mentor this bug, please note:

• This is slightly larger and more complex than a normal "good first bug".
• This bug will be auto-assigned when the first patch is attached.

Here's what to do:

1. Ensure you have a [working Firefox build](https://firefox-source-docs.mozilla.org/contributing/contribution_quickref.html, an artifact build is sufficient thought it doesn't matter if you have a full build.

2. Run ./mach eslint --setup to set up ESLint, then ensure your editor has an integration set-up. It is not essential to do this, but it helps to show if you've made a mistake, or if formatting needs fixing whilst you're still in a particular file.

3. Find the cases to replace, this search is a good starting point. Please skip Extension.jsm and ExtensionParent.jsm since they are acceptable uses of GlobalManager at the moment.

4. Edit the various pieces of code to replace:

let extension = GlobalManager.extensionMap.get(arg.extensionId);

with

let policy = WebExtensionPolicy.getByID(extension.id);
let extension = policy?.extension;

5. In the files you've changed, look for where GlobalManager is imported, typically this will be via ChromeUtils.import but it could also be XPCOMUtils.defineLazyGetter. Remove the appropriate code so that GlobalManager is no longer imported.
6. From the files where you've removed the imports, see if they can be removed from the top-level .eslintrc.js file. To do this run ./mach eslint path/to/file and then see if any errors are generated relating to reject-chromeutils-import-params.

• If there are errors and they're not to do with the GlobalManager, then you can put the line back into the .eslintrc.js file.
• If there are other errors mentioned, then you may need to fix those as well.

8. For the test files you have changed, run the tests to make sure they still pass.

• You can use ./mach mochitest path/to/test
• We will do a run on our try servers, and make sure that the rest of the tests have past.

9. Commit the changes and submit for review with a message something like Bug nnnnnnn - Replace calls to GlobalManager.extensionMap.get() with WebExtensionPolicy.getByID(). r?Standard8.
10. Please be prepared to address comments from review.

Comment 1

[Mark Banner (:standard8)]

As I mentioned in comment 0, not sure this is a good first bug, as it is slightly larger than what we'd do normally. Probably is a good next bug though.

Comment 2

[Rob Wu [:robwu]]

The steps needed to resolve this bug are quite straightforward (and spelled out in the first comment). It's not significantly more complex than other good-first-bugs in the extension component. In any case I'll defer to you since you've self-nominated yourself as mentor. Thanks :)

PS. For contributions to extension code, I usually refer new contributors to https://wiki.mozilla.org/WebExtensions/Contribution_Onramp. Part of a new contribution is often writing tests, which is often challenging for beginners, but in this bug it just suffices to run existing tests, which actually makes this easier, hence a good-first-bug in my opinion.

Comment 3

[Jasleen Kaur]

Attachment: Bug 1716642 - Replaced calls to GlobalManager.extensionMap.get() with WebExtensionPolicy.getByID(). r?Standard8

Comment 4

[Jasleen Kaur]

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

We want to replace calls to GlobalManager.extensionMap.get() with WebExtensionPolicy.getByID(). This will allow us to remove imports of GlobalManager which is not explicitly exported.

I'm happy to mentor this bug, please note:

• This is slightly larger and more complex than a normal "good first bug".
• This bug will be auto-assigned when the first patch is attached.

Here's what to do:

1. Ensure you have a [working Firefox build](https://firefox-source-docs.mozilla.org/contributing/contribution_quickref.html, an artifact build is sufficient thought it doesn't matter if you have a full build.

2. Run ./mach eslint --setup to set up ESLint, then ensure your editor has an integration set-up. It is not essential to do this, but it helps to show if you've made a mistake, or if formatting needs fixing whilst you're still in a particular file.

3. Find the cases to replace, this search is a good starting point. Please skip Extension.jsm and ExtensionParent.jsm since they are acceptable uses of GlobalManager at the moment.

4. Edit the various pieces of code to replace:

let extension = GlobalManager.extensionMap.get(arg.extensionId);

with

let policy = WebExtensionPolicy.getByID(extension.id);
let extension = policy?.extension;

5. In the files you've changed, look for where GlobalManager is imported, typically this will be via ChromeUtils.import but it could also be XPCOMUtils.defineLazyGetter. Remove the appropriate code so that GlobalManager is no longer imported.
6. From the files where you've removed the imports, see if they can be removed from the top-level .eslintrc.js file. To do this run ./mach eslint path/to/file and then see if any errors are generated relating to reject-chromeutils-import-params.

• If there are errors and they're not to do with the GlobalManager, then you can put the line back into the .eslintrc.js file.
• If there are other errors mentioned, then you may need to fix those as well.

8. For the test files you have changed, run the tests to make sure they still pass.

• You can use ./mach mochitest path/to/test
• We will do a run on our try servers, and make sure that the rest of the tests have past.

9. Commit the changes and submit for review with a message something like Bug nnnnnnn - Replace calls to GlobalManager.extensionMap.get() with WebExtensionPolicy.getByID(). r?Standard8.
10. Please be prepared to address comments from review.

Hi Sir! I've submitted a patch. Thank You!

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f020a73949bf
Replaced calls to GlobalManager.extensionMap.get() with WebExtensionPolicy.getByID(). r=Standard8,kmag,robwu

Comment 6

[Cristian Brindusan [:cbrindusan]]

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

Comment 7

[Caitlin Neiman [:caitmuenster]]

Hi Jasleen, thank you so much for your patch! 🎉 Your contribution has been added to our recognition wiki at https://wiki.mozilla.org/Add-ons/Contribute/Recognition.

Hope to see you around the project in the future!

Comment 8

[Jasleen Kaur]

(In reply to Caitlin Neiman [:caitmuenster] from comment #7)

Hi Jasleen, thank you so much for your patch! 🎉 Your contribution has been added to our recognition wiki at https://wiki.mozilla.org/Add-ons/Contribute/Recognition.

Hope to see you around the project in the future!

Thank You Mam! :)