Bug 63395 – [WebKit2] Add logging initialization for WebProcess

• Component: WebKit2
• Status: RESOLVED FIXED

[Lukasz Slachciak]

Logging initialization was added for WebProcess. Similar initialization is already done for UIProcess.

[Lukasz Slachciak]

Created attachment 98619 [details]
Patch for logging initialization for WebProcess

[Leandro Pereira]

Comment on attachment 98619 [details]
Patch for logging initialization for WebProcess

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

> Source/WebKit2/ChangeLog:8
> +        Logging initialization was added for WebProcess. Similar initialization is already done for UIProcess.

Watch out for 80-column limit on changelogs.

> Source/WebKit2/WebProcess/WebProcess.cpp:143
> +#if !LOG_DISABLED

I haven't seen this idiom lately on WebKit -- wouldn't it be better if it were an ENABLE(LOGGING) macro call?

> Source/WebKit2/WebProcess/WebProcess.cpp:147
> +    WebKit::initializeLogChannelsIfNecessary();
> +#endif
>  
>      WebCore::InitializeLoggingChannelsIfNecessary();

A couple of questions here:

1) Shouldn't the LOG_DISABLED macro also disable WebCore's initialization?
2) Why are WebKit's and WebCore's log channel initialization functions are named differently ('Log' vs. 'Logging')?
3) Also, unrelated to this patch, looks like WebCore's method is wrongly named (begins with a capital letter).

[Lukasz Slachciak]

Comment on attachment 98619 [details]
Patch for logging initialization for WebProcess

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

Leaonardo, I generally agree with you that there is a lot of logging inconsistency in WebKit. There is no ENABLE(LOGGING) feature.
I started some time a go discussion about it: https://lists.webkit.org/pipermail/webkit-dev/2011-June/017278.html
I'm planning to continue to do logging cleanup in WebKit (and propose new patches).
But this what I would like to do in this bug - just to enable logging following current schemes and standard, so I can fully utilize it in WebKit2.

>> Source/WebKit2/ChangeLog:8
>> +        Logging initialization was added for WebProcess. Similar initialization is already done for UIProcess.
> 
> Watch out for 80-column limit on changelogs.

Ok, I'll fix this

>> Source/WebKit2/WebProcess/WebProcess.cpp:143
>> +#if !LOG_DISABLED
> 
> I haven't seen this idiom lately on WebKit -- wouldn't it be better if it were an ENABLE(LOGGING) macro call?

In mac port it is commonly used in Webkit and WebKit2, using ENABLE(LOGGING) would require defining LOGGING feature in Platform.h
in WebCore it is commonly used.
grep -r "\!LOG_DISABLED" Source/ | wc -l
shows me 89 occurences

>> Source/WebKit2/WebProcess/WebProcess.cpp:147
>>      WebCore::InitializeLoggingChannelsIfNecessary();
> 
> A couple of questions here:
> 
> 1) Shouldn't the LOG_DISABLED macro also disable WebCore's initialization?
> 2) Why are WebKit's and WebCore's log channel initialization functions are named differently ('Log' vs. 'Logging')?
> 3) Also, unrelated to this patch, looks like WebCore's method is wrongly named (begins with a capital letter).

1) I was following original WebKit2 design as in case of Source/WebKit2/UIProcess/WebContext.cpp
 where WebProcess is initialized - WebKit logging initialization depends on the LOG_DISABLED macro while WebCore logging initialization doesn't depend on it.
In fact in the Source/WebKit WebCore logging initialization is also not depending on the LOG_DISABLED
2) I think that it was code author's invention
3) dito

[Eric Seidel (no email)]

There is no 80c limit in WebKit, for any code or file.

[Eric Seidel (no email)]

Comment on attachment 98619 [details]
Patch for logging initialization for WebProcess

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

>>> Source/WebKit2/ChangeLog:8
>>> +        Logging initialization was added for WebProcess. Similar initialization is already done for UIProcess.
>> 
>> Watch out for 80-column limit on changelogs.
> 
> Ok, I'll fix this

This is incorrect advice.

>>> Source/WebKit2/WebProcess/WebProcess.cpp:143
>>> +#if !LOG_DISABLED
>> 
>> I haven't seen this idiom lately on WebKit -- wouldn't it be better if it were an ENABLE(LOGGING) macro call?
> 
> In mac port it is commonly used in Webkit and WebKit2, using ENABLE(LOGGING) would require defining LOGGING feature in Platform.h
> in WebCore it is commonly used.
> grep -r "\!LOG_DISABLED" Source/ | wc -l
> shows me 89 occurences

You are correct !LOG_DISABLED Is how the check is written.  The logging code is very old and crufty.  As far as I can tell it's not used much these days.

[Lukasz Slachciak]

Created attachment 99254 [details]
Logging initialization for WebProcess

[Lukasz Slachciak]

As for 80c limit looks like there was discussion about it and it was considered as no more needed requirement.
https://lists.webkit.org/pipermail/webkit-dev/2010-April/012528.html

Anyway, for me my new patch with comments in two lines looks more clear.

[Lukasz Slachciak]

Eric, Leonardo is this patch ok for you?

[Eric Seidel (no email)]

Sam wenig would know if this is needed for WebKit2.

[Chang Shu]

It looks to me the LOG_DISABLED flag is forced to be 1 by default when not defined. So the patch should be pretty safe and cause no regression.

[WebKit Review Bot]

Comment on attachment 99254 [details]
Logging initialization for WebProcess

Rejecting attachment 99254 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
rsed 2 diffs from patch file(s).
patching file Source/WebKit2/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file Source/WebKit2/WebProcess/WebProcess.cpp
Hunk #1 FAILED at 31.
Hunk #2 succeeded at 153 with fuzz 2 (offset 14 lines).
1 out of 2 hunks FAILED -- saving rejects to file Source/WebKit2/WebProcess/WebProcess.cpp.rej

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force', u'--reviewer', u'Chang Shu']" exit_code: 1 cwd: /mnt/git/webkit-commit-queue/

Full output: http://queues.webkit.org/results/11535132

[Alexey Proskuryakov]

Unsure if this specific patch ever got landed, but we do initialize logging now.