Bug 171523 - Allow PAL to log messages
Summary: Allow PAL to log messages
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Myles C. Maxfield
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-05-01 16:35 PDT by Myles C. Maxfield
Modified: 2017-10-12 11:21 PDT (History)
9 users (show)

See Also:


Attachments
Patch (247.09 KB, patch)
2017-05-01 16:43 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Patch (33.32 KB, patch)
2017-10-10 16:45 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
CMake changes (744 bytes, patch)
2017-10-11 11:10 PDT, Don Olmstead
no flags Details | Formatted Diff | Diff
Patch (32.88 KB, patch)
2017-10-11 12:06 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Myles C. Maxfield 2017-05-01 16:35:50 PDT
[PAL] Migrate WebCore/platform/Logging files to PAL
Comment 1 Myles C. Maxfield 2017-05-01 16:43:06 PDT
Created attachment 308785 [details]
Patch
Comment 2 Simon Fraser (smfr) 2017-05-01 16:47:52 PDT
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.
Comment 3 Yoshiaki Jitsukawa 2017-05-01 16:50:49 PDT
*** Bug 171519 has been marked as a duplicate of this bug. ***
Comment 4 Sam Weinig 2017-05-01 17:00:37 PDT
All of the #include lists need to be resorted.
Comment 5 Myles C. Maxfield 2017-05-01 17:04:48 PDT
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.
Comment 6 Myles C. Maxfield 2017-05-01 17:07:26 PDT
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.
Comment 7 Don Olmstead 2017-08-09 19:09:15 PDT
With TextStream moved over this should be unblocked now.
Comment 8 Don Olmstead 2017-08-16 10:58:57 PDT
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?
Comment 9 Myles C. Maxfield 2017-10-10 14:47:30 PDT
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.
Comment 10 Myles C. Maxfield 2017-10-10 16:45:24 PDT
Created attachment 323359 [details]
Patch
Comment 11 Myles C. Maxfield 2017-10-10 16:45:48 PDT
Don, can you help me with the CMake pieces of this?
Comment 12 Don Olmstead 2017-10-10 17:53:31 PDT
(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.
Comment 13 Myles C. Maxfield 2017-10-10 19:51:56 PDT
(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?
Comment 14 Don Olmstead 2017-10-11 11:10:23 PDT
Created attachment 323431 [details]
CMake changes

This should get cmake going for you
Comment 15 Myles C. Maxfield 2017-10-11 12:06:26 PDT
Created attachment 323438 [details]
Patch
Comment 16 WebKit Commit Bot 2017-10-11 13:30:20 PDT
Comment on attachment 323438 [details]
Patch

Clearing flags on attachment: 323438

Committed r223206: <https://trac.webkit.org/changeset/223206>
Comment 17 WebKit Commit Bot 2017-10-11 13:30:22 PDT
All reviewed patches have been landed.  Closing bug.
Comment 18 Radar WebKit Bug Importer 2017-10-11 13:30:41 PDT
<rdar://problem/34940334>
Comment 19 Ryan Haddad 2017-10-11 17:23:05 PDT
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
Comment 20 Myles C. Maxfield 2017-10-12 11:21:18 PDT
Committed r223241: <https://trac.webkit.org/changeset/223241>