Implemented logging for GTK platform in WebKit2 - function initializeLogChannel is called for all ports, so added missing implementation. Aslo helper function added for getting channels from name.
Created attachment 98588 [details] Loggging for WebKit2 - GTK
Created attachment 98589 [details] Logging - WK2 GTK Changelog fix
Comment on attachment 98589 [details] Logging - WK2 GTK View in context: https://bugs.webkit.org/attachment.cgi?id=98589&action=review Nice. Some comments below. > Source/WebKit2/Platform/Logging.cpp:48 > +WTFLogChannel* getChannelFromName(const String& channelName) Why not just put this in LoggingGtk and make it static? > Source/WebKit2/Platform/gtk/LoggingGtk.cpp:44 > + // unfortunatelly this is initialized and enumerated for every WTFLogChannel This seems like an expensive thing to do for every log channel. Why not just do it statically? > Source/WebKit2/Platform/gtk/LoggingGtk.cpp:45 > + gchar** logv = g_strsplit(logEnv, " ", -1); I would prefer you to do this with WTF::String instead of GLib. You can avoid manual memory management that way. > Source/WebKit2/Platform/gtk/LoggingGtk.cpp:56 > +#endif /* !LOG_DISABLED */ C++ style comment here, I think.
Comment on attachment 98589 [details] Logging - WK2 GTK View in context: https://bugs.webkit.org/attachment.cgi?id=98589&action=review >> Source/WebKit2/Platform/Logging.cpp:48 >> +WTFLogChannel* getChannelFromName(const String& channelName) > > Why not just put this in LoggingGtk and make it static? I put this function here because I think that in future other ports will want to use it too. In WebCore, similar function is used by GTK, EFL, QT, WX >> Source/WebKit2/Platform/gtk/LoggingGtk.cpp:44 >> + // unfortunatelly this is initialized and enumerated for every WTFLogChannel > > This seems like an expensive thing to do for every log channel. Why not just do it statically? ok, done. I was thinking about it earlier but was afraid about keeping in memory things till the end. But we will do this only in case of !LOG_DISABLED so this shouldn't be a problem. >> Source/WebKit2/Platform/gtk/LoggingGtk.cpp:45 > > I would prefer you to do this with WTF::String instead of GLib. You can avoid manual memory management that way. done >> Source/WebKit2/Platform/gtk/LoggingGtk.cpp:56 >> +#endif /* !LOG_DISABLED */ > > C++ style comment here, I think. Right. Fixed.
Created attachment 99293 [details] Logging WK2 GTK
Comment on attachment 99293 [details] Logging WK2 GTK View in context: https://bugs.webkit.org/attachment.cgi?id=99293&action=review Looks pretty good, but I have a few nits. > Source/WebKit2/Platform/gtk/LoggingGtk.cpp:39 > + const static String logEnv(g_getenv("WEBKIT_DEBUG")); > + static Vector<String> logv; Please don't use abbreviations for variable names. I suggest something like activatedChannels here, instead of logv. > Source/WebKit2/Platform/gtk/LoggingGtk.cpp:52 > + if (logv.isEmpty()) > + logEnv.split(" ", logv); > + > + for (unsigned int i = 0; i < logv.size(); i++) { > + if (channel == getChannelFromName(logv[i])) { > + channel->state = WTFLogChannelOn; > + break; > + } > + } Instead of iterating and parsing log channel names for every single channel, why not build a Vector of WTFLogChannel once and then simply do activatedChannels.contains each time? This would take up less space and be faster, I believe.
Comment on attachment 99293 [details] Logging WK2 GTK View in context: https://bugs.webkit.org/attachment.cgi?id=99293&action=review >> Source/WebKit2/Platform/gtk/LoggingGtk.cpp:39 >> + static Vector<String> logv; > > Please don't use abbreviations for variable names. I suggest something like activatedChannels here, instead of logv. done >> Source/WebKit2/Platform/gtk/LoggingGtk.cpp:52 >> + } > > Instead of iterating and parsing log channel names for every single channel, why not build a Vector of WTFLogChannel once and then simply do activatedChannels.contains each time? This would take up less space and be faster, I believe. Yes you are right. Code will be little more complicated now, but faster. No need to iterate each time.
Created attachment 100864 [details] WebKit2 Logging for GTK port implementation
And one more note. To have Logging working in WebProcess (not only UIProcess) we need to add logging initialization in WebProcess. This is covered by another, independent bug: https://bugs.webkit.org/show_bug.cgi?id=63395
Comment on attachment 100864 [details] WebKit2 Logging for GTK port implementation View in context: https://bugs.webkit.org/attachment.cgi?id=100864&action=review > Source/WebKit2/Platform/gtk/LoggingGtk.cpp:43 > + // fulfill activatedChannels vector only once basing on names set in logEnv This comment should probably read: Fill activatedChannels vector only once based on names set in logEnv. According to WebKit style guidelines, comments need to be a full sentence with a capital and period.
Comment on attachment 100864 [details] WebKit2 Logging for GTK port implementation View in context: https://bugs.webkit.org/attachment.cgi?id=100864&action=review >> Source/WebKit2/Platform/gtk/LoggingGtk.cpp:43 >> + // fulfill activatedChannels vector only once basing on names set in logEnv > > This comment should probably read: > > Fill activatedChannels vector only once based on names set in logEnv. > > According to WebKit style guidelines, comments need to be a full sentence with a capital and period. Fixed.
Created attachment 101387 [details] Logging implementation for WebKit2 GTK Setting cq ? only. Previous patch was set r+. Changes: - comment fixed according to Martin comments. - WebKit2/Changelog updated according to the new style (short note - review by - long note)
Comment on attachment 101387 [details] Logging implementation for WebKit2 GTK Clearing flags on attachment: 101387 Committed r91307: <http://trac.webkit.org/changeset/91307>
All reviewed patches have been landed. Closing bug.
This patch broke Windows and Mac :( http://build.webkit.org/builders/SnowLeopard%20Intel%20Release%20%28Build%29/builds/32102 http://build.webkit.org/builders/Windows%20Release%20%28Build%29/builds/18981
Comment on attachment 101387 [details] Logging implementation for WebKit2 GTK View in context: https://bugs.webkit.org/attachment.cgi?id=101387&action=review Since I'm already here, I'm going to comment on the landed patch. > Source/WebKit2/Platform/gtk/LoggingGtk.cpp:2 > + * Copyright (C) 2011 Samsung Electronics I don't think you should claim the copyright of the entire file you're copying the existing file and modifying it. > Source/WebKit2/Platform/gtk/LoggingGtk.cpp:13 > + * THIS SOFTWARE IS PROVIDED BY APPLE INC. AND ITS CONTRIBUTORS ``AS IS'' Also, this paragraph mentions Apple.
(In reply to comment #16) > (From update of attachment 101387 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=101387&action=review > > Since I'm already here, I'm going to comment on the landed patch. > > > Source/WebKit2/Platform/gtk/LoggingGtk.cpp:2 > > + * Copyright (C) 2011 Samsung Electronics > > I don't think you should claim the copyright of the entire file you're copying the existing file and modifying it. > > > Source/WebKit2/Platform/gtk/LoggingGtk.cpp:13 > > + * THIS SOFTWARE IS PROVIDED BY APPLE INC. AND ITS CONTRIBUTORS ``AS IS'' > > Also, this paragraph mentions Apple. It seems like all of this is accidental, caused by git.
Comment on attachment 101387 [details] Logging implementation for WebKit2 GTK View in context: https://bugs.webkit.org/attachment.cgi?id=101387&action=review >>> Source/WebKit2/Platform/gtk/LoggingGtk.cpp:2 >>> + * Copyright (C) 2011 Samsung Electronics >> >> I don't think you should claim the copyright of the entire file you're copying the existing file and modifying it. > > It seems like all of this is accidental, caused by git. Initially the idea of logging was basing on the Source/WebCore/platform/gtk/LoggingGtk.cpp and even had note about it. But patch was redesigned and now don't have common main code. >> Source/WebKit2/Platform/gtk/LoggingGtk.cpp:13 >> + * THIS SOFTWARE IS PROVIDED BY APPLE INC. AND ITS CONTRIBUTORS ``AS IS'' > > Also, this paragraph mentions Apple. right, I'll fix this. In fact I see a lot of errors like this in other files: Source/WebKit2/efl/MainEfl.cpp
Thank you Ryosuke for fixing problem on Mac/Win
(In reply to comment #19) > Thank you Ryosuke for fixing problem on Mac/Win Oh sorry, I forgot to mention that the Mac/Win builds were fixed in http://trac.webkit.org/changeset/91317.