Closed Bug 1716642 Opened 6 months ago Closed 5 months ago

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

Categories

(WebExtensions :: General, task, P5)

task

Tracking

(firefox91 fixed)

RESOLVED FIXED
91 Branch
Tracking Status
firefox91 --- fixed

People

(Reporter: standard8, Assigned: jasleenbhambra01, Mentored)

References

(Blocks 1 open bug)

Details

(Whiteboard: [lang=js])

Attachments

(1 file)

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;
  1. 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.
  2. 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.
  1. 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.
  1. 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.
  2. Please be prepared to address comments from review.
Blocks: 1609271
Severity: -- → N/A
Priority: -- → P5
Keywords: good-first-bug

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.

Keywords: good-first-bug

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.

Assignee: nobody → jasleenbhambra01
Status: NEW → ASSIGNED

(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;
  1. 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.
  2. 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.
  1. 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.
  1. 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.
  2. Please be prepared to address comments from review.

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

Pushed by mbanner@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f020a73949bf
Replaced calls to GlobalManager.extensionMap.get() with WebExtensionPolicy.getByID(). r=Standard8,kmag,robwu
Status: ASSIGNED → RESOLVED
Closed: 5 months ago
Resolution: --- → FIXED
Target Milestone: --- → 91 Branch

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!

(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! :)

You need to log in before you can comment on or make changes to this bug.