Summary: | [WPE][GTK] Provide an implementation of PAL::logLevelString() | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Adrian Perez <aperez> | ||||||||
Component: | WPE WebKit | Assignee: | Adrian Perez <aperez> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | annulen, bugs-noreply, cgarcia, clopez, commit-queue, mcatanzaro, mmaxfield, rniwa, webkit-bug-importer, zan | ||||||||
Priority: | P2 | ||||||||||
Version: | Other | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Bug Depends on: | |||||||||||
Bug Blocks: | 178276 | ||||||||||
Attachments: |
|
Description
Adrian Perez
2017-10-13 08:15:11 PDT
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. |