Bug 135606 - [EFL] Prevent the client from creating ewk_view when EWebkit is not initialized
Summary: [EFL] Prevent the client from creating ewk_view when EWebkit is not initialized
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit EFL (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Grzegorz Czajkowski
URL:
Keywords:
Depends on:
Blocks: 136822
  Show dependency treegraph
 
Reported: 2014-08-05 06:34 PDT by Grzegorz Czajkowski
Modified: 2014-09-15 07:08 PDT (History)
9 users (show)

See Also:


Attachments
Patch (19.72 KB, patch)
2014-08-05 06:38 PDT, Grzegorz Czajkowski
no flags Details | Formatted Diff | Diff
Apply Gyuyoung and Ryuan comments (12.15 KB, patch)
2014-08-06 05:40 PDT, Grzegorz Czajkowski
no flags Details | Formatted Diff | Diff
Patch (14.74 KB, patch)
2014-08-11 05:18 PDT, Grzegorz Czajkowski
gyuyoung.kim: review+
g.czajkowski: commit-queue+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Grzegorz Czajkowski 2014-08-05 06:34:07 PDT
Similarly to EFL modules (eina, evas etc.), application using EWebKit has to initialize it using ewk_init(). Do not allow the client to create ewk_view if ewk_init has not been called.
Comment 1 Grzegorz Czajkowski 2014-08-05 06:38:13 PDT
Created attachment 236028 [details]
Patch
Comment 2 Gyuyoung Kim 2014-08-05 18:59:11 PDT
Comment on attachment 236028 [details]
Patch

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

Thank you for nice fix !

> Source/WebKit2/UIProcess/efl/EwkMain.cpp:3
> +    Copyright (C) 2009-2011 Samsung Electronics

Please update 2011 -> 2014.

> Source/WebKit2/UIProcess/efl/EwkMain.cpp:73
> +        EINA_LOG_CRIT("could not register log domain 'ewebkit2'");

I hope to use same macro as below. CRITICAL()

> Source/WebKit2/UIProcess/efl/EwkMain.cpp:80
> +        eina_log_domain_unregister(m_ewkLogDomainId);

How about adding a function to process shutdown ? For example,

if (!evas_init()) {
    CRITICAL("could not init evas.");
    shutdownEFLLibraries(EVAS_INIT);
}

if (!ecore_init()) {
    CRITICAL("could not init ecore.");
    shutdownEFLLibraries(EVAS_INIT);
}

EwkMain::shutdownEFLLibraries(int failed) {
    switch(failed) {
    case ecore_init:
        break;
    case ecore_evas_init:
        break;
    case ecore_imf_init:
        ecore_evas_shutdown();
        ecore_shutdown();
        evas_shutdown();
        break;
    case efreet_init:
        break;
    }

    eina_log_domain_unregister(m_ewkLogDomainId);
    m_ewkLogDomainId = -1;
    eina_shutdown();
    return 0;
}
Comment 3 Ryuan Choi 2014-08-05 22:39:55 PDT
Comment on attachment 236028 [details]
Patch

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

IMHO, EwkMain in ewk_main_private.h looks fine (without WK).

> Source/WebKit2/UIProcess/API/C/efl/WKMain.cpp:33
> +int WKMainInitialize()

Why WKxxx ?

It looks not quite related to WK APIs to me.

> Source/WebKit2/UIProcess/efl/EwkMain.cpp:34
> +#include <glib-object.h>
> +#include <glib.h>

Should we still include this?

> Source/WebKit2/UIProcess/efl/EwkMain.cpp:64
> +    if (isInitialized())
> +        return ++m_ewkInitCount;

Nit,
Just for me,
if (m_ewkInitCount) looks readable.
Comment 4 Grzegorz Czajkowski 2014-08-06 05:40:18 PDT
Created attachment 236094 [details]
Apply Gyuyoung and Ryuan comments
Comment 5 Grzegorz Czajkowski 2014-08-06 05:53:09 PDT
Comment on attachment 236028 [details]
Patch

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

Thank you guys for the review!

>> Source/WebKit2/UIProcess/API/C/efl/WKMain.cpp:33
>> +int WKMainInitialize()
> 
> Why WKxxx ?
> 
> It looks not quite related to WK APIs to me.

Yeahh, that was overkill a bit. What I wanted to achieve was to use WK C API (WKMainIsInitialized) in ewk_view.cpp. It seems that the style in ewk is completely mixed.

>> Source/WebKit2/UIProcess/efl/EwkMain.cpp:3
>> +    Copyright (C) 2009-2011 Samsung Electronics
> 
> Please update 2011 -> 2014.

Done.

>> Source/WebKit2/UIProcess/efl/EwkMain.cpp:34
>> +#include <glib.h>
> 
> Should we still include this?

Good catch! Indeed they are not needed any longer.

>> Source/WebKit2/UIProcess/efl/EwkMain.cpp:64
>> +        return ++m_ewkInitCount;
> 
> Nit,
> Just for me,
> if (m_ewkInitCount) looks readable.

Restored.

>> Source/WebKit2/UIProcess/efl/EwkMain.cpp:73
>> +        EINA_LOG_CRIT("could not register log domain 'ewebkit2'");
> 
> I hope to use same macro as below. CRITICAL()

Unfortunately we ca not use CRITICAL() macro here as the registration of new log domain (which CRITICAL uses) has failed. Similarly to !eina_init() - fixed in patch #2.

>> Source/WebKit2/UIProcess/efl/EwkMain.cpp:80
>> +        eina_log_domain_unregister(m_ewkLogDomainId);
> 
> How about adding a function to process shutdown ? For example,
> 
> if (!evas_init()) {
>     CRITICAL("could not init evas.");
>     shutdownEFLLibraries(EVAS_INIT);
> }
> 
> if (!ecore_init()) {
>     CRITICAL("could not init ecore.");
>     shutdownEFLLibraries(EVAS_INIT);
> }
> 
> EwkMain::shutdownEFLLibraries(int failed) {
>     switch(failed) {
>     case ecore_init:
>         break;
>     case ecore_evas_init:
>         break;
>     case ecore_imf_init:
>         ecore_evas_shutdown();
>         ecore_shutdown();
>         evas_shutdown();
>         break;
>     case efreet_init:
>         break;
>     }
> 
>     eina_log_domain_unregister(m_ewkLogDomainId);
>     m_ewkLogDomainId = -1;
>     eina_shutdown();
>     return 0;
> }

That's is cool! Added. Thanks!
Comment 6 Ryuan Choi 2014-08-08 05:19:04 PDT
Comment on attachment 236094 [details]
Apply Gyuyoung and Ryuan comments

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

> Source/WebKit2/UIProcess/API/efl/ewk_main_private.h:31
> +typedef unsigned EFLLibraryInitFailure;

How about using enum class or exposing enum ?

> Source/WebKit2/UIProcess/API/efl/ewk_main_private.h:48
> +    int m_ewkInitCount;
> +    int m_ewkLogDomainId;

ewk looks meaningless.

> Source/WebKit2/UIProcess/API/efl/ewk_private.h:29
> +#include "ewk_main_private.h"

Hmm. should we keep ewk_private.h?
Comment 7 Gyuyoung Kim 2014-08-08 20:04:42 PDT
Comment on attachment 236094 [details]
Apply Gyuyoung and Ryuan comments

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

> Source/WebKit2/UIProcess/API/efl/ewk_main.cpp:189
> +int ewk_init(void)

IIRC, we decided that void parameter is only used for webkit-efl's public header.

> Source/WebKit2/UIProcess/API/efl/ewk_main.cpp:194
> +int ewk_shutdown(void)

ditto.
Comment 8 Grzegorz Czajkowski 2014-08-10 11:40:42 PDT
Comment on attachment 236094 [details]
Apply Gyuyoung and Ryuan comments

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

Thanks for the review. I will take it into consideration in patch #3.

>> Source/WebKit2/UIProcess/API/efl/ewk_private.h:29
>> +#include "ewk_main_private.h"
> 
> Hmm. should we keep ewk_private.h?

It seems that ewk_private.h is responsible for defining debug macros. How about renaming it to EwkDebug,h and moving out of API/ directory?
Comment 9 Ryuan Choi 2014-08-10 16:33:22 PDT
(In reply to comment #8)
> (From update of attachment 236094 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=236094&action=review
> 
> Thanks for the review. I will take it into consideration in patch #3.
> 
> >> Source/WebKit2/UIProcess/API/efl/ewk_private.h:29
> >> +#include "ewk_main_private.h"
> > 
> > Hmm. should we keep ewk_private.h?
> 
> It seems that ewk_private.h is responsible for defining debug macros. How about renaming it to EwkDebug,h and moving out of API/ directory?

+1
Comment 10 Grzegorz Czajkowski 2014-08-11 05:18:48 PDT
Created attachment 236364 [details]
Patch
Comment 11 Grzegorz Czajkowski 2014-08-11 05:26:32 PDT
(In reply to comment #9)
> (In reply to comment #8)
> > (From update of attachment 236094 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=236094&action=review
> > 
> > Thanks for the review. I will take it into consideration in patch #3.
> > 
> > >> Source/WebKit2/UIProcess/API/efl/ewk_private.h:29
> > >> +#include "ewk_main_private.h"
> > > 
> > > Hmm. should we keep ewk_private.h?
> > 
> > It seems that ewk_private.h is responsible for defining debug macros. How about renaming it to EwkDebug,h and moving out of API/ directory?
> 
> +1

I'll do it separately at bug 135797.
Comment 12 Gyuyoung Kim 2014-08-11 21:09:54 PDT
Comment on attachment 236364 [details]
Patch

LGTM
Comment 13 Grzegorz Czajkowski 2014-08-12 00:44:04 PDT
Committed r172430: <http://trac.webkit.org/changeset/172430>