Bug 178263 – [WPE][GTK] Provide an implementation of PAL::logLevelString()

• Component: WPE WebKit
• Status: RESOLVED FIXED

[Adrian Perez]

The debug builds of the WPE and GTK+ port are currently broken due the
“PAL::logLevelString” symbol being missing. For example:

   https://build.webkit.org/builders/GTK%20Linux%2064-bit%20Debug%20(Build)/builds/5976/steps/compile-webkit/logs/stdio

We need to provide an implementation for it.

[Adrian Perez]

Created attachment 323671 [details]
Patch

[Konstantin Tokarev]

Comment on attachment 323671 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=323671&action=review

> Source/WebCore/PAL/pal/unix/LoggingUnix.cpp:44
> +        return String("NotYetImplemented,") + logEnv;

ASCIILiteral("NotYetImplemented,") may be more appropriate here

[Konstantin Tokarev]

Comment on attachment 323671 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=323671&action=review

> Source/WebCore/PAL/pal/unix/LoggingUnix.cpp:47
> +    return emptyString();

I think it should be just String() because we return String, not const String&

[Adrian Perez]

(In reply to Konstantin Tokarev from comment #2)
> Comment on attachment 323671 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=323671&action=review
> 
> > Source/WebCore/PAL/pal/unix/LoggingUnix.cpp:44
> > +        return String("NotYetImplemented,") + logEnv;
> 
> ASCIILiteral("NotYetImplemented,") may be more appropriate here

ASCIILiteral does not have an overloadded “+” with the RHS being a char*.
We can use the following, at the cost of having one more temporary:

   return ASCIILiteral("NotYetImplemented,") + String(logEnv);

I guess the extra temporary is okay and not a big deal.

[Adrian Perez]

(In reply to Konstantin Tokarev from comment #3)
> Comment on attachment 323671 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=323671&action=review
> 
> > Source/WebCore/PAL/pal/unix/LoggingUnix.cpp:47
> > +    return emptyString();
> 
> I think it should be just String() because we return String, not const
> String&

Sure, I'll change this and re-upload. Probably later on we would also
want to change “Source/WebCore/platform/unix/LoggingUnix.cpp” and
“Source/WebKit/Platform/unix/LoggingUnix.cpp” to use “return String()”.

[Adrian Perez]

Created attachment 323688 [details]
Patch

[Konstantin Tokarev]

Comment on attachment 323671 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=323671&action=review

>>> Source/WebCore/PAL/pal/unix/LoggingUnix.cpp:44
>>> +        return String("NotYetImplemented,") + logEnv;
>> 
>> ASCIILiteral("NotYetImplemented,") may be more appropriate here
> 
> ASCIILiteral does not have an overloadded “+” with the RHS being a char*.
> We can use the following, at the cost of having one more temporary:
> 
>    return ASCIILiteral("NotYetImplemented,") + String(logEnv);
> 
> I guess the extra temporary is okay and not a big deal.

Oh, indeed. Looks like the next approach is better:

    makeString("NotYetImplemented,", logEnv);

[Adrian Perez]

Created attachment 323691 [details]
Patch

Updated to use the WEBKIT_DEBUG environment variable, as
per discussion with Konstantin on IRC, it seems better to have a single
variable control logging instead of having to remember to set different
ones.

[Carlos Alberto Lopez Perez]

Comment on attachment 323671 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=323671&action=review

> Source/WebCore/PAL/pal/unix/LoggingUnix.cpp:41
> +#if defined(NDEBUG)
> +        WTFLogAlways("WEBCORE_DEBUG is not empty, but this is a release build. Notice that many log messages will only appear in a debug build.");
> +#endif

Not sure about this. I think it only adds extra noise.

As you can now build release with logging enabled by simple setting on your environment CXXFLAGS="-DLOG_DISABLED=0" (I do that)

I was even looking forward to do this by default on developer builds at bug 177459

[Carlos Alberto Lopez Perez]

Comment on attachment 323691 [details]
Patch

r=me due to the urgency of this (build currently broken).
Please open a new bug (or don't close this one) to address the remaining comments

[Adrian Perez]

(In reply to Carlos Alberto Lopez Perez from comment #10)
> Comment on attachment 323691 [details]
> Patch
> 
> r=me due to the urgency of this (build currently broken).
> Please open a new bug (or don't close this one) to address the remaining
> comments

I have filed bug #178276 as follow-up to try to unify and improve the
three logLevelString() functions.

[WebKit Commit Bot]

Comment on attachment 323691 [details]
Patch

Clearing flags on attachment: 323691

Committed r223296: <https://trac.webkit.org/changeset/223296>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.