1353143 - Stop including prlog.h in mozilla/Logging.h

Status: RESOLVED FIXED

(Core :: XPCOM, enhancement)

Description

[Eric Rahm [:erahm]]

Currently we provide a fallback of MOZ_LOG that handles PRLogModuleInfo instances. This was intended to just be used to transition from PRLogModuleInfo, but now that we've converted virtually all usages of PRLogModuleInfo in gecko to LazyLogModule we should be able to remove the fallback and the #include "prlog.h"

Issues that will need to be dealt with:

*PR_LOGGING*

There are still some instances of |#ifdef PR_LOGGING|. Those just need to be deleted, logging is always enabled.

*PR_LogPrint*

There's some code that calls |PR_LogPrint| directly. Some of that can just be switched to printf_stderr, others can be switched to MOZ_LOG.

*PR_(BEGIN|END)_MACRO*

There are areas that indirectly rely on mozilla/Logging.h including prlog.h which includes prtypes.h. The only issue I found here was with |PR_BEGIN_MACRO| and |PR_END_MACRO|. I propose just using "do {" and "} while(0)" directly.

Comment 1

[Eric Rahm [:erahm]]

Also some code expects PR_ASSERT to be around, we can switch that to MOZ_ASSERT.

Comment 2

[Eric Rahm [:erahm]]

Attachment: Part 1: Explicitly include prlog.h in mtransport/logging.h

This updates mtransport logging to explicitly include prlog.h and defines
MOZ_MTLOG in terms of PR_LOG rather than MOZ_LOG when building outside of XUL.

This is a necessary step in preparation for removing prlog references from
mozilla/Logging.h.

Comment 3

[Nils Ohlmeier [:drno]]

Comment on attachment 8854236 [details] [diff] [review]
Part 1: Explicitly include prlog.h in mtransport/logging.h

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

LGTM

Comment 4

[Eric Rahm [:erahm]]

Attachment: Part 2: Remove usage of PR_LOGGING

Remove ifdefs that handle if logging is disabled, we always force enable it.

Comment 5

[Eric Rahm [:erahm]]

Attachment: Part 3: Switch PR_ASSERT usage to MOZ_ASSERT

Comment 6

[Eric Rahm [:erahm]]

Attachment: Part 4: Replace a usage of PR_*_MACRO with do/while

This file was expecting PR_BEGIN_MACRO and PR_END_MACRO defined in prtypes.h
to be included via prlog.h. In order to avoid more usage of prtypes.h we can
just use "do {" and "} while(0)" directly.

Comment 7

[Eric Rahm [:erahm]]

Attachment: Part 5: Replace direct usage of PR_LogPrint

This replaces the usage of |PR_LogPrint| with either |printf_stderr| or
|MOZ_LOG| where appropriate. |printf_stderr| is used where a logger is not
actually available or if log levels are not being used as expected.

Comment 8

[Eric Rahm [:erahm]]

Attachment: Part 6: Remove prlog.h from mozilla/Logging.h

This removes NSPR logging references from mozilla logging.

MozReview-Commit-ID: 8Zq2tbhdCv

Comment 9

[Nathan Froyd [:froydnj]]

Comment on attachment 8854246 [details] [diff] [review]
Part 3: Switch PR_ASSERT usage to MOZ_ASSERT

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

I have a slight preference for MOZ_ASSERT(false), but whatever.

Comment 10

[Nathan Froyd [:froydnj]]

Comment on attachment 8854248 [details] [diff] [review]
Part 5: Replace direct usage of PR_LogPrint

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

Many questions below.

::: gfx/2d/Logging.h
@@ +179,5 @@
>        printf_stderr("%s%s", aString.c_str(), aNoNewline ? "" : "\n");
>  #else
>  #if defined(MOZ_LOGGING)
>        if (MOZ_LOG_TEST(GetGFX2DLog(), PRLogLevelForLevel(aLevel))) {
> +        MOZ_LOG(GetGFX2DLog(), PRLogLevelForLevel(aLevel), ("%s%s", aString.c_str(), aNoNewline ? "" : "\n"));

Hm, do we really want to pass PRLogLevelForLevel into the mozilla:: logging framework?

::: gfx/layers/ipc/LayerTransactionParent.cpp
@@ +503,5 @@
>        }
>        mLayerManager->VisualFrameWarning(severity);
> +      printf_stderr("LayerTransactionParent::RecvUpdate transaction from process %d took %f ms",
> +                    OtherPid(),
> +                    latency.ToMilliseconds());

I'm inclined to say this should be MOZ_LAYERS_LOG like all the other logging statements in this file.  Not really sure, though; ask kats or botond?

::: layout/generic/nsFrame.cpp
@@ +10542,5 @@
>  {
>    if (NS_FRAME_LOG_TEST(sFrameLogModule, NS_FRAME_TRACE_CALLS)) {
>      char tagbuf[40];
>      GetTagName(this, mContent, sizeof(tagbuf), tagbuf);
> +    printf_stderr("%s: %s %s", tagbuf, aEnter ? "enter" : "exit", aMethod);

Likewise, I think these should all be NS_FRAME_TRACE.

::: layout/generic/nsFrame.h
@@ +35,5 @@
>  #ifdef DEBUG
>  #define NS_FRAME_LOG(_bit,_args)                                \
>    PR_BEGIN_MACRO                                                \
>      if (NS_FRAME_LOG_TEST(nsFrame::sFrameLogModule,_bit)) {  \
> +      printf_stderr _args; \

This seems like it should get printed to the appropriate log module.

Comment 11

[Nathan Froyd [:froydnj]]

Comment on attachment 8854249 [details] [diff] [review]
Part 6: Remove prlog.h from mozilla/Logging.h

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

Yay!

Comment 12

[Eric Rahm [:erahm]]

(In reply to Nathan Froyd [:froydnj] from comment #10)
> Comment on attachment 8854248 [details] [diff] [review]
> Part 5: Replace direct usage of PR_LogPrint
> 
> Review of attachment 8854248 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Many questions below.
> 
> ::: gfx/2d/Logging.h
> @@ +179,5 @@
> >        printf_stderr("%s%s", aString.c_str(), aNoNewline ? "" : "\n");
> >  #else
> >  #if defined(MOZ_LOGGING)
> >        if (MOZ_LOG_TEST(GetGFX2DLog(), PRLogLevelForLevel(aLevel))) {
> > +        MOZ_LOG(GetGFX2DLog(), PRLogLevelForLevel(aLevel), ("%s%s", aString.c_str(), aNoNewline ? "" : "\n"));
> 
> Hm, do we really want to pass PRLogLevelForLevel into the mozilla:: logging
> framework?

It's a bit of a misnomer, |PRLogLevelForLevel| returns a mozilla::LogLevel [1].

> ::: gfx/layers/ipc/LayerTransactionParent.cpp
> @@ +503,5 @@
> >        }
> >        mLayerManager->VisualFrameWarning(severity);
> > +      printf_stderr("LayerTransactionParent::RecvUpdate transaction from process %d took %f ms",
> > +                    OtherPid(),
> > +                    latency.ToMilliseconds());
> 
> I'm inclined to say this should be MOZ_LAYERS_LOG like all the other logging
> statements in this file.  Not really sure, though; ask kats or botond?

They use printf_stderr a few lines above and MOZ_LAYERS_LOG elsewhere in the function, it's confusing.

By using PR_LogPrint directly they're bypassing a log test which means it's an unconditional print, but it's also in an if block that's dependent on a pref being set, so I went with printf_stderr. Given the dependence on the pref I think printf_stderr is correct.

> ::: layout/generic/nsFrame.cpp
> @@ +10542,5 @@
> >  {
> >    if (NS_FRAME_LOG_TEST(sFrameLogModule, NS_FRAME_TRACE_CALLS)) {
> >      char tagbuf[40];
> >      GetTagName(this, mContent, sizeof(tagbuf), tagbuf);
> > +    printf_stderr("%s: %s %s", tagbuf, aEnter ? "enter" : "exit", aMethod);
> 
> Likewise, I think these should all be NS_FRAME_TRACE.

Will do.

> ::: layout/generic/nsFrame.h
> @@ +35,5 @@
> >  #ifdef DEBUG
> >  #define NS_FRAME_LOG(_bit,_args)                                \
> >    PR_BEGIN_MACRO                                                \
> >      if (NS_FRAME_LOG_TEST(nsFrame::sFrameLogModule,_bit)) {  \
> > +      printf_stderr _args; \
> 
> This seems like it should get printed to the appropriate log module.

They're using invalid log levels (note the bitwise AND in NS_FRAME_LOG_TEST [2]), so we can't use MOZ_LOG as it will crash when trying to stringify the log level [3].

[1] http://searchfox.org/mozilla-central/rev/381a7b8f8a78b481336cfa384cb0a5536e217e4a/gfx/2d/Logging.h#35-49
[2] http://searchfox.org/mozilla-central/rev/381a7b8f8a78b481336cfa384cb0a5536e217e4a/layout/generic/nsFrame.h#22-33
[3] http://searchfox.org/mozilla-central/rev/381a7b8f8a78b481336cfa384cb0a5536e217e4a/xpcom/base/Logging.cpp#94

Comment 13

[Eric Rahm [:erahm]]

(In reply to Eric Rahm [:erahm] from comment #12)
> (In reply to Nathan Froyd [:froydnj] from comment #10)
> > ::: layout/generic/nsFrame.cpp
> > @@ +10542,5 @@
> > >  {
> > >    if (NS_FRAME_LOG_TEST(sFrameLogModule, NS_FRAME_TRACE_CALLS)) {
> > >      char tagbuf[40];
> > >      GetTagName(this, mContent, sizeof(tagbuf), tagbuf);
> > > +    printf_stderr("%s: %s %s", tagbuf, aEnter ? "enter" : "exit", aMethod);
> > 
> > Likewise, I think these should all be NS_FRAME_TRACE.
> 
> Will do.

Oh right, NS_FRAME_TRACE calls TraceMsg so there's not much point (we'd just be adding more NS_FRAME_LOG_TEST calls).

Comment 14

[Eric Rahm [:erahm]]

Comment on attachment 8854248 [details] [diff] [review]
Part 5: Replace direct usage of PR_LogPrint

Given the explanations there probably don't need to be any changes, asking for review again.

Comment 15

[Nathan Froyd [:froydnj]]

(In reply to Eric Rahm [:erahm] from comment #12)
> > ::: gfx/2d/Logging.h
> > @@ +179,5 @@
> > >        printf_stderr("%s%s", aString.c_str(), aNoNewline ? "" : "\n");
> > >  #else
> > >  #if defined(MOZ_LOGGING)
> > >        if (MOZ_LOG_TEST(GetGFX2DLog(), PRLogLevelForLevel(aLevel))) {
> > > +        MOZ_LOG(GetGFX2DLog(), PRLogLevelForLevel(aLevel), ("%s%s", aString.c_str(), aNoNewline ? "" : "\n"));
> > 
> > Hm, do we really want to pass PRLogLevelForLevel into the mozilla:: logging
> > framework?
> 
> It's a bit of a misnomer, |PRLogLevelForLevel| returns a mozilla::LogLevel
> [1].

That's what I get for assuming things.  Maybe we should rename that once this all goes in, WDYT?

> > ::: gfx/layers/ipc/LayerTransactionParent.cpp
> > @@ +503,5 @@
> > >        }
> > >        mLayerManager->VisualFrameWarning(severity);
> > > +      printf_stderr("LayerTransactionParent::RecvUpdate transaction from process %d took %f ms",
> > > +                    OtherPid(),
> > > +                    latency.ToMilliseconds());
> > 
> > I'm inclined to say this should be MOZ_LAYERS_LOG like all the other logging
> > statements in this file.  Not really sure, though; ask kats or botond?
> 
> They use printf_stderr a few lines above and MOZ_LAYERS_LOG elsewhere in the
> function, it's confusing.
> 
> By using PR_LogPrint directly they're bypassing a log test which means it's
> an unconditional print, but it's also in an if block that's dependent on a
> pref being set, so I went with printf_stderr. Given the dependence on the
> pref I think printf_stderr is correct.

OK.

> > ::: layout/generic/nsFrame.h
> > @@ +35,5 @@
> > >  #ifdef DEBUG
> > >  #define NS_FRAME_LOG(_bit,_args)                                \
> > >    PR_BEGIN_MACRO                                                \
> > >      if (NS_FRAME_LOG_TEST(nsFrame::sFrameLogModule,_bit)) {  \
> > > +      printf_stderr _args; \
> > 
> > This seems like it should get printed to the appropriate log module.
> 
> They're using invalid log levels (note the bitwise AND in NS_FRAME_LOG_TEST
> [2]), so we can't use MOZ_LOG as it will crash when trying to stringify the
> log level [3].

Blah.  Fair enough.  Can you note that in a comment somewhere?

Comment 16

[Eric Rahm [:erahm]]

(In reply to Nathan Froyd [:froydnj] from comment #15)
> (In reply to Eric Rahm [:erahm] from comment #12)
> > > ::: gfx/2d/Logging.h
> > > @@ +179,5 @@
> > > >        printf_stderr("%s%s", aString.c_str(), aNoNewline ? "" : "\n");
> > > >  #else
> > > >  #if defined(MOZ_LOGGING)
> > > >        if (MOZ_LOG_TEST(GetGFX2DLog(), PRLogLevelForLevel(aLevel))) {
> > > > +        MOZ_LOG(GetGFX2DLog(), PRLogLevelForLevel(aLevel), ("%s%s", aString.c_str(), aNoNewline ? "" : "\n"));
> > > 
> > > Hm, do we really want to pass PRLogLevelForLevel into the mozilla:: logging
> > > framework?
> > 
> > It's a bit of a misnomer, |PRLogLevelForLevel| returns a mozilla::LogLevel
> > [1].
> 
> That's what I get for assuming things.  Maybe we should rename that once
> this all goes in, WDYT?

Sure, I'll file a follow up.

> > > ::: gfx/layers/ipc/LayerTransactionParent.cpp
> > > @@ +503,5 @@
> > > >        }
> > > >        mLayerManager->VisualFrameWarning(severity);
> > > > +      printf_stderr("LayerTransactionParent::RecvUpdate transaction from process %d took %f ms",
> > > > +                    OtherPid(),
> > > > +                    latency.ToMilliseconds());
> > > 
> > > I'm inclined to say this should be MOZ_LAYERS_LOG like all the other logging
> > > statements in this file.  Not really sure, though; ask kats or botond?
> > 
> > They use printf_stderr a few lines above and MOZ_LAYERS_LOG elsewhere in the
> > function, it's confusing.
> > 
> > By using PR_LogPrint directly they're bypassing a log test which means it's
> > an unconditional print, but it's also in an if block that's dependent on a
> > pref being set, so I went with printf_stderr. Given the dependence on the
> > pref I think printf_stderr is correct.
> 
> OK.
> 
> > > ::: layout/generic/nsFrame.h
> > > @@ +35,5 @@
> > > >  #ifdef DEBUG
> > > >  #define NS_FRAME_LOG(_bit,_args)                                \
> > > >    PR_BEGIN_MACRO                                                \
> > > >      if (NS_FRAME_LOG_TEST(nsFrame::sFrameLogModule,_bit)) {  \
> > > > +      printf_stderr _args; \
> > > 
> > > This seems like it should get printed to the appropriate log module.
> > 
> > They're using invalid log levels (note the bitwise AND in NS_FRAME_LOG_TEST
> > [2]), so we can't use MOZ_LOG as it will crash when trying to stringify the
> > log level [3].
> 
> Blah.  Fair enough.  Can you note that in a comment somewhere?

Good idea, I'll add a comment.

Comment 17

[Eric Rahm [:erahm]]

Attachment: Part 5: Replace direct usage of PR_LogPrint

Updated with a comment on why we MOZ_LOG cannot be used directly.

Comment 18

[Nathan Froyd [:froydnj]]

Comment on attachment 8854551 [details] [diff] [review]
Part 5: Replace direct usage of PR_LogPrint

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

Thanks.

Comment 19

[Eric Rahm [:erahm]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a3028b822e1a154d528ec6c7d9dca4fd239a8c5f
Bug 1353143 - Part 1: Explicitly include prlog.h in mtransport/logging.h. r=drno

https://hg.mozilla.org/integration/mozilla-inbound/rev/32a10e1b56cc20c0490f101752a32e49dab4bc5c
Bug 1353143 - Part 2: Remove usage of PR_LOGGING. r=froydnj

https://hg.mozilla.org/integration/mozilla-inbound/rev/03a873ca3963aa48938bfbc65f0eb4113e875873
Bug 1353143 - Part 3: Switch PR_ASSERT usage to MOZ_ASSERT. r=froydnj

https://hg.mozilla.org/integration/mozilla-inbound/rev/94250fe023460aabe7831695293784800f0d476a
Bug 1353143 - Part 4: Replace a usage of PR_*_MACRO with do/while. r=froydnj

https://hg.mozilla.org/integration/mozilla-inbound/rev/c6228238bdcb2db487663051cabb147ab85da5bd
Bug 1353143 - Part 5: Replace direct usage of PR_LogPrint. r=froydnj

https://hg.mozilla.org/integration/mozilla-inbound/rev/c8e078d00d470a4da27a61425404de90782d9943
Bug 1353143 - Part 6: Remove prlog.h from mozilla/Logging.h. r=froydnj

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a3028b822e1a
https://hg.mozilla.org/mozilla-central/rev/32a10e1b56cc
https://hg.mozilla.org/mozilla-central/rev/03a873ca3963
https://hg.mozilla.org/mozilla-central/rev/94250fe02346
https://hg.mozilla.org/mozilla-central/rev/c6228238bdcb
https://hg.mozilla.org/mozilla-central/rev/c8e078d00d47

Comment 21

[Jorg K (CEST = GMT+2)]

A little heads-up for your TB friends would have been nice here :-(

Comment 22

[Eric Rahm [:erahm]]

(In reply to Jorg K (GMT+2) from comment #21)
> A little heads-up for your TB friends would have been nice here :-(

Sorry about that! I realized after landing on inbound it might cause TB bustage and put a PoC patch up in bug 1222244.