Remove usage of |#ifdef PR_LOGGING| from netwerk

RESOLVED FIXED in Firefox 40

Status

()

Core
Networking
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: erahm, Assigned: erahm)

Tracking

Trunk
mozilla40
Points:
---

Firefox Tracking Flags

(firefox40 fixed)

Details

Attachments

(2 attachments)

In bug 1161238 we plan on removing |--disable-logging| which makes |#ifdef PR_LOGGING| redundant.
Created attachment 8602440 [details] [diff] [review]
Part 1: Remove instances of #ifdef PR_LOGGING in netwerk

PR_LOGGING is now always defined, we can remove #ifdefs checking for it.
Attachment #8602440 - Flags: review?(nfroyd)
Assignee: nobody → erahm
Status: NEW → ASSIGNED
Created attachment 8602441 [details] [diff] [review]
Part 2: Wrap expensive calls in PR_LOG_TEST

Check that logging is enabled before performing potentially expensive
operations.
Attachment #8602441 - Flags: review?(nfroyd)
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 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)
(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.
Doh, I had meant to look at the file, but apparently forgot.  OK, these are fine, then.
Attachment #8602441 - Flags: feedback?(honzab.moz)
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.
Will remove those two changes before I land.
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)
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)
https://hg.mozilla.org/mozilla-central/rev/0313bc7f6052
https://hg.mozilla.org/mozilla-central/rev/f326f7d6ff52
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox40: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
You need to log in before you can comment on or make changes to this bug.