Closed Bug 1133846 Opened 9 years ago Closed 9 years ago

ActivateTimeoutTick logging is missing arguments

Categories

(Core :: Networking: HTTP, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla39
Tracking Status
firefox39 --- fixed

People

(Reporter: jdm, Assigned: tvaleev, Mentored)

Details

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

Attachments

(1 file, 1 obsolete file)

In nsHttpConnetionMgr.cpp, the logging call in ActivateTimeoutTick contains %p references but doesn't have any corresponding arguments. This makes the logs from https://developer.mozilla.org/en-US/docs/Mozilla/Debugging/HTTP_logging contain confusing and incorrect data. We should add the missing arguments.
Typo: nsHttpConnectionMgr.cpp
Hi. I created the patch. Is it good?
Looks good to me! If you follow the steps at https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/How_to_Submit_a_Patch#Getting_the_patch_reviewed, you can select a reviewer from the network team at https://wiki.mozilla.org/Modules/Core#Necko or you can use the `suggested reviewers` link that appears when you set the `review` flag to `?`.
Thank you. But could you please advise me...
When I change `review` flag to `?` I see 7 suggested people who can review the patch. I selected network module owner Patrick McManus. Am I right?
Flags: needinfo?(josh)
Attachment #8565894 - Flags: review?(mcmanus)
Yes, that's fine :)
Flags: needinfo?(josh)
Comment on attachment 8565894 [details] [diff] [review]
1133846_fix_missing_arguments_v0.1.patch

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

Timur - thanks for the patch!
Attachment #8565894 - Flags: review?(mcmanus) → review+
protip: you can put the reviewer in your comment line (e.g. r=mcmanus) assuming you get an r+ even when you are just requesting the review .. that way you don't need to change the patch before going to checkin-needed

also, to use checkin-needed the sheriffs require try runs. try is great - but it requires L1 hg access. If you don't have that yet I'm happy to vouch for anybody that is posting networking patches.

https://www.mozilla.org/en-US/about/governance/policies/commit/
Thank you for these tips, Patrick. 
Yes I don't have L1 hg access (as I understand its Try/User/Incubator Access).
But it will be interesting for me to start participate in networking programming.
I filed a bug in the "Repository Account Requests" component. So if it possible please vouch for me :)
Oh sorry Patrick. I forget add a link to the bug.
Here it is https://bugzilla.mozilla.org/show_bug.cgi?id=1134220.
Hi Patrick. Looks like the pacth is ok. Should I do additional steps to commit it?
Flags: needinfo?(mcmanus)
you can use the checkin-needed keyword on the bug to have r+'d patches with green try runs committed to the tree on your behalf.

to do so, you need to re-upload it with a reviewer name in the description.. in this case it would look like:

Bug 1133846 - Add missing arguments to logging call in ActivateTimeoutTick. r=mcmanus

when you do that you can set the r+ flag on the "new" patch yourself. that's  only controlled by the honor system.
Flags: needinfo?(mcmanus)
Attachment #8565894 - Attachment is obsolete: true
Attachment #8568425 - Flags: review+
Attachment #8568425 - Flags: checkin+
Hi Patrick! I've done additional steps. Am I right?
Flags: needinfo?(mcmanus)
thanks!
Flags: needinfo?(mcmanus)
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/fd1c4a3ba4e3
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
You need to log in before you can comment on or make changes to this bug.