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.
Created attachment 323671 [details] Patch
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 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&
(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.
(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()”.
Created attachment 323688 [details] Patch
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);
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 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 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
(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 on attachment 323691 [details] Patch Clearing flags on attachment: 323691 Committed r223296: <https://trac.webkit.org/changeset/223296>
All reviewed patches have been landed. Closing bug.