[webkit-reviews] review denied: [Bug 63395] [WebKit2] Add logging initialization for WebProcess : [Attachment 98619] Patch for logging initialization for WebProcess

[bugzilla-daemon at webkit.org]

Leandro Pereira <leandro at profusion.mobi> has denied Lukasz Slachciak
<l.slachciak at samsung.com>'s request for review:
Bug 63395: [WebKit2] Add logging initialization for WebProcess
https://bugs.webkit.org/show_bug.cgi?id=63395

Attachment 98619: Patch for logging initialization for WebProcess
https://bugs.webkit.org/attachment.cgi?id=98619&action=review

------- Additional Comments from Leandro Pereira <leandro at profusion.mobi>
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).