Bug 40807 – [EFL] add support for logging

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Joone Hur]

Logging is not fully implemented for EFL port.

WebKit/WebCore/platform/efl/LoggingEfl.cpp


namespace WebCore {

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

}

[Joone Hur]

Created attachment 59066 [details]
patch

[Lucas De Marchi]

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).

[Lucas De Marchi]

(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

[Joone Hur]

(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 :-)

[Gustavo Sverzut Barbieri]

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.

[Joone Hur]

(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.

[Joone Hur]

Created attachment 59457 [details]
The comment was fixed

@gustavo, do you agree with my opinion?

[Gustavo Sverzut Barbieri]

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.

[Joone Hur]

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.

[Gustavo Noronha (kov)]

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 =)

[Gustavo Sverzut Barbieri]

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,...).

[Joone Hur]

Created attachment 60836 [details]
updated patch

I fixed all you commented.
Thanks for reviewing.

[Antonio Gomes]

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).

[Joone Hur]

Created attachment 60872 [details]
updated patch2

[Joone Hur]

Created attachment 60875 [details]
updated patch3

OK, I removed the reviewer name.
Thanks.

[Lucas De Marchi]

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);

[Joone Hur]

(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)?

[Joone Hur]

Created attachment 60897 [details]
updated patch4

I applied EINA_SAFETY_ON_NULL_RETURN for checking whether eina_str_split returns NULL.

[Lucas De Marchi]

(In reply to comment #17)
> You mean EINA_SAFETY_ON_NULL_RETURN(logv)?

Yes.

[Antonio Gomes]

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?

[Gustavo Sverzut Barbieri]

as I said, I rather go the eina way, but I'm not rejecting or blocking the current implementation.

[Joone Hur]

(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.

[Joone Hur]

There seems no other comments on this patch.
Antonio, would you set commit-queue+ please?

[Antonio Gomes]

Comment on attachment 60897 [details]
updated patch4

> +#include <string.h>

do you really need this include?

[Joone Hur]

Created attachment 62299 [details]
updated patch5

I fixed it, Thanks.

[Antonio Gomes]

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?

[WebKit Commit Bot]

Comment on attachment 62299 [details]
updated patch5

Clearing flags on attachment: 62299

Committed r63901: <http://trac.webkit.org/changeset/63901>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.