ActivateTimeoutTick logging is missing arguments

RESOLVED FIXED in Firefox 39

Status

()

Core
Networking: HTTP
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: jdm, Assigned: Timur Valeev, Mentored)

Tracking

unspecified
mozilla39
x86
Mac OS X
Points:
---

Firefox Tracking Flags

(firefox39 fixed)

Details

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

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

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

Comment 1

3 years ago
Typo: nsHttpConnectionMgr.cpp
(Assignee)

Comment 2

3 years ago
Created attachment 8565894 [details] [diff] [review]
1133846_fix_missing_arguments_v0.1.patch

Hi. I created the patch. Is it good?
(Reporter)

Comment 3

3 years ago
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 `?`.
(Assignee)

Comment 4

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

Updated

3 years ago
Attachment #8565894 - Flags: review?(mcmanus)
(Reporter)

Comment 5

3 years ago
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/
this is a try run
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=05a734c31733
(Assignee)

Comment 9

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

Comment 10

3 years ago
Oh sorry Patrick. I forget add a link to the bug.
Here it is https://bugzilla.mozilla.org/show_bug.cgi?id=1134220.
(Assignee)

Comment 11

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

Comment 13

3 years ago
Created attachment 8568425 [details] [diff] [review]
1133846_fix_missing_arguments_v0.3.patch
Attachment #8565894 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Attachment #8568425 - Flags: review+
(Assignee)

Updated

3 years ago
Attachment #8568425 - Flags: checkin+
(Assignee)

Comment 14

3 years ago
Hi Patrick! I've done additional steps. Am I right?
Flags: needinfo?(mcmanus)
thanks!
Flags: needinfo?(mcmanus)
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/fd1c4a3ba4e3
Assignee: nobody → tvaleev
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/fd1c4a3ba4e3
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox39: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
You need to log in before you can comment on or make changes to this bug.