Bug 40807 - [EFL] add support for logging
Summary: [EFL] add support for logging
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-06-17 19:58 PDT by Joone Hur
Modified: 2010-07-22 10:48 PDT (History)
11 users (show)

See Also:


Attachments
patch (1.75 KB, patch)
2010-06-17 20:20 PDT, Joone Hur
no flags Details | Formatted Diff | Diff
The comment was fixed (1.74 KB, patch)
2010-06-22 18:39 PDT, Joone Hur
gns: review-
Details | Formatted Diff | Diff
updated patch (1.78 KB, patch)
2010-07-07 22:07 PDT, Joone Hur
no flags Details | Formatted Diff | Diff
updated patch2 (1.73 KB, patch)
2010-07-08 06:56 PDT, Joone Hur
no flags Details | Formatted Diff | Diff
updated patch3 (1.77 KB, patch)
2010-07-08 07:03 PDT, Joone Hur
no flags Details | Formatted Diff | Diff
updated patch4 (1.81 KB, patch)
2010-07-08 09:04 PDT, Joone Hur
tonikitoo: review+
Details | Formatted Diff | Diff
updated patch5 (1.78 KB, patch)
2010-07-22 08:17 PDT, Joone Hur
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Joone Hur 2010-06-17 19:58:54 PDT
Logging is not fully implemented for EFL port.

WebKit/WebCore/platform/efl/LoggingEfl.cpp


namespace WebCore {

void InitializeLoggingChannelsIfNecessary()
{
    LogNotYetImplemented.state = WTFLogChannelOn;
}

}
Comment 1 Joone Hur 2010-06-17 20:20:25 PDT
Created attachment 59066 [details]
patch
Comment 2 Lucas De Marchi 2010-06-18 05:32:01 PDT
Comment on attachment 59066 [details]
patch

Patch looks good. I'm not an official reviewer, so I'll give an informal review.

>diff --git a/WebCore/ChangeLog b/WebCore/ChangeLog
>+    char* logv = strtok(logEnv, " ");

I'd prefer a "," separator as QT guys do because:
   1) It's how eina sets log levels with EINA_LOG_LEVELS
   2) It allows to pass log levels without quoting

However, it's just my preference. I'm CCing the author of eina_log to have his opinion on this.


>+
>+    while (logv) {
>+        if (WTFLogChannel* channel = getChannelFromName(String(logv)))
>+            channel->state = WTFLogChannelOn;
>+        logv = strtok(0, " ");
>+    }
>+
>+    // to disable logging notImplemented set the DISABLE_NI_WARNING
>+    // environment variable to 1

Comments start with capital letter and ends with a dot/punctuation mark. (taken from a review I received on #39821).
Comment 3 Lucas De Marchi 2010-06-18 08:31:59 PDT
(In reply to comment #0)
> Logging is not fully implemented for EFL port.
> 
> WebKit/WebCore/platform/efl/LoggingEfl.cpp
> 
> 
> namespace WebCore {
> 
> void InitializeLoggingChannelsIfNecessary()
> {
>     LogNotYetImplemented.state = WTFLogChannelOn;
> }
> 
> }

Ohh.. Re-thinking about this and the function's name ("IfNecessary"): it's not needed at all because eina_log was already initialized in ewk_main(). With this patch you are justing creating another environment variable to re-implement what is done in eina_log. You should just use

EINA_LOG_LEVELS=<your_log_channel_1>:<your_log_level_1>

Using this environment variable, each log channel is separated by "," like in:

EINA_LOG_LEVELS=ewebkit:2,ewebkit-demo:5 ewebkit-demo
Comment 4 Joone Hur 2010-06-18 09:54:30 PDT
(In reply to comment #3)
> (In reply to comment #0)
> > Logging is not fully implemented for EFL port.
> > 
> > WebKit/WebCore/platform/efl/LoggingEfl.cpp
> > 
> > 
> > namespace WebCore {
> > 
> > void InitializeLoggingChannelsIfNecessary()
> > {
> >     LogNotYetImplemented.state = WTFLogChannelOn;
> > }
> > 
> > }
> 
> Ohh.. Re-thinking about this and the function's name ("IfNecessary"): it's not needed at all because eina_log was already initialized in ewk_main(). With this patch you are justing creating another environment variable to re-implement what is done in eina_log. You should just use
> 
> EINA_LOG_LEVELS=<your_log_channel_1>:<your_log_level_1>
> 
> Using this environment variable, each log channel is separated by "," like in:
> 
> EINA_LOG_LEVELS=ewebkit:2,ewebkit-demo:5 ewebkit-demo

This logging feature is used for printing log information of WebCore.
It is not relevant to ewk stuff.

Regarding "," separator, GTK port uses " " separator. 
It seems no problem because all log channel names of WebCore don't have space

About the comment rule you guided, I will follow it.

Thanks for the review :-)
Comment 5 Gustavo Sverzut Barbieri 2010-06-18 10:23:58 PDT
Hi Joone Hur,

Thanks for your patch, but it is not the way to go. EFL provides Eina Logging, and we should use it as it is appropriate. We must register all the known WTFLogChannel (WebCore/platform/Logging.h) as eina log domains with the eina_log primitives being used in JavaScriptCore/wtf/Assertions.h and JavaScriptCore/wtf/Assertions.cpp as done for Symbian, Mac, Windows and others. From a quick look Eina is fully capable of providing the requirements, if not just let me know and I'll extend eina to provide it, or explain another solution.

Your point about "it's webkit and not ewk" log is not fine, as it should behave as a single component, and replicating environment variables will not help. With a very minor effort in the above mentioned files we can make it fully integrated and consistent.

I hope you understand our point.
Comment 6 Joone Hur 2010-06-18 21:34:49 PDT
(In reply to comment #5)
> Hi Joone Hur,
> 
> Thanks for your patch, but it is not the way to go. EFL provides Eina Logging, and we should use it as it is appropriate. We must register all the known WTFLogChannel (WebCore/platform/Logging.h) as eina log domains with the eina_log primitives being used in JavaScriptCore/wtf/Assertions.h and JavaScriptCore/wtf/Assertions.cpp as done for Symbian, Mac, Windows and others. From a quick look Eina is fully capable of providing the requirements, if not just let me know and I'll extend eina to provide it, or explain another solution.
> 
> Your point about "it's webkit and not ewk" log is not fine, as it should behave as a single component, and replicating environment variables will not help. With a very minor effort in the above mentioned files we can make it fully integrated and consistent.
> 
> I hope you understand our point.

Hi Gustavo,
We couldn't add eina_log primitives in Assertion.h, Assertion.cpp because other ports just try to keep compatibility with WebCore logging. They didn't add their logging primitives.

WebKitGtk is also using WebCore logging without any changes.
Comment 7 Joone Hur 2010-06-22 18:39:01 PDT
Created attachment 59457 [details]
The comment was fixed

@gustavo, do you agree with my opinion?
Comment 8 Gustavo Sverzut Barbieri 2010-06-22 20:33:47 PDT
Hi Joone Hur,

Sorry taking long to reply, but I'm on a trip so quite busy and with restricted access to internet during the day.

The only observation is that I prefer "," instead of " " for separator for the same reasons Lucas said, particularly for consistency with Eina's log variable.

From the EFL-port point-of-view I disagree with it and think we should go eina-log fully. But I'm not a reviewer and will not try to block your patch getting it if you find a reviewer to get your r+/cq+, I'm fine. I'd 

WebKit reviewers might correct me, but I'd do an AssertionsEfl.cpp that is linked instead of Assertions.cpp and it should implement WTFLogVerbose and friends using eina_log primitives directly. Another, bit worse solution, is to replace vprintf_stderr_common() implementation for EFL with eina_log_print(), as done for Windows, Symbian and others. However the output will not look as good as Eina will replicate the some formatting, it is better to feed function, line number, file name straight to Eina then everything works fine and matches what user (developer) would expect.

As added benefit of using Eina is that one might add different logging systems, like syslog, network log or even to serial to help debug in different scenarios.
Comment 9 Joone Hur 2010-06-22 22:15:58 PDT
We need to get reviewed by maintainers.
If they agree with using eina_log primitives, it could be used for enabling logging of EFL port.
Comment 10 Gustavo Noronha (kov) 2010-07-01 12:00:52 PDT
Comment on attachment 59457 [details]
The comment was fixed

WebCore/platform/efl/LoggingEfl.cpp:48
 +      char* logv = strtok(logEnv, " ");
This is wrong. strtok modifies the string it is given, while getenv returns a pointer to a string that is part of the actual environment, so modifying it modifies the environment. Isn't the EFL port using glib for some other stuff already? Why not share the same implementation with GTK+, since for now you're going with the same approach?

r- for the reason above; in a later patch you may want to set the commit-queue flag to ?, to indicate you wish it to be automatically committed, unless you want one of the committers to do it manually =)
Comment 11 Gustavo Sverzut Barbieri 2010-07-01 13:52:32 PDT
Kov, the only thing we use from glib so far is the integration with its main loop and that's due libSoup, maybe it will go away when we have time to change that networking code.

But EFL has the same tokenizer as eina_str_split().

One comment I did before:  Joone Hur please use "," instead of space as it is the same for Eina log.

Last but not least, I'd still follow other ports and have better logging by using Eina log system instead of general purpose fprintf(stderr,...).
Comment 12 Joone Hur 2010-07-07 22:07:01 PDT
Created attachment 60836 [details]
updated patch

I fixed all you commented.
Thanks for reviewing.
Comment 13 Antonio Gomes 2010-07-08 06:34:08 PDT
Comment on attachment 60836 [details]
updated patch


> +        Reviewed by Gustavo Noronha Silva.

pls do not pre-full "Reviewed by" field. Let the commit-bot fill it for you with the name of the reviewer (which is not necessarily Gustavo).
Comment 14 Joone Hur 2010-07-08 06:56:54 PDT
Created attachment 60872 [details]
updated patch2
Comment 15 Joone Hur 2010-07-08 07:03:20 PDT
Created attachment 60875 [details]
updated patch3

OK, I removed the reviewer name.
Thanks.
Comment 16 Lucas De Marchi 2010-07-08 07:54:42 PDT
Comment on attachment 60875 [details]
updated patch3

> diff --git a/WebCore/platform/efl/LoggingEfl.cpp b/WebCore/platform/efl/LoggingEfl.cpp
>  void InitializeLoggingChannelsIfNecessary()
>  {
> +    static bool didInitializeLoggingChannels = false;
> +    if (didInitializeLoggingChannels)
> +        return;
> +
> +    didInitializeLoggingChannels = true;
> +
> +    char* logEnv = getenv("WEBKIT_DEBUG");
> +    if (!logEnv)
> +        return;
> +
> +#if defined(NDEBUG)
> +    EINA_LOG_WARN("WEBKIT_DEBUG is not empty, but this is a release build. Notice that many log messages will only appear in a debug build.");
> +#endif
> +
> +    char** logv = eina_str_split(logEnv, ",", -1);
eina_str_split might return NULL if it can not allocate space for either the new string or the array. It would crash on next line since you'd dereference a null pointer. Just add an EINA_SAFETY here.
EINA_SAFETY_ON_NULL_RETURN(logEnv);
Comment 17 Joone Hur 2010-07-08 08:35:36 PDT
(In reply to comment #16)
> (From update of attachment 60875 [details])
> > diff --git a/WebCore/platform/efl/LoggingEfl.cpp b/WebCore/platform/efl/LoggingEfl.cpp
> >  void InitializeLoggingChannelsIfNecessary()
> >  {
> > +    static bool didInitializeLoggingChannels = false;
> > +    if (didInitializeLoggingChannels)
> > +        return;
> > +
> > +    didInitializeLoggingChannels = true;
> > +
> > +    char* logEnv = getenv("WEBKIT_DEBUG");
> > +    if (!logEnv)
> > +        return;
> > +
> > +#if defined(NDEBUG)
> > +    EINA_LOG_WARN("WEBKIT_DEBUG is not empty, but this is a release build. Notice that many log messages will only appear in a debug build.");
> > +#endif
> > +
> > +    char** logv = eina_str_split(logEnv, ",", -1);
> eina_str_split might return NULL if it can not allocate space for either the new string or the array. It would crash on next line since you'd dereference a null pointer. Just add an EINA_SAFETY here.
> EINA_SAFETY_ON_NULL_RETURN(logEnv);

You mean EINA_SAFETY_ON_NULL_RETURN(logv)?
Comment 18 Joone Hur 2010-07-08 09:04:30 PDT
Created attachment 60897 [details]
updated patch4

I applied EINA_SAFETY_ON_NULL_RETURN for checking whether eina_str_split returns NULL.
Comment 19 Lucas De Marchi 2010-07-08 09:08:44 PDT
(In reply to comment #17)
> You mean EINA_SAFETY_ON_NULL_RETURN(logv)?

Yes.
Comment 20 Antonio Gomes 2010-07-12 20:47:42 PDT
Joone, I think the code looks fine and you've addressed all comments from Lucas, Gustavo Noronha, and Gustavo Barbieri. However in comment #3, comment #5, comment #8 it was expressed desire for another direction for logging: use the already existent eina logging system.

Gustavo, Lucas, Joone, do we have an agreement about the way to go?
Comment 21 Gustavo Sverzut Barbieri 2010-07-13 08:07:24 PDT
as I said, I rather go the eina way, but I'm not rejecting or blocking the current implementation.
Comment 22 Joone Hur 2010-07-13 17:26:51 PDT
(In reply to comment #21)
> as I said, I rather go the eina way, but I'm not rejecting or blocking the current implementation.

I would consider it if possible to apply eina_log primitives to WebCore.
Thanks for comments.
Comment 23 Joone Hur 2010-07-16 08:14:51 PDT
There seems no other comments on this patch.
Antonio, would you set commit-queue+ please?
Comment 24 Antonio Gomes 2010-07-21 13:08:53 PDT
Comment on attachment 60897 [details]
updated patch4

> +#include <string.h>

do you really need this include?
Comment 25 Joone Hur 2010-07-22 08:17:50 PDT
Created attachment 62299 [details]
updated patch5

I fixed it, Thanks.
Comment 26 Antonio Gomes 2010-07-22 08:33:49 PDT
Comment on attachment 62299 [details]
updated patch5

Note for next patches, since I've already r+'ed it, you do not need to ask for review again. You can fix the problems, re-upload the patch with "Reviewed by Antonio Gomes" filled and just ask cq?
Comment 27 WebKit Commit Bot 2010-07-22 10:48:48 PDT
Comment on attachment 62299 [details]
updated patch5

Clearing flags on attachment: 62299

Committed r63901: <http://trac.webkit.org/changeset/63901>
Comment 28 WebKit Commit Bot 2010-07-22 10:48:55 PDT
All reviewed patches have been landed.  Closing bug.