Bug 148802

Summary:

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

Product:

WebKit

Reporter:

Blaze Burg <bburg>

Component:

JavaScriptCore

Assignee:

Blaze Burg <bburg>

Status:

NEW

Severity:

Normal

CC:

basile_clement, beidson, commit-queue, darin, fpizlo, ggaren, joepeck, msaboff, saam

Priority:

Version:

WebKit Nightly Build

Hardware:

All

OS:

All

Bug Depends on:

Bug Blocks:

147066, 149074

Attachments:

Description	Flags
Proposed Fix	beidson: review-

Blaze Burg

Reported 2015-09-04 10:09:27 PDT

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.

Attachments
Proposed Fix (36.68 KB, patch) 2015-09-04 13:13 PDT, Blaze Burg	beidson: review-	Details Formatted Diff Diff
View All Add attachment proposed patch, testcase, etc.

Blaze Burg

Comment 1 2015-09-04 13:13:04 PDT

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

Comment 2 2015-09-04 13:15:59 PDT

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

Comment 3 2015-09-11 08:57:12 PDT

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 4 2015-09-11 09:02:05 PDT

Comment on attachment 260609 [details] Proposed Fix Seems like we’d move Logging into WTF rather than duplicating it to JavaScriptCore.

Blaze Burg

Comment 5 2015-09-11 09:28:32 PDT

(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

Comment 6 2015-09-11 10:13:18 PDT

(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

Comment 7 2015-09-11 10:27:13 PDT

> 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 8 2017-04-24 19:09:26 PDT

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.

Note You need to log in before you can comment on or make changes to this bug.