Remove unused JSID_IS_ZERO function.

RESOLVED FIXED in Firefox 52

Status

()

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: arai, Assigned: userajay0, Mentored)

Tracking

({good-first-bug})

Trunk
mozilla52
Points:
---

Firefox Tracking Flags

(firefox51 fix-optional, firefox52 fixed)

Details

(Whiteboard: [good first bug] [lang=c++])

Attachments

(2 attachments, 1 obsolete attachment)

Reporter

Description

3 years ago
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

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

Comment 2

3 years ago
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

3 years ago
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 .
Reporter

Comment 4

3 years ago
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

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

Comment 6

3 years ago
(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.
Posted patch Patch file (obsolete) — Splinter Review
Hello,
I am a first timer. Please review the patch and tell me if something needs to be changed.

Thanks!
Attachment #8793169 - Flags: review?(arai.unmht)
(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. :(
Reporter

Comment 9

3 years ago
Comment on attachment 8793169 [details] [diff] [review]
Patch file

per comment #8.
Attachment #8793169 - Attachment is obsolete: true
Attachment #8793169 - Flags: review?(arai.unmht)

Comment 10

3 years ago
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?
Reporter

Comment 11

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

Comment 12

3 years ago
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.
Reporter

Comment 13

3 years ago
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
Reporter

Comment 15

3 years ago
(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
Assignee

Updated

3 years ago
Attachment #8793493 - Flags: review?(arai.unmht)
Reporter

Comment 16

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

please read comment #15 ;)
Attachment #8793493 - Flags: review?(arai.unmht)
Assignee

Comment 17

3 years ago
(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
Reporter

Comment 18

3 years ago
(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
Assignee

Comment 19

3 years ago
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 hidden (mozreview-request)
Reporter

Comment 22

3 years ago
mozreview-review
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.
Attachment #8793560 - Flags: review?(arai.unmht)
Reporter

Updated

3 years ago
Assignee: nobody → ajay.krishn97
Status: NEW → ASSIGNED
Reporter

Comment 23

3 years ago
pass2pawan, ping me if you're looking for other C++ bug.
Assignee

Comment 24

3 years ago
mozreview-review-reply
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
Reporter

Comment 25

3 years ago
mozreview-review-reply
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 hidden (mozreview-request)
Reporter

Comment 27

3 years ago
mozreview-review
Comment on attachment 8793560 [details]
Bug 1303708 - Remove unused JSID_IS_ZERO(jsid id)  function.

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

Perfect :)
Attachment #8793560 - Flags: review?(arai.unmht) → review+
Assignee

Comment 28

3 years ago
mozreview-review-reply
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
Reporter

Comment 29

3 years ago
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

3 years ago
Pushed by arai_a@mac.com:
https://hg.mozilla.org/integration/autoland/rev/ef5324ac2924
Remove unused JSID_IS_ZERO(jsid id)  function. r=arai
Assignee

Comment 31

3 years ago
mozreview-review-reply
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
Reporter

Comment 32

3 years ago
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

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/ef5324ac2924
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Assignee

Comment 34

3 years ago
(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
Reporter

Comment 35

3 years ago
Yes, finished.
Thank you again for your contribution :)
Mark 51 as fix-optional. If it's worth uplift to 51, feel free to nominate it.
You need to log in before you can comment on or make changes to this bug.