1788013 - Rename ModuleCache::hasModule to hasModuleClass

Status: RESOLVED FIXED

(Remote Protocol :: Agent, task, P3)

Description

[Julian Descottes [:jdescottes]]

See discussion at https://phabricator.services.mozilla.com/D155059#inline-857818

The hasModule method on ModuleCache might be misleading because it doesn't actually tell if a given module is already in the cache. It only checks if there is an implementation (ie a Class) for the given moduleName + destination.

But for instance if you are trying to avoid initializing a Module if it was not already created, this method should not be used. So we should clarify the method's name. We can either keep the current behavior and update the name to be hasModuleClass. Or we update the behavior to check if there is a module instantiated in the cache, but note that we can't really support destinations then. A root ModuleCache can check if a root/log or a windowglobal-in-root/log module is already instantiated, but it has no idea about windowglobal/log.

Can be a good mentored bug after discussion in triage.

Comment 1

[Julian Descottes [:jdescottes]]

Let's update the name to hasModuleClass and keep the behavior identical. If anyone wants to use this as a first bug to setup the development environment, happy to mentor.

Comment 2

[LyScott123]

Hello! I would love to use this as my first bug! I got mozilla up and running. Not familiar with mercurial but wishing to learn! Would love to have this assigned to me! Thank you!

Comment 3

[Julian Descottes [:jdescottes]]

(In reply to LyScott123 from comment #2)

Hello! I would love to use this as my first bug! I got mozilla up and running. Not familiar with mercurial but wishing to learn! Would love to have this assigned to me! Thank you!

Hi there! Somebody contacted me on Elements to get started on this bug but it seems they didn't comment on Bugzilla yet. I would like to hear back from them before assigning it to you. Sorry about that, but thanks for your interest. Feel free to either pick another bug, or wait until I get confirmation from that person. If I don't hear back before tomorrow, I will consider the bug as available and can assign it to you then.

Sorry again!

Comment 4

[Julian Descottes [:jdescottes]]

Ok, the bug is available to you if you still want it :)

Comment 5

[LyScott123]

Awesome! Thank you! I'll get started on it tomorrow! Might need some help with Phrabricator and Mercurial!

Comment 6

[Julian Descottes [:jdescottes]]

(In reply to LyScott123 from comment #5)

Awesome! Thank you! I'll get started on it tomorrow! Might need some help with Phrabricator and Mercurial!

Sounds good, if you need support with your dev environment setup, you can chat at https://chat.mozilla.org/#/room/#introduction:mozilla.org or https://chat.mozilla.org/#/room/#webdriver:mozilla.org

Make sure you look at the documentation as well: https://firefox-source-docs.mozilla.org/contributing/contribution_quickref.html

Comment 7

[Julian Descottes [:jdescottes]]

Hi there,

Just wanted to check if you needed some help to get started?

Comment 8

[LyScott123]

Hey! Sorry! I had some moving to do so was a little busy these past 2 weeks! I got my environment up and running and I read through the tutorials you provided.

Kind of nervous submitting my first patch! I'll have time on Wednesday to work on it. I'll let you know if I run into and trouble with my first attempt!

Thank you!!

Comment 9

[Julian Descottes [:jdescottes]]

No worries, great to see that your environment is up and running. Don't hesitate to reach out if you have any question.

Comment 10

[LyScott123]

Attachment: Bug 1788013 - changed hasModule to hasModuleClass r?jdescottes

Comment 11

[LyScott123]

Hello! Just submitted! Let me know if there is anything I need to change. Still not quite sure about how Mercurial and Phabricator works, but I'll keep reading up on it.

Would it be okay if I looked for another bug to fix at this point in time?

Thank you again!

Comment 12

[Julian Descottes [:jdescottes]]

(In reply to LyScott123 from comment #11)

Hello! Just submitted! Let me know if there is anything I need to change. Still not quite sure about how Mercurial and Phabricator works, but I'll keep reading up on it.

Thanks for submitting a patch!

Would it be okay if I looked for another bug to fix at this point in time?

Feel free to start scouting for other bugs, sure (the patch added here needs some more changes before it can land though, so let's try to focus on landing this one :) )

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/dcc49d61ce31
changed hasModule to hasModuleClass r=jdescottes,webdriver-reviewers

Comment 14

[Narcis Beleuzu [:NarcisB]]

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