1301629 - netwerk/* comment doesn't match the parameter names

Status: RESOLVED FIXED

(Core :: General, defect)

Description

[Sylvestre Ledru [:Sylvestre]]

This is a trivial bug. This has been found by clang-tidy ( http://clang.llvm.org/extra/clang-tidy/checks/misc-argument-comment.html ).
The idea is to give an easy bug for someone who wants to understand how to contribute to Firefox.

netwerk/cookie/nsCookieService.cpp
@ Line 35	argument name 'holdsWeak' in comment does not match parameter name 'ownsWeak'

Comment 1

[Umesh Panchaksharaiah]

Hi, I am a newbie to Mozilla and will be working on
this bug as there is no assignee. Thanks.

Comment 2

[Umesh Panchaksharaiah]

Hi Sylvestre, as this is a trivial change that has been
verified with mac build and mach run. Any suggestions on
additional tests that I should run? Any idea how I can run
clang-tidy to verify that the error is indeed fixed?
If I should be asking these questions to someone else, 
please let me know as well. Thanks in advance.

Comment 3

[Sylvestre Ledru [:Sylvestre]]

Build is enough :)

Comment 4

[Umesh Panchaksharaiah]

Attachment: Fix incorrect parameter name in comments

Comment 5

[Umesh Panchaksharaiah]

Hi Sylvestre, I have uploaded a patch and requested a review from
you. Is there anyone else I should request review from?
Thanks.

Comment 6

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8791341 [details] [diff] [review]
Fix incorrect parameter name in comments

I cannot review this code but you will find a review here:
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Introduction#Step_4_-_Get_your_code_reviewed

Comment 7

[Umesh Panchaksharaiah]

Comment on attachment 8791341 [details] [diff] [review]
Fix incorrect parameter name in comments

Hi, I am requesting this review to you as you
added the line in question per hg blame. Thank you
in advance. Please let me know if this needs to be
assigned to someone else.

Comment 8

[Ethan Tseng [:ethan]]

Comment on attachment 8791341 [details] [diff] [review]
Fix incorrect parameter name in comments

Hi Umesh,

Welcome to Bugzilla!  Thanks for your contribution. :)

Actually that line was added by jduell, not me.
However the change is only one word in comment, I believe it's fine. r=me.

Comment 9

[Ethan Tseng [:ethan]]

Sylvestre,

I see in total there are four such misspellings (holdsWeak) on DXR.
Did you file bugs for each of them?
If not, maybe we should fix all of them here. What do you think?

Comment 10

[Sylvestre Ledru [:Sylvestre]]

Ethan, I filed bugs for the mismatch only, not for the typo.
We should probably open a new bug for the followup, this will be easier for everyone.

Comment 11

[Ethan Tseng [:ethan]]

(In reply to Sylvestre Ledru [:sylvestre] from comment #10)
> Ethan, I filed bugs for the mismatch only, not for the typo.
> We should probably open a new bug for the followup, this will be easier for
> everyone.

Sorry I used the wrong word (misspelling) which caused misunderstanding.
Actually I meant mismatch, not typo.
Search the word "holdsWeak" on DXR, there will be 4 results.
They are all codes which call nsIObserverService::AddObserver() and they all mismatch the name of the third parameter.  I was simply saying, they are the same bug.

Comment 12

[Sylvestre Ledru [:Sylvestre]]

ok, we should do it in the same bug them :)

Umesh, I just added keywords which means two things:
* we are asking help to a sheriff to get your patch landed in our code.
* the bug should not be fixed as we will have follow up.

Could you care take of the other issues that Ethan is talking about? dxr= http://dxr.mozilla.org/

Comment 13

[Umesh Panchaksharaiah]

Thanks folks for the review and follow-up.
I will address the other locations with the
same typo and upload another patch.

Comment 14

[Umesh Panchaksharaiah]

Attachment: Fix incorrect parameter name in comments

Comment 15

[Umesh Panchaksharaiah]

Comment on attachment 8792541 [details] [diff] [review]
Fix incorrect parameter name in comments

Hi Ethan,  I have made the suggested change. Please let
me know if you will be able to look at this change or If I
need to submit the review to someone else. Thanks.

Comment 16

[Sylvestre Ledru [:Sylvestre]]

Congrats for acting that fast!

Comment 17

[Ethan Tseng [:ethan]]

Comment on attachment 8792541 [details] [diff] [review]
Fix incorrect parameter name in comments

Review of attachment 8792541 [details] [diff] [review]:
-----------------------------------------------------------------

Let's go land your first bug!  :)

Comment 18

[Umesh Panchaksharaiah]

Thanks much Ethan for the review. Based on the developer
guide, I need to run tests. However, Sylvestre indicated
that build is enough for this as it is a simple comment change.
He has also marked the bug as 'checkin-needed'. So, as per
developer guide, the bug will get checked in by someone with
the correct permissions and nothing more to be done by my
side for this bug.

Comment 19

[Ethan Tseng [:ethan]]

(In reply to Umesh Panchaksharaiah from comment #18)
Almost, except one thing.

You should update your patch by adding "r=ettseng" into the end of the commit comment message.
(Check the commit history you'll know more clearly about what the message looks like.)
Meanwhile, obsolete the previous two patches.  Then it's done from your side.

Comment 20

[Umesh Panchaksharaiah]

Attachment: Fix incorrect parameter name in comments

Comment 21

[Umesh Panchaksharaiah]

Thanks Ethan. I updated the commit message and uploaded another patch.
It looks like the previous patch have been marked as obsoleted already.
I guess I am done now for this bug fix.

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5a199ae8917c
Fix incorrect parameter name in comments. r=ettseng

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5a199ae8917c