Summary: | [EFL][WK2] Implement missing initializeLogChannel function | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | KwangYong Choi <ky0.choi> | ||||||
Component: | WebKit EFL | Assignee: | KwangYong Choi <ky0.choi> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | gyuyoung.kim, lucas.de.marchi, rakuco, ryuan.choi, webkit.review.bot | ||||||
Priority: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Bug Depends on: | 96500 | ||||||||
Bug Blocks: | |||||||||
Attachments: |
|
Description
KwangYong Choi
2012-09-12 01:16:22 PDT
Created attachment 163539 [details]
Patch
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.
(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. I want to change previous routine on GTK and QT first. (Bug 96500) 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); "," -> ',' ? 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. (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. 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. Created attachment 163808 [details]
Patch
(In reply to comment #9) > Created an attachment (id=163808) [details] > Patch Looks good to me. Comment on attachment 163808 [details] Patch Clearing flags on attachment: 163808 Committed r128537: <http://trac.webkit.org/changeset/128537> All reviewed patches have been landed. Closing bug. |