Bug 178263 - [WPE][GTK] Provide an implementation of PAL::logLevelString()
Summary: [WPE][GTK] Provide an implementation of PAL::logLevelString()
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WPE WebKit (show other bugs)
Version: Other
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Adrian Perez
URL:
Keywords:
Depends on:
Blocks: 178276
  Show dependency treegraph
 
Reported: 2017-10-13 08:15 PDT by Adrian Perez
Modified: 2017-10-13 12:05 PDT (History)
10 users (show)

See Also:


Attachments
Patch (4.20 KB, patch)
2017-10-13 08:20 PDT, Adrian Perez
no flags Details | Formatted Diff | Diff
Patch (4.21 KB, patch)
2017-10-13 10:10 PDT, Adrian Perez
no flags Details | Formatted Diff | Diff
Patch (4.21 KB, patch)
2017-10-13 10:23 PDT, Adrian Perez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adrian Perez 2017-10-13 08:15:11 PDT
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.
Comment 1 Adrian Perez 2017-10-13 08:20:48 PDT
Created attachment 323671 [details]
Patch
Comment 2 Konstantin Tokarev 2017-10-13 08:41:01 PDT
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
Comment 3 Konstantin Tokarev 2017-10-13 08:42:47 PDT
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&
Comment 4 Adrian Perez 2017-10-13 10:03:19 PDT
(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.
Comment 5 Adrian Perez 2017-10-13 10:09:26 PDT
(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()”.
Comment 6 Adrian Perez 2017-10-13 10:10:41 PDT
Created attachment 323688 [details]
Patch
Comment 7 Konstantin Tokarev 2017-10-13 10:13:56 PDT
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);
Comment 8 Adrian Perez 2017-10-13 10:23:07 PDT
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.
Comment 9 Carlos Alberto Lopez Perez 2017-10-13 10:28:37 PDT
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
Comment 10 Carlos Alberto Lopez Perez 2017-10-13 10:36:29 PDT
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
Comment 11 Adrian Perez 2017-10-13 11:52:01 PDT
(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.
Comment 12 WebKit Commit Bot 2017-10-13 12:04:27 PDT
Comment on attachment 323691 [details]
Patch

Clearing flags on attachment: 323691

Committed r223296: <https://trac.webkit.org/changeset/223296>
Comment 13 WebKit Commit Bot 2017-10-13 12:04:29 PDT
All reviewed patches have been landed.  Closing bug.