Bug 63608

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

Created attachment 120927 [details] patch for ModuleEfl

Looks OK.

Comment on attachment 120927 [details] patch for ModuleEfl LGTM too.

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;

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

Created attachment 121985 [details] patch for ModuleEfl

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

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

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

After thinking more, It looks good to use OwnPtr<Eina_Module>. youngtaeck, could you make a bug to introduce OwnPtr<Eina_Module> ?

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

Created attachment 122401 [details] patch for ModuleEfl

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

Created attachment 125802 [details] Patch

Comment on attachment 125802 [details] Patch Thank you kling.

Comment on attachment 125802 [details] Patch Clearing flags on attachment: 125802 Committed r106921: <http://trac.webkit.org/changeset/106921>

All reviewed patches have been landed. Closing bug.