Bug 63608 – [EFL][WK2] Add ModuleEfl.cpp

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

[YoungTaeck Song]

Add First version of ModuleEfl.cpp for WebKit2

[Tomasz Morawski]

(In reply to comment #0)
> Add First version of ModuleEfl.cpp for WebKit2
I seems that you have not added your patch. Could you please add it.

[YoungTaeck Song]

Created attachment 120927 [details]
patch for ModuleEfl

[Raphael Kubo da Costa (:rakuco)]

Looks OK.

[Gyuyoung Kim]

Comment on attachment 120927 [details]
patch for ModuleEfl

LGTM too.

[Ryuan Choi]

Comment on attachment 120927 [details]
patch for ModuleEfl

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

> Source/WebKit2/Platform/efl/ModuleEfl.cpp:51
> +    void* symbolPointer = 0;
> +
> +    if (m_module)
> +        symbolPointer = eina_module_symbol_get(m_module, functionName);
> +
> +    return symbolPointer;

Almost looks good to me.

As small thing, below is clear for me.
if (m_module)
    return eina_module_symbol_get(m_module, functionName);

return 0;

[YoungTaeck Song]

(In reply to comment #5)
> (From update of attachment 120927 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=120927&action=review
> 
> > Source/WebKit2/Platform/efl/ModuleEfl.cpp:51
> > +    void* symbolPointer = 0;
> > +
> > +    if (m_module)
> > +        symbolPointer = eina_module_symbol_get(m_module, functionName);
> > +
> > +    return symbolPointer;
> 
> Almost looks good to me.
> 
> As small thing, below is clear for me.
> if (m_module)
>     return eina_module_symbol_get(m_module, functionName);
> 
> return 0;

Sorry. I don't agree with you. because platformFunctionPointer relies on the eina_module_symbol_get, My code is more clear.

[YoungTaeck Song]

Created attachment 121985 [details]
patch for ModuleEfl

[Ryuan Choi]

(In reply to comment #7)
> Created an attachment (id=121985) [details]
> patch for ModuleEfl

LGTM, too.

[Andreas Kling]

Comment on attachment 121985 [details]
patch for ModuleEfl

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

> Source/WebKit2/Platform/efl/ModuleEfl.cpp:33
> +    m_module = eina_module_new(m_path.utf8().data());
> +    if (m_module && eina_module_load(m_module))
> +        return true;
> +
> +    return false;

If eina_module_load() fails, don't you need to call eina_module_free() before returning false?

> Source/WebKit2/Platform/efl/ModuleEfl.cpp:41
> +    if (m_module) {
> +        eina_module_unload(m_module);
> +        m_module = 0;
> +    }

Is it safe to call eina_module_unload() here if m_module is non-null, but eina_module_load() failed in Module::load()?

> Source/WebKit2/Platform/efl/ModuleEfl.cpp:47
> +    if (m_module)
> +        return eina_module_symbol_get(m_module, functionName);

Is it safe to call eina_module_symbol_get() here if m_module is non-null, but eina_module_load() failed in Module::load()?

[Ryuan Choi]

(In reply to comment #9)
> (From update of attachment 121985 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=121985&action=review
> 
> > Source/WebKit2/Platform/efl/ModuleEfl.cpp:33
> > +    m_module = eina_module_new(m_path.utf8().data());
> > +    if (m_module && eina_module_load(m_module))
> > +        return true;
> > +
> > +    return false;
> 
> If eina_module_load() fails, don't you need to call eina_module_free() before returning false?
> 
You are right.
I checked eina_module, so your point looks correct.

And also, nobody calls eina_module_free.

> > Source/WebKit2/Platform/efl/ModuleEfl.cpp:41
> > +    if (m_module) {
> > +        eina_module_unload(m_module);
> > +        m_module = 0;
> > +    }
> 
> Is it safe to call eina_module_unload() here if m_module is non-null, but eina_module_load() failed in Module::load()?
> 
> > Source/WebKit2/Platform/efl/ModuleEfl.cpp:47
> > +    if (m_module)
> > +        return eina_module_symbol_get(m_module, functionName);
> 
> Is it safe to call eina_module_symbol_get() here if m_module is non-null, but eina_module_load() failed in Module::load()?

It is safe because eina_module_symbol_get take care of it.
However, we'd better to make it clear in this patch.

[Ryuan Choi]

After thinking more,
It looks good to use OwnPtr<Eina_Module>.

youngtaeck,
could you make a bug to introduce OwnPtr<Eina_Module> ?

[YoungTaeck Song]

(In reply to comment #11)
> After thinking more,
> It looks good to use OwnPtr<Eina_Module>.
> 
> youngtaeck,
> could you make a bug to introduce OwnPtr<Eina_Module> ?

Thank. I make a bug for OwnPtr<Eina_Module>, 76255.
and I attached new patch using OwnPtr.

[YoungTaeck Song]

Created attachment 122401 [details]
patch for ModuleEfl

[Ryuan Choi]

(In reply to comment #13)
> Created an attachment (id=122401) [details]
> patch for ModuleEfl

Added kling in CCs.

Could you take a look at this bug once more ?

[Andreas Kling]

Comment on attachment 122401 [details]
patch for ModuleEfl

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

r=me, with two suggestions:

> Source/WebKit2/Platform/efl/ModuleEfl.cpp:32
> +    if (!m_module.get() || !eina_module_load(m_module.get())) {

You don't need the .get() when null-checking an OwnPtr as it overrides operator!, this will work just as well:
if (!m_module || !eina_module_load(m_module.get())) {

> Source/WebKit2/Platform/efl/ModuleEfl.cpp:47
> +    if (m_module.get())

Same here:
if (m_module)

[YoungTaeck Song]

Created attachment 125802 [details]
Patch

[Ryuan Choi]

Comment on attachment 125802 [details]
Patch

Thank you kling.

[WebKit Review Bot]

Comment on attachment 125802 [details]
Patch

Clearing flags on attachment: 125802

Committed r106921: <http://trac.webkit.org/changeset/106921>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.