63395 2011-06-26 02:03:23 -0700 [WebKit2] Add logging initialization for WebProcess 2013-03-16 22:52:03 -0700 1 1 1 Unclassified WebKit WebKit2 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 0 l.slachciak webkit-unassigned ap cshu eric gyuyoung.kim sam webkit.review.bot oldest_to_newest 427494 0 l.slachciak 2011-06-26 02:03:23 -0700 Logging initialization was added for WebProcess. Similar initialization is already done for UIProcess. 427495 1 98619 l.slachciak 2011-06-26 02:08:23 -0700 Created attachment 98619 Patch for logging initialization for WebProcess 428056 2 98619 leandro 2011-06-27 10:23:56 -0700 Comment on attachment 98619 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). 430240 3 98619 l.slachciak 2011-06-30 01:03:32 -0700 Comment on attachment 98619 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 430242 4 eric 2011-06-30 01:07:10 -0700 There is no 80c limit in WebKit, for any code or file. 430243 5 98619 eric 2011-06-30 01:08:29 -0700 Comment on attachment 98619 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. 430244 6 99254 l.slachciak 2011-06-30 01:11:57 -0700 Created attachment 99254 Logging initialization for WebProcess 430245 7 l.slachciak 2011-06-30 01:25:25 -0700 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. 433668 8 l.slachciak 2011-07-07 13:17:40 -0700 Eric, Leonardo is this patch ok for you? 465883 9 eric 2011-09-12 15:51:13 -0700 Sam wenig would know if this is needed for WebKit2. 555158 10 cshu 2012-02-13 03:45:08 -0800 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. 558006 11 99254 webkit.review.bot 2012-02-16 02:23:53 -0800 Comment on attachment 99254 Logging initialization for WebProcess Rejecting attachment 99254 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 856677 12 ap 2013-03-16 22:52:03 -0700 Unsure if this specific patch ever got landed, but we do initialize logging now. 98619 2011-06-26 02:08:23 -0700 2011-06-30 01:11:57 -0700 Patch for logging initialization for WebProcess loginit.patch text/plain 1480 l.slachciak ZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJLaXQyL0NoYW5nZUxvZyBiL1NvdXJjZS9XZWJLaXQyL0No YW5nZUxvZwppbmRleCBmMTNmNGEyLi5lYTg5YzY0IDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViS2l0 Mi9DaGFuZ2VMb2cKKysrIGIvU291cmNlL1dlYktpdDIvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMTUg QEAKKzIwMTEtMDYtMjYgIEx1a2FzeiBTbGFjaGNpYWsgIDxsLnNsYWNoY2lha0BzYW1zdW5nLmNv bT4KKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICBbV2Vi S2l0Ml0gQWRkIGxvZ2dpbmcgaW5pdGlhbGl6YXRpb24gZm9yIFdlYlByb2Nlc3MuCisgICAgICAg IGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD02MzM5NQorCisgICAgICAg IExvZ2dpbmcgaW5pdGlhbGl6YXRpb24gd2FzIGFkZGVkIGZvciBXZWJQcm9jZXNzLiBTaW1pbGFy IGluaXRpYWxpemF0aW9uIGlzIGFscmVhZHkgZG9uZSBmb3IgVUlQcm9jZXNzLgorCisgICAgICAg ICogV2ViUHJvY2Vzcy9XZWJQcm9jZXNzLmNwcDoKKyAgICAgICAgKFdlYktpdDo6V2ViUHJvY2Vz czo6V2ViUHJvY2Vzcyk6IExvZ2dpbmcgaW5pdGlhbGl6YXRpb24gYWRkZWQuCisKIDIwMTEtMDYt MjQgIEFuZGVycyBDYXJsc3NvbiAgPGFuZGVyc2NhQGFwcGxlLmNvbT4KIAogICAgICAgICBSZXZp ZXdlZCBieSBKb2huIFN1bGxpdmFuLgpkaWZmIC0tZ2l0IGEvU291cmNlL1dlYktpdDIvV2ViUHJv Y2Vzcy9XZWJQcm9jZXNzLmNwcCBiL1NvdXJjZS9XZWJLaXQyL1dlYlByb2Nlc3MvV2ViUHJvY2Vz cy5jcHAKaW5kZXggNGZlYzkxOS4uMjM5N2QyNSAxMDA2NDQKLS0tIGEvU291cmNlL1dlYktpdDIv V2ViUHJvY2Vzcy9XZWJQcm9jZXNzLmNwcAorKysgYi9Tb3VyY2UvV2ViS2l0Mi9XZWJQcm9jZXNz L1dlYlByb2Nlc3MuY3BwCkBAIC0zMSw2ICszMSw3IEBACiAjaW5jbHVkZSAiSW5qZWN0ZWRCdW5k bGUuaCIKICNpbmNsdWRlICJJbmplY3RlZEJ1bmRsZU1lc3NhZ2VLaW5kcy5oIgogI2luY2x1ZGUg IkluamVjdGVkQnVuZGxlVXNlck1lc3NhZ2VDb2RlcnMuaCIKKyNpbmNsdWRlICJMb2dnaW5nLmgi CiAjaW5jbHVkZSAiUnVuTG9vcC5oIgogI2luY2x1ZGUgIlNhbmRib3hFeHRlbnNpb24uaCIKICNp bmNsdWRlICJXZWJBcHBsaWNhdGlvbkNhY2hlTWFuYWdlci5oIgpAQCAtMTM5LDYgKzE0MCw5IEBA IFdlYlByb2Nlc3M6OldlYlByb2Nlc3MoKQogICAgIC8vIEluaXRpYWxpemUgb3VyIHBsYXRmb3Jt IHN0cmF0ZWdpZXMuCiAgICAgV2ViUGxhdGZvcm1TdHJhdGVnaWVzOjppbml0aWFsaXplKCk7CiAj ZW5kaWYgLy8gVVNFKFBMQVRGT1JNX1NUUkFURUdJRVMpCisjaWYgIUxPR19ESVNBQkxFRAorICAg IFdlYktpdDo6aW5pdGlhbGl6ZUxvZ0NoYW5uZWxzSWZOZWNlc3NhcnkoKTsKKyNlbmRpZgogCiAg ICAgV2ViQ29yZTo6SW5pdGlhbGl6ZUxvZ2dpbmdDaGFubmVsc0lmTmVjZXNzYXJ5KCk7CiB9Cg== 99254 2011-06-30 01:11:57 -0700 2012-02-16 02:23:53 -0800 Logging initialization for WebProcess loginit2.patch text/plain 1478 l.slachciak ZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJLaXQyL0NoYW5nZUxvZyBiL1NvdXJjZS9XZWJLaXQyL0No YW5nZUxvZwppbmRleCBlMWIxNzdkLi4zMDRlYjgwIDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViS2l0 Mi9DaGFuZ2VMb2cKKysrIGIvU291cmNlL1dlYktpdDIvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMTYg QEAKKzIwMTEtMDYtMzAgIEx1a2FzeiBTbGFjaGNpYWsgIDxsLnNsYWNoY2lha0BzYW1zdW5nLmNv bT4KKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICBbV2Vi S2l0Ml0gQWRkIGxvZ2dpbmcgaW5pdGlhbGl6YXRpb24gZm9yIFdlYlByb2Nlc3MuCisgICAgICAg IGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD02MzM5NQorCisgICAgICAg IExvZ2dpbmcgaW5pdGlhbGl6YXRpb24gYWRkZWQgZm9yIFdlYlByb2Nlc3MuCisgICAgICAgIFNp bWlsYXIgaW5pdGlhbGl6YXRpb24gaXMgYWxyZWFkeSBkb25lIGZvciBVSVByb2Nlc3MuCisKKyAg ICAgICAgKiBXZWJQcm9jZXNzL1dlYlByb2Nlc3MuY3BwOgorICAgICAgICAoV2ViS2l0OjpXZWJQ cm9jZXNzOjpXZWJQcm9jZXNzKTogTG9nZ2luZyBpbml0aWFsaXphdGlvbiBhZGRlZC4KKwogMjAx MS0wNi0yOSAgRGFyaW4gQWRsZXIgIDxkYXJpbkBhcHBsZS5jb20+CiAKICAgICAgICAgUmV2aWV3 ZWQgYnkgRGFuIEJlcm5zdGVpbi4KZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJLaXQyL1dlYlByb2Nl c3MvV2ViUHJvY2Vzcy5jcHAgYi9Tb3VyY2UvV2ViS2l0Mi9XZWJQcm9jZXNzL1dlYlByb2Nlc3Mu Y3BwCmluZGV4IDRmZWM5MTkuLjIzOTdkMjUgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJLaXQyL1dl YlByb2Nlc3MvV2ViUHJvY2Vzcy5jcHAKKysrIGIvU291cmNlL1dlYktpdDIvV2ViUHJvY2Vzcy9X ZWJQcm9jZXNzLmNwcApAQCAtMzEsNiArMzEsNyBAQAogI2luY2x1ZGUgIkluamVjdGVkQnVuZGxl LmgiCiAjaW5jbHVkZSAiSW5qZWN0ZWRCdW5kbGVNZXNzYWdlS2luZHMuaCIKICNpbmNsdWRlICJJ bmplY3RlZEJ1bmRsZVVzZXJNZXNzYWdlQ29kZXJzLmgiCisjaW5jbHVkZSAiTG9nZ2luZy5oIgog I2luY2x1ZGUgIlJ1bkxvb3AuaCIKICNpbmNsdWRlICJTYW5kYm94RXh0ZW5zaW9uLmgiCiAjaW5j bHVkZSAiV2ViQXBwbGljYXRpb25DYWNoZU1hbmFnZXIuaCIKQEAgLTEzOSw2ICsxNDAsOSBAQCBX ZWJQcm9jZXNzOjpXZWJQcm9jZXNzKCkKICAgICAvLyBJbml0aWFsaXplIG91ciBwbGF0Zm9ybSBz dHJhdGVnaWVzLgogICAgIFdlYlBsYXRmb3JtU3RyYXRlZ2llczo6aW5pdGlhbGl6ZSgpOwogI2Vu ZGlmIC8vIFVTRShQTEFURk9STV9TVFJBVEVHSUVTKQorI2lmICFMT0dfRElTQUJMRUQKKyAgICBX ZWJLaXQ6OmluaXRpYWxpemVMb2dDaGFubmVsc0lmTmVjZXNzYXJ5KCk7CisjZW5kaWYKIAogICAg IFdlYkNvcmU6OkluaXRpYWxpemVMb2dnaW5nQ2hhbm5lbHNJZk5lY2Vzc2FyeSgpOwogfQo=