Bug 63381 - [GTK] [WK2] Implement missing initializeLogChannel function.
Summary: [GTK] [WK2] Implement missing initializeLogChannel function.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-06-25 04:14 PDT by Lukasz Slachciak
Modified: 2011-07-20 00:02 PDT (History)
3 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Lukasz Slachciak 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.
Comment 1 Lukasz Slachciak 2011-06-25 04:27:37 PDT
Created attachment 98588 [details]
Loggging for WebKit2 - GTK
Comment 2 Lukasz Slachciak 2011-06-25 04:30:52 PDT
Created attachment 98589 [details]
Logging - WK2 GTK

Changelog fix
Comment 3 Martin Robinson 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.
Comment 4 Lukasz Slachciak 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.
Comment 5 Lukasz Slachciak 2011-06-30 06:50:37 PDT
Created attachment 99293 [details]
Logging WK2 GTK
Comment 6 Martin Robinson 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.
Comment 7 Lukasz Slachciak 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.
Comment 8 Lukasz Slachciak 2011-07-14 14:23:57 PDT
Created attachment 100864 [details]
WebKit2 Logging for GTK port implementation
Comment 9 Lukasz Slachciak 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
Comment 10 Martin Robinson 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.
Comment 11 Lukasz Slachciak 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.
Comment 12 Lukasz Slachciak 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)
Comment 13 WebKit Review Bot 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>
Comment 14 WebKit Review Bot 2011-07-19 15:07:29 PDT
All reviewed patches have been landed.  Closing bug.
Comment 16 Ryosuke Niwa 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.
Comment 17 Ryosuke Niwa 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.
Comment 18 Lukasz Slachciak 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
Comment 19 Lukasz Slachciak 2011-07-19 23:05:57 PDT
Thank you Ryosuke for fixing problem on Mac/Win
Comment 20 Ryosuke Niwa 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.