Bug 63608 - [EFL][WK2] Add ModuleEfl.cpp
Summary: [EFL][WK2] Add ModuleEfl.cpp
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit EFL (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on: 76255
Blocks: 61838
  Show dependency treegraph
 
Reported: 2011-06-28 22:33 PDT by YoungTaeck Song
Modified: 2012-02-07 03:45 PST (History)
7 users (show)

See Also:


Attachments
patch for ModuleEfl (3.46 KB, patch)
2012-01-03 03:47 PST, YoungTaeck Song
no flags Details | Formatted Diff | Diff
patch for ModuleEfl (3.17 KB, patch)
2012-01-10 23:50 PST, YoungTaeck Song
no flags Details | Formatted Diff | Diff
patch for ModuleEfl (2.98 KB, patch)
2012-01-13 02:20 PST, YoungTaeck Song
no flags Details | Formatted Diff | Diff
Patch (3.13 KB, patch)
2012-02-07 03:25 PST, YoungTaeck Song
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description YoungTaeck Song 2011-06-28 22:33:33 PDT
Add First version of ModuleEfl.cpp for WebKit2
Comment 1 Tomasz Morawski 2011-10-27 23:25:47 PDT
(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.
Comment 2 YoungTaeck Song 2012-01-03 03:47:07 PST
Created attachment 120927 [details]
patch for ModuleEfl
Comment 3 Raphael Kubo da Costa (:rakuco) 2012-01-03 04:28:08 PST
Looks OK.
Comment 4 Gyuyoung Kim 2012-01-03 16:29:04 PST
Comment on attachment 120927 [details]
patch for ModuleEfl

LGTM too.
Comment 5 Ryuan Choi 2012-01-10 19:32:11 PST
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;
Comment 6 YoungTaeck Song 2012-01-10 22:20:26 PST
(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.
Comment 7 YoungTaeck Song 2012-01-10 23:50:14 PST
Created attachment 121985 [details]
patch for ModuleEfl
Comment 8 Ryuan Choi 2012-01-10 23:58:25 PST
(In reply to comment #7)
> Created an attachment (id=121985) [details]
> patch for ModuleEfl

LGTM, too.
Comment 9 Andreas Kling 2012-01-12 00:45:39 PST
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()?
Comment 10 Ryuan Choi 2012-01-12 01:10:58 PST
(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.
Comment 11 Ryuan Choi 2012-01-12 01:34:52 PST
After thinking more,
It looks good to use OwnPtr<Eina_Module>.

youngtaeck,
could you make a bug to introduce OwnPtr<Eina_Module> ?
Comment 12 YoungTaeck Song 2012-01-13 02:19:55 PST
(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.
Comment 13 YoungTaeck Song 2012-01-13 02:20:43 PST
Created attachment 122401 [details]
patch for ModuleEfl
Comment 14 Ryuan Choi 2012-02-07 02:58:53 PST
(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 ?
Comment 15 Andreas Kling 2012-02-07 03:03:49 PST
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)
Comment 16 YoungTaeck Song 2012-02-07 03:25:43 PST
Created attachment 125802 [details]
Patch
Comment 17 Ryuan Choi 2012-02-07 03:28:30 PST
Comment on attachment 125802 [details]
Patch

Thank you kling.
Comment 18 WebKit Review Bot 2012-02-07 03:45:45 PST
Comment on attachment 125802 [details]
Patch

Clearing flags on attachment: 125802

Committed r106921: <http://trac.webkit.org/changeset/106921>
Comment 19 WebKit Review Bot 2012-02-07 03:45:51 PST
All reviewed patches have been landed.  Closing bug.