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.
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.
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.
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.
Comment on attachment 260609 [details] Proposed Fix Seems like we’d move Logging into WTF rather than duplicating it to JavaScriptCore.
(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) ?
(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.
> 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.
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.