Bug 63395 - [WebKit2] Add logging initialization for WebProcess
Summary: [WebKit2] Add logging initialization for WebProcess
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-06-26 02:03 PDT by Lukasz Slachciak
Modified: 2013-03-16 22:52 PDT (History)
6 users (show)

See Also:


Attachments
Patch for logging initialization for WebProcess (1.45 KB, patch)
2011-06-26 02:08 PDT, Lukasz Slachciak
leandro: review-
leandro: commit-queue-
Details | Formatted Diff | Diff
Logging initialization for WebProcess (1.44 KB, patch)
2011-06-30 01:11 PDT, Lukasz Slachciak
cshu: review+
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Lukasz Slachciak 2011-06-26 02:03:23 PDT
Logging initialization was added for WebProcess. Similar initialization is already done for UIProcess.
Comment 1 Lukasz Slachciak 2011-06-26 02:08:23 PDT
Created attachment 98619 [details]
Patch for logging initialization for WebProcess
Comment 2 Leandro Pereira 2011-06-27 10:23:56 PDT
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).
Comment 3 Lukasz Slachciak 2011-06-30 01:03:32 PDT
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
Comment 4 Eric Seidel (no email) 2011-06-30 01:07:10 PDT
There is no 80c limit in WebKit, for any code or file.
Comment 5 Eric Seidel (no email) 2011-06-30 01:08:29 PDT
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.
Comment 6 Lukasz Slachciak 2011-06-30 01:11:57 PDT
Created attachment 99254 [details]
Logging initialization for WebProcess
Comment 7 Lukasz Slachciak 2011-06-30 01:25:25 PDT
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.
Comment 8 Lukasz Slachciak 2011-07-07 13:17:40 PDT
Eric, Leonardo is this patch ok for you?
Comment 9 Eric Seidel (no email) 2011-09-12 15:51:13 PDT
Sam wenig would know if this is needed for WebKit2.
Comment 10 Chang Shu 2012-02-13 03:45:08 PST
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.
Comment 11 WebKit Review Bot 2012-02-16 02:23:53 PST
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
Comment 12 Alexey Proskuryakov 2013-03-16 22:52:03 PDT
Unsure if this specific patch ever got landed, but we do initialize logging now.