Bug 96478 – [EFL][WK2] Implement missing initializeLogChannel function

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit EFL

[KwangYong Choi]

Implemented log channel initialization function for EFL platform in WebKit2.

[KwangYong Choi]

Created attachment 163539 [details]
Patch

[Raphael Kubo da Costa (:rakuco)]

Comment on attachment 163539 [details]
Patch

Why not use the implementation we already have in WebCore? Plus the use of glib here is absolutely unnecessary.

[KwangYong Choi]

(In reply to comment #2)
> (From update of attachment 163539 [details])
> Why not use the implementation we already have in WebCore? Plus the use of glib here is absolutely unnecessary.

The implementation of logging on WebKit is little bit different from WebCore. The routine is called once in case of WebCore implementation, but the routine is called many times in case of WebKit implementation.

I think, it's enough to call initializeLogChannel() once like WebCore implementation.

[KwangYong Choi]

I want to change previous routine on GTK and QT first. (Bug 96500)

[Gyuyoung Kim]

Comment on attachment 163539 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=163539&action=review

> Source/WebKit2/Platform/efl/LoggingEfl.cpp:37
> +    DEFINE_STATIC_LOCAL(const String, logValue, (g_getenv("WEBKIT_DEBUG")));

If we use getenv instead of g_getenv, can't we remove "#include <glib.h>" ? If possible, EFL port should not have glib dependency.

> Source/WebKit2/Platform/efl/LoggingEfl.cpp:46
> +        logValue.split(",", activatedNames);

"," -> ',' ?

[Ryuan Choi]

Comment on attachment 163539 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=163539&action=review

> Source/WebKit2/Platform/efl/LoggingEfl.cpp:37
> +    DEFINE_STATIC_LOCAL(const String, logValue, (g_getenv("WEBKIT_DEBUG")));

I think that it can be moved into below 44 line. (without static)

> Source/WebKit2/Platform/efl/LoggingEfl.cpp:38
> +    static Vector<WTFLogChannel*> activatedChannels;

Why this is not static ?

> Source/WebKit2/Platform/efl/LoggingEfl.cpp:45
> +        static Vector<String> activatedNames;

Should it be static?

> Source/WebKit2/Platform/efl/LoggingEfl.cpp:46
> +        logValue.split(",", activatedNames);

As following http://trac.webkit.org/wiki/EfficientStrings, we recommand ASCIILiteral like logValue.split(ASCIILiteral(",", ...)

> Source/WebKit2/Platform/efl/LoggingEfl.cpp:47
> +        for (unsigned int i = 0; i < activatedNames.size(); i++) {

I prefered size_t.

[Ryuan Choi]

(In reply to comment #6)
> (From update of attachment 163539 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=163539&action=review
> 
> > Source/WebKit2/Platform/efl/LoggingEfl.cpp:46
> > +        logValue.split(",", activatedNames);
> 
Oops, I think that gyuyoung is right. first this can be ','.
If then, logValue.split(',', activatedNames); is reasonable.

[KwangYong Choi]

Comment on attachment 163539 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=163539&action=review

I tried to change previous routine in GTK and QT but, it can not be done easily. I just want add this feature to EFL.

>>> Source/WebKit2/Platform/efl/LoggingEfl.cpp:37
>>> +    DEFINE_STATIC_LOCAL(const String, logValue, (g_getenv("WEBKIT_DEBUG")));
>> 
>> If we use getenv instead of g_getenv, can't we remove "#include <glib.h>" ? If possible, EFL port should not have glib dependency.
> 
> I think that it can be moved into below 44 line. (without static)

I will remove glib dependency.

I agree with Ryuan. It can be moved into below if block.

>> Source/WebKit2/Platform/efl/LoggingEfl.cpp:38
>> +    static Vector<WTFLogChannel*> activatedChannels;
> 
> Why this is not static ?

I think it may be static to avoid recalculation.

>> Source/WebKit2/Platform/efl/LoggingEfl.cpp:45
>> +        static Vector<String> activatedNames;
> 
> Should it be static?

No.

>>>> Source/WebKit2/Platform/efl/LoggingEfl.cpp:46
>>>> +        logValue.split(",", activatedNames);
>>> 
>>> "," -> ',' ?
>> 
>> As following http://trac.webkit.org/wiki/EfficientStrings, we recommand ASCIILiteral like logValue.split(ASCIILiteral(",", ...)
> 
> Oops, I think that gyuyoung is right. first this can be ','.
> If then, logValue.split(',', activatedNames); is reasonable.

I will use ',' instead of ","

>> Source/WebKit2/Platform/efl/LoggingEfl.cpp:47
>> +        for (unsigned int i = 0; i < activatedNames.size(); i++) {
> 
> I prefered size_t.

I do.

[KwangYong Choi]

Created attachment 163808 [details]
Patch

[Ryuan Choi]

(In reply to comment #9)
> Created an attachment (id=163808) [details]
> Patch

Looks good to me.

[WebKit Review Bot]

Comment on attachment 163808 [details]
Patch

Clearing flags on attachment: 163808

Committed r128537: <http://trac.webkit.org/changeset/128537>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.