Note: this bug is currently reserved for Outreachy applicants for the Spring/Summer 2021 cycle. If it has not been completed by the end of the application cycle, we will open it up.

Background

The JSID_IS_ATOM calls reflect an old style of SpiderMonkey programming that we think we can start to get away from, internally to the engine.

This Bug

This bug is about removing uses of JSID_IS_ATOM within the js/src/vm/ArgumentsObject.cpp file. Instead of using JSID_IS_ATOM we should be using JS::PropertyKey::isAtom, which is actually how JSID_IS_ATOM is implemented.

Prerequisites

Before getting started, you'll want to

• You Have a checkout of the Firefox source code
• Make sure you can build and test SpiderMonkey
• Read this walkthrough about how development works in Firefox

How you’ll know you’re done

• The use of JSID_IS_ATOM is removed
• SpiderMonkey builds
• The tests run successfully.

Bonus:

• If you happen to see nearby uses of JSID_TO_ATOM, we should also remove those.

Getting Help

Feel free to leave comments on this bug for questions, or, if you have more synchronous questions about this bug, feel free to drop into the #spidermonkey channel on chat.mozilla.org.

Tips:

• Not sure if the code you've been editing is getting run? Insert a call to MOZ_CRASH, a macro which will crash when executed, and run the entire test suite with an optimized build (for speed). If you see crashes, you can then use a debug build to make sure it's crashing in your code!

Hey Matthew,
it's Jane here from Outreachy applicants. I would love to give ti a go and try to work on this issue ( even though for now it looks a bit complicated for me but I looove riddles so here we go :)
Could I be assigned for the task?

And a few clarification questions - first of all, checkout of Firefox source code - do you mean that nightly is running and all works?
Secondly, with the spider monkey, step mentions -

Build only the JS shell

ac_add_options --enable-application=js
and here I am lost a bit. Like I just put the code in terminal or I need to do some installations?

I already used Phabricator for a couple of issues here. So hopefully step 3 wouldn't be hardest for me haha.
Finally, with the code.
When I am changing the:

static MOZ_ALWAYS_INLINE bool JSID_IS_ATOM(jsid id) { return id.isAtom(); }

static MOZ_ALWAYS_INLINE bool JSID_IS_ATOM(jsid id, JSAtom* atom) {
return id.

with:
MOZ_ALWAYS_INLINE bool isAtom() const { return isString(); }

MOZ_ALWAYS_INLINE bool isAtom(JSAtom* atom) const {
MOZ_ASSERT(PropertyKey::isNonIntAtom(atom));
return isAtom() && toAtom() == atom;

Do I have to find manually all bits of code where we use JSID_IS_ATOM or just change this bit and thats it? And when we talking about tests, even though I found some information I would love if it is possible to find a link where could I understand how to imply tests and how to choose proper ones.
Thank you heaps, hopefully its not too much to ask!

(In reply to Matthew Gaudet (he/him) [:mgaudet] from comment #0)

Note: this bug is currently reserved for Outreachy applicants for the Spring/Summer 2021 cycle. If it has not been completed by the end of the application cycle, we will open it up.

Background

The JSID_IS_ATOM calls reflect an old style of SpiderMonkey programming that we think we can start to get away from, internally to the engine.

This Bug

This bug is about removing uses of JSID_IS_ATOM within the js/src/vm/ArgumentsObject.cpp file. Instead of using JSID_IS_ATOM we should be using JS::PropertyKey::isAtom, which is actually how JSID_IS_ATOM is implemented.

Prerequisites

Before getting started, you'll want to

• You Have a checkout of the Firefox source code
• Make sure you can build and test SpiderMonkey
• Read this walkthrough about how development works in Firefox

How you’ll know you’re done

• The use of JSID_IS_ATOM is removed
• SpiderMonkey builds
• The tests run successfully.

Bonus:

• If you happen to see nearby uses of JSID_TO_ATOM, we should also remove those.

Getting Help

Feel free to leave comments on this bug for questions, or, if you have more synchronous questions about this bug, feel free to drop into the #spidermonkey channel on chat.mozilla.org.

Tips:

• Not sure if the code you've been editing is getting run? Insert a call to MOZ_CRASH, a macro which will crash when executed, and run the entire test suite with an optimized build (for speed). If you see crashes, you can then use a debug build to make sure it's crashing in your code!

Omg I am so sorry I forgot to check the markdown of the file and how it looks like :(
I will re-write the comment here so its easier to comprehend all my messy writing ( as far as I understood that I am not allowed to change my ow comments)

Hey Matthew,
it's Jane here from Outreachy applicants. I would love to give ti a go and try to work on this issue ( even though for now it looks a bit complicated for me but I looove riddles so here we go :)
Could I be assigned for the task?

And a few clarification questions -

1. first of all, checkout of Firefox source code - do you mean that nightly is running and all works?

2. Secondly, with the spider monkey, step mentions -

• Build only the JS shell
• ac_add_options --enable-application=js
  and here I am lost a bit. Like I just put the code in terminal or I need to do some installations?

I already used Phabricator for a couple of issues here. So hopefully step 3 wouldn't be hardest for me haha.
3. Finally, with the code.
When I am changing the:

• static MOZ_ALWAYS_INLINE bool JSID_IS_ATOM(jsid id) { return id.isAtom(); }
  static MOZ_ALWAYS_INLINE bool JSID_IS_ATOM(jsid id, JSAtom* atom)
  { return id.

with:

• MOZ_ALWAYS_INLINE bool isAtom() const { return isString(); }
  MOZ_ALWAYS_INLINE bool isAtom(JSAtom* atom) const {
  MOZ_ASSERT(PropertyKey::isNonIntAtom(atom));
  return isAtom() && toAtom() == atom;

Do I have to find manually all bits of code where we use JSID_IS_ATOM or just change this bit and thats it?

And when we talking about tests, even though I found some information I would love if it is possible to find a link where could I understand how to imply tests and how to choose proper ones.

Thank you heaps, hopefully its not too much to ask!

Hi Jane,

1. Best place to start would be this detailed document with links to more detailed instructions. Be sure to follow the bootstrap instructions for "Build Firefox For Desktop", not "Firefox Artifact Build".

2. That ac_add_options line will go in a file called a mozconfiig; you'll come across it in the building instructions above.

3. To give you a clearer picture of the changes, here's a link to an example change we've already merged

4. For this bug, you can just use your editor's search functionality in js/src/vm/ArgumentsObject.cpp, but a great resource for finding things in Firefox is https://searchfox.org/

Hi Mathew,

I'm hoping since i'm already working on a related Bug 1703596, can I at least try this out?

Hey Mathew, I definitely would love to try this one!
I have built Firefox already, however as I rede,ber I think I choose artifact for desktop. Not quite sure. How could I check it? Or do I have to built new one (which is not a problem at all)
Everything else looking pretty clear right now, just will need a bit of the help with test section. - which one should I run. Apart of that everything seems so exciting, cannot wait to start!

(In reply to Matthew Gaudet (he/him) [:mgaudet] from comment #3)

Hi Jane,

1. Best place to start would be this detailed document with links to more detailed instructions. Be sure to follow the bootstrap instructions for "Build Firefox For Desktop", not "Firefox Artifact Build".

2. That ac_add_options line will go in a file called a mozconfiig; you'll come across it in the building instructions above.

3. To give you a clearer picture of the changes, here's a link to an example change we've already merged

4. For this bug, you can just use your editor's search functionality in js/src/vm/ArgumentsObject.cpp, but a great resource for finding things in Firefox is https://searchfox.org/

(In reply to Bukola Felicia Akinnadeju from comment #4)

Hi Mathew,

I'm hoping since i'm already working on a related Bug 1703596, can I at least try this out?

Hi Bukola, I've opened Bug 1704976, which you can take a peek at. We'll leave this one to Jane, who has already started.

(In reply to Evgenia Kotovich from comment #5)

Hey Mathew, I definitely would love to try this one!
I have built Firefox already, however as I rede,ber I think I choose artifact for desktop. Not quite sure. How could I check it? Or do I have to built new one (which is not a problem at all)
Everything else looking pretty clear right now, just will need a bit of the help with test section. - which one should I run. Apart of that everything seems so exciting, cannot wait to start!

You can run mach bootstrap again, and this time select Firefox for Desktop; it'll install the tools we need to build.

For testing, start with ./mach jit-test. This link has some more details

Oh, thanks for that Mathew.
Should I be aware of all my changes with I pushed ( however they are waiting to be reviewed.) Wont they disappear in my local machine if I run again mach bootstrap?
Like I can just duplicate one as an option, and make this one as a desktop.
And is it a way to check which one I have built on mine already ( I remember I have chosen an option 1 but don't remember which one is it)

(In reply to Matthew Gaudet (he/him) [:mgaudet] from comment #6)

(In reply to Bukola Felicia Akinnadeju from comment #4)

Hi Mathew,

I'm hoping since i'm already working on a related Bug 1703596, can I at least try this out?

Hi Bukola, I've opened Bug 1704976, which you can take a peek at. We'll leave this one to Jane, who has already started.

(In reply to Evgenia Kotovich from comment #5)

Hey Mathew, I definitely would love to try this one!
I have built Firefox already, however as I rede,ber I think I choose artifact for desktop. Not quite sure. How could I check it? Or do I have to built new one (which is not a problem at all)
Everything else looking pretty clear right now, just will need a bit of the help with test section. - which one should I run. Apart of that everything seems so exciting, cannot wait to start!

You can run mach bootstrap again, and this time select Firefox for Desktop; it'll install the tools we need to build.

For testing, start with ./mach jit-test. This link has some more details

(In reply to Evgenia Kotovich from comment #7)

Should I be aware of all my changes with I pushed ( however they are waiting to be reviewed.) Wont they disappear in my local machine if I run again mach bootstrap?

Bootstrap won't affect anything committed (nor, I believe, will it affect changes in your local copy). Developers on Firefox tend to run bootstrap once every month or two to update dependencies as they evolve.

And is it a way to check which one I have built on mine already ( I remember I have chosen an option 1 but don't remember which one is it)

Option 1 is Artifact mode; I am honestly not sure how to check, or if it's possible.

(PS: In a bug, top-posting where you keep all quoted reply is quote noisy, so I'd encourage you to avoid it).

Depends on D111743

https://hg.mozilla.org/mozilla-central/rev/7d11da47ae4e