Bug 178276 – [WPE][GTK] Unify/simplify/improve PAL::logLevelString, WebCore::logLevelString, and WebKit::logLevelString

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Adrian Perez]

We currently have:

 - PAL::logLevelString (Source/WebCore/PAL/pal/unix/LoggingUnix.cpp)
 - WebCore::logLevelString (Source/WebCore/platform/unix/LoggingUnix.cpp)
 - WebKit::logLevelString (Source/WebKit/Platform/unix/LoggingUnix.cpp)

Essentially, the three do the same, which is:

 1. Picking the value from the WEBKIT_DEBUG environment variable.
 2. If the variable is not present, return an empty string.
 3. If the variable is defined:
     3.a. Warn with “WTFLogAlways” that some logging is only available
          in debug builds.
     3.b. Prepend “NotYetImplemented,” to the obtained value (except
          for WebKit::logLevelString).
     3.c. Return the value.

During a discussion with Konstantin Tokarev and Carlos López we came up
with the following possible improvements/doubts:

 - Unifying all of them, by making both WebCore::logLevelString and
   WebKit::logLevelString just call PAL::logLevelString.
 - Removing the message emitted with WTFLogAlways().
 - Does it make sense to always prepend “NotYetImplemented,”? Why was it
   not being done for the WebKit version?

[Adrian Perez]

(In reply to Adrian Perez from comment #0)
> We currently have:
> 
>  - PAL::logLevelString (Source/WebCore/PAL/pal/unix/LoggingUnix.cpp)

This instance still has some custom code, it should be changed to call WTF::logLevelString() -- read on below.

>  - WebCore::logLevelString (Source/WebCore/platform/unix/LoggingUnix.cpp)
>  - WebKit::logLevelString (Source/WebKit/Platform/unix/LoggingUnix.cpp)

These two instances have been changed to call WTF::logLevelString() in 260601@main 

> Essentially, the three do the same, which is:
> 
>  1. Picking the value from the WEBKIT_DEBUG environment variable.
>  2. If the variable is not present, return an empty string.
>  3. If the variable is defined:
>      3.a. Warn with “WTFLogAlways” that some logging is only available
>           in debug builds.
>      3.b. Prepend “NotYetImplemented,” to the obtained value (except
>           for WebKit::logLevelString).
>      3.c. Return the value.
> 
> During a discussion with Konstantin Tokarev and Carlos López we came up
> with the following possible improvements/doubts:
> 
>  - Unifying all of them, by making both WebCore::logLevelString and
>    WebKit::logLevelString just call PAL::logLevelString.

This was partially done in 260601@main -- except for the PAL one, as outlined above.

>  - Removing the message emitted with WTFLogAlways().
>  - Does it make sense to always prepend “NotYetImplemented,”? Why was it
>    not being done for the WebKit version?

Still no idea about these two.

[Adrian Perez]

(In reply to Adrian Perez from comment #1)
> (In reply to Adrian Perez from comment #0)
> > We currently have:
> > 
> >  - PAL::logLevelString (Source/WebCore/PAL/pal/unix/LoggingUnix.cpp)
> 
> This instance still has some custom code, it should be changed to call
> WTF::logLevelString() -- read on below.

Actually, nothing seems to call PAL::logLevelString() presently. I am looking into removing it in bug #295481

> >  - WebCore::logLevelString (Source/WebCore/platform/unix/LoggingUnix.cpp)
> >  - WebKit::logLevelString (Source/WebKit/Platform/unix/LoggingUnix.cpp)
> 
> These two instances have been changed to call WTF::logLevelString() in
> 260601@main 

And because those two have been simplified already, once the patch for #295481 lands, we can close this one as well.

[Adrian Perez]

(In reply to Adrian Perez from comment #2)
> (In reply to Adrian Perez from comment #1)
> > (In reply to Adrian Perez from comment #0)
> > > We currently have:
> > > 
> > >  - PAL::logLevelString (Source/WebCore/PAL/pal/unix/LoggingUnix.cpp)
> > 
> > This instance still has some custom code, it should be changed to call
> > WTF::logLevelString() -- read on below.
> 
> Actually, nothing seems to call PAL::logLevelString() presently. I am
> looking into removing it in bug #295481

The patch for that landed, so I think we can consider this issue solved.