1303708 - Remove unused JSID_IS_ZERO function.

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Tooru Fujisawa [:arai]]

JSID_IS_ZERO function was added 5 years ago by bug 601457 to catch unexpected case, and it's not used anymore.
we could remove it now.
(if not, we would need to add a comment that explains the purpose of the function)

https://dxr.mozilla.org/mozilla-central/rev/f0f15b7c6aa77a0c5750918aa0a1cb3dc82185bc/js/public/Id.h#72
> static MOZ_ALWAYS_INLINE bool
> JSID_IS_ZERO(jsid id)
> {
>     return JSID_BITS(id) == 0;
> }

Comment 1

[pass2pawan]

Hello ! 
I am new here , Can i request to get this bug assigned to me ?

Comment 2

[Tooru Fujisawa [:arai]]

Thank you for your comment :)
I'll assign bug when a patch is posted.

if you have any question related to building, testing, etc, feel free to ask here or in IRC #introduction channel.

Comment 3

[pass2pawan]

Although i do understand what needs to be done , can you tell me how do i go about providing a patch ? accessing this bug and rectifying it . 
thanks for your help .

Comment 4

[Tooru Fujisawa [:arai]]

you can provide a patch by either
  * push your changeset to review repository
  * export your patch as a patch file and attach it to this bug

about whole workflow, these documents will help
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/Simple_Firefox_build
https://mozilla-version-control-tools.readthedocs.io/en/latest/hgmozilla/firefoxworkflow.html

Comment 5

[Amal Santhosh]

Hi! I'm new here and finding my way about.
Can you help my way around here.
Thank you :)

Comment 6

[Tooru Fujisawa [:arai]]

(In reply to Amal Santhosh from comment #5)
> Hi! I'm new here and finding my way about.
> Can you help my way around here.
> Thank you :)

pass2pawan already commented they're going to work on this.
I filed bug 1304167 if you're looking for C++ good-first-bug.

Comment 7

[vinayakagarwal6996]

Attachment: Patch file

Hello,
I am a first timer. Please review the patch and tell me if something needs to be changed.

Thanks!

Comment 8

[vinayakagarwal6996]

(In reply to vinayakagarwal6996 from comment #7)
> Created attachment 8793169 [details] [diff] [review]
> Patch file
> 
> Hello,
> I am a first timer. Please review the patch and tell me if something needs
> to be changed.
> 
> Thanks!

Please delete this. I'm sorry. Wrong bug. :(

Comment 9

[Tooru Fujisawa [:arai]]

Comment on attachment 8793169 [details] [diff] [review]
Patch file

per comment #8.

Comment 10

[pass2pawan]

Tooru , i have the build in mercurial , is there any comprehensive guide which i can follow to track down the code and debug and patch it ? or could you help me in anyway for doing that?

Comment 11

[Tooru Fujisawa [:arai]]

the file is in js/public/Id.h, under mozilla-central (or mozilla-unified)
about debugging and patching, what's your question?

Comment 12

[pass2pawan]

I found the file and saved it in my build . now after this how do i create a patch ? what commands do i run on mercurial.

Comment 13

[Tooru Fujisawa [:arai]]

here's guide for workflow
  https://mozilla-version-control-tools.readthedocs.io/en/latest/hgmozilla/firefoxworkflow.html

about creating patch, you need to create a changeset, by
  hg commit -m 'Bug 1303708 - Remove unused JSID_IS_ZERO function.'

then, push it to review repository after configuring this:
  https://mozilla-version-control-tools.readthedocs.io/en/latest/mozreview-user.html#mozreview-user

or, just export it and attach the file here:
  hg export -r . > PATH_TO_OUTPUT_PATCH_FILE.patch

Comment 14

[Ajay]

Attachment: The function to be removed has been removed

Comment 15

[Tooru Fujisawa [:arai]]

(In reply to ajay.krishn97 from comment #14)
> Created attachment 8793493 [details] [diff] [review]
> The function to be removed has been removed

you need to export your change as a patch file:
  https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Introduction#Step_4_-_Get_your_code_reviewed
or submit it to review repository:
  https://mozilla-version-control-tools.readthedocs.io/en/latest/hgmozilla/firefoxworkflow.html

Comment 16

[Tooru Fujisawa [:arai]]

Comment on attachment 8793493 [details] [diff] [review]
The function to be removed has been removed

please read comment #15 ;)

Comment 17

[Ajay]

(In reply to Tooru Fujisawa [:arai] from comment #16)
> Comment on attachment 8793493 [details] [diff] [review]
> The function to be removed has been removed
> 
> please read comment #15 ;)

Is your Mozreview Username 'arai'?

Thanks

Comment 18

[Tooru Fujisawa [:arai]]

(In reply to ajay.krishn97 from comment #17)
> (In reply to Tooru Fujisawa [:arai] from comment #16)
> > Comment on attachment 8793493 [details] [diff] [review]
> > The function to be removed has been removed
> > 
> > please read comment #15 ;)
> 
> Is your Mozreview Username 'arai'?
> 
> Thanks

yes

Comment 19

[Ajay]

After making the change I ran hg commit with following commit message 
              r?arai Bug 1303708
              Removed the function that had to be removed
Then I got the ouput 'created new head'
Then I ran hg push review and I get the message
               pushing to https://reviewboard-hg.mozilla.org/autoreview
               searching for appropriate review repository
               redirecting push to https://reviewboard-hg.mozilla.org/gecko
               abort: you must set mozilla.ircnick in your hgrc config file to your IRC nickname in order to perform code reviews

Could you help me solve this issue

Thanks

Comment 20

[Tooru Fujisawa [:arai]]

here's documentation
https://mozilla-version-control-tools.readthedocs.io/en/latest/mozreview/install-mercurial.html#mozreview-install-mercurial

I think running `./mach mercurial-setup` will help

Comment 21

[Ajay]

Attachment: Bug 1303708 - Remove unused JSID_IS_ZERO(jsid id) function.

Removed the function that had to be removed

Review commit: https://reviewboard.mozilla.org/r/80276/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/80276/

Comment 22

[Tooru Fujisawa [:arai]]

Comment on attachment 8793560 [details]
Bug 1303708 - Remove unused JSID_IS_ZERO(jsid id)  function.

https://reviewboard.mozilla.org/r/80276/#review78918

Thank you for your patch :)

the code change looks good.
Can you change the commit message to follow this rule?
  https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Committing_Rules_and_Responsibilities#Commit_message_restrictions

so, put bug number and summary in the same line, and also describe the change more specifically, like:
  Bug 1303708 - Remove unused JSID_IS_ZERO function.
or something similar.

Comment 23

[Tooru Fujisawa [:arai]]

pass2pawan, ping me if you're looking for other C++ bug.

Comment 24

[Ajay]

Comment on attachment 8793560 [details]
Bug 1303708 - Remove unused JSID_IS_ZERO(jsid id)  function.

https://reviewboard.mozilla.org/r/80276/#review78918

So do I have to change the commit message(through 'hg commit --amend') and 'hg push review' it again or can I change it elsewhere(for example on reviewboard.mozilla.org)
I'm sorry if the solution to my doubts are too obvious, I'm a new user here.

Thanks

Comment 25

[Tooru Fujisawa [:arai]]

Comment on attachment 8793560 [details]
Bug 1303708 - Remove unused JSID_IS_ZERO(jsid id)  function.

https://reviewboard.mozilla.org/r/80276/#review78918

`hg commit --amend` will work, I think.

Comment 26

[Ajay]

Comment on attachment 8793560 [details]
Bug 1303708 - Remove unused JSID_IS_ZERO(jsid id)  function.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/80276/diff/1-2/

Comment 27

[Tooru Fujisawa [:arai]]

Comment on attachment 8793560 [details]
Bug 1303708 - Remove unused JSID_IS_ZERO(jsid id)  function.

https://reviewboard.mozilla.org/r/80276/#review78928

Perfect :)

Comment 28

[Ajay]

Comment on attachment 8793560 [details]
Bug 1303708 - Remove unused JSID_IS_ZERO(jsid id)  function.

https://reviewboard.mozilla.org/r/80276/#review78928

Could you tell me what I have to do next?

Thanks

Comment 29

[Tooru Fujisawa [:arai]]

I pushed your patch to try server.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=14ccea76c706

after it successfully finishes, the patch can be landed.
if you don't have a permission to do so (I'm not sure who can/cannot, on MozReview), ping me.

Comment 30

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ef5324ac2924
Remove unused JSID_IS_ZERO(jsid id)  function. r=arai

Comment 31

[Ajay]

Comment on attachment 8793560 [details]
Bug 1303708 - Remove unused JSID_IS_ZERO(jsid id)  function.

https://reviewboard.mozilla.org/r/80276/#review78928

Is there anything still left to do with regard to this bug or is the whole process finished

Comment 32

[Tooru Fujisawa [:arai]]

almost done.
just wait for the patch be merged to mozilla-central.

if something goes wrong (like it causes some issue), the patch will be backed out and you'll need to fix it,
if nothing goes wrong, it's finished :)

Comment 33

[Carsten Book [:Tomcat]]

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

Comment 34

[Ajay]

(In reply to Tooru Fujisawa [:arai] from comment #32)
> almost done.
> just wait for the patch be merged to mozilla-central.
> 
> if something goes wrong (like it causes some issue), the patch will be
> backed out and you'll need to fix it,
> if nothing goes wrong, it's finished :)

So everything regarding resolving this bug is finished right?
 Thank you for all the help

Comment 35

[Tooru Fujisawa [:arai]]

Yes, finished.
Thank you again for your contribution :)

Comment 36

[Gerry Chang [:gchang]]

Mark 51 as fix-optional. If it's worth uplift to 51, feel free to nominate it.