Closed
Bug 1162336
Opened 9 years ago
Closed 9 years ago
Remove usage of |#ifdef PR_LOGGING| from netwerk
Categories
(Core :: Networking, defect)
Core
Networking
Tracking
()
RESOLVED
FIXED
mozilla40
Tracking | Status | |
---|---|---|
firefox40 | --- | fixed |
People
(Reporter: erahm, Assigned: erahm)
References
Details
Attachments
(2 files)
111.09 KB,
patch
|
froydnj
:
review+
|
Details | Diff | Splinter Review |
17.46 KB,
patch
|
froydnj
:
review+
|
Details | Diff | Splinter Review |
In bug 1161238 we plan on removing |--disable-logging| which makes |#ifdef PR_LOGGING| redundant.
Assignee | ||
Comment 1•9 years ago
|
||
PR_LOGGING is now always defined, we can remove #ifdefs checking for it.
Attachment #8602440 -
Flags: review?(nfroyd)
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → erahm
Status: NEW → ASSIGNED
Assignee | ||
Comment 2•9 years ago
|
||
Check that logging is enabled before performing potentially expensive operations.
Attachment #8602441 -
Flags: review?(nfroyd)
Assignee | ||
Comment 3•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e249ee55a442
Assignee | ||
Comment 4•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=be9b7577631b
Comment 5•9 years ago
|
||
Comment on attachment 8602440 [details] [diff] [review] Part 1: Remove instances of #ifdef PR_LOGGING in netwerk Review of attachment 8602440 [details] [diff] [review]: ----------------------------------------------------------------- ::: netwerk/base/nsPACMan.cpp @@ -25,5 @@ > using namespace mozilla; > using namespace mozilla::net; > > -#if defined(PR_LOGGING) > -#endif :)
Attachment #8602440 -
Flags: review?(nfroyd) → review+
Comment 6•9 years ago
|
||
Comment on attachment 8602441 [details] [diff] [review] Part 2: Wrap expensive calls in PR_LOG_TEST Review of attachment 8602441 [details] [diff] [review]: ----------------------------------------------------------------- Honza, please see the comments for the particular feedback requested here. ::: netwerk/cache2/CacheFileContextEvictor.cpp @@ +248,5 @@ > > nsAutoCString path; > + if (LOG_ENABLED()) { > + file->GetNativePath(path); > + } I tend to think moving this block down into the NS_WARN_IF block: if (NS_WARN_IF(...)) { if (LOG_ENABLED()) { nsAutoCString path; ... LOG((...)); } } is better, but I can also see that one might want to break on the OpenNSPRFileDesc call conditionally on the value of path. So I'm going to ask Honza about this one... @@ +287,5 @@ > > nsAutoCString path; > + if (LOG_ENABLED()) { > + file->GetNativePath(path); > + } ...and this one, since this is code he's familiar with.
Attachment #8602441 -
Flags: review?(nfroyd)
Attachment #8602441 -
Flags: review+
Attachment #8602441 -
Flags: feedback?(honzab.moz)
Assignee | ||
Comment 7•9 years ago
|
||
(In reply to Nathan Froyd [:froydnj] [:nfroyd] from comment #6) > Comment on attachment 8602441 [details] [diff] [review] > Part 2: Wrap expensive calls in PR_LOG_TEST > > Review of attachment 8602441 [details] [diff] [review]: > ----------------------------------------------------------------- > > Honza, please see the comments for the particular feedback requested here. > > ::: netwerk/cache2/CacheFileContextEvictor.cpp > @@ +248,5 @@ > > > > nsAutoCString path; > > + if (LOG_ENABLED()) { > > + file->GetNativePath(path); > > + } > > I tend to think moving this block down into the NS_WARN_IF block: > > if (NS_WARN_IF(...)) { > if (LOG_ENABLED()) { > nsAutoCString path; > ... > LOG((...)); > } > } > > is better, but I can also see that one might want to break on the > OpenNSPRFileDesc call conditionally on the value of path. So I'm going to > ask Honza about this one... > I originally did that, but it broke the build. |path| is used again further down in the function (which of course is not obvious as we don't have expandable context in splinter). > @@ +287,5 @@ > > > > nsAutoCString path; > > + if (LOG_ENABLED()) { > > + file->GetNativePath(path); > > + } > > ...and this one, since this is code he's familiar with. Same deal.
Comment 8•9 years ago
|
||
Doh, I had meant to look at the file, but apparently forgot. OK, these are fine, then.
Updated•9 years ago
|
Attachment #8602441 -
Flags: feedback?(honzab.moz)
Comment 9•9 years ago
|
||
Actually, scratch that. Since we've had logging globally enabled for some time, this doesn't seem to be a problem, so let's forgo the conditionally-initialized variables in this case. The other cases are straightforward code, so let's leave them as-is.
Assignee | ||
Comment 10•9 years ago
|
||
Will remove those two changes before I land.
Assignee | ||
Comment 11•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/0170a6c2a5be https://hg.mozilla.org/integration/mozilla-inbound/rev/8952a7fa4050
Both commits backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/a74ede09893f for breaking at least Windows builds: https://treeherder.mozilla.org/logviewer.html#?job_id=9657250&repo=mozilla-inbound
Flags: needinfo?(erahm)
Assignee | ||
Comment 13•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=01f2a7e1a2bd
Assignee | ||
Comment 14•9 years ago
|
||
It would appear netwerk/system/win32/nsNotifyAddrListener.cpp was not including prlog.h, pushed a few windows try builds to verify that's it.
Flags: needinfo?(erahm)
Assignee | ||
Comment 15•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/0313bc7f6052 https://hg.mozilla.org/integration/mozilla-inbound/rev/f326f7d6ff52
https://hg.mozilla.org/mozilla-central/rev/0313bc7f6052 https://hg.mozilla.org/mozilla-central/rev/f326f7d6ff52
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
You need to log in
before you can comment on or make changes to this bug.
Description
•