WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
63381
[GTK] [WK2] Implement missing initializeLogChannel function.
https://bugs.webkit.org/show_bug.cgi?id=63381
Summary
[GTK] [WK2] Implement missing initializeLogChannel function.
Lukasz Slachciak
Reported
2011-06-25 04:14:29 PDT
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.
Attachments
Loggging for WebKit2 - GTK
(6.39 KB, patch)
2011-06-25 04:27 PDT
,
Lukasz Slachciak
no flags
Details
Formatted Diff
Diff
Logging - WK2 GTK
(6.38 KB, patch)
2011-06-25 04:30 PDT
,
Lukasz Slachciak
mrobinson
: review-
Details
Formatted Diff
Diff
Logging WK2 GTK
(6.27 KB, patch)
2011-06-30 06:50 PDT
,
Lukasz Slachciak
mrobinson
: review-
Details
Formatted Diff
Diff
WebKit2 Logging for GTK port implementation
(6.57 KB, patch)
2011-07-14 14:23 PDT
,
Lukasz Slachciak
no flags
Details
Formatted Diff
Diff
Logging implementation for WebKit2 GTK
(6.61 KB, patch)
2011-07-19 14:20 PDT
,
Lukasz Slachciak
no flags
Details
Formatted Diff
Diff
Show Obsolete
(4)
View All
Add attachment
proposed patch, testcase, etc.
Lukasz Slachciak
Comment 1
2011-06-25 04:27:37 PDT
Created
attachment 98588
[details]
Loggging for WebKit2 - GTK
Lukasz Slachciak
Comment 2
2011-06-25 04:30:52 PDT
Created
attachment 98589
[details]
Logging - WK2 GTK Changelog fix
Martin Robinson
Comment 3
2011-06-28 20:08:39 PDT
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.
Lukasz Slachciak
Comment 4
2011-06-30 06:39:37 PDT
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.
Lukasz Slachciak
Comment 5
2011-06-30 06:50:37 PDT
Created
attachment 99293
[details]
Logging WK2 GTK
Martin Robinson
Comment 6
2011-07-11 15:52:51 PDT
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.
Lukasz Slachciak
Comment 7
2011-07-14 14:22:56 PDT
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.
Lukasz Slachciak
Comment 8
2011-07-14 14:23:57 PDT
Created
attachment 100864
[details]
WebKit2 Logging for GTK port implementation
Lukasz Slachciak
Comment 9
2011-07-15 13:08:11 PDT
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
Martin Robinson
Comment 10
2011-07-19 11:55:46 PDT
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.
Lukasz Slachciak
Comment 11
2011-07-19 14:16:54 PDT
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.
Lukasz Slachciak
Comment 12
2011-07-19 14:20:12 PDT
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)
WebKit Review Bot
Comment 13
2011-07-19 15:07:24 PDT
Comment on
attachment 101387
[details]
Logging implementation for WebKit2 GTK Clearing flags on attachment: 101387 Committed
r91307
: <
http://trac.webkit.org/changeset/91307
>
WebKit Review Bot
Comment 14
2011-07-19 15:07:29 PDT
All reviewed patches have been landed. Closing bug.
Ryosuke Niwa
Comment 15
2011-07-19 15:20:29 PDT
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
Ryosuke Niwa
Comment 16
2011-07-19 15:23:53 PDT
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.
Ryosuke Niwa
Comment 17
2011-07-19 15:33:30 PDT
(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.
Lukasz Slachciak
Comment 18
2011-07-19 22:52:46 PDT
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
Lukasz Slachciak
Comment 19
2011-07-19 23:05:57 PDT
Thank you Ryosuke for fixing problem on Mac/Win
Ryosuke Niwa
Comment 20
2011-07-20 00:02:12 PDT
(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
.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug