Logging should be configurable using human-readable channel names rather than crazy bitmasks

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Mark Rowe (bdash)]

Having to do something like 'defaults write com.apple.Safari WebCoreLogLevel "0xc000"' to enable logging is crazy. I always have to look up the numeric values and then do some math to or them together. Why not "defaults write com.apple.Safari WebCoreLogLevel history,pagecache"?

[Mark Rowe (bdash)]

Created attachment 207370 [details]
Initial work in progress

I'm attaching my initial work in progress. It updates each of the three projects to support a comma separated list of logging channels to enable. It also supports a special "all" channel to enable everything, and provides the ability to disable particular channels by prefixing their name with a -.

Work that remains is:
1) Fixing all of the other ports.
2) Fixing binary-compatibility since Safari makes use of WTFLog and this change has altered the shape of the WTFLogChannel structure.

[WebKit Commit Bot]

Attachment 207370 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WTF/wtf/Assertions.cpp', u'Source/WTF/wtf/Assertions.h', u'Source/WTF/wtf/RefCountedLeakCounter.cpp', u'Source/WebCore/platform/Logging.cpp', u'Source/WebCore/platform/Logging.h', u'Source/WebCore/platform/mac/LoggingMac.mm', u'Source/WebKit/mac/Misc/WebKitLogging.h', u'Source/WebKit/mac/Misc/WebKitLogging.m', u'Source/WebKit/mac/WebView/WebPreferenceKeysPrivate.h', u'Source/WebKit2/Platform/Logging.cpp', u'Source/WebKit2/Platform/Logging.h', u'Source/WebKit2/Platform/mac/Logging.mac.mm']" exit_code: 1
Source/WTF/wtf/Assertions.cpp:442:  Declaration has space between type name and * in WTFLogChannel *channel  [whitespace/declaration] [3]
Source/WebCore/platform/Logging.cpp:37:  JOIN_LOG_CHANNEL_WITH_PREFIX is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebKit2/Platform/Logging.cpp:37:  JOIN_LOG_CHANNEL_WITH_PREFIX is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebKit2/Platform/Logging.h:53:  JOIN_LOG_CHANNEL_WITH_PREFIX is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebCore/platform/Logging.h:78:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Source/WebCore/platform/Logging.h:71:  JOIN_LOG_CHANNEL_WITH_PREFIX is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 6 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Mark Rowe (bdash)]

I should also note that I renamed the user default names that are used to control the logging. WebCoreLogLevel is now WebCoreLogging, and similar for the other defaults. I did this to prevent people from having to reset their logging defaults when switching between working on versions of WebKit before and after this change.

[Alexey Proskuryakov]

This is so cool!

[Mark Rowe (bdash)]

Created attachment 207580 [details]
Patch v1

I've made an effort to update all of the ports but haven't compiled anything but Mac. I'm guessing there'll be errors in some, if not all, of the ports. They may not be caught by EWS unless every platform builds with logging enabled.

[WebKit Commit Bot]

Attachment 207580 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WTF/ChangeLog', u'Source/WTF/wtf/Assertions.cpp', u'Source/WTF/wtf/Assertions.h', u'Source/WTF/wtf/RefCountedLeakCounter.cpp', u'Source/WebCore/ChangeLog', u'Source/WebCore/GNUmakefile.list.am', u'Source/WebCore/Target.pri', u'Source/WebCore/WebCore.xcodeproj/project.pbxproj', u'Source/WebCore/platform/InitializeLogging.h', u'Source/WebCore/platform/Logging.cpp', u'Source/WebCore/platform/Logging.h', u'Source/WebCore/platform/blackberry/LoggingBlackBerry.cpp', u'Source/WebCore/platform/efl/LoggingEfl.cpp', u'Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp', u'Source/WebCore/platform/gtk/LoggingGtk.cpp', u'Source/WebCore/platform/mac/LoggingMac.mm', u'Source/WebCore/platform/qt/LoggingQt.cpp', u'Source/WebCore/platform/win/LoggingWin.cpp', u'Source/WebKit/blackberry/Api/BlackBerryGlobal.cpp', u'Source/WebKit/blackberry/ChangeLog', u'Source/WebKit/efl/ChangeLog', u'Source/WebKit/efl/ewk/ewk_main.cpp', u'Source/WebKit/gtk/ChangeLog', u'Source/WebKit/gtk/webkit/webkitglobals.cpp', u'Source/WebKit/mac/ChangeLog', u'Source/WebKit/mac/Misc/WebKitLogging.h', u'Source/WebKit/mac/Misc/WebKitLogging.m', u'Source/WebKit/mac/WebView/WebPreferenceKeysPrivate.h', u'Source/WebKit/mac/WebView/WebView.mm', u'Source/WebKit/qt/ChangeLog', u'Source/WebKit/qt/WebCoreSupport/InitWebCoreQt.cpp', u'Source/WebKit/win/ChangeLog', u'Source/WebKit/win/WebKitLogging.cpp', u'Source/WebKit/win/WebKitLogging.h', u'Source/WebKit/win/WebView.cpp', u'Source/WebKit2/ChangeLog', u'Source/WebKit2/NetworkProcess/NetworkProcess.cpp', u'Source/WebKit2/Platform/Logging.cpp', u'Source/WebKit2/Platform/Logging.h', u'Source/WebKit2/Platform/efl/LoggingEfl.cpp', u'Source/WebKit2/Platform/gtk/LoggingGtk.cpp', u'Source/WebKit2/Platform/mac/Logging.mac.mm', u'Source/WebKit2/Platform/qt/LoggingQt.cpp', u'Source/WebKit2/Shared/WebKit2Initialize.cpp', u'Source/WebKit2/UIProcess/WebContext.cpp']" exit_code: 1
Source/WebCore/platform/Logging.h:78:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Source/WebCore/platform/Logging.h:71:  JOIN_LOG_CHANNEL_WITH_PREFIX is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebCore/platform/Logging.cpp:38:  JOIN_LOG_CHANNEL_WITH_PREFIX is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebKit/win/WebKitLogging.cpp:32:  JOIN_LOG_CHANNEL_WITH_PREFIX is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebKit2/Platform/Logging.cpp:37:  JOIN_LOG_CHANNEL_WITH_PREFIX is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebKit2/Platform/Logging.h:53:  JOIN_LOG_CHANNEL_WITH_PREFIX is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebKit/win/WebKitLogging.h:64:  JOIN_LOG_CHANNEL_WITH_PREFIX is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 7 in 41 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Mark Rowe (bdash)]

The fact that the EWSs are all green suggests they're building with logging disabled. Since I'm going to pre-announce the change on webkit-dev to let folks know about the change to how they will enable logging, I'll also ask for people on non-Mac ports to give it a whirl and let me know about any build issues that come up. Once it's all reviewed, that is.

[Sam Weinig]

Comment on attachment 207580 [details]
Patch v1

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

> Source/WTF/wtf/Assertions.cpp:440
> +    String nameString = name;

It seems unfortunate to create String just to do equalIgnoringCase.  Can we add an override that takes only const char*s?

> Source/WTF/wtf/Assertions.cpp:463
> +        String component = components[i];

It might be nice to allow for whitespace, thoughts?

> Source/WTF/wtf/Assertions.cpp:476
> +        if (WTFLogChannel* channel = WTFLogChannelByName(channels, count, component.utf8().data()))

If we don't fix the above issue, it seems unfortunate to to pull out the data only to construct a String in WTFLogChannelByName.

[Mark Rowe (bdash)]

(In reply to comment #8)
> (From update of attachment 207580 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=207580&action=review
> 
> > Source/WTF/wtf/Assertions.cpp:440
> > +    String nameString = name;
> 
> It seems unfortunate to create String just to do equalIgnoringCase.  Can we add an override that takes only const char*s?

It looks like we have a version of equalIgnoringCase that takes two const LChar* arguments and a length. I'll adjust this method to use that instead.

> 
> > Source/WTF/wtf/Assertions.cpp:463
> > +        String component = components[i];
> 
> It might be nice to allow for whitespace, thoughts?

I'm not sure that it's necessary. If it's something that people find confusing we could add support for it later on.

[Build Bot]

Comment on attachment 207580 [details]
Patch v1

Attachment 207580 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/1295205

[Mark Rowe (bdash)]

Landed in r153736.

Will fix any build breakages as I see them.