[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
bugzilla-daemon at webkit.org
Mon Jun 27 10:23:56 PDT 2011
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).
More information about the webkit-reviews
mailing list