Bug 171523 – Allow PAL to log messages

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Myles C. Maxfield]

[PAL] Migrate WebCore/platform/Logging files to PAL

[Myles C. Maxfield]

Created attachment 308785 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 308785 [details]
Patch

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

> Source/WebCore/PAL/pal/Logging.h:93
> +#define PAL_LOG_CHANNELS(M) \
> +    M(Animations) \
> +    M(Archives) \
> +    M(Compositing) \
> +    M(ContentFiltering) \
> +    M(DisplayLists) \
> +    M(DOMTimers) \
> +    M(Editing) \
> +    M(Events) \
> +    M(FileAPI) \
> +    M(Frames) \
> +    M(FTP) \
> +    M(Fullscreen) \
> +    M(Gamepad) \
> +    M(History) \
> +    M(IconDatabase) \
> +    M(Images) \
> +    M(IndexedDB) \
> +    M(IndexedDBOperations) \
> +    M(Layers) \
> +    M(Layout) \
> +    M(Loading) \
> +    M(Media) \
> +    M(MediaSource) \
> +    M(MediaSourceSamples) \
> +    M(MediaCaptureSamples) \
> +    M(MemoryPressure) \
> +    M(Network) \
> +    M(NotYetImplemented) \
> +    M(PageCache) \
> +    M(PerformanceLogging) \
> +    M(PlatformLeaks) \
> +    M(Plugins) \
> +    M(PopupBlocking) \
> +    M(Progress) \
> +    M(RemoteInspector) \
> +    M(ResourceLoading) \
> +    M(ResourceLoadObserver) \
> +    M(Scrolling) \
> +    M(Services) \
> +    M(SpellingAndGrammar) \
> +    M(SQLDatabase) \
> +    M(StorageAPI) \
> +    M(SVG) \
> +    M(TextAutosizing) \
> +    M(Tiling) \
> +    M(Threading) \
> +    M(URLParser) \
> +    M(WebAudio) \
> +    M(WebGL) \
> +    M(WebGPU) \
> +    M(WebRTC) \
> +    M(WebReplay) \
> +    M(WheelEventTestTriggers) \

Nope, these are WebCore-specifc and should not be in PAL.

[Yoshiaki Jitsukawa]

*** Bug 171519 has been marked as a duplicate of this bug. ***

[Sam Weinig]

All of the #include lists need to be resorted.

[Myles C. Maxfield]

Comment on attachment 308785 [details]
Patch

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

>> Source/WebCore/PAL/pal/Logging.h:93
>> +    M(WheelEventTestTriggers) \
> 
> Nope, these are WebCore-specifc and should not be in PAL.

Seems like we should put these in a "utils" folder in WebCore, since they don't belong in PAL nor WebCore/Platform.

[Myles C. Maxfield]

Comment on attachment 308785 [details]
Patch

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

> Source/WebCore/PAL/pal/Logging.h:101
> +String logLevelString();

This is platform-specific.

[Don Olmstead]

With TextStream moved over this should be unblocked now.

[Don Olmstead]

Simon do you have any thoughts on what should happen with Logging now that TextStream is in WTF and this is unblocked?

I don't think we want to duplicate the .cpp files in WebCore and PAL. I'm also wondering if some of this should end up in WTF.

Any thoughts on what you would like Logging to look like going forward?

[Myles C. Maxfield]

Comment on attachment 308785 [details]
Patch

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

>>> Source/WebCore/PAL/pal/Logging.h:93
>>> +    M(WheelEventTestTriggers) \
>> 
>> Nope, these are WebCore-specifc and should not be in PAL.
> 
> Seems like we should put these in a "utils" folder in WebCore, since they don't belong in PAL nor WebCore/Platform.

PAL itself will use the following log channels (because they are currently used in WebCore/platform/):

Animations
ContentFiltering
DisplayLists
Events
Frames
Fullscreen
Gamepad
IOSurface
Images
Media
MediaCaptureSamples
MediaSource
MediaSourceSamples
MediaStream
MemoryPressure
Network
PerformanceLogging
ResourceLoadStatistics
SQLDatabase
WebAudio
WebGL
WebGPU
WebRTC

So we need a way to declare some log channels in PAL but some in WebCore.

[Myles C. Maxfield]

Created attachment 323359 [details]
Patch

[Myles C. Maxfield]

Don, can you help me with the CMake pieces of this?

[Don Olmstead]

(In reply to Myles C. Maxfield from comment #11)
> Don, can you help me with the CMake pieces of this?

The problem for gtk and wpe appear to be due to the script to create forwarding headers as there's a conflict in names for it.

That script should probably not run over stuff in PAL.

Forwarding headers are a bit of an open question right now. There is a CMake way to do things that the CMake Mac port is using and then a script that the other ports are using.

[Myles C. Maxfield]

(In reply to Don Olmstead from comment #12)
> (In reply to Myles C. Maxfield from comment #11)
> > Don, can you help me with the CMake pieces of this?
> 
> The problem for gtk and wpe appear to be due to the script to create
> forwarding headers as there's a conflict in names for it.
> 
> That script should probably not run over stuff in PAL.
> 
> Forwarding headers are a bit of an open question right now. There is a CMake
> way to do things that the CMake Mac port is using and then a script that the
> other ports are using.

What do you think we should do to fix it? Should I just rename the file?

[Don Olmstead]

Created attachment 323431 [details]
CMake changes

This should get cmake going for you

[Myles C. Maxfield]

Created attachment 323438 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 323438 [details]
Patch

Clearing flags on attachment: 323438

Committed r223206: <https://trac.webkit.org/changeset/223206>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/34940334>

[Ryan Haddad]

This change broke the Windows debug build:

PAL.lib(Logging.obj) : error LNK2019: unresolved external symbol "class WTF::String __cdecl PAL::logLevelString(void)" (?logLevelString@PAL@@YA?AVString@WTF@@XZ) referenced in function "void __cdecl PAL::initializeLogChannelsIfNecessary(class std::optional<class WTF::String>)" (?initializeLogChannelsIfNecessary@PAL@@YAXV?$optional@VString@WTF@@@std@@@Z) [C:\cygwin\home\buildbot\slave\win-debug\build\WebKitBuild\Debug\Source\WebKitLegacy\WebKitLegacy.vcxproj]

https://build.webkit.org/builders/Apple%20Win%20Debug%20%28Build%29/builds/4691

[Myles C. Maxfield]

Committed r223241: <https://trac.webkit.org/changeset/223241>