Bug 148802 – Add support for LOG(level, ...) in JavaScriptCore parts of Web Inspector code

• Component: JavaScriptCore
• Status: NEW

[Blaze Burg]

I want to add logging to both JSC and WebCore parts of Web Inspector for tracing things like frontends connecting/disconnecting, instrumentation being plugged/unplugged, etc. But, Logging.h (which allows toggling logging with NSUserDefaults) is only available in WebCore and WebKit/2. This patch would copy Logging.h down to JSC, and initialize logging channels in InitializeThreading.cpp alongside Options and other stuff. Then, it would be possible to use this log channel from JSC or WebCore, and flip one switch to enable all Inspector log tracing regardless of whether it's in JSC, WebCore, WK2.

Logging code gets compiled out if LOG_DISABLED is defined; by default, LOG_DISABLED == NDEBUG. This should not have any performance impact.

[Blaze Burg]

Created attachment 260609 [details]
Proposed Fix

This will probably require a few rounds with EWS, due to adding multiple platform implementations and a new forwarding header directory.

[WebKit Commit Bot]

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


ERROR: Source/JavaScriptCore/tools/Logging.cpp:38:  JOIN_LOG_CHANNEL_WITH_PREFIX is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 1 in 27 files


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

[Blaze Burg]

I used this patch in a branch to help diagnose https://webkit.org/b/149006. It was very helpful. I'd love to be able to commit some of the lifecycle logging so I don't have to re-add it for the next bug.

Can someone review? I have not heard any objections... runtime memory use will increase a little bit in !LOG_DISABLED builds (cf. https://bugs.webkit.org/show_bug.cgi?id=143506) but not really relevant for debug builds.

[Darin Adler]

Comment on attachment 260609 [details]
Proposed Fix

Seems like we’d move Logging into WTF rather than duplicating it to JavaScriptCore.

[Blaze Burg]

(In reply to comment #4)
> Comment on attachment 260609 [details]
> Proposed Fix
> 
> Seems like we’d move Logging into WTF rather than duplicating it to
> JavaScriptCore.

The way it's set up now, there are copies of Logging in JSC, WebCore, WebKit, WebKit2, and some in Internal. Each has a different NSUserDefaults key.

So, which ones are you proposing should be combined? And, should we preserve separate defaults keys, i.e., WebKitLogging, WebCoreLogging, and so on, or just have one (likely WebKitLogging) ?

[Darin Adler]

(In reply to comment #5)
> (In reply to comment #4)
> > Comment on attachment 260609 [details]
> > Proposed Fix
> > 
> > Seems like we’d move Logging into WTF rather than duplicating it to
> > JavaScriptCore.
> 
> The way it's set up now, there are copies of Logging in JSC, WebCore,
> WebKit, WebKit2, and some in Internal. Each has a different NSUserDefaults
> key.
> 
> So, which ones are you proposing should be combined? And, should we preserve
> separate defaults keys, i.e., WebKitLogging, WebCoreLogging, and so on, or
> just have one (likely WebKitLogging) ?

(Re: "some in Internal", logging outside the WebKit project is a separate issue and probably shouldn’t be discussed here.)

Within the WebKit project, I think it would be good to avoid code duplication and to be able to improve the logging machinery easily once. But I suppose it’s OK if we want each framework to have a separate key and a separate set of channels.

Maybe that’s already how this works and my objection is really no objection at all.

[Blaze Burg]

> Within the WebKit project, I think it would be good to avoid code
> duplication and to be able to improve the logging machinery easily once. But
> I suppose it’s OK if we want each framework to have a separate key and a
> separate set of channels.
> 
> Maybe that’s already how this works and my objection is really no objection
> at all.

I think you are correct.

wtf/Assertions.cpp (WTFInitializeLogChannelStatesFromString) is where all the interesting machinery is located. The rest is a lot of duplicated x-macros, which I suppose could be centralized, but it doesn't buy much in terms of maintainability.

The separate Logging.h files define the logging channels, prefix, and the static WTFLogChannel* logChannels[]. These are currently initialized as needed; so WebKit2 channels aren't reserved by JSC's Logging.h. Maybe each framework could have its own Group of channels that get set up at the same time, but this seems like over-engineering.

[Brady Eidson]

Comment on attachment 260609 [details]
Proposed Fix

This patch has been pending review since 2015 with no recent activity.
It seems unlikely that it would even still apply to trunk in its current form.

Clearing from the review queue.

Feel free to update and resubmit if the patch is still relevant.